Skip to content

Commit d5d2205

Browse files
david-rowleydavid-rowley
committed
Fix assert failure when planning setop subqueries with CTEs
66c0185 adjusted the UNION planner to request that union child queries produce Paths correctly ordered to implement the UNION by way of MergeAppend followed by Unique. The code there made a bad assumption that if the root->parent_root->parse had setOperations set that the query must be the child subquery of a set operation. That's not true when it comes to planning a non-inlined CTE which is parented by a set operation. This causes issues as the CTE's targetlist has no requirement to match up to the SetOperationStmt's groupClauses Fix this by adding a new parameter to both subquery_planner() and grouping_planner() to explicitly pass the SetOperationStmt only when planning set operation child subqueries. Thank you to Tom Lane for helping to rationalize the decision on the best function signature for subquery_planner(). Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/242fc7c6-a8aa-2daf-ac4c-0a231e2619c1@gmail.com
1 parent 3622c80 commit d5d2205

File tree

7 files changed

+66
-35
lines changed

7 files changed

+66
-35
lines changed

‎src/backend/optimizer/path/allpaths.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2645,9 +2645,8 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
26452645
Assert(root->plan_params == NIL);
26462646

26472647
/* Generate a subroot and Paths for the subquery */
2648-
rel->subroot = subquery_planner(root->glob, subquery,
2649-
root,
2650-
false, tuple_fraction);
2648+
rel->subroot = subquery_planner(root->glob, subquery, root, false,
2649+
tuple_fraction, NULL);
26512650

26522651
/* Isolate the params needed by this specific subplan */
26532652
rel->subplan_params = root->plan_params;

‎src/backend/optimizer/plan/planner.c

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,8 @@ typedef struct
127127
/* Local functions */
128128
static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind);
129129
static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode);
130-
static void grouping_planner(PlannerInfo *root, double tuple_fraction);
130+
static void grouping_planner(PlannerInfo *root, double tuple_fraction,
131+
SetOperationStmt *setops);
131132
static grouping_sets_data *preprocess_grouping_sets(PlannerInfo *root);
132133
static List *remap_to_groupclause_idx(List *groupClause, List *gsets,
133134
int *tleref_to_colnum_map);
@@ -411,8 +412,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
411412
}
412413

413414
/* primary planning entry point (may recurse for subqueries) */
414-
root = subquery_planner(glob, parse, NULL,
415-
false, tuple_fraction);
415+
root = subquery_planner(glob, parse, NULL, false, tuple_fraction, NULL);
416416

417417
/* Select best Path and turn it into a Plan */
418418
final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
@@ -603,6 +603,10 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
603603
* hasRecursion is true if this is a recursive WITH query.
604604
* tuple_fraction is the fraction of tuples we expect will be retrieved.
605605
* tuple_fraction is interpreted as explained for grouping_planner, below.
606+
* setops is used for set operation subqueries to provide the subquery with
607+
* the context in which it's being used so that Paths correctly sorted for the
608+
* set operation can be generated. NULL when not planning a set operation
609+
* child.
606610
*
607611
* Basically, this routine does the stuff that should only be done once
608612
* per Query object. It then calls grouping_planner. At one time,
@@ -621,9 +625,9 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
621625
*--------------------
622626
*/
623627
PlannerInfo *
624-
subquery_planner(PlannerGlobal *glob, Query *parse,
625-
PlannerInfo *parent_root,
626-
bool hasRecursion, double tuple_fraction)
628+
subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
629+
bool hasRecursion, double tuple_fraction,
630+
SetOperationStmt *setops)
627631
{
628632
PlannerInfo *root;
629633
List *newWithCheckOptions;
@@ -1085,7 +1089,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
10851089
/*
10861090
* Do the main planning.
10871091
*/
1088-
grouping_planner(root, tuple_fraction);
1092+
grouping_planner(root, tuple_fraction, setops);
10891093

10901094
/*
10911095
* Capture the set of outer-level param IDs we have access to, for use in
@@ -1283,7 +1287,11 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr)
12831287
* 0 < tuple_fraction < 1: expect the given fraction of tuples available
12841288
* from the plan to be retrieved
12851289
* tuple_fraction >= 1: tuple_fraction is the absolute number of tuples
1286-
* expected to be retrieved (ie, a LIMIT specification)
1290+
* expected to be retrieved (ie, a LIMIT specification).
1291+
* setops is used for set operation subqueries to provide the subquery with
1292+
* the context in which it's being used so that Paths correctly sorted for the
1293+
* set operation can be generated. NULL when not planning a set operation
1294+
* child.
12871295
*
12881296
* Returns nothing; the useful output is in the Paths we attach to the
12891297
* (UPPERREL_FINAL, NULL) upperrel in *root. In addition,
@@ -1294,7 +1302,8 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr)
12941302
*--------------------
12951303
*/
12961304
static void
1297-
grouping_planner(PlannerInfo *root, double tuple_fraction)
1305+
grouping_planner(PlannerInfo *root, double tuple_fraction,
1306+
SetOperationStmt *setops)
12981307
{
12991308
Query *parse = root->parse;
13001309
int64 offset_est = 0;
@@ -1510,16 +1519,10 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
15101519
qp_extra.gset_data = gset_data;
15111520

15121521
/*
1513-
* Check if we're a subquery for a set operation. If we are, store
1514-
* the SetOperationStmt in qp_extra.
1522+
* If we're a subquery for a set operation, store the SetOperationStmt
1523+
* in qp_extra.
15151524
*/
1516-
if (root->parent_root != NULL &&
1517-
root->parent_root->parse->setOperations != NULL &&
1518-
IsA(root->parent_root->parse->setOperations, SetOperationStmt))
1519-
qp_extra.setop =
1520-
(SetOperationStmt *) root->parent_root->parse->setOperations;
1521-
else
1522-
qp_extra.setop = NULL;
1525+
qp_extra.setop = setops;
15231526

15241527
/*
15251528
* Generate the best unsorted and presorted paths for the scan/join

‎src/backend/optimizer/plan/subselect.c

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,8 @@ make_subplan(PlannerInfo *root, Query *orig_subquery,
218218
Assert(root->plan_params == NIL);
219219

220220
/* Generate Paths for the subquery */
221-
subroot = subquery_planner(root->glob, subquery,
222-
root,
223-
false, tuple_fraction);
221+
subroot = subquery_planner(root->glob, subquery, root, false,
222+
tuple_fraction, NULL);
224223

225224
/* Isolate the params needed by this specific subplan */
226225
plan_params = root->plan_params;
@@ -266,9 +265,8 @@ make_subplan(PlannerInfo *root, Query *orig_subquery,
266265
if (subquery)
267266
{
268267
/* Generate Paths for the ANY subquery; we'll need all rows */
269-
subroot = subquery_planner(root->glob, subquery,
270-
root,
271-
false, 0.0);
268+
subroot = subquery_planner(root->glob, subquery, root, false, 0.0,
269+
NULL);
272270

273271
/* Isolate the params needed by this specific subplan */
274272
plan_params = root->plan_params;
@@ -967,9 +965,8 @@ SS_process_ctes(PlannerInfo *root)
967965
* Generate Paths for the CTE query. Always plan for full retrieval
968966
* --- we don't have enough info to predict otherwise.
969967
*/
970-
subroot = subquery_planner(root->glob, subquery,
971-
root,
972-
cte->cterecursive, 0.0);
968+
subroot = subquery_planner(root->glob, subquery, root,
969+
cte->cterecursive, 0.0, NULL);
973970

974971
/*
975972
* Since the current query level doesn't yet contain any RTEs, it

‎src/backend/optimizer/prep/prepunion.c

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,7 @@ recurse_set_operations(Node *setOp, PlannerInfo *root,
244244
{
245245
RangeTblRef *rtr = (RangeTblRef *) setOp;
246246
RangeTblEntry *rte = root->simple_rte_array[rtr->rtindex];
247+
SetOperationStmt *setops;
247248
Query *subquery = rte->subquery;
248249
PlannerInfo *subroot;
249250
List *tlist;
@@ -257,11 +258,16 @@ recurse_set_operations(Node *setOp, PlannerInfo *root,
257258
/* plan_params should not be in use in current query level */
258259
Assert(root->plan_params == NIL);
259260

261+
/*
262+
* Pass the set operation details to the subquery_planner to have it
263+
* consider generating Paths correctly ordered for the set operation.
264+
*/
265+
setops = castNode(SetOperationStmt, root->parse->setOperations);
266+
260267
/* Generate a subroot and Paths for the subquery */
261-
subroot = rel->subroot = subquery_planner(root->glob, subquery,
262-
root,
263-
false,
264-
root->tuple_fraction);
268+
subroot = rel->subroot = subquery_planner(root->glob, subquery, root,
269+
false, root->tuple_fraction,
270+
setops);
265271

266272
/*
267273
* It should not be possible for the primitive query to contain any

‎src/include/optimizer/planner.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ extern PlannedStmt *standard_planner(Query *parse, const char *query_string,
4444

4545
extern PlannerInfo *subquery_planner(PlannerGlobal *glob, Query *parse,
4646
PlannerInfo *parent_root,
47-
bool hasRecursion, double tuple_fraction);
47+
bool hasRecursion, double tuple_fraction,
48+
SetOperationStmt *setops);
4849

4950
extern RowMarkType select_rowmark_type(RangeTblEntry *rte,
5051
LockClauseStrength strength);

‎src/test/regress/expected/union.out

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1035,6 +1035,20 @@ select from generate_series(1,5) except all select from generate_series(1,3);
10351035
--
10361036
(2 rows)
10371037

1038+
-- Try a variation of the above but with a CTE which contains a column, again
1039+
-- with an empty final select list.
1040+
-- Ensure we get the expected 1 row with 0 columns
1041+
with cte as materialized (select s from generate_series(1,5) s)
1042+
select from cte union select from cte;
1043+
--
1044+
(1 row)
1045+
1046+
-- Ensure we get the same result as the above.
1047+
with cte as not materialized (select s from generate_series(1,5) s)
1048+
select from cte union select from cte;
1049+
--
1050+
(1 row)
1051+
10381052
reset enable_hashagg;
10391053
reset enable_sort;
10401054
--

‎src/test/regress/sql/union.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,17 @@ select from generate_series(1,5) intersect all select from generate_series(1,3);
330330
select from generate_series(1,5) except select from generate_series(1,3);
331331
select from generate_series(1,5) except all select from generate_series(1,3);
332332

333+
-- Try a variation of the above but with a CTE which contains a column, again
334+
-- with an empty final select list.
335+
336+
-- Ensure we get the expected 1 row with 0 columns
337+
with cte as materialized (select s from generate_series(1,5) s)
338+
select from cte union select from cte;
339+
340+
-- Ensure we get the same result as the above.
341+
with cte as not materialized (select s from generate_series(1,5) s)
342+
select from cte union select from cte;
343+
333344
reset enable_hashagg;
334345
reset enable_sort;
335346

0 commit comments

Comments
 (0)