Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove changing volatile to singleqe for queries with outer quals #1163

Open
wants to merge 6 commits into
base: adb-7.2.0
Choose a base branch
from

Conversation

red1452
Copy link

@red1452 red1452 commented Dec 18, 2024

Remove changing volatile to singleqe for queries with outer quals

Function handle_gen_seggen_volatile_path is used in 6X to make single QE query
from queries on replicated tables with volatile function. It adds Motion and
Result nodes to make query at one segment correctly. In 7X it does not need to
do, because if query contains outer quals, nodes result and materialize will be
added by bring_to_outer_query.

Function handle_gen_seggen_volatile_path was not removed, because it contains
check, which restricts DML queries on replicated tables with filter, which
contains volatile functions.

Also, this patch adds tests from commit 7ef4218.

src/backend/cdb/cdbllize.c Outdated Show resolved Hide resolved
src/backend/cdb/cdbllize.c Outdated Show resolved Hide resolved
src/test/regress/expected/rpt.out Show resolved Hide resolved
@red1452 red1452 marked this pull request as draft December 19, 2024 07:43
@red1452 red1452 marked this pull request as ready for review December 27, 2024 18:46
Copy link
Collaborator

@Stolb27 Stolb27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reword PR description. Why adding of Motion by the mentioned function is incorrect? Which consequences is leaded? The bring_to_outer_query is able to produce both kind of motions (Gather or Broadcast). What is the case when the handle_gen_seggen_volatile_path still needed that makes you to exclude it only partially?

*/
if (rel->reloptkind == RELOPT_BASEREL)
if (rel->upperrestrictinfo == NULL && rel->reloptkind == RELOPT_BASEREL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does empty upperrestrictinfo sign that there is no outer queries?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment above.

Copy link
Collaborator

@Stolb27 Stolb27 Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You filtered out cases for bring_to_outer_query from this condition. Why problem manifests itself only in case of not null upperrestrictinfo? What are other cases to execute Segment General only on Single QE?

Function handle_gen_seggen_volatile_path was not removed, because it contains
check, which restricts DML queries on replicated tables with filter, which
contains volatile functions.

It's quite strange to leave complex transformation logic only for exact checks. Do we still needed these transformations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions handle_gen_seggen_volatile_path, bring_to_outer_query and bring_to_singleQE make the same things, but in the different cases. Function handle_gen_seggen_volatile_path works when locus is general or segment general or replicated and there is volatile function. Function bring_to_outer_query works when locus is not general and not outer query and there is outer quals. Function bring_to_singleQE works when locus is not general and not entry and not single QE and not outer query.
I think removing or union of these functions must be done in the other ticket, which will be complex refactoring of it. My patch just fix double work of these functions, when handle_gen_seggen_volatile_path and bring_to_outer_query work together. It does not needed to execute function handle_gen_seggen_volatile_path if function bring_to_outer_query will be executed (in case of non null upperrestrictinfo).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of some queries, which will be changed if handle_gen_seggen_volatile_path is removed:

create table t_hashdist(a int, b int, c int) distributed by (a);
create table t_replicate_volatile(a int, b int, c int) distributed replicated;
explain (costs off) select * from t_replicate_volatile, t_hashdist where t_replicate_volatile.a > random();

Plan before:

                              QUERY PLAN                              
----------------------------------------------------------------------
 Nested Loop
   ->  Gather Motion 3:1  (slice1; segments: 3)
         ->  Seq Scan on t_hashdist
   ->  Materialize
         ->  Result
               ->  Gather Motion 1:1  (slice2; segments: 1)
                     ->  Seq Scan on t_replicate_volatile
                           Filter: ((a)::double precision > random())
 Optimizer: Postgres query optimizer
(9 rows)

Plan after:

                        QUERY PLAN                        
----------------------------------------------------------
 Gather Motion 3:1  (slice1; segments: 3)
   ->  Nested Loop
         ->  Seq Scan on t_replicate_volatile
               Filter: ((a)::double precision > random())
         ->  Materialize
               ->  Seq Scan on t_hashdist
 Optimizer: Postgres-based planner
(7 rows)

@red1452 red1452 changed the title Fix adding Motion node to correlated Subplan Remove changing volatile to singleqe for queries with outer quals Jan 20, 2025
@Stolb27
Copy link
Collaborator

Stolb27 commented Jan 21, 2025

Function handle_gen_seggen_volatile_path is used in 6X to make single QE query
from queries on replicated tables with volatile function. It adds Motion and
Result nodes to make query at one segment correctly.

In my opinion, the function transforms only part of query, not the whole one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants