public inbox for [email protected]
help / color / mirror / Atom feedFrom: SATYANARAYANA NARLAPURAM <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns
Date: Fri, 17 Apr 2026 13:03:36 -0700
Message-ID: <CAHg+QDc_TwzSgb=B_QgNLt3mvZdmRK23rLb+RkanSQkDF40GjA@mail.gmail.com> (raw)
Hi hackers,
UPDATE and DELETE with WHERE CURRENT OF cursor fail on tables that have
virtual generated columns, erroring with "WHERE CURRENT OF on a view is not
implemented" even though the target is a regular table, not a view.
Repro:
create table gtest_cursor (id int primary key, a int, b int generated
always as (a * 2) virtual);
insert into gtest_cursor values (1, 10), (2, 20), (3, 30);
begin;
declare cur1 cursor for select * from gtest_cursor order by id for update;
fetch 1 from cur1;
update gtest_cursor set a = 99 where current of cur1;
select * from gtest_cursor order by id;
commit;
Analysis:
The bug stems from replace_rte_variables_mutator() in rewriteManip.c, which
unconditionally errors on any CurrentOfExpr referencing the target
relation. This appears to a check designed for view rewriting, where WHERE
CURRENT OF cannot be translated through a view. However, virtual generated
column (VGC) expansion also routes through this mutator. The rewriter's
expand_generated_columns_internal() calls ReplaceVarsFromTargetList(), and
the planner's expand_virtual_generated_columns() calls
pullup_replace_vars(), which calls replace_rte_variables(). Since virtual
generated columns use same mutator, while expanding virtual generated
columns returns the same error even though the table is not a view and the
cursor position is perfectly valid.
The fix adds bool error_on_current_of to replace_rte_variables_context. The
existing replace_rte_variables() is refactored into a static
replace_rte_variables_internal() that accepts the flag, with two public
wrappers: replace_rte_variables() (passes true, preserving existing
behavior) and replace_rte_variables_ext() (exposes the flag). The same
pattern is applied to ReplaceVarsFromTargetList() /
ReplaceVarsFromTargetListExtended(). In replace_rte_variables_mutator(),
the CurrentOfExpr error is now conditional on context->error_on_current_of.
The two VGC expansion call sites pass false; all other callers pass true.
The down side of this approach is that it is adding additional public API.
Alternative considered: RTE-lookup approach. Instead of a flag, the mutator
could look up the target RTE in the query's range table and check
rte->rtekind, if it is RTE_RELATION, skip the error. Since the mutator
doesn't have access to the range table and threading an RTE or range table
pointer through the context would be equally invasive I didn't pursue this
further. Went with the flag approach because it is simpler, explicit, and
keeps the mutator's contract clean.
Thoughts or any other ideas how to fix this?
Thanks,
Satya
Attachments:
[application/octet-stream] v1-0001-vgc-where-current-of-fix.patch (11.9K, 3-v1-0001-vgc-where-current-of-fix.patch)
download | inline diff:
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 95bf5160..654e3768 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -82,6 +82,7 @@ typedef struct pullup_replace_vars_context
int varno; /* varno of subquery */
ReplaceWrapOption wrap_option; /* do we need certain outputs to be PHVs? */
Node **rv_cache; /* cache for results with PHVs */
+ bool error_on_current_of; /* error on CurrentOfExpr? */
} pullup_replace_vars_context;
typedef struct reduce_outer_joins_pass1_state
@@ -566,6 +567,12 @@ expand_virtual_generated_columns(PlannerInfo *root, Query *parse,
if (parse->groupingSets)
rvcontext.wrap_option = REPLACE_WRAP_ALL;
+ /*
+ * Don't error on CurrentOfExpr during VGC expansion; the table is
+ * real, not a view, so WHERE CURRENT OF is legitimate.
+ */
+ rvcontext.error_on_current_of = false;
+
/*
* Apply pullup variable replacement throughout the query tree.
*/
@@ -1579,6 +1586,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
sizeof(Node *));
+ rvcontext.error_on_current_of = true;
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
@@ -2124,6 +2132,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
sizeof(Node *));
+ rvcontext.error_on_current_of = true;
/*
* Replace all of the top query's references to the RTE's outputs with
@@ -2292,6 +2301,7 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(rvcontext.targetlist) + 1) *
sizeof(Node *));
+ rvcontext.error_on_current_of = true;
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
@@ -2754,11 +2764,12 @@ replace_vars_in_jointree(Node *jtnode,
static Node *
pullup_replace_vars(Node *expr, pullup_replace_vars_context *context)
{
- return replace_rte_variables(expr,
- context->varno, 0,
- pullup_replace_vars_callback,
- context,
- context->outer_hasSubLinks);
+ return replace_rte_variables_ext(expr,
+ context->varno, 0,
+ pullup_replace_vars_callback,
+ context,
+ context->outer_hasSubLinks,
+ context->error_on_current_of);
}
static Node *
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 021c73f1..97f94715 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4574,10 +4574,12 @@ expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
Assert(list_length(tlist) > 0);
- node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
- result_relation,
- REPLACEVARS_CHANGE_VARNO, rt_index,
- NULL);
+ node = ReplaceVarsFromTargetListExtended(node, rt_index, 0,
+ rte, tlist,
+ result_relation,
+ REPLACEVARS_CHANGE_VARNO,
+ rt_index,
+ NULL, false);
}
return node;
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 4bf4aa0d..5b6872ef 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1443,11 +1443,12 @@ remove_nulling_relids_mutator(Node *node,
* convenient to recurse directly to the mutator on sub-expressions of
* what they will return.
*/
-Node *
-replace_rte_variables(Node *node, int target_varno, int sublevels_up,
- replace_rte_variables_callback callback,
- void *callback_arg,
- bool *outer_hasSubLinks)
+static Node *
+replace_rte_variables_internal(Node *node, int target_varno, int sublevels_up,
+ replace_rte_variables_callback callback,
+ void *callback_arg,
+ bool *outer_hasSubLinks,
+ bool error_on_current_of)
{
Node *result;
replace_rte_variables_context context;
@@ -1456,6 +1457,7 @@ replace_rte_variables(Node *node, int target_varno, int sublevels_up,
context.callback_arg = callback_arg;
context.target_varno = target_varno;
context.sublevels_up = sublevels_up;
+ context.error_on_current_of = error_on_current_of;
/*
* We try to initialize inserted_sublink to true if there is no need to
@@ -1490,6 +1492,37 @@ replace_rte_variables(Node *node, int target_varno, int sublevels_up,
return result;
}
+Node *
+replace_rte_variables(Node *node, int target_varno, int sublevels_up,
+ replace_rte_variables_callback callback,
+ void *callback_arg,
+ bool *outer_hasSubLinks)
+{
+ return replace_rte_variables_internal(node, target_varno, sublevels_up,
+ callback, callback_arg,
+ outer_hasSubLinks, true);
+}
+
+/*
+ * replace_rte_variables_ext -
+ * As above, but with control over CurrentOfExpr error behavior.
+ *
+ * If error_on_current_of is false, CurrentOfExpr nodes referencing the
+ * target relation will be left unchanged rather than raising an error.
+ */
+Node *
+replace_rte_variables_ext(Node *node, int target_varno, int sublevels_up,
+ replace_rte_variables_callback callback,
+ void *callback_arg,
+ bool *outer_hasSubLinks,
+ bool error_on_current_of)
+{
+ return replace_rte_variables_internal(node, target_varno, sublevels_up,
+ callback, callback_arg,
+ outer_hasSubLinks,
+ error_on_current_of);
+}
+
Node *
replace_rte_variables_mutator(Node *node,
replace_rte_variables_context *context)
@@ -1519,7 +1552,8 @@ replace_rte_variables_mutator(Node *node,
CurrentOfExpr *cexpr = (CurrentOfExpr *) node;
if (cexpr->cvarno == context->target_varno &&
- context->sublevels_up == 0)
+ context->sublevels_up == 0 &&
+ context->error_on_current_of)
{
/*
* We get here if a WHERE CURRENT OF expression turns out to apply
@@ -1978,6 +2012,37 @@ ReplaceVarsFromTargetList(Node *node,
ReplaceVarsNoMatchOption nomatch_option,
int nomatch_varno,
bool *outer_hasSubLinks)
+{
+ return ReplaceVarsFromTargetListExtended(node, target_varno, sublevels_up,
+ target_rte, targetlist,
+ result_relation, nomatch_option,
+ nomatch_varno, outer_hasSubLinks,
+ true);
+}
+
+/*
+ * ReplaceVarsFromTargetListExtended -
+ * As above, but with additional control over CurrentOfExpr handling.
+ *
+ * If error_on_current_of is true (the default for ReplaceVarsFromTargetList),
+ * an error is raised if a CurrentOfExpr is found referencing the target
+ * relation. This is appropriate for view rewriting, where WHERE CURRENT OF
+ * cannot be translated to the underlying table.
+ *
+ * If error_on_current_of is false, CurrentOfExpr nodes are left unchanged.
+ * This is used by virtual generated column expansion, where we are only
+ * replacing column references, not changing the target relation.
+ */
+Node *
+ReplaceVarsFromTargetListExtended(Node *node,
+ int target_varno, int sublevels_up,
+ RangeTblEntry *target_rte,
+ List *targetlist,
+ int result_relation,
+ ReplaceVarsNoMatchOption nomatch_option,
+ int nomatch_varno,
+ bool *outer_hasSubLinks,
+ bool error_on_current_of)
{
ReplaceVarsFromTargetList_context context;
@@ -1987,8 +2052,9 @@ ReplaceVarsFromTargetList(Node *node,
context.nomatch_option = nomatch_option;
context.nomatch_varno = nomatch_varno;
- return replace_rte_variables(node, target_varno, sublevels_up,
- ReplaceVarsFromTargetList_callback,
- &context,
- outer_hasSubLinks);
+ return replace_rte_variables_internal(node, target_varno, sublevels_up,
+ ReplaceVarsFromTargetList_callback,
+ &context,
+ outer_hasSubLinks,
+ error_on_current_of);
}
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index a6d4e888..eb66b714 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -32,6 +32,7 @@ struct replace_rte_variables_context
int target_varno; /* RTE index to search for */
int sublevels_up; /* (current) nesting depth */
bool inserted_sublink; /* have we inserted a SubLink? */
+ bool error_on_current_of; /* error on CurrentOfExpr? */
};
typedef enum ReplaceVarsNoMatchOption
@@ -96,6 +97,12 @@ extern Node *replace_rte_variables(Node *node,
replace_rte_variables_callback callback,
void *callback_arg,
bool *outer_hasSubLinks);
+extern Node *replace_rte_variables_ext(Node *node,
+ int target_varno, int sublevels_up,
+ replace_rte_variables_callback callback,
+ void *callback_arg,
+ bool *outer_hasSubLinks,
+ bool error_on_current_of);
extern Node *replace_rte_variables_mutator(Node *node,
replace_rte_variables_context *context);
@@ -118,5 +125,13 @@ extern Node *ReplaceVarsFromTargetList(Node *node,
ReplaceVarsNoMatchOption nomatch_option,
int nomatch_varno,
bool *outer_hasSubLinks);
-
+extern Node *ReplaceVarsFromTargetListExtended(Node *node,
+ int target_varno, int sublevels_up,
+ RangeTblEntry *target_rte,
+ List *targetlist,
+ int result_relation,
+ ReplaceVarsNoMatchOption nomatch_option,
+ int nomatch_varno,
+ bool *outer_hasSubLinks,
+ bool error_on_current_of);
#endif /* REWRITEMANIP_H */
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index fc41c480..c238ef8a 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1723,3 +1723,43 @@ select * from gtest33 where b is null;
reset constraint_exclusion;
drop table gtest33;
+create table gtest_cursor (id int primary key, a int, b int generated always as (a * 2) virtual);
+insert into gtest_cursor values (1, 10), (2, 20), (3, 30);
+-- UPDATE via cursor
+begin;
+declare cur1 cursor for select * from gtest_cursor order by id for update;
+fetch 1 from cur1;
+ id | a | b
+----+----+----
+ 1 | 10 | 20
+(1 row)
+
+update gtest_cursor set a = 99 where current of cur1;
+select * from gtest_cursor order by id;
+ id | a | b
+----+----+-----
+ 1 | 99 | 198
+ 2 | 20 | 40
+ 3 | 30 | 60
+(3 rows)
+
+commit;
+-- DELETE via cursor
+begin;
+declare cur2 cursor for select * from gtest_cursor order by id for update;
+fetch 1 from cur2;
+ id | a | b
+----+----+-----
+ 1 | 99 | 198
+(1 row)
+
+delete from gtest_cursor where current of cur2;
+select * from gtest_cursor order by id;
+ id | a | b
+----+----+----
+ 2 | 20 | 40
+ 3 | 30 | 60
+(2 rows)
+
+commit;
+drop table gtest_cursor;
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index 9b32413e..5bab7d35 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -906,3 +906,24 @@ select * from gtest33 where b is null;
reset constraint_exclusion;
drop table gtest33;
+
+create table gtest_cursor (id int primary key, a int, b int generated always as (a * 2) virtual);
+insert into gtest_cursor values (1, 10), (2, 20), (3, 30);
+
+-- UPDATE via cursor
+begin;
+declare cur1 cursor for select * from gtest_cursor order by id for update;
+fetch 1 from cur1;
+update gtest_cursor set a = 99 where current of cur1;
+select * from gtest_cursor order by id;
+commit;
+
+-- DELETE via cursor
+begin;
+declare cur2 cursor for select * from gtest_cursor order by id for update;
+fetch 1 from cur2;
+delete from gtest_cursor where current of cur2;
+select * from gtest_cursor order by id;
+commit;
+
+drop table gtest_cursor;
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns
In-Reply-To: <CAHg+QDc_TwzSgb=B_QgNLt3mvZdmRK23rLb+RkanSQkDF40GjA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox