On Mon, Feb 10, 2025 at 11:54 AM Richard Guo <guofengli...@gmail.com> wrote: > > On Sun, Feb 9, 2025 at 7:02 PM Zhang Mingli <zmlpostg...@gmail.com> wrote: > > On Feb 9, 2025 at 16:00 +0800, Alexander Lakhin <exclus...@gmail.com>, > > wrote: > > Please look at a planner error with a virtual generated column triggered > > by the following script: > > CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a * 1)); > > > > SELECT SUM(CASE WHEN t.b = 1 THEN 1 ELSE 1 END) OVER (PARTITION BY t.a) > > FROM t AS t1 LEFT JOIN T ON true; > > > > ERROR: XX000: wrong varnullingrels (b) (expected (b 3)) for Var 2/1 > > LOCATION: search_indexed_tlist_for_var, setrefs.c:2901 > > > During the parse stage, we set the Var->varnullingrels in the > > parse_analyze_fixedparams function. > > Later, when rewriting the parse tree in pg_rewrite_query() to expand > > virtual columns, we replace the expression column b with a new Var that > > includes a, since b is defined as a * 1. > > Unfortunately, we overlooked updating the Var->varnullingrels at this point. > > As a result, when we enter search_indexed_tlist_for_var, it leads to a > > failure. > > While we do have another target entry with the correct varnullingrels, the > > expression involving the virtual column generates another column reference, > > which causes the error. > > Currently, I don't have a solid fix. > > One potential solution is to correct the Vars at or after the rewrite stage > > by traversing the parse tree again using markNullableIfNeeded. > > However, this approach may require exposing the ParseState, which doesn't > > seem ideal. > > It appears that the virtual column generation function during the rewrite > > stage does not account for the Var field settings, leading to the errors we > > are encountering. > > Hmm, would it be possible to propagate any varnullingrels into the > replacement expression in ReplaceVarsFromTargetList_callback()? > in ReplaceVarsFromTargetList_callback, we have ``if (var->varlevelsup > 0)`` ``if (var->varreturningtype != VAR_RETURNING_DEFAULT)``
we can also have. ``if (var->varnullingrels != NULL)`` please check attached. > BTW, I was curious about what happens if the replacement expression is > constant, so I tried running the query below. > > CREATE TABLE t (a int, b int GENERATED ALWAYS AS (1 + 1)); > 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 > ---+--- > | 2 > | 2 > (2 rows) > > Is this the expected behavior? I was expecting that t2.b should be > all NULLs. > SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE; should be same as SELECT t2.a, 2 as b FROM t t1 LEFT JOIN t t2 ON FALSE; so i think this is expected.
From 78f272701afb23994021a0b98fa6deaf9d37cc04 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Mon, 10 Feb 2025 12:51:17 +0800 Subject: [PATCH v1 1/1] fix expand virtual generated column Var node varnullingrels field discussion: https://postgr.es/m/75eb1a6f-d59f-42e6-8a78-124ee808c...@gmail.com --- src/backend/rewrite/rewriteManip.c | 54 +++++++++++++++++++ src/include/rewrite/rewriteManip.h | 2 + .../regress/expected/generated_virtual.out | 37 +++++++++++++ src/test/regress/sql/generated_virtual.sql | 9 ++++ src/tools/pgindent/typedefs.list | 1 + 5 files changed, 103 insertions(+) diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index a115b217c9..9d81d5441a 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -766,6 +766,41 @@ typedef struct int min_sublevels_up; } IncrementVarSublevelsUp_context; +typedef struct +{ + int sublevels_up; + 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->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); +} + static bool IncrementVarSublevelsUp_walker(Node *node, IncrementVarSublevelsUp_context *context) @@ -846,6 +881,21 @@ IncrementVarSublevelsUp_walker(Node *node, return expression_tree_walker(node, IncrementVarSublevelsUp_walker, context); } +void +SetVarNullingrels(Node *node, int sublevels_up, Bitmapset *varnullingrels) +{ + SetVarNullingrels_context context; + + context.sublevels_up = sublevels_up; + context.varnullingrels = varnullingrels; + + /* Expect to start with an expression */ + query_or_expression_tree_walker(node, + SetVarNullingrels_walker, + &context, + QTW_EXAMINE_RTES_BEFORE); +} + void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up, int min_sublevels_up) @@ -1832,6 +1882,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->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 512823033b..cca38a6fd7 100644 --- a/src/include/rewrite/rewriteManip.h +++ b/src/include/rewrite/rewriteManip.h @@ -48,6 +48,8 @@ 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, + 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 8660904a34..d6fe82fb13 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -181,6 +181,43 @@ 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) + 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 3207e5cb1f..10638b29f8 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -80,6 +80,15 @@ 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; + DROP TABLE gtestx; -- test UPDATE/DELETE quals diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 9a3bee93de..a9e15be55f 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2632,6 +2632,7 @@ SetOperation SetOperationStmt SetQuantifier SetToDefault +SetVarNullingrels_context SetVarReturningType_context SetupWorkerPtrType ShDependObjectInfo -- 2.34.1
