public inbox for [email protected]help / color / mirror / Atom feed
[BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns 6+ messages / 2 participants [nested] [flat]
* [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-17 20:03 SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2026-04-17 20:03 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> 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; ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-19 10:42 Dean Rasheed <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Dean Rasheed @ 2026-04-19 10:42 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Fri, 17 Apr 2026 at 21:04, SATYANARAYANA NARLAPURAM <[email protected]> wrote: > > 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. > Nice catch! > 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. > Hmm, it seems to me that a much simpler fix is to check for use of WHERE CURRENT OF on a view at parse time, and throw the error there. Then the problematic rewriter check can simply be removed, as in the attached v2 patch. Regards, Dean Attachments: [text/x-patch] v2-0001-vgc-where-current-of-fix.patch (4.5K, 2-v2-0001-vgc-where-current-of-fix.patch) download | inline diff: diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c new file mode 100644 index 84deed9..d984535 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -595,6 +595,14 @@ transformDeleteStmt(ParseState *pstate, ACL_DELETE); nsitem = pstate->p_target_nsitem; + /* disallow DELETE ... WHERE CURRENT OF on a view */ + if (stmt->whereClause && + IsA(stmt->whereClause, CurrentOfExpr) && + pstate->p_target_relation->rd_rel->relkind == RELKIND_VIEW) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WHERE CURRENT OF on a view is not implemented")); + /* there's no DISTINCT in DELETE */ qry->distinctClause = NIL; @@ -2868,6 +2876,14 @@ transformUpdateStmt(ParseState *pstate, true, ACL_UPDATE); + /* disallow UPDATE ... WHERE CURRENT OF on a view */ + if (stmt->whereClause && + IsA(stmt->whereClause, CurrentOfExpr) && + pstate->p_target_relation->rd_rel->relkind == RELKIND_VIEW) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WHERE CURRENT OF on a view is not implemented")); + if (stmt->forPortionOf) qry->forPortionOf = transformForPortionOfClause(pstate, qry->resultRelation, diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c new file mode 100644 index 4bf4aa0..9aa7ef6 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1514,25 +1514,6 @@ replace_rte_variables_mutator(Node *node } /* otherwise fall through to copy the var normally */ } - else if (IsA(node, CurrentOfExpr)) - { - CurrentOfExpr *cexpr = (CurrentOfExpr *) node; - - if (cexpr->cvarno == context->target_varno && - context->sublevels_up == 0) - { - /* - * We get here if a WHERE CURRENT OF expression turns out to apply - * to a view. Someday we might be able to translate the - * expression to apply to an underlying table of the view, but - * right now it's not implemented. - */ - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("WHERE CURRENT OF on a view is not implemented"))); - } - /* otherwise fall through to copy the expr normally */ - } else if (IsA(node, Query)) { /* Recurse into RTE subquery or not-yet-planned sublink subquery */ diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out new file mode 100644 index fc41c48..c238ef8 --- 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 new file mode 100644 index 9b32413..5bab7d3 --- 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; ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-20 00:32 SATYANARAYANA NARLAPURAM <[email protected]> parent: Dean Rasheed <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2026-04-20 00:32 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> HI, On Sun, Apr 19, 2026 at 3:42 AM Dean Rasheed <[email protected]> wrote: > On Fri, 17 Apr 2026 at 21:04, SATYANARAYANA NARLAPURAM > <[email protected]> wrote: > > > > 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. > > > > Nice catch! > > > 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. > > > > Hmm, it seems to me that a much simpler fix is to check for use of > WHERE CURRENT OF on a view at parse time, and throw the error there. > This patch looks simple and neat, is there any reason why it was done differently earlier? > Then the problematic rewriter check can simply be removed, as in the > attached v2 patch. > I reviewed the patch, and it addresses the original bug and other existing tests. I verified it rejects the view with WHERE CURRENT OF on update and delete. Updated the patch to include a test case to reject view update with WHERE CURRENT OF. Thanks, Satya Attachments: [application/octet-stream] v3-0001-vgc-where-current-of-fix.patch (5.3K, 3-v3-0001-vgc-where-current-of-fix.patch) download | inline diff: diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 84deed9a..d9845358 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -595,6 +595,14 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt) ACL_DELETE); nsitem = pstate->p_target_nsitem; + /* disallow DELETE ... WHERE CURRENT OF on a view */ + if (stmt->whereClause && + IsA(stmt->whereClause, CurrentOfExpr) && + pstate->p_target_relation->rd_rel->relkind == RELKIND_VIEW) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WHERE CURRENT OF on a view is not implemented")); + /* there's no DISTINCT in DELETE */ qry->distinctClause = NIL; @@ -2868,6 +2876,14 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) true, ACL_UPDATE); + /* disallow UPDATE ... WHERE CURRENT OF on a view */ + if (stmt->whereClause && + IsA(stmt->whereClause, CurrentOfExpr) && + pstate->p_target_relation->rd_rel->relkind == RELKIND_VIEW) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WHERE CURRENT OF on a view is not implemented")); + if (stmt->forPortionOf) qry->forPortionOf = transformForPortionOfClause(pstate, qry->resultRelation, diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 4bf4aa0d..9aa7ef60 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1514,25 +1514,6 @@ replace_rte_variables_mutator(Node *node, } /* otherwise fall through to copy the var normally */ } - else if (IsA(node, CurrentOfExpr)) - { - CurrentOfExpr *cexpr = (CurrentOfExpr *) node; - - if (cexpr->cvarno == context->target_varno && - context->sublevels_up == 0) - { - /* - * We get here if a WHERE CURRENT OF expression turns out to apply - * to a view. Someday we might be able to translate the - * expression to apply to an underlying table of the view, but - * right now it's not implemented. - */ - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("WHERE CURRENT OF on a view is not implemented"))); - } - /* otherwise fall through to copy the expr normally */ - } else if (IsA(node, Query)) { /* Recurse into RTE subquery or not-yet-planned sublink subquery */ diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index fc41c480..58cdf310 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -1723,3 +1723,57 @@ 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; +-- WHERE CURRENT OF via a view on a table with VGC (should fail) +create view gtest_cursor_view as select * from gtest_cursor; +begin; +declare cur3 cursor for select * from gtest_cursor_view for update; +fetch 1 from cur3; + id | a | b +----+----+---- + 2 | 20 | 40 +(1 row) + +update gtest_cursor_view set a = 55 where current of cur3; -- fail +ERROR: WHERE CURRENT OF on a view is not implemented +rollback; +drop view gtest_cursor_view; +drop table gtest_cursor; diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index 9b32413e..48daf31a 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -906,3 +906,33 @@ 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; + +-- WHERE CURRENT OF via a view on a table with VGC (should fail) +create view gtest_cursor_view as select * from gtest_cursor; +begin; +declare cur3 cursor for select * from gtest_cursor_view for update; +fetch 1 from cur3; +update gtest_cursor_view set a = 55 where current of cur3; -- fail +rollback; +drop view gtest_cursor_view; + +drop table gtest_cursor; ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-20 07:56 Dean Rasheed <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Dean Rasheed @ 2026-04-20 07:56 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, 20 Apr 2026 at 01:33, SATYANARAYANA NARLAPURAM <[email protected]> wrote: > > On Sun, Apr 19, 2026 at 3:42 AM Dean Rasheed <[email protected]> wrote: >> >> Hmm, it seems to me that a much simpler fix is to check for use of >> WHERE CURRENT OF on a view at parse time, and throw the error there. > > This patch looks simple and neat, is there any reason why it was done differently earlier? > I'm not sure, but possibly because it used to be possible to turn a table into a view by defining a SELECT rule on it, which could have rendered a parse-time check insufficient. That's no longer the case though, and we now have similar parse-time relkind tests elsewhere (e.g., for MERGE). > Updated the patch to include a test case to reject view update with WHERE CURRENT OF. > That's already tested in portals.sql, which seems like a better place for that test, since it's not related to virtual generated columns. I don't think another test is necessary -- admittedly portals.sql only tests DELETE, but the UPDATE code is the same, so I think the existing test is sufficient. We don't obsessively try to achieve 100% coverage in our tests. Regards, Dean ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-20 08:03 SATYANARAYANA NARLAPURAM <[email protected]> parent: Dean Rasheed <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2026-04-20 08:03 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi On Mon, Apr 20, 2026 at 12:56 AM Dean Rasheed <[email protected]> wrote: > On Mon, 20 Apr 2026 at 01:33, SATYANARAYANA NARLAPURAM > <[email protected]> wrote: > > > > On Sun, Apr 19, 2026 at 3:42 AM Dean Rasheed <[email protected]> > wrote: > >> > >> Hmm, it seems to me that a much simpler fix is to check for use of > >> WHERE CURRENT OF on a view at parse time, and throw the error there. > > > > This patch looks simple and neat, is there any reason why it was done > differently earlier? > > > > I'm not sure, but possibly because it used to be possible to turn a > table into a view by defining a SELECT rule on it, which could have > rendered a parse-time check insufficient. That's no longer the case > though, and we now have similar parse-time relkind tests elsewhere > (e.g., for MERGE). > > > Updated the patch to include a test case to reject view update with > WHERE CURRENT OF. > > > > That's already tested in portals.sql, which seems like a better place > for that test, since it's not related to virtual generated columns. I > don't think another test is necessary -- admittedly portals.sql only > tests DELETE, but the UPDATE code is the same, so I think the existing > test is sufficient. We don't obsessively try to achieve 100% coverage > in our tests. > Sounds good. Thanks for letting me know. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns @ 2026-04-22 11:07 Dean Rasheed <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Dean Rasheed @ 2026-04-22 11:07 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, 20 Apr 2026 at 09:03, SATYANARAYANA NARLAPURAM <[email protected]> wrote: > >> > Updated the patch to include a test case to reject view update with WHERE CURRENT OF. >> >> That's already tested in portals.sql, which seems like a better place >> for that test, since it's not related to virtual generated columns. I >> don't think another test is necessary -- admittedly portals.sql only >> tests DELETE, but the UPDATE code is the same, so I think the existing >> test is sufficient. We don't obsessively try to achieve 100% coverage >> in our tests. > I changed my mind on that and decided to add another test to portals.sql, just in case, since it is a separate code branch. I also spent some time looking to see whether there were any other ways that replace_rte_variables() could have been called (e.g., subquery pullup) that would fall foul of this, but I couldn't find any, so I think this bug only affects virtual generated columns. Pushed and back-patched to v18. Thanks for the report! Regards, Dean ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-04-22 11:07 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-04-17 20:03 [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns SATYANARAYANA NARLAPURAM <[email protected]> 2026-04-19 10:42 ` Dean Rasheed <[email protected]> 2026-04-20 00:32 ` SATYANARAYANA NARLAPURAM <[email protected]> 2026-04-20 07:56 ` Dean Rasheed <[email protected]> 2026-04-20 08:03 ` SATYANARAYANA NARLAPURAM <[email protected]> 2026-04-22 11:07 ` Dean Rasheed <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox