Обсуждение: pgsql: Make Vars be outer-join-aware.
Make Vars be outer-join-aware. Traditionally we used the same Var struct to represent the value of a table column everywhere in parse and plan trees. This choice predates our support for SQL outer joins, and it's really a pretty bad idea with outer joins, because the Var's value can depend on where it is in the tree: it might go to NULL above an outer join. So expression nodes that are equal() per equalfuncs.c might not represent the same value, which is a huge correctness hazard for the planner. To improve this, decorate Var nodes with a bitmapset showing which outer joins (identified by RTE indexes) may have nulled them at the point in the parse tree where the Var appears. This allows us to trust that equal() Vars represent the same value. A certain amount of klugery is still needed to cope with cases where we re-order two outer joins, but it's possible to make it work without sacrificing that core principle. PlaceHolderVars receive similar decoration for the same reason. In the planner, we include these outer join bitmapsets into the relids that an expression is considered to depend on, and in consequence also add outer-join relids to the relids of join RelOptInfos. This allows us to correctly perceive whether an expression can be calculated above or below a particular outer join. This change affects FDWs that want to plan foreign joins. They *must* follow suit when labeling foreign joins in order to match with the core planner, but for many purposes (if postgres_fdw is any guide) they'd prefer to consider only base relations within the join. To support both requirements, redefine ForeignScan.fs_relids as base+OJ relids, and add a new field fs_base_relids that's set up by the core planner. Large though it is, this commit just does the minimum necessary to install the new mechanisms and get check-world passing again. Follow-up patches will perform some cleanup. (The README additions and comments mention some stuff that will appear in the follow-up.) Patch by me; thanks to Richard Guo for review. Discussion: https://postgr.es/m/830269.1656693747@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/2489d76c4906f4461a364ca8ad7e0751ead8aa0d Modified Files -------------- contrib/postgres_fdw/deparse.c | 12 +- contrib/postgres_fdw/postgres_fdw.c | 12 +- doc/src/sgml/fdwhandler.sgml | 11 + src/backend/commands/explain.c | 2 +- src/backend/executor/execScan.c | 2 +- src/backend/nodes/makefuncs.c | 10 +- src/backend/nodes/nodeFuncs.c | 3 +- src/backend/nodes/queryjumblefuncs.c | 5 + src/backend/optimizer/README | 433 ++++++++-- src/backend/optimizer/geqo/geqo_eval.c | 2 +- src/backend/optimizer/path/allpaths.c | 36 +- src/backend/optimizer/path/clausesel.c | 45 +- src/backend/optimizer/path/costsize.c | 8 + src/backend/optimizer/path/equivclass.c | 132 ++- src/backend/optimizer/path/indxpath.c | 9 +- src/backend/optimizer/path/joinpath.c | 65 +- src/backend/optimizer/path/joinrels.c | 30 +- src/backend/optimizer/path/tidpath.c | 1 + src/backend/optimizer/plan/analyzejoins.c | 61 +- src/backend/optimizer/plan/createplan.c | 17 +- src/backend/optimizer/plan/initsplan.c | 1121 ++++++++++++++++++++------ src/backend/optimizer/plan/planner.c | 7 +- src/backend/optimizer/plan/setrefs.c | 234 ++++-- src/backend/optimizer/prep/prepjointree.c | 544 +++++++------ src/backend/optimizer/util/appendinfo.c | 59 +- src/backend/optimizer/util/clauses.c | 6 +- src/backend/optimizer/util/joininfo.c | 19 +- src/backend/optimizer/util/orclauses.c | 4 + src/backend/optimizer/util/pathnode.c | 15 +- src/backend/optimizer/util/placeholder.c | 123 ++- src/backend/optimizer/util/relnode.c | 389 +++++++-- src/backend/optimizer/util/restrictinfo.c | 93 ++- src/backend/optimizer/util/var.c | 252 +++++- src/backend/parser/analyze.c | 3 +- src/backend/parser/parse_agg.c | 8 +- src/backend/parser/parse_clause.c | 235 ++++-- src/backend/parser/parse_coerce.c | 2 +- src/backend/parser/parse_expr.c | 5 + src/backend/parser/parse_relation.c | 40 +- src/backend/parser/parse_target.c | 2 +- src/backend/rewrite/rewriteManip.c | 227 +++++- src/backend/utils/adt/selfuncs.c | 2 +- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 8 + src/include/nodes/pathnodes.h | 216 +++-- src/include/nodes/plannodes.h | 4 +- src/include/nodes/primnodes.h | 10 + src/include/optimizer/optimizer.h | 2 +- src/include/optimizer/pathnode.h | 4 +- src/include/optimizer/placeholder.h | 5 +- src/include/optimizer/prep.h | 3 +- src/include/optimizer/restrictinfo.h | 3 + src/include/parser/parse_node.h | 8 + src/include/parser/parse_relation.h | 3 +- src/include/rewrite/rewriteManip.h | 7 + src/test/regress/expected/aggregates.out | 20 +- src/test/regress/expected/join.out | 163 +++- src/test/regress/expected/partition_join.out | 14 +- src/test/regress/sql/aggregates.sql | 14 +- src/test/regress/sql/join.sql | 72 ++ 60 files changed, 3878 insertions(+), 966 deletions(-)
Hi Tom,
On Tue, 31 Jan 2023 at 05:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Make Vars be outer-join-aware.
>
Since this commit, my test setup is triggering asserts every few minutes.
The repro SQL itself doesn't make much sense, but it's the smallest I could
narrow it down to:
create table t1(a int);
create table t2(a int);
SELECT table_catalog AS c2
FROM public.t1
INNER JOIN (t2
LEFT JOIN information_schema.column_udt_usage ON NULL)
ON NULL;
Sample backtrace (can provide full backtrace if it helps):
Core was generated by `postgres: 8c1cd726c5@master@sqith: u76 postgres
127.0.0.1(55412) SELECT '.
Program terminated with signal SIGABRT, Aborted.
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f4e32fbf859 in __GI_abort () at abort.c:79
#2 0x000055bc9fb256bf in ExceptionalCondition (
conditionName=0x55bc9fcdaaf1 "phv->phnullingrels == NULL",
fileName=0x55bc9fcda72f "setrefs.c", lineNumber=2208) at assert.c:66
#3 0x000055bc9f8147a4 in fix_scan_expr_mutator (node=0x55bca21c78b0,
context=0x7fff72760960) at setrefs.c:2208
#4 0x000055bc9f768081 in expression_tree_mutator_impl (node=0x55bca21c9dc0,
mutator=0x55bc9f8144a2 <fix_scan_expr_mutator>, context=0x7fff72760960)
at nodeFuncs.c:3051
#5 0x000055bc9f81482c in fix_scan_expr_mutator (node=0x55bca21c9dc0,
context=0x7fff72760960) at setrefs.c:2217
#6 0x000055bc9f76848c in expression_tree_mutator_impl (node=0x55bca21c9e10,
mutator=0x55bc9f8144a2 <fix_scan_expr_mutator>, context=0x7fff72760960)
at nodeFuncs.c:3137
#7 0x000055bc9f81482c in fix_scan_expr_mutator (node=0x55bca21c9e10,
context=0x7fff72760960) at setrefs.c:2217
#8 0x000055bc9f814473 in fix_scan_expr (root=0x55bca217e358,
node=0x55bca21c9e10, rtoffset=1, num_exec=0) at setrefs.c:2127
TRAP: failed Assert("phv->phnullingrels == NULL"), File: "setrefs.c",
Line: 2208, PID: 1045580
Thanks to SQLSmith / SQLReduce for help on this.
-
Robins Tharakan
Amazon Web Services
Robins Tharakan <tharakan@gmail.com> writes:
> Since this commit, my test setup is triggering asserts every few minutes.
> The repro SQL itself doesn't make much sense, but it's the smallest I could
> narrow it down to:
> create table t1(a int);
> create table t2(a int);
> SELECT table_catalog AS c2
> FROM public.t1
> INNER JOIN (t2
> LEFT JOIN information_schema.column_udt_usage ON NULL)
> ON NULL;
Thanks, I'll take a look.
regards, tom lane
Robins Tharakan <tharakan@gmail.com> writes:
> Since this commit, my test setup is triggering asserts every few minutes.
I concluded that assert was just wrong, and removed it. Maybe there
is some weaker assertion we could make, but it'd require passing down
more context than fix_scan_expr gets now, and it doesn't seem worth
the trouble.
Thanks for the report!
regards, tom lane