🌐 AI搜索 & 代理 主页
Skip to content

Commit eb7d043

Browse files
david-rowleydavid-rowley
committed
Fix incorrect logic for determining safe WindowAgg run conditions
The logic added in 9d9c02c to determine when a qual can be used as a WindowClause run condition failed to correctly check for subqueries in the qual. This was being done correctly for normal subquery qual pushdowns, it's just that 9d9c02c failed to follow the lead on that. This also fixes various other cases where transforming the qual into a WindowClause run condition in the subquery should have been disallowed. Bug: #17826 Reported-by: Anban Company Discussion: https://postgr.es/m/17826-7d8750952f19a5f5@postgresql.org Backpatch-through: 15, where 9d9c02c was introduced.
1 parent b9f8d1c commit eb7d043

File tree

3 files changed

+161
-84
lines changed

3 files changed

+161
-84
lines changed

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

Lines changed: 119 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,32 @@
5252
#include "utils/lsyscache.h"
5353

5454

55+
/* Bitmask flags for pushdown_safety_info.unsafeFlags */
56+
#define UNSAFE_HAS_VOLATILE_FUNC (1 << 0)
57+
#define UNSAFE_HAS_SET_FUNC (1 << 1)
58+
#define UNSAFE_NOTIN_DISTINCTON_CLAUSE (1 << 2)
59+
#define UNSAFE_NOTIN_PARTITIONBY_CLAUSE (1 << 3)
60+
#define UNSAFE_TYPE_MISMATCH (1 << 4)
61+
5562
/* results of subquery_is_pushdown_safe */
5663
typedef struct pushdown_safety_info
5764
{
58-
bool *unsafeColumns; /* which output columns are unsafe to use */
65+
unsigned char *unsafeFlags; /* bitmask of reasons why this target list
66+
* column is unsafe for qual pushdown, or 0 if
67+
* no reason. */
5968
bool unsafeVolatile; /* don't push down volatile quals */
6069
bool unsafeLeaky; /* don't push down leaky quals */
6170
} pushdown_safety_info;
6271

72+
/* Return type for qual_is_pushdown_safe */
73+
typedef enum pushdown_safe_type
74+
{
75+
PUSHDOWN_UNSAFE, /* unsafe to push qual into subquery */
76+
PUSHDOWN_SAFE, /* safe to push qual into subquery */
77+
PUSHDOWN_WINDOWCLAUSE_RUNCOND /* unsafe, but may work as WindowClause
78+
* run condition */
79+
} pushdown_safe_type;
80+
6381
/* These parameters are set by GUC */
6482
bool enable_geqo = false; /* just in case GUC doesn't set it */
6583
int geqo_threshold;
@@ -136,9 +154,9 @@ static void check_output_expressions(Query *subquery,
136154
static void compare_tlist_datatypes(List *tlist, List *colTypes,
137155
pushdown_safety_info *safetyInfo);
138156
static bool targetIsInAllPartitionLists(TargetEntry *tle, Query *query);
139-
static bool qual_is_pushdown_safe(Query *subquery, Index rti,
140-
RestrictInfo *rinfo,
141-
pushdown_safety_info *safetyInfo);
157+
static pushdown_safe_type qual_is_pushdown_safe(Query *subquery, Index rti,
158+
RestrictInfo *rinfo,
159+
pushdown_safety_info *safetyInfo);
142160
static void subquery_push_qual(Query *subquery,
143161
RangeTblEntry *rte, Index rti, Node *qual);
144162
static void recurse_push_qual(Node *setOp, Query *topquery,
@@ -2218,6 +2236,10 @@ find_window_run_conditions(Query *subquery, RangeTblEntry *rte, Index rti,
22182236
if (!IsA(wfunc, WindowFunc))
22192237
return false;
22202238

2239+
/* can't use it if there are subplans in the WindowFunc */
2240+
if (contain_subplans((Node *) wfunc))
2241+
return false;
2242+
22212243
prosupport = get_func_support(wfunc->winfnoid);
22222244

22232245
/* Check if there's a support function for 'wfunc' */
@@ -2500,13 +2522,14 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
25002522
/*
25012523
* Zero out result area for subquery_is_pushdown_safe, so that it can set
25022524
* flags as needed while recursing. In particular, we need a workspace
2503-
* for keeping track of unsafe-to-reference columns. unsafeColumns[i]
2504-
* will be set true if we find that output column i of the subquery is
2505-
* unsafe to use in a pushed-down qual.
2525+
* for keeping track of the reasons why columns are unsafe to reference.
2526+
* These reasons are stored in the bits inside unsafeFlags[i] when we
2527+
* discover reasons that column i of the subquery is unsafe to be used in
2528+
* a pushed-down qual.
25062529
*/
25072530
memset(&safetyInfo, 0, sizeof(safetyInfo));
2508-
safetyInfo.unsafeColumns = (bool *)
2509-
palloc0((list_length(subquery->targetList) + 1) * sizeof(bool));
2531+
safetyInfo.unsafeFlags = (unsigned char *)
2532+
palloc0((list_length(subquery->targetList) + 1) * sizeof(unsigned char));
25102533

25112534
/*
25122535
* If the subquery has the "security_barrier" flag, it means the subquery
@@ -2549,37 +2572,50 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
25492572
RestrictInfo *rinfo = (RestrictInfo *) lfirst(l);
25502573
Node *clause = (Node *) rinfo->clause;
25512574

2552-
if (!rinfo->pseudoconstant &&
2553-
qual_is_pushdown_safe(subquery, rti, rinfo, &safetyInfo))
2575+
if (rinfo->pseudoconstant)
25542576
{
2555-
/* Push it down */
2556-
subquery_push_qual(subquery, rte, rti, clause);
2577+
upperrestrictlist = lappend(upperrestrictlist, rinfo);
2578+
continue;
25572579
}
2558-
else
2580+
2581+
switch (qual_is_pushdown_safe(subquery, rti, rinfo, &safetyInfo))
25592582
{
2560-
/*
2561-
* Since we can't push the qual down into the subquery, check
2562-
* if it happens to reference a window function. If so then
2563-
* it might be useful to use for the WindowAgg's runCondition.
2564-
*/
2565-
if (!subquery->hasWindowFuncs ||
2566-
check_and_push_window_quals(subquery, rte, rti, clause,
2567-
&run_cond_attrs))
2568-
{
2583+
case PUSHDOWN_SAFE:
2584+
/* Push it down */
2585+
subquery_push_qual(subquery, rte, rti, clause);
2586+
break;
2587+
2588+
case PUSHDOWN_WINDOWCLAUSE_RUNCOND:
2589+
25692590
/*
2570-
* subquery has no window funcs or the clause is not a
2571-
* suitable window run condition qual or it is, but the
2572-
* original must also be kept in the upper query.
2591+
* Since we can't push the qual down into the subquery,
2592+
* check if it happens to reference a window function. If
2593+
* so then it might be useful to use for the WindowAgg's
2594+
* runCondition.
25732595
*/
2596+
if (!subquery->hasWindowFuncs ||
2597+
check_and_push_window_quals(subquery, rte, rti, clause,
2598+
&run_cond_attrs))
2599+
{
2600+
/*
2601+
* subquery has no window funcs or the clause is not a
2602+
* suitable window run condition qual or it is, but
2603+
* the original must also be kept in the upper query.
2604+
*/
2605+
upperrestrictlist = lappend(upperrestrictlist, rinfo);
2606+
}
2607+
break;
2608+
2609+
case PUSHDOWN_UNSAFE:
25742610
upperrestrictlist = lappend(upperrestrictlist, rinfo);
2575-
}
2611+
break;
25762612
}
25772613
}
25782614
rel->baserestrictinfo = upperrestrictlist;
25792615
/* We don't bother recomputing baserestrict_min_security */
25802616
}
25812617

2582-
pfree(safetyInfo.unsafeColumns);
2618+
pfree(safetyInfo.unsafeFlags);
25832619

25842620
/*
25852621
* The upper query might not use all the subquery's output columns; if
@@ -3517,13 +3553,13 @@ standard_join_search(PlannerInfo *root, int levels_needed, List *initial_rels)
35173553
*
35183554
* In addition, we make several checks on the subquery's output columns to see
35193555
* if it is safe to reference them in pushed-down quals. If output column k
3520-
* is found to be unsafe to reference, we set safetyInfo->unsafeColumns[k]
3521-
* to true, but we don't reject the subquery overall since column k might not
3522-
* be referenced by some/all quals. The unsafeColumns[] array will be
3523-
* consulted later by qual_is_pushdown_safe(). It's better to do it this way
3524-
* than to make the checks directly in qual_is_pushdown_safe(), because when
3525-
* the subquery involves set operations we have to check the output
3526-
* expressions in each arm of the set op.
3556+
* is found to be unsafe to reference, we set the reason for that inside
3557+
* safetyInfo->unsafeFlags[k], but we don't reject the subquery overall since
3558+
* column k might not be referenced by some/all quals. The unsafeFlags[]
3559+
* array will be consulted later by qual_is_pushdown_safe(). It's better to
3560+
* do it this way than to make the checks directly in qual_is_pushdown_safe(),
3561+
* because when the subquery involves set operations we have to check the
3562+
* output expressions in each arm of the set op.
35273563
*
35283564
* Note: pushing quals into a DISTINCT subquery is theoretically dubious:
35293565
* we're effectively assuming that the quals cannot distinguish values that
@@ -3571,9 +3607,9 @@ subquery_is_pushdown_safe(Query *subquery, Query *topquery,
35713607

35723608
/*
35733609
* If we're at a leaf query, check for unsafe expressions in its target
3574-
* list, and mark any unsafe ones in unsafeColumns[]. (Non-leaf nodes in
3575-
* setop trees have only simple Vars in their tlists, so no need to check
3576-
* them.)
3610+
* list, and mark any reasons why they're unsafe in unsafeFlags[].
3611+
* (Non-leaf nodes in setop trees have only simple Vars in their tlists,
3612+
* so no need to check them.)
35773613
*/
35783614
if (subquery->setOperations == NULL)
35793615
check_output_expressions(subquery, safetyInfo);
@@ -3644,9 +3680,9 @@ recurse_pushdown_safe(Node *setOp, Query *topquery,
36443680
*
36453681
* There are several cases in which it's unsafe to push down an upper-level
36463682
* qual if it references a particular output column of a subquery. We check
3647-
* each output column of the subquery and set unsafeColumns[k] to true if
3648-
* that column is unsafe for a pushed-down qual to reference. The conditions
3649-
* checked here are:
3683+
* each output column of the subquery and set flags in unsafeFlags[k] when we
3684+
* see that column is unsafe for a pushed-down qual to reference. The
3685+
* conditions checked here are:
36503686
*
36513687
* 1. We must not push down any quals that refer to subselect outputs that
36523688
* return sets, else we'd introduce functions-returning-sets into the
@@ -3670,7 +3706,9 @@ recurse_pushdown_safe(Node *setOp, Query *topquery,
36703706
* every row of any one window partition, and totally excluding some
36713707
* partitions will not change a window function's results for remaining
36723708
* partitions. (Again, this also requires nonvolatile quals, but
3673-
* subquery_is_pushdown_safe handles that.)
3709+
* subquery_is_pushdown_safe handles that.). Subquery columns marked as
3710+
* unsafe for this reason can still have WindowClause run conditions pushed
3711+
* down.
36743712
*/
36753713
static void
36763714
check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
@@ -3684,40 +3722,44 @@ check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
36843722
if (tle->resjunk)
36853723
continue; /* ignore resjunk columns */
36863724

3687-
/* We need not check further if output col is already known unsafe */
3688-
if (safetyInfo->unsafeColumns[tle->resno])
3689-
continue;
3690-
36913725
/* Functions returning sets are unsafe (point 1) */
36923726
if (subquery->hasTargetSRFs &&
3727+
(safetyInfo->unsafeFlags[tle->resno] &
3728+
UNSAFE_HAS_SET_FUNC) == 0 &&
36933729
expression_returns_set((Node *) tle->expr))
36943730
{
3695-
safetyInfo->unsafeColumns[tle->resno] = true;
3731+
safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_HAS_SET_FUNC;
36963732
continue;
36973733
}
36983734

36993735
/* Volatile functions are unsafe (point 2) */
3700-
if (contain_volatile_functions((Node *) tle->expr))
3736+
if ((safetyInfo->unsafeFlags[tle->resno] &
3737+
UNSAFE_HAS_VOLATILE_FUNC) == 0 &&
3738+
contain_volatile_functions((Node *) tle->expr))
37013739
{
3702-
safetyInfo->unsafeColumns[tle->resno] = true;
3740+
safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_HAS_VOLATILE_FUNC;
37033741
continue;
37043742
}
37053743

37063744
/* If subquery uses DISTINCT ON, check point 3 */
37073745
if (subquery->hasDistinctOn &&
3746+
(safetyInfo->unsafeFlags[tle->resno] &
3747+
UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
37083748
!targetIsInSortList(tle, InvalidOid, subquery->distinctClause))
37093749
{
37103750
/* non-DISTINCT column, so mark it unsafe */
3711-
safetyInfo->unsafeColumns[tle->resno] = true;
3751+
safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_DISTINCTON_CLAUSE;
37123752
continue;
37133753
}
37143754

37153755
/* If subquery uses window functions, check point 4 */
37163756
if (subquery->hasWindowFuncs &&
3757+
(safetyInfo->unsafeFlags[tle->resno] &
3758+
UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
37173759
!targetIsInAllPartitionLists(tle, subquery))
37183760
{
37193761
/* not present in all PARTITION BY clauses, so mark it unsafe */
3720-
safetyInfo->unsafeColumns[tle->resno] = true;
3762+
safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
37213763
continue;
37223764
}
37233765
}
@@ -3729,16 +3771,16 @@ check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
37293771
* subquery columns that suffer no type coercions in the set operation.
37303772
* Otherwise there are possible semantic gotchas. So, we check the
37313773
* component queries to see if any of them have output types different from
3732-
* the top-level setop outputs. unsafeColumns[k] is set true if column k
3733-
* has different type in any component.
3774+
* the top-level setop outputs. We set the UNSAFE_TYPE_MISMATCH bit in
3775+
* unsafeFlags[k] if column k has different type in any component.
37343776
*
37353777
* We don't have to care about typmods here: the only allowed difference
37363778
* between set-op input and output typmods is input is a specific typmod
37373779
* and output is -1, and that does not require a coercion.
37383780
*
37393781
* tlist is a subquery tlist.
37403782
* colTypes is an OID list of the top-level setop's output column types.
3741-
* safetyInfo->unsafeColumns[] is the result array.
3783+
* safetyInfo is the pushdown_safety_info to set unsafeFlags[] for.
37423784
*/
37433785
static void
37443786
compare_tlist_datatypes(List *tlist, List *colTypes,
@@ -3756,7 +3798,7 @@ compare_tlist_datatypes(List *tlist, List *colTypes,
37563798
if (colType == NULL)
37573799
elog(ERROR, "wrong number of tlist entries");
37583800
if (exprType((Node *) tle->expr) != lfirst_oid(colType))
3759-
safetyInfo->unsafeColumns[tle->resno] = true;
3801+
safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_TYPE_MISMATCH;
37603802
colType = lnext(colTypes, colType);
37613803
}
37623804
if (colType != NULL)
@@ -3816,28 +3858,28 @@ targetIsInAllPartitionLists(TargetEntry *tle, Query *query)
38163858
* 5. rinfo's clause must not refer to any subquery output columns that were
38173859
* found to be unsafe to reference by subquery_is_pushdown_safe().
38183860
*/
3819-
static bool
3861+
static pushdown_safe_type
38203862
qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo,
38213863
pushdown_safety_info *safetyInfo)
38223864
{
3823-
bool safe = true;
3865+
pushdown_safe_type safe = PUSHDOWN_SAFE;
38243866
Node *qual = (Node *) rinfo->clause;
38253867
List *vars;
38263868
ListCell *vl;
38273869

38283870
/* Refuse subselects (point 1) */
38293871
if (contain_subplans(qual))
3830-
return false;
3872+
return PUSHDOWN_UNSAFE;
38313873

38323874
/* Refuse volatile quals if we found they'd be unsafe (point 2) */
38333875
if (safetyInfo->unsafeVolatile &&
38343876
contain_volatile_functions((Node *) rinfo))
3835-
return false;
3877+
return PUSHDOWN_UNSAFE;
38363878

38373879
/* Refuse leaky quals if told to (point 3) */
38383880
if (safetyInfo->unsafeLeaky &&
38393881
contain_leaked_vars(qual))
3840-
return false;
3882+
return PUSHDOWN_UNSAFE;
38413883

38423884
/*
38433885
* Examine all Vars used in clause. Since it's a restriction clause, all
@@ -3864,7 +3906,7 @@ qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo,
38643906
*/
38653907
if (!IsA(var, Var))
38663908
{
3867-
safe = false;
3909+
safe = PUSHDOWN_UNSAFE;
38683910
break;
38693911
}
38703912

@@ -3876,7 +3918,7 @@ qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo,
38763918
*/
38773919
if (var->varno != rti)
38783920
{
3879-
safe = false;
3921+
safe = PUSHDOWN_UNSAFE;
38803922
break;
38813923
}
38823924

@@ -3886,15 +3928,26 @@ qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo,
38863928
/* Check point 4 */
38873929
if (var->varattno == 0)
38883930
{
3889-
safe = false;
3931+
safe = PUSHDOWN_UNSAFE;
38903932
break;
38913933
}
38923934

38933935
/* Check point 5 */
3894-
if (safetyInfo->unsafeColumns[var->varattno])
3936+
if (safetyInfo->unsafeFlags[var->varattno] != 0)
38953937
{
3896-
safe = false;
3897-
break;
3938+
if (safetyInfo->unsafeFlags[var->varattno] &
3939+
(UNSAFE_HAS_VOLATILE_FUNC | UNSAFE_HAS_SET_FUNC |
3940+
UNSAFE_NOTIN_DISTINCTON_CLAUSE | UNSAFE_TYPE_MISMATCH))
3941+
{
3942+
safe = PUSHDOWN_UNSAFE;
3943+
break;
3944+
}
3945+
else
3946+
{
3947+
/* UNSAFE_NOTIN_PARTITIONBY_CLAUSE is ok for run conditions */
3948+
safe = PUSHDOWN_WINDOWCLAUSE_RUNCOND;
3949+
/* don't break, we might find another Var that's unsafe */
3950+
}
38983951
}
38993952
}
39003953

0 commit comments

Comments
 (0)