Skip to content

ADBDEV-2885: Postgres planner produces bogus plan for query to replicated table with SIRV function#365

Closed
RekGRpth wants to merge 7 commits into
adb-6.x-devfrom
ADBDEV-2885
Closed

ADBDEV-2885: Postgres planner produces bogus plan for query to replicated table with SIRV function#365
RekGRpth wants to merge 7 commits into
adb-6.x-devfrom
ADBDEV-2885

Conversation

@RekGRpth

@RekGRpth RekGRpth commented Aug 3, 2022

Copy link
Copy Markdown
Member

Query with subplan containing volatile functions on distributed replicated tables does not make motion gather. This produces wrong plan and leads to segfault.
Steps to reproduce this problem:

create table t (i int) distributed replicated;
insert into t select 1;
create function f(i int) returns int language sql security definer as $$ select i; $$;
select (select f(i) from t); -- <-- segfault here!!!
select (select f(i) from t group by f(i)); -- <-- segfault here!!!
select (select i from t group by i having f(i) > 0); -- <-- segfault here!!!

Wrong plans:

explain (costs off, verbose) SELECT (select f(i) from t);
             QUERY PLAN              
-------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)
     ->  Seq Scan on public.t
           Output: f(i)
 Optimizer: Postgres query optimizer
(6 rows)
explain (costs off, verbose) SELECT (select f(i) from t group by f(i));
             QUERY PLAN              
-------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)
     ->  HashAggregate
           Output: (f(i))
           Group Key: f(t.i)
           ->  Seq Scan on public.t
                 Output: i, f(i)
 Optimizer: Postgres query optimizer
(9 rows)
explain (costs off, verbose) SELECT (select i from t group by i having f(i) > 0);
             QUERY PLAN              
-------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)
     ->  HashAggregate
           Output: i
           Group Key: t.i
           Filter: (f(t.i) > 0)
           ->  Seq Scan on public.t
                 Output: i
 Optimizer: Postgres query optimizer
(10 rows)

Solving problem includes make explicit motion gather for such subplan and only for it.
Right plans:

explain (costs off, verbose) SELECT (select f(i) from t);
                    QUERY PLAN                    
--------------------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)  (slice2)
     ->  Gather Motion 1:1  (slice1; segments: 1)
           Output: (f(i))
           ->  Seq Scan on public.t
                 Output: f(i)
 Optimizer: Postgres query optimizer
(8 rows)
explain (costs off, verbose) SELECT (select f(i) from t group by f(i));
                    QUERY PLAN                    
--------------------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)  (slice2)
     ->  Gather Motion 1:1  (slice1; segments: 1)
           Output: (f(i))
           ->  HashAggregate
                 Output: (f(i))
                 Group Key: f(t.i)
                 ->  Seq Scan on public.t
                       Output: i, f(i)
 Optimizer: Postgres query optimizer
(11 rows)
explain (costs off, verbose) SELECT (select i from t group by i having f(i) > 0);
                    QUERY PLAN                    
--------------------------------------------------
 Result
   Output: $0
   InitPlan 1 (returns $0)  (slice2)
     ->  Gather Motion 1:1  (slice1; segments: 1)
           Output: i
           ->  HashAggregate
                 Output: i
                 Group Key: t.i
                 Filter: (f(t.i) > 0)
                 ->  Seq Scan on public.t
                       Output: i
 Optimizer: Postgres query optimizer
(12 rows)

@RekGRpth RekGRpth marked this pull request as ready for review August 3, 2022 11:50
@RekGRpth RekGRpth requested a review from a team August 3, 2022 11:50
@RekGRpth RekGRpth marked this pull request as draft August 4, 2022 02:45
@RekGRpth RekGRpth marked this pull request as ready for review August 4, 2022 08:05
Comment thread src/backend/cdb/cdbllize.c Outdated
Comment on lines 1110 to 1111

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please describe the meaning of both code lines in the commit and PR description.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for query (on distributed replicated tables) containing volatile functions consider adjust plan flow

Comment thread src/test/regress/expected/rpt.out Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please provide test description.
It's not recommended to limit test execution by some planner (except cases when it leads to creation new copy of out file, or reproduction steps uses fault injector and freezes on the different optimizer).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

Comment thread src/test/regress/expected/rpt.out Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You may use costs off to decrease patch footprint in this case. More, I think that explain analyze is enough (valid plan + SIGSEGV absence).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@Stolb27 Stolb27 requested a review from a team August 5, 2022 05:08
@RekGRpth RekGRpth requested a review from Stolb27 August 5, 2022 11:39
@Stolb27

Stolb27 commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator

The next queries are planned incorrect with current version of patch (SIGSEGV without patch):

SELECT (select f(i) from t group by f(i));
SELECT (select i from t group by i having f(i) > 0);

In my opinion, you are trying to solve exact problem, but the problem is wider. Replicated table implementation in gpdb should be rewritten from scratch. GPDB refuses to apply motion for data outside the coordinator.

@RekGRpth RekGRpth marked this pull request as draft August 8, 2022 06:15
@RekGRpth RekGRpth force-pushed the ADBDEV-2885 branch 2 times, most recently from 756a562 to b86e435 Compare August 10, 2022 10:11
@RekGRpth RekGRpth marked this pull request as ready for review August 12, 2022 10:22
Comment thread src/include/nodes/primnodes.h
HustonMmmavr
HustonMmmavr previously approved these changes Aug 18, 2022
@RekGRpth RekGRpth marked this pull request as draft August 18, 2022 06:25
@RekGRpth RekGRpth closed this Aug 23, 2022
ADB 6.21.1_arenadata36 release
@Stolb27 Stolb27 deleted the ADBDEV-2885 branch August 24, 2022 07:17
@RekGRpth RekGRpth restored the ADBDEV-2885 branch August 25, 2022 03:19
@RekGRpth RekGRpth reopened this Aug 25, 2022
@RekGRpth RekGRpth changed the base branch from adb-6.x-dev to adb-6.x August 25, 2022 08:36
@RekGRpth RekGRpth changed the base branch from adb-6.x to adb-6.x-dev August 25, 2022 08:37
@RekGRpth RekGRpth closed this Aug 29, 2022
@RekGRpth RekGRpth deleted the ADBDEV-2885 branch December 11, 2025 08:27
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.

3 participants