On Tue, Feb 11, 2025 at 10:34 AM Richard Guo <guofengli...@gmail.com> wrote: > > On Mon, Feb 10, 2025 at 1:16 PM Zhang Mingli <zmlpostg...@gmail.com> wrote: > > I believe virtual columns should behave like stored columns, except they > > don't actually use storage. > > Virtual columns are computed when the table is read, and they should adhere > > to the same rules of join semantics. > > I agree with Richard, the result seems incorrect. The right outcome should > > be: > > gpadmin=# SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE; > > a | b > > ------+------ > > NULL | NULL > > NULL | NULL > > (2 rows) > > Yeah, I also feel that the virtual generated columns should adhere to > outer join semantics, rather than being unconditionally replaced by > the generation expressions. But maybe I'm wrong. > > If that's the case, this incorrect-result issue isn't limited to > constant expressions; it could also occur with non-strict ones. > > CREATE TABLE t (a int, b int GENERATED ALWAYS AS (COALESCE(a, 100))); > INSERT INTO t VALUES (1); > INSERT INTO t VALUES (2); > > # SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE; > a | b > ---+----- > | 100 > | 100 > (2 rows) >
Now I agree with you. I think the following two should return the same result. SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE; SELECT t2.a, t2.b FROM t t1 LEFT JOIN (select * from t) t2 ON FALSE; ------------------------ atatch refined patch solves the failure to copy the nullingrel bits for the virtual generated columns. in ReplaceVarsFromTargetList_callback. I tried to just use add_nulling_relids, but failed, so I did the similar thing as SetVarReturningType. I didn't solve the out join semantic issue. i am wondering, can we do the virtual generated column expansion in the rewrite stage as is, and wrap the expressions in PHVs if the virtual generated columns come from the nullable side of an outer join. I am looking at pullup_replace_vars_callback, but it seems not very helpful to us.
From 2f9bd9ec737227b1b2dc52a4800d006bfa228003 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Thu, 13 Feb 2025 11:28:57 +0800 Subject: [PATCH v2 1/1] fix expand virtual generated column Var node varnullingrels field To expand the virtual generated column Var node, we need to copy the fields of the original virtual generated column Var node, including varnullingrels, varlevelsup, varreturningtype, and varattno, to the newly expanded Var node. ReplaceVarsFromTargetList_callback didn't taken care of varnullingrels, this patch fix this issue. discussion: https://postgr.es/m/75eb1a6f-d59f-42e6-8a78-124ee808c...@gmail.com --- src/backend/rewrite/rewriteManip.c | 62 +++++++++++++++++++ src/include/rewrite/rewriteManip.h | 3 + .../regress/expected/generated_virtual.out | 45 ++++++++++++++ src/test/regress/sql/generated_virtual.sql | 11 ++++ src/tools/pgindent/typedefs.list | 1 + 5 files changed, 122 insertions(+) diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index a115b217c91..69701ce6ecd 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -884,6 +884,64 @@ IncrementVarSublevelsUp_rtable(List *rtable, int delta_sublevels_up, QTW_EXAMINE_RTES_BEFORE); } +/* + * SetVarNullingrels - adjust Var nodes for a specified varnullingrels. + * + * Find all Var nodes referring to the specified varno in the given + * expression and set their varnullingrels to the specified value. + */ +typedef struct +{ + int sublevels_up; + int varno; + Bitmapset *varnullingrels; +} SetVarNullingrels_context; + +static bool +SetVarNullingrels_walker(Node *node, + SetVarNullingrels_context *context) +{ + if (node == NULL) + return false; + + if (IsA(node, Var)) + { + Var *var = (Var *) node; + + if (var->varlevelsup == context->sublevels_up && + var->varno == context->varno) + var->varnullingrels = bms_union(context->varnullingrels, + var->varnullingrels); + + return false; + } + + if (IsA(node, Query)) + { + /* Recurse into subselects */ + bool result; + + context->sublevels_up++; + result = query_tree_walker((Query *) node, SetVarNullingrels_walker, + context, 0); + context->sublevels_up--; + return result; + } + return expression_tree_walker(node, SetVarNullingrels_walker, context); +} +void +SetVarNullingrels(Node *node, int sublevels_up, int varno, Bitmapset *varnullingrels) +{ + SetVarNullingrels_context context; + + context.sublevels_up = sublevels_up; + context.varno = varno; + context.varnullingrels = varnullingrels; + + /* Expect to start with an expression */ + SetVarNullingrels_walker(node, &context); +} + /* * SetVarReturningType - adjust Var nodes for a specified varreturningtype. * @@ -1832,6 +1890,10 @@ ReplaceVarsFromTargetList_callback(Var *var, if (var->varlevelsup > 0) IncrementVarSublevelsUp((Node *) newnode, var->varlevelsup, 0); + if (var->varnullingrels != NULL) + SetVarNullingrels((Node *) newnode, var->varlevelsup, + var->varno, + var->varnullingrels); /* * Check to see if the tlist item contains a PARAM_MULTIEXPR Param, * and throw error if so. This case could only happen when expanding diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h index 512823033b9..e869f651fc8 100644 --- a/src/include/rewrite/rewriteManip.h +++ b/src/include/rewrite/rewriteManip.h @@ -48,6 +48,9 @@ extern void ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up); extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up, int min_sublevels_up); +extern void SetVarNullingrels(Node *node, int sublevels_up, + int varno, + Bitmapset *varnullingrels); extern void IncrementVarSublevelsUp_rtable(List *rtable, int delta_sublevels_up, int min_sublevels_up); diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index 35638812be9..56e7e6fd6b0 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -181,6 +181,51 @@ SELECT * FROM gtestx, gtest1 WHERE gtestx.y = gtest1.a; 22 | 2 | 2 | 4 (2 rows) +--bug: https://postgr.es/m/75eb1a6f-d59f-42e6-8a78-124ee808c...@gmail.com +SELECT SUM(t.b) OVER (PARTITION BY t.a) FROM gtestx AS t1 left JOIN gtest1 T ON true; + sum +----- + 6 + 6 + 6 + 12 + 12 + 12 +(6 rows) + +--this query result should be same as as the above +SELECT SUM((select t.b from ((select t.b from gtest1 as t order by t.b limit 1)))) OVER (PARTITION BY t.a) +FROM gtestx AS t1 left JOIN gtest1 T ON true; + sum +----- + 6 + 6 + 6 + 12 + 12 + 12 +(6 rows) + +SELECT SUM((select t.b from gtest1 as t order by t.b limit 1)) OVER (PARTITION BY t.a) +FROM gtestx AS t1 left JOIN gtest1 T ON true; + sum +----- + 6 + 6 + 6 + 6 + 6 + 6 +(6 rows) + +SELECT t.x FROM gtestx t LEFT JOIN gtest1 vt on t.x = vt.a WHERE coalesce(vt.b, 1) = 1 OR t.x IS NULL; + x +---- + 11 + 22 + 33 +(3 rows) + DROP TABLE gtestx; -- test UPDATE/DELETE quals SELECT * FROM gtest1 ORDER BY a; diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index 34870813910..d76439ca993 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -80,6 +80,17 @@ DELETE FROM gtest1 WHERE a = 2000000000; CREATE TABLE gtestx (x int, y int); INSERT INTO gtestx VALUES (11, 1), (22, 2), (33, 3); SELECT * FROM gtestx, gtest1 WHERE gtestx.y = gtest1.a; +--bug: https://postgr.es/m/75eb1a6f-d59f-42e6-8a78-124ee808c...@gmail.com +SELECT SUM(t.b) OVER (PARTITION BY t.a) FROM gtestx AS t1 left JOIN gtest1 T ON true; +--this query result should be same as as the above +SELECT SUM((select t.b from ((select t.b from gtest1 as t order by t.b limit 1)))) OVER (PARTITION BY t.a) +FROM gtestx AS t1 left JOIN gtest1 T ON true; + +SELECT SUM((select t.b from gtest1 as t order by t.b limit 1)) OVER (PARTITION BY t.a) +FROM gtestx AS t1 left JOIN gtest1 T ON true; + +SELECT t.x FROM gtestx t LEFT JOIN gtest1 vt on t.x = vt.a WHERE coalesce(vt.b, 1) = 1 OR t.x IS NULL; + DROP TABLE gtestx; -- test UPDATE/DELETE quals diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b6c170ac249..6cb39ac76c5 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2633,6 +2633,7 @@ SetOperation SetOperationStmt SetQuantifier SetToDefault +SetVarNullingrels_context SetVarReturningType_context SetupWorkerPtrType ShDependObjectInfo -- 2.34.1
