When prepare_sort_from_pathkeys() cannot find a pathkey's expression
in the input plan's targetlist, it adds a resjunk entry computing an
EquivalenceClass member's expression. It copied that expression
as-is, without replacing lateral references with nestloop Params the
way build_path_tlist() does for the rest of the targetlist.
This matters when the sort key contains a PlaceHolderVar whose
expression has a lateral reference, and the sort is placed directly
atop the plan node that evaluates the PlaceHolderVar. The targetlist
already contains the PlaceHolderVar, so the sort key is considered
computable, but the entry added for it still has the outer relation's
Var inside the PlaceHolderVar. If the node is a join, setrefs.c fails
with "variable not found in subplan target lists". If it is a scan,
setrefs.c leaves the Var in place, and the executor fetches it from
the scan tuple, leading to wrong results.
To fix, pass root to prepare_sort_from_pathkeys() and apply
replace_nestloop_params() to the expression before adding it to the
targetlist.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49LWNm7_OxuW1V71wLi+Ev3KbfY1bMY36uydNKwSPnbtQ@mail.gmail.com
Backpatch-through: 14
When remove_useless_result_rtes() removes an RTE_RESULT, any
PlaceHolderVars that were to be evaluated there must be evaluated
somewhere else. If the RTE_RESULT is the nullable side of an outer
join, there is no other place that would preserve the PHVs' nulling
behavior, so find_dependent_phvs() refuses the removal if any such PHV
exists anywhere in the query. If the RTE_RESULT is a child of an
inner join, its PHVs can instead be evaluated at the join of all its
sibling rels, unless a sibling needs them before that join is formed.
find_dependent_phvs_in_jointree() checks for that by looking for such
PHVs in the join quals and LATERAL RTEs of the siblings.
That misses a PHV that is evaluated at some but not all of the sibling
rels and contains one of the RTE_RESULT's PHVs. Such a PHV can appear
anywhere in the query, such as in the targetlist. This can happen
when pulled-up expressions are wrapped in PHVs because of outer joins
or grouping sets. Once the RTE_RESULT is removed, the outer PHV needs
a value that is only available at a join above its own evaluation
level. The outer PHV's rel thus becomes laterally dependent on the
other sibling rels. If one of those rels laterally references it in
turn, we end up with a rel that is laterally dependent on itself,
causing an assertion failure in create_lateral_join_info().
To fix, also search the whole query for PHVs that are evaluated at
some but not all of the sibling rels, and refuse to remove the
RTE_RESULT if any of them contains one of its PHVs. An outer PHV that
is evaluated at all of the sibling rels is not a problem, since the
moved PHV is available there, so we don't block removal in that common
case.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_hLOsW8_SePp_3EmYTLX8Oc4zEZEvypyPRszpoMhDdGQ@mail.gmail.com
Backpatch-through: 14
make_outerjoininfo() decides whether outer join identity 3 applies
between an outer join and a lower one in its LHS by checking that the
join clause is strict for the lower join's RHS. It relied on the
clause then necessarily referencing that RHS. That is not so when the
strictness comes through a PlaceHolderVar: find_nonnullable_rels()
looks inside the PHV's expression and reports rels it references
laterally, while pull_varnos() reports only where the PHV is
evaluated.
In that case we marked the two joins as commutable although the clause
does not use the lower join's RHS at all. Joinrels for the same set
of base rels could then be built both with and without the upper
join's relid, leading to an assertion failure in
standard_join_search(). The bogus commutation also caused an extra
clone of the upper join's clause to be generated, which could be
applied at a higher join in addition to the clone already applied
below it. In non-assert builds, this could lead to planner errors
such as "variable not found in subplan target lists".
To fix, ignore rels that the clause is strict for but does not
reference, consistent with clause_relids treating a PHV as computed at
its ph_eval_at.
Back-patch to v16, where this commutation logic was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49xvh4-uPv3rxWkk_rtMT1ic+psHvEAxaFEXu2Rf-32Tw@mail.gmail.com
Backpatch-through: 16
Commit 767c59895 made set_append_rel_size() skip join quals that came
from above an outer join that can null the parent rel. Such quals are
of no use while generating paths for the child, and skipping them also
keeps them away from adjust_appendrel_attrs(), which cannot apply
nullingrels to a non-Var translation.
However, it identified such quals by looking at their clause_relids.
That misses a reference to the parent rel that is nulled by such an
outer join but appears only within a PlaceHolderVar, because
pull_varnos() reports only where the PHV is evaluated. Such a qual
could then reach adjust_appendrel_attrs(), which failed with "failed
to apply nullingrels to a non-Var" if the referenced output of a UNION
ALL member was not a plain Var.
To fix, also skip quals whose clause_relids overlap the parent rel's
lateral_referencers. A reference to the parent rel within a PHV that
is evaluated elsewhere is necessarily a lateral reference, so this
catches all such quals. These quals are no use for the child either,
since they can never be enforced by a scan of the parent rel; this is
the same reasoning join_clause_is_movable_to() uses.
Back-patch to v16, where commit 767c59895 was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_fzrrN9G6-wLwwrdM-n=fwj3Uhr1zkgj3o91EEC-CuEA@mail.gmail.com
Backpatch-through: 16
find_var_for_subquery_tle() searches a subquery rel's targetlist for a
Var referencing a given subquery output column, and asserted that any
Var it finds there belongs to that rel. That does not hold for an
appendrel child. set_append_rel_size() builds the child's targetlist
by translating the parent's, and as noted there the result can contain
arbitrary expressions. In particular, when a member of a LATERAL UNION
ALL subquery is pulled up and its output is a lateral reference, the
translated expression is a Var of the laterally referenced rel. If the
child is itself a subquery with sorted paths, convert_subquery_pathkeys()
would trip the Assert.
In non-assert builds, the function went on to compare such a Var's
varattno with the subquery column's resno, and so could match a Var of
an unrelated rel.
To fix, ignore Vars that do not belong to the subquery rel, the same
way set_rel_width() already does. Also correct the adjacent comment,
which claimed that the only non-Var entries are placeholders.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48fFiPi7Q5yJCoM_nAs47zd5XgvPHd9nZyM_ewq+k_jpQ@mail.gmail.com
Backpatch-through: 14
If a proposed join has lateral references to rels outside it,
join_is_legal() checks that those rels can still be joined to from
outside. To do that it collects the rels that must end up on the
inner side of an outer join with the proposed join, directly or
indirectly, and rejects the join if any of its lateral references
point to one of them.
That search followed only outer joins, from min_lefthand to
min_righthand. It failed to notice that a rel that laterally
references the proposed join, or any rel found by the search, cannot
supply a parameter to the proposed join either, and that the same
holds for rels on the inner side of an outer join with such a rel. As
a result, we could approve a join whose lateral references could never
be satisfied.
Such a join can never appear in a complete plan. Worse, as the
existing comment warns, its apparent legality could mislead the
clauseless-join heuristics into thinking that some other join rel need
not be formed. In particular, have_join_order_restriction() would not
force a clauseless outer join if one of its input rels appeared to
have a legal clause join elsewhere, and if that outer join was
required, we would fail with "failed to build any N-way joins".
To fix, also include in the search any input rel of the current join
search that laterally references a rel already found. The input rels
are taken from root->initial_rels rather than from the query's base
rels, because an input can be the joinrel of a sub-joinlist, as
happens for a full join or when from_collapse_limit or
join_collapse_limit prevents flattening. Such a joinrel is joined as
a unit, so it has to be included as a whole.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_p6iWbsjNcYpd6-6mWFnog3UJ2fZKrxWAQTsyNoyeQTw@mail.gmail.com
Backpatch-through: 14
When pull_up_sublinks() converts a sublink to an antijoin, it builds a
JoinExpr with no RTE, on the grounds that nothing above the join can
reference its RHS. That overlooks PlaceHolderVars. A PHV with no
Vars of its own is evaluated at its phrels, and for the outputs of a
pulled-up subquery phrels is all the relids in the subquery's
jointree, including the RHS of such a join.
A qual using the PHV thus appears to reference the RHS rels, and can
be pushed down into the RHS as a parameterized join clause. There it
has the opposite of the intended effect: outer rows that fail the qual
find no match and are emitted, giving wrong results. For an outer
join, including an antijoin made by reducing a left join, this cannot
happen because the join's relid is among the qual's relids. Before
v16, the qual's nullable_relids prevented it.
To fix, make a join RTE for an antijoin built from a sublink, so that
it has a relid like any other outer join. Semijoins are left as they
are: a qual evaluated within a semijoin's RHS does not change the
result.
Back-patch to v16. Before that, the qual's nullable_relids included
the antijoin's RHS, which kept the qual from being moved there; that
protection went away when nullable_relids was replaced by outer-join
relids.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_rEdcG89BgC-AO7b+Bf2z8w3976+4zP==Urt=CXB1hsg@mail.gmail.com
Backpatch-through: 16
When a join alias Var with nonempty varnullingrels expands to an
expression that cannot carry the nullingrels itself, such as the
RowExpr of a whole-row Var, add_nullingrels_if_needed() wraps it in a
PlaceHolderVar. The choice of the PHV's phrels had several problems.
First, phrels was taken from all Vars in the expression. If a LATERAL
subquery within the join has been pulled up, the expression can
contain lateral references to rels outside the join, so the PHV could
be evaluated above the outer join that is supposed to null it. This
gave wrong results, or "wrong phnullingrels" errors.
Second, for a variable-free expression we fell back to the join's
relids but removed the join's own outer-join relid. That leaves a set
spanning both sides of the outer join without the join itself, which
can make the PHV look nullable by that join and cause "wrong
phnullingrels" errors.
Third, that fallback raised "unsupported join alias expression" when
the Var is an outer reference from a subquery. The restriction is
unnecessary: every planner caller passes root->parse as the query, so
the Var always belongs to root->parse regardless of its varlevelsup.
To fix, restrict phrels to the rels within the join, and if nothing is
left, evaluate the PHV at the join including its outer-join relid, at
any varlevelsup. Add an assertion that the query is root->parse.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
When a query has aggregates or GROUP BY, parseCheckAggregates()
flattens join alias Vars in the grouping expressions, target list and
HAVING clause so that aliased and unaliased references compare equal.
The parser has no PlannerInfo, so this relies on every join alias
expression being able to carry varnullingrels without a PHV. That
doesn't hold for a whole-row Var of a join on the nullable side of an
outer join: it expands to a RowExpr, and pushing the nulling bits down
into its fields would turn a NULL row into ROW(NULL, ...). Such
queries therefore failed with "unsupported join alias expression".
To fix, leave a nulled whole-row join Var unexpanded when no root is
available. The planner expands it later and wraps it in a
PlaceHolderVar as before. Nothing is lost for GROUP BY matching: a
nulled whole-row Var can only be equal to the same Var, never to an
expression over the join's columns.
This has been broken since the introduction of varnullingrels in v16.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
pull_up_simple_subquery() rechecks is_simple_subquery() after pulling
up the subquery's own subqueries, but it flattened join alias Vars in
the subquery's targetlist only after that, and never in its quals. A
join alias Var within the subquery, such as a whole-row Var of a join,
can expand to an expression containing lateral references to the outer
query once the join's inputs have been pulled up, and until it is
flattened is_simple_subquery() cannot see them. We could thus pull up
a LATERAL subquery whose targetlist or quals reference rels outside
the lowest outer join above it. This leads to assertion failures in
distribute_qual_to_rels() or "wrong phnullingrels" errors, and in some
cases to wrong query results, where rows that should have been
preserved by an outer join above the subquery are discarded.
To fix, flatten join alias Vars in the targetlist, and in the jointree
quals if the subquery is LATERAL, before the recheck. If the recheck
fails, the flattened copy is discarded along with the rest of the
modified subquery, so no harm is done.
Back-patch to all supported branches, as this can lead to wrong query
results.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48GFZ=3Bjc1ug9JRsB0Qupg+CGcb9js5vmTM6kY2OGWOg@mail.gmail.com
Backpatch-through: 14
Since commit e28cf96e9, the copies of a query level's PlaceHolderVars
that were pushed down into its subqueries are preprocessed by that
level before its SubLinks are turned into SubPlans, so such a copy may
already contain SubPlans by the time the subquery is planned. That
commit taught build_subplan not to process the SubLinks of a PHV that
is passed down as a SubPlan argument, but overlooked that such a copy
can also appear within the arguments of an outer-level Aggref,
GroupingFunc, or ReturningExpr. The SubLinks in those arguments are
still processed when build_subplan collects the node, and that
processing descended into the already-preprocessed PHV and tripped the
no-SubPlan assertion in process_sublinks_mutator.
To fix, don't descend into PHVs when processing the SubLinks in such a
node's arguments. Any PHV found there is a pushed-down copy that its
owning level has already preprocessed.
Back-patch to v16, as with commit e28cf96e9.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com
Backpatch-through: 16
When a LATERAL UNION ALL subquery is flattened into an appendrel, a
child EquivalenceMember's expression can contain lateral references to
other relations. create_join_clause() builds a clause from such a
member with make_restrictinfo(), which computes clause_relids from the
expression, and so includes the relids of the lateral references along
with those of any outer joins that null them. Commit 03107b4ed made
sure that clause_relids include the child's relids, but it only ever
added relids.
This is a problem when a lateral reference is nullable. The child is
parameterized by its lateral_relids, which do not include the outer
joins that null the lateral references, so the clause fails the
join_clause_is_movable_into() assertion that
get_baserel_parampathinfo() applies to EC-derived clauses.
To fix, set a child clause's clause_relids from its members' em_relids
rather than from their expressions. This matches how
adjust_appendrel_attrs() computes clause_relids for a child's other
join clauses. The lateral references need not be included, since
every path for the child is parameterized by at least its
lateral_relids. Also update num_base_rels to match, which commit
03107b4ed neglected to do.
Back-patch to v16, where outer-join relids were added to
clause_relids. Before that, the relids of a lateral reference are
always part of the child's parameterization, so the problem does not
arise.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4-cRxpQNyQJW=PkDagSDGQ=3Rp2t_fSAyROKsDStjKHUQ@mail.gmail.com
Backpatch-through: 16
When a join has an alias, the parser marks every Var referenced
through that alias with the join's RT index as its syntactic referent
(varnosyn), even though varno points at the underlying base relation.
Since commit 2ebf25e7d, join removal strips the removed relids from
the query tree using ChangeVarNodes() with INVALID_VAR, which asserts
that the relid doesn't appear in any field identifying a single
relation. A varnosyn that names the removed join violates that, so a
query like
SELECT j.x FROM (t1 s(x) LEFT JOIN t2 ON s.x = t2.a) j;
with t2.a unique fails an assertion. In non-assert builds, varnosyn
is silently set to INVALID_VAR.
Such syntactic references are still valid after the removal: the join
is only taken out of the jointree, and its RTE remains in the
rangetable through to the finished plan. So, when deleting a relid,
leave varnosyn unchanged, as join removal did before 2ebf25e7d.
Back-patch to v16, as with commit 2ebf25e7d.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMbWs484SQSdzWTgZYKvwtuC6J4m7acw0LfBjp3aWPvav7gC_w@mail.gmail.com
Backpatch-through: 16
Commit e28cf96e9 preprocessed the copies of a query level's
PlaceHolderVars that were pushed down into its subqueries in a single
walk at the start of subquery_planner. That was wrong in two ways.
First, such copies can also be inserted later, when a join alias Var
of the outer level is expanded within a LATERAL subquery or a
SubLink's subselect and the alias expression contains a PHV. Those
copies were never preprocessed, so a SubLink within them survived into
the subquery's lateral references or into a SubPlan's argument list,
tripping an assertion in identify_current_nestloop_params or failing
in cost_qual_eval.
Second, when a pushed-down copy directly contains another PHV of the
same level, the walk preprocessed the inner one first and then the
outer one, so the outer one's preprocessing ran into the SubPlans just
created in the inner one and tripped an assertion in
flatten_join_alias_vars.
To fix, preprocess the copies right after join alias expansion
instead: within preprocess_expression, just before SubLinks are turned
into SubPlans, and for LATERAL subquery RTEs, right after their
aliases are flattened. Preprocessing a copy handles everything within
it, including any further copies nested inside its SubLinks, so the
walker no longer looks inside a copy.
Back-patch to v16, as with commit e28cf96e9.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH-acRnAk1=Ki4EiAJEt67+Rz=_jyg7pbP_c6BK4oHZ4Q@mail.gmail.com
Backpatch-through: 16
When a subquery references an output of another subquery that gets
pulled up, and that output must be wrapped in a PlaceHolderVar because
of an intermediate outer join, the PHV expression is pushed down into
the subquery. That copy is not preprocessed along with the outer
query's expressions, so the two copies can diverge. This used to be
harmless, but since commit 2ebf25e7d join removal edits the whole
query tree, walking into subqueries, and can trip an assert in
ChangeVarNodes if it removes a rel whose Var survives only in such a
copy.
To fix, preprocess these copies at their owning query level, early in
subquery_planner, before anything can consume them (in particular
before SubLinks are turned into SubPlans). This covers copies pushed
into both LATERAL subquery RTEs and SubLink subselects, and handles
nested copies innermost-first. extract_lateral_references no longer
preprocesses the copies it pulls out.
Correspondingly, the subquery's own processing must leave the contents
of an upper-level PHV alone, since the owning level has already
preprocessed them: eval_const_expressions returns such a PHV
unchanged, and flatten_join_alias_vars no longer recurses into it.
Back-patch to v16, as with commit 2ebf25e7d.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHewXN=kWGAXV537mKtSyBYobGdHhYJVDJJMXXZEmmPWE_zaPw@mail.gmail.com
Backpatch-through: 16
When reduce_outer_joins reduces an outer join to an antijoin, any IS
NULL qual on a Var from the antijoin's nullable side is necessarily
true. Previously, such quals were discarded later in
distribute_qual_to_rels, mainly to avoid bogus selectivity estimates.
But that discard was incomplete: the qual remained in the jointree,
while its Vars were not counted in attr_needed. Since commit
2ebf25e7d, join removal edits the jointree and expects it to contain
no other references to a removed rel, so it could remove a rel that
such a discarded qual still references, and then trip an assertion on
the qual's stale Var.
To fix, move this processing to an earlier phase: such quals are now
removed from the jointree by reduce_outer_joins itself. This way
later phases see a consistent query tree, and
check_redundant_nullability_qual is no longer needed, so remove it.
Back-patch to v16, as with commit 2ebf25e7d.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHewXNk8b0TsSy4dL=CO7FXL2W3WBm0BcdP-zwNJePa-Qj4HzA@mail.gmail.com
Backpatch-through: 16
analyzejoins.c decided which joins could be dropped by consulting the
planner's derived data structures, but then implemented the removal
by updating those structures in-place. That is a lot of fiddly work,
and nothing keeps it in step with the rest of the planner:
remove_leftjoinrel_from_query only bothered to update "parts of the
planner's data structures that will actually be consulted later", with
no good way to know what those are. Bug #19560 is one consequence.
In that report, removing a join leaves an EquivalenceClass that now
gives rise to a base restriction clause, but base restriction clauses
have already been generated and nothing reconsiders them, so the WHERE
condition disappears from the plan and we return wrong answers.
The self-join elimination code has the same design and the same type
of hazard. We have seen many related bugs over the years too, so it's
time to do something drastic.
To fix, do the removals by editing root->parse->jointree (which is a
far simpler and more stable representation than the derived data),
and then have query_planner() discard everything it computed from the
jointree and derive it over again. This requires quite a bit less
code, and doesn't require touching analyzejoins.c every time we change
the data derived by query_planner(). For typical cases it can actually
save a bit of planning time, though in cases where we have to iterate
the derivation loop many times it does add some time.
reduce_unique_semijoins() gets the same treatment: rather than deleting
the semijoin's SpecialJoinInfo and relying on the jointree not being
consulted again, it now changes the JoinExpr's jointype to JOIN_INNER
and recalculates everything.
Some plans change in the join regression test. Qual evaluation order
shifts in a few cases, because the conditions now reach later planning
in jointree order rather than in whatever order the removal code
re-distributed them. A few plans improve, since the rebuilt relation
targetlists no longer carry columns that only a removed join needed.
We also detect a constant-false filter condition whose test used to
carry a FIXME label. One plan gets marginally worse, because the old
code recomputed attr_needed from equivalence classes after a join
removal; that is more accurate than what deconstruct_jointree()
derives from the original clauses, but we no longer do that. Making
that recomputation happen anyway could be worth doing, but it should
be considered independently and perhaps implemented differently.
Back-patch to v16, on the grounds that the introduction of
varnullingrels in v16 made the old approach significantly more complex
and bug-prone; notably, bug #19560 does not manifest before v16.
In released branches, do not remove externally-visible fixup
functions such as remove_join_clause_from_rels, in case any
extensions are relying on them; but they're no longer used by core
code. But we must nonetheless break API/ABI for remove_useless_joins,
reduce_unique_semijoins, and remove_useless_self_joins, as those now
have different outputs and very different behavior than before.
It seems unlikely that any extensions are calling those; but just in
case, make the breakage more obvious by renaming remove_useless_joins
to remove_useless_outer_joins, which is a more sensible name for it
anyway since the addition of remove_useless_self_joins.
Full disclosure: initial drafts of this patch were made with
Claude Opus 4.8.
Bug: #19560
Reported-by: Orestis Markou <orestis@orestis.gr>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Thom Brown <thom@linux.com>
Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com>
Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us
Backpatch-through: 16
query_is_distinct_for() treated a subquery's DISTINCT ON clause as
proof that its output is unique over the DISTINCT ON columns, even if
the targetlist contains set-returning functions. That's not true:
when the query has an ORDER BY, the planner postpones evaluation of
SRFs that are not DISTINCT ON or ORDER BY columns until after the
Unique step, so the subquery can produce duplicates of the DISTINCT ON
columns. Relying on this bogus uniqueness proof allowed join removal
and unique-inner joins to produce wrong results.
Plain DISTINCT is not affected, since all tlist columns are DISTINCT
columns there, and so any SRFs get expanded before the Unique step.
To fix, make query_supports_distinctness() and query_is_distinct_for()
refuse to prove distinctness via DISTINCT ON if the targetlist
contains any SRFs. This is more conservative than necessary, since
the SRFs are only postponed when there is an ORDER BY and none of them
appear in a sort/group column, but it doesn't seem worth the trouble
to check that precisely.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMbWs4-hfd1Pyy_zBejsVUSy-3dx16rz2hgUakkKnAg3qg2q=Q@mail.gmail.com
Backpatch-through: 14
find_dependent_phvs() and find_dependent_phvs_in_jointree() decide
whether a PlaceHolderVar depends on the RTE_RESULT rel we're
considering removing by comparing the PHV's phrels to a singleton set
containing that rel's RT index, reasoning that if phrels contains any
other relid bits then those define an appropriate place where we can
evaluate the PHV. But since this code was originally written, we've
redefined phrels to include outer-join relids, and that breaks this
logic, potentially allowing us to remove an RTE_RESULT that leaves no
valid place to evaluate the PHV. The planner doesn't throw an error
when that happens, but it does produce an incorrect plan that will not
replace the PHV's value with NULL when needed.
In the known test case for this bug, the "extra" OJ relid is one that
we've actually decided to remove but haven't yet cleaned out of the
query's PHVs. It's not entirely clear though that that would always
be the case. Let's restore this code to the way it was designed to
work, by considering only base relids within the PHV's phrels.
Bug: #19553
Reported-by: Viktor Leis <leis@in.tum.de>
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Co-authored-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19553-4561747f93f368a7@postgresql.org
Backpatch-through: 16
The original implementation of remove_rel_from_restrictinfo()
thought it could skate by with removing no-longer-valid relid
bits from only the clause_relids and required_relids fields.
This is quite bogus, although somehow we had not run across a
counterexample before now. At minimum, the left_relids and
right_relids fields need to be fixed because they will be
examined later by clause_sides_match_join(). But it seems
pretty foolish not to fix all the relid fields, so do that.
This needs to be back-patched as far as v16, because the
bug report shows a planner failure that does not occur
before v16. I'm a little nervous about back-patching,
because this could cause unexpected plan changes due to
opening up join possibilities that were rejected before.
But it's hard to argue that this isn't a regression. Also,
the fact that this changes no existing regression test results
suggests that the scope of changes may be fairly narrow.
I'll refrain from back-patching further though, since no
adverse effects have been demonstrated in older branches.
Bug: #19460
Reported-by: François Jehl <francois.jehl@pigment.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/19460-5625143cef66012f@postgresql.org
Backpatch-through: 16
When adjust_appendrel_attrs translates a Var referencing a parent
relation into a Var referencing a child relation, it propagates
varnullingrels from the parent Var to the translated Var. Previously,
the code simply overwrote the translated Var's varnullingrels with
those of the parent.
This was incorrect because the translated Var might already possess
nonempty varnullingrels. This happens, for example, when a LATERAL
subquery within a UNION ALL references a Var from the nullable side of
an outer join. In such cases, the translated Var correctly carries
the outer join's relid in its varnullingrels. Overwriting these bits
with the parent Var's set caused the planner to lose track of the fact
that the Var could be nulled by that outer join.
In the reported case, because the underlying column had a NOT NULL
constraint, the planner incorrectly deduced that the Var could never
be NULL and discarded essential IS NOT NULL filters. This led to
incorrect query results where NULL rows were returned instead of being
filtered out.
To fix, use bms_add_members to merge the parent Var's varnullingrels
into the translated Var's existing set, preserving both sources of
nullability.
Back-patch to v16. Although the reported case does not seem to cause
problems in v16, leaving incorrect varnullingrels in the tree seems
like a trap for the unwary.
Bug: #19412
Reported-by: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19412-1d0318089b86859e@postgresql.org
Backpatch-through: 16
When looking up statistical data about an expression, we do not need
to concern ourselves with the outer joins that could null the
Vars/PHVs contained in the expression. Accounting for nullingrels in
the expression could cause estimate_num_groups to count the same Var
multiple times if it's marked with different nullingrels. This is
incorrect, and could lead to "ERROR: corrupt MVNDistinct entry" when
searching for multivariate n-distinct.
Furthermore, the nullingrels could prevent us from matching an
expression to expressional index columns or to the expressions in
extended statistics, leading to inaccurate estimates.
To fix, strip out all the nullingrels from the expression before we
look up statistical data about it. There is one ensuing plan change
in the regression tests, but it looks reasonable and does not
compromise its original purpose.
This patch could result in plan changes, but it fixes an actual bug,
so back-patch to v16 where the outer-join-aware-Var infrastructure was
introduced.
Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4-2Z4k+nFTiZe0Qbu5n8juUWenDAtMzi98bAZQtwHx0-w@mail.gmail.com
For an inner_unique join, we always assume that the executor will stop
scanning for matches after the first match. Therefore, for a mergejoin
that is inner_unique and whose mergeclauses are sufficient to identify a
match, we set the skip_mark_restore flag to true, indicating that the
executor need not do mark/restore calls. However, merge-right-anti-join
did not get this memo and continues scanning the inner side for matches
after the first match. If there are duplicates in the outer scan, we
may incorrectly skip matching some inner tuples, which can lead to wrong
results.
Here we fix this issue by ensuring that merge-right-anti-join also
advances to next outer tuple after the first match in inner_unique
cases. This also saves cycles by avoiding unnecessary scanning of inner
tuples after the first match.
Although hash-right-anti-join does not suffer from this wrong results
issue, we apply the same change to it as well, to help save cycles for
the same reason.
Per bug #18522 from Antti Lampinen, and bug #18526 from Feliphe Pozzer.
Back-patch to v16 where right-anti-join was introduced.
Author: Richard Guo
Discussion: https://postgr.es/m/18522-c7a8956126afdfd0@postgresql.org
04e72ed617 pushed the skip fetch optimization (allowing bitmap heap
scans to operate like index-only scans if none of the underlying data is
needed) into heap AM implementations of bitmap table scan callbacks.
04e72ed617 added an assert that all tuples in blocks eligible for the
optimization had been NULL-filled and emitted by the end of the scan.
This assert is incorrect when not all tuples need be scanned to execute
the query; for example: a join in which not all inner tuples need to be
scanned before skipping to the next outer tuple.
Remove the assert and reset the field on which it previously asserted to
avoid incorrectly emitting NULL-filled tuples from a previous scan on
rescan.
Author: Melanie Plageman
Reviewed-by: Tomas Vondra, Michael Paquier, Alvaro Herrera
Reported-by: Melanie Plageman
Reproduced-by: Tomas Vondra, Richard Guo
Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com
Replace (expr op C1) OR (expr op C2) ... with expr op ANY(ARRAY[C1, C2, ...])
on the preliminary stage of optimization when we are still working with the
expression tree.
Here Cn is a n-th constant expression, 'expr' is non-constant expression, 'op'
is an operator which returns boolean result and has a commuter (for the case
of reverse order of constant and non-constant parts of the expression,
like 'Cn op expr').
Sometimes it can lead to not optimal plan. This is why there is a
or_to_any_transform_limit GUC. It specifies a threshold value of length of
arguments in an OR expression that triggers the OR-to-ANY transformation.
Generally, more groupable OR arguments mean that transformation will be more
likely to win than to lose.
Discussion: https://postgr.es/m/567ED6CA.2040504%40sigaev.ru
Author: Alena Rybakina <lena.ribackina@yandex.ru>
Author: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Reviewed-by: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Ranier Vilela <ranier.vf@gmail.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Jian He <jian.universality@gmail.com>
This commit introduces a new field 'sublevels_up' in ReplaceVarnoContext,
and enhances replace_varno_walker() to:
1) recurse into subselects with sublevels_up increased, and
2) perform the replacement only when varlevelsup is equal to sublevels_up.
This commit also fixes some outdated comments. And besides adding relevant
test cases, it makes some unification over existing SJE test cases.
Discussion: https://postgr.es/m/CAMbWs4-%3DPO6Mm9gNnySbx0VHyXjgnnYYwbN9dth%3DTLQweZ-M%2Bg%40mail.gmail.com
Author: Richard Guo
Reviewed-by: Andrei Lepikhov, Alexander Korotkov
For ANY-SUBLINK, we adopted a two-stage pull-up approach to handle
different types of scenarios. In the first stage, the sublink is pulled up
as a subquery. Because of this, when writing this code, we did not have
the ability to perform lateral joins, and therefore, we were unable to
pull up Var with varlevelsup=1. Now that we have the ability to use
lateral joins, we can eliminate this limitation.
Author: Andy Fan <zhihui.fan1213@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Alena Rybakina <lena.ribackina@yandex.ru>
Reviewed-by: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
When removing a useless join, we'd remove PHVs that are not used at join
partner rels or above the join. A PHV that references the join's relid
in ph_eval_at is logically "above" the join and thus should not be
removed. We have the following check for that:
!bms_is_member(ojrelid, phinfo->ph_eval_at)
However, in the case of SJE removing a useless inner join, 'ojrelid' is
set to -1, which would trigger the "negative bitmapset member not
allowed" error in bms_is_member().
Fix it by skipping examining ojrelid for inner joins in this check.
Reported-by: Zuming Jiang
Bug: #18260
Discussion: https://postgr.es/m/18260-1b6a0c4ae311b837%40postgresql.org
Author: Richard Guo
Reviewed-by: Andrei Lepikhov
The code for wrapping subquery output expressions in PlaceHolderVars
believed that if the expression already was a PlaceHolderVar, it was
never necessary to wrap that in another one. That's wrong if the
expression is underneath an outer join and involves a lateral
reference to outside that scope: failing to add an additional PHV
risks evaluating the expression at the wrong place and hence not
forcing it to null when the outer join should do so. This is an
oversight in commit 9e7e29c75, which added logic to forcibly wrap
lateral-reference Vars in PlaceHolderVars, but didn't see that the
adjacent case for PlaceHolderVars needed the same treatment.
The test case we have for this doesn't fail before 4be058fe9, but now
that I see the problem I wonder if it is possible to demonstrate
related errors before that. That's moot though, since all such
branches are out of support.
Per bug #18284 from Holger Reise. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/18284-47505a20c23647f8@postgresql.org
The target relation for INSERT/UPDATE/DELETE/MERGE has a different behavior
than other relations in EvalPlanQual() and RETURNING clause. This is why we
forbid target relation to be either source or target relation in SJE.
It's not clear if we could ever support this.
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/b9e8f460-f9a6-0e9b-e8ba-60d59f0bc22c%40gmail.com
When SJE uses RelOptInfo.unique_for_rels cache, it passes filtered quals to
innerrel_is_unique_ext(). That might lead to an invalid match to cache entries
made by previous non self-join checking calls. Add UniqueRelInfo.self_join
flag to prevent such cases. Also, fix that SJE should require a strict match
of outerrelids to make sure UniqueRelInfo.extra_clauses are valid.
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/4788f781-31bd-9796-d7d6-588a751c8787%40gmail.com
When the SJE code handles the transfer of qual clauses from the removed
relation to the remaining one, it replaces the Vars of the removed
relation with the Vars of the remaining relation for each clause, and
then reintegrates these clauses into the appropriate restriction or join
clause lists, while attempting to avoid duplicates.
However, the code compares RestrictInfo->clause to determine if two
clauses are duplicates. This is just flat wrong. Two RestrictInfos
with the same clause can have different required_relids,
incompatible_relids, is_pushed_down, and so on. This can cause qual
clauses to be mistakenly omitted, leading to wrong results.
This patch fixes it by comparing the entire RestrictInfos not just their
clauses ignoring 'rinfo_serial' field (otherwise almost all RestrictInfos will
be unique). Making 'rinfo_serial' equal_ignore would break other code. This
is why this commit implements our own comparison function for checking the
equality of RestrictInfos.
Reported-by: Zuming Jiang
Bug: #18261
Discussion: https://postgr.es/m/18261-2a75d748c928609b%40postgresql.org
Author: Richard Guo
There are a lot of situations when we share the same pointer to a Bitmapset
structure across different places. In order to evade undesirable side effects
replace_relid() function should always return a copy.
Reported-by: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4_wJthNtYBL%2BSsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw%40mail.gmail.com
Reviewed-by: Richard Guo, Andres Freund, Ashutosh Bapat, Andrei Lepikhov
Self-join removal appears to be safe to apply with placeholder variables
as long as we handle PlaceHolderVar in replace_varno_walker() and replace
relid in phinfo->ph_lateral.
Discussion: https://postgr.es/m/18187-831da249cbd2ff8e%40postgresql.org
Author: Richard Guo
Reviewed-by: Andrei Lepikhov
It seems that a PHV evaluated/needed at or below the self join should not have
a problem if we remove the self join. But this requires further investigation.
For now, we just do not remove self joins if the rel to be removed is laterally
referenced by PHVs.
Discussion: https://postgr.es/m/CAMbWs4-ns73VF9gi37q61G3dS6Xuos+HtryMaBh37WQn=BsaJw@mail.gmail.com
Author: Richard Guo
We can simplify FieldSelect on a whole-row Var into a plain Var
for the selected field. However, we should copy the whole-row Var's
varnullingrels when we do so, because the new Var is clearly nullable
by exactly the same rels as the original. Failure to do this led to
errors like "wrong varnullingrels (b) (expected (b 3)) for Var 2/2".
Richard Guo, per bug #18184 from Marian Krucina. Back-patch to
v16 where varnullingrels was introduced.
Discussion: https://postgr.es/m/18184-5868dd258782058e@postgresql.org
recurse_set_operations() uses the parse tree for the group number estimation,
because of the "varno 0" hack. At the same time 2489d76c49 made root->parse
and corresponding parent_root->simple_rte_array[]->subquery distinct copies
of the parse tree, while d3d55ce571 introduced self-join removal replacing
relid of removed relation only in one of the copies.
The present commit fixes this bug by making recurse_set_operations() call
estimate_num_groups() with the copy of the parse tree processed by self-join
removal.
In future, we may think about maintaining just one copy of the parse tree
and/or keeping removed relids as aliases.
Reported-by: Zuming Jiang
Bug: #18170
Discussion: https://postgr.es/m/flat/18170-f1d17bf9a0d58b24%40postgresql.org
Author: Richard Guo, Alexander Korotkov
Reviewed-by: Andrei Lepikhov
The Self Join Elimination (SJE) feature removes an inner join of a plain table
to itself in the query tree if is proved that the join can be replaced with
a scan without impacting the query result. Self join and inner relation are
replaced with the outer in query, equivalence classes, and planner info
structures. Also, inner restrictlist moves to the outer one with removing
duplicated clauses. Thus, this optimization reduces the length of the range
table list (this especially makes sense for partitioned relations), reduces
the number of restriction clauses === selectivity estimations, and potentially
can improve total planner prediction for the query.
The SJE proof is based on innerrel_is_unique machinery.
We can remove a self-join when for each outer row:
1. At most one inner row matches the join clause.
2. Each matched inner row must be (physically) the same row as the outer one.
In this patch we use the next approach to identify a self-join:
1. Collect all merge-joinable join quals which look like a.x = b.x
2. Add to the list above the baseretrictinfo of the inner table.
3. Check innerrel_is_unique() for the qual list. If it returns false, skip
this pair of joining tables.
4. Check uniqueness, proved by the baserestrictinfo clauses. To prove
the possibility of self-join elimination inner and outer clauses must have
an exact match.
The relation replacement procedure is not trivial and it is partly combined
with the one, used to remove useless left joins. Tests, covering this feature,
were added to join.sql. Some regression tests changed due to self-join removal
logic.
Discussion: https://postgr.es/m/flat/64486b0b-0404-e39e-322d-0801154901f3%40postgrespro.ru
Author: Andrey Lepikhov, Alexander Kuzmenkov
Reviewed-by: Tom Lane, Robert Haas, Andres Freund, Simon Riggs, Jonathan S. Katz
Reviewed-by: David Rowley, Thomas Munro, Konstantin Knizhnik, Heikki Linnakangas
Reviewed-by: Hywel Carver, Laurenz Albe, Ronan Dunklau, vignesh C, Zhihong Yu
Reviewed-by: Greg Stark, Jaime Casanova, Michał Kłeczek, Alena Rybakina
Reviewed-by: Alexander Korotkov
An outer join cannot be formed using an input path that is parameterized
by a value that is supposed to be nulled by the outer join. This is
obviously nonsensical, and it could lead to a bad plan being selected;
although currently it seems that we'll hit various sanity-check
assertions first.
I think that such cases were formerly prevented by the delay_upper_joins
mechanism, but now that that's gone we need an explicit check.
(Perhaps we should avoid generating baserel paths that could
lead to this situation in the first place; but it seems like
having a defense at the join level would be a good idea anyway.)
Richard Guo and Tom Lane, per report from Jaime Casanova
Discussion: https://postgr.es/m/CAJKUy5g2uZRrUDZJ8p-=giwcSHVUn0c9nmdxPSY0jF0Ov8VoEA@mail.gmail.com
I removed the delay_upper_joins mechanism in commit b448f1c8d,
reasoning that it was only needed when we have a single-table
(SELECT ... WHERE) as the immediate RHS child of a left join,
and we could get rid of that by hoisting the WHERE condition into
the parent join's quals. However that new code missed a case:
we could have "foo LEFT JOIN ((SELECT ... WHERE) LEFT JOIN bar)",
and if the two left joins can be commuted then we now have the
problematic query shape. We can fix this too easily enough,
by allowing the syntactically-lower left join to pass through
its parent qual location pointer recursively. That lets
prepjointree.c discard the SELECT by temporarily hoisting the
WHERE condition into the ancestor join's qual.
Per bug #17978 from Zuming Jiang.
Discussion: https://postgr.es/m/17978-12f3d93a55297266@postgresql.org
This avoids an assertion failure when outer joins are rearranged
per identity 3. Listing only the baserels from a PlaceHolderVar's
ph_lateral set should be enough to ensure that the required values
are available when we need to compute the PHV --- it's what we
did before inventing nullingrel sets, after all. It's a bit
unsatisfying; but with beta2 hard upon us, there's not time to
look for an aesthetically cleaner fix.
Richard Guo and Tom Lane
Discussion: https://postgr.es/m/CAMbWs48Jcw-NvnxT23WiHP324wG44DvzcH1j4hc0Zn+3sR9cfg@mail.gmail.com
It turns out that the fixes we applied in commits bfd332b3f
and 63e4f13d2 were not nearly enough to solve the problem.
We'd focused narrowly on subquery RTEs with lateral references,
but lateral references can occur in several other RTE kinds
such as function RTEs. Putting the same hack into half a dozen
code paths seems quite unattractive. Hence, revert the code changes
(but not the test cases) from those commits and instead solve it
centrally in identify_current_nestloop_params(), as Richard proposed
originally. This is a bit annoying because it could mask erroneous
nullingrels in nestloop params that are generated from non-LATERAL
parameterized paths; but on balance I don't see a better way.
Maybe at some future time we'll be motivated to find a more rigorous
approach to nestloop params, but that's not happening for beta2.
Richard Guo and Tom Lane
Discussion: https://postgr.es/m/CAMbWs48Jcw-NvnxT23WiHP324wG44DvzcH1j4hc0Zn+3sR9cfg@mail.gmail.com
