[Bug] Add the missing RTE_GRAPH_TABLE case to transformLockingClause()

在 2026/5/7 15:37, SATYANARAYANA NARLAPURAM 写道:

Hi Hackers,

Add the missing RTE_GRAPH_TABLE case to transformLockingClause().
Without this four
row-locking strengths applied to a GRAPH_TABLE alias triggers not give
a user friendly
error message.

Repro:

CREATE TABLE v(id int PRIMARY KEY, vname text);
CREATE PROPERTY GRAPH g VERTEX TABLES (v);
SELECT * FROM GRAPH_TABLE(g MATCH (a) COLUMNS (a.vname)) gt
FOR UPDATE OF gt;
-- ERROR: unrecognized RTE type: 8

Attached a patch that returns ERRCODE_FEATURE_NOT_SUPPORTED "FOR ...
cannot be
applied to a GRAPH_TABLE" with a position pointer, matching the
convention used by
the function/tablefunc etc. Patch includes tests for all four locking
strengths.
Since the code path looks simple we can just keep one of them as well
and trim other
tests. Thoughts?

Thanks,
Satya

The change is straightforward, clearly an improvement to me. I think you
may keep all 4 cases as they are light.

Regards,
Zhenwei Shang

On Thu, May 7, 2026 at 1:07 PM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:

Hi Hackers,

Add the missing RTE_GRAPH_TABLE case to transformLockingClause(). Without this four
row-locking strengths applied to a GRAPH_TABLE alias triggers not give a user friendly
error message.

Repro:

CREATE TABLE v(id int PRIMARY KEY, vname text);
CREATE PROPERTY GRAPH g VERTEX TABLES (v);
SELECT * FROM GRAPH_TABLE(g MATCH (a) COLUMNS (a.vname)) gt
FOR UPDATE OF gt;
-- ERROR: unrecognized RTE type: 8

Attached a patch that returns ERRCODE_FEATURE_NOT_SUPPORTED "FOR ... cannot be
applied to a GRAPH_TABLE" with a position pointer, matching the convention used by
the function/tablefunc etc.

In a fully matured SQL/PGQ support, a row mark on GRAPH_TABLE should
be pushed down to the resulting subqueries in rewriteGraphTable(). I
see this similar to how the row mark is pushed down into views during
the rewrite phase. I don't think the code changes will be massive, it
will be a matter of invoking markQueryForLocking() on the query that
replaces the graph pattern in rewriteGraphTable(). I am not sure
whether we want to do that now when stabilizing the feature. Peter,
what do you think?

If we decide to not to support it, the code changes look on the right
path. We need to update the following comment which appears earlier in
the function to mention GRAPH_TABLE, /* ignore JOIN, SPECIAL,
FUNCTION, VALUES, CTE RTEs */. I think it will be better to specify
that this restriction is temporary in the comments at both the places;
but that's arguable.

Patch includes tests for all four locking strengths.
Since the code path looks simple we can just keep one of them as well and trim other
tests. Thoughts?

I don't think we need all the four tests, but we need a test for
locking clause without relation since that takes a different code
path.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Wed, 13 May 2026 at 10:57, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

Repro:

CREATE TABLE v(id int PRIMARY KEY, vname text);
CREATE PROPERTY GRAPH g VERTEX TABLES (v);
SELECT * FROM GRAPH_TABLE(g MATCH (a) COLUMNS (a.vname)) gt
FOR UPDATE OF gt;
-- ERROR: unrecognized RTE type: 8

Attached a patch that returns ERRCODE_FEATURE_NOT_SUPPORTED "FOR ...

cannot be

applied to a GRAPH_TABLE" with a position pointer, matching the

convention used by

the function/tablefunc etc.

In a fully matured SQL/PGQ support, a row mark on GRAPH_TABLE should
be pushed down to the resulting subqueries in rewriteGraphTable(). I
see this similar to how the row mark is pushed down into views during
the rewrite phase. I don't think the code changes will be massive, it
will be a matter of invoking markQueryForLocking() on the query that
replaces the graph pattern in rewriteGraphTable(). I am not sure
whether we want to do that now when stabilizing the feature. Peter,
what do you think?

I wanted to implement this by trying to push FOR UPDATE/SHARE
down into the relational tables underlying the GRAPH_TABLE (the
"fully matured SQL/PGQ" direction mentioned). But had
some doubts around the implementation.

I'd like input on a few design questions from you/Peter/hackers:

Q1. Is the right place to do the pushdown rewriteGraphTable(),
mirroring what ApplyRetrieveRule() does for views (i.e. call
markQueryForLocking() on the freshly built subquery)? That
would require exposing markQueryForLocking() outside
rewriteHandler.c.

Q2. How should FOR UPDATE behave when the MATCH expands to more
than one path and rewriteGraphTable() produces a UNION?
markQueryForLocking() refuses to descend into set-operations,
same as it does for views. Options:
(a) reject FOR UPDATE on multi-path MATCH with a clear
"cannot lock rows from a set-operation result" error,
matching the view behaviour;
(b) silently fall back to the current
"raise relation lock only" behaviour and document it.
I think it should be (a).

Q3. SQL/PGQ does not really define UPDATE/DELETE semantics for
graph paths. Is the intent here that FOR UPDATE/SHARE on
GRAPH_TABLE is purely a concurrency hint rather than a
precursor to UPDATE WHERE CURRENT OF? If so, that should
probably be documented next to the GRAPH_TABLE syntax in
the SQL reference.

If we decide to not to support it, the code changes look on the right
path. We need to update the following comment which appears earlier in
the function to mention GRAPH_TABLE, /* ignore JOIN, SPECIAL,
FUNCTION, VALUES, CTE RTEs */. I think it will be better to specify
that this restriction is temporary in the comments at both the places;
but that's arguable.

Patch includes tests for all four locking strengths.
Since the code path looks simple we can just keep one of them as well

and trim other

tests. Thoughts?

I don't think we need all the four tests, but we need a test for
locking clause without relation since that takes a different code
path.

Attached is v2-0001, rebased on master and addressing
the comments you added in case we plan on parking the feature
work for once v20 opens.

Regards,
Ayush

On Wed, Jun 10, 2026 at 1:03 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Hi,

On Wed, 13 May 2026 at 10:57, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:

Repro:

CREATE TABLE v(id int PRIMARY KEY, vname text);
CREATE PROPERTY GRAPH g VERTEX TABLES (v);
SELECT * FROM GRAPH_TABLE(g MATCH (a) COLUMNS (a.vname)) gt
FOR UPDATE OF gt;
-- ERROR: unrecognized RTE type: 8

Attached a patch that returns ERRCODE_FEATURE_NOT_SUPPORTED "FOR ... cannot be
applied to a GRAPH_TABLE" with a position pointer, matching the convention used by
the function/tablefunc etc.

In a fully matured SQL/PGQ support, a row mark on GRAPH_TABLE should
be pushed down to the resulting subqueries in rewriteGraphTable(). I
see this similar to how the row mark is pushed down into views during
the rewrite phase. I don't think the code changes will be massive, it
will be a matter of invoking markQueryForLocking() on the query that
replaces the graph pattern in rewriteGraphTable(). I am not sure
whether we want to do that now when stabilizing the feature. Peter,
what do you think?

I wanted to implement this by trying to push FOR UPDATE/SHARE
down into the relational tables underlying the GRAPH_TABLE (the
"fully matured SQL/PGQ" direction mentioned). But had
some doubts around the implementation.

I'd like input on a few design questions from you/Peter/hackers:

Q1. Is the right place to do the pushdown rewriteGraphTable(),
mirroring what ApplyRetrieveRule() does for views (i.e. call
markQueryForLocking() on the freshly built subquery)? That
would require exposing markQueryForLocking() outside
rewriteHandler.c.

That's right. I think making markQueryForLocking non-static is fine
since we are using it in rewriter code itself.

Q2. How should FOR UPDATE behave when the MATCH expands to more
than one path and rewriteGraphTable() produces a UNION?
markQueryForLocking() refuses to descend into set-operations,
same as it does for views. Options:
(a) reject FOR UPDATE on multi-path MATCH with a clear
"cannot lock rows from a set-operation result" error,
matching the view behaviour;
(b) silently fall back to the current
"raise relation lock only" behaviour and document it.
I think it should be (a).

Building a UNION query is an implementation artifact of GRAPH_TABLE,
locking clause should not depend upon it. I think we should build the
subqueries within the setop with a locking clause of their own. That's
supported.

Q3. SQL/PGQ does not really define UPDATE/DELETE semantics for
graph paths. Is the intent here that FOR UPDATE/SHARE on
GRAPH_TABLE is purely a concurrency hint rather than a
precursor to UPDATE WHERE CURRENT OF? If so, that should
probably be documented next to the GRAPH_TABLE syntax in
the SQL reference.

I don't understand the WHERE CURRENT OF reference here. FOR
UPDATE/SHARE is generally used with an intent of locking the read rows
so that they don't get modified while the transaction is in progress.
One can use just UPDATE to update the FOR UPDATE locked rows as well.

If we decide to not to support it, the code changes look on the right
path. We need to update the following comment which appears earlier in
the function to mention GRAPH_TABLE, /* ignore JOIN, SPECIAL,
FUNCTION, VALUES, CTE RTEs */. I think it will be better to specify
that this restriction is temporary in the comments at both the places;
but that's arguable.

Patch includes tests for all four locking strengths.
Since the code path looks simple we can just keep one of them as well and trim other
tests. Thoughts?

I don't think we need all the four tests, but we need a test for
locking clause without relation since that takes a different code
path.

Attached is v2-0001, rebased on master and addressing
the comments you added in case we plan on parking the feature
work for once v20 opens.

Thanks. I revised it a bit. Reworded the commit message to describe
user facing behaviour instead of the internals, which are quite
obvious from the code changes. Other cases in transformLockingClause()
don't mention XXX, they just have ereport(). Removed that comment.
Also massaged comments in the test - we usually don't describe the
expected behaviour or error when it is obvious from the output.
Attached revised patch.

--
Best Wishes,
Ashutosh Bapat

On 10.06.26 14:49, Ashutosh Bapat wrote:

Attached is v2-0001, rebased on master and addressing
the comments you added in case we plan on parking the feature
work for once v20 opens.

Thanks. I revised it a bit. Reworded the commit message to describe
user facing behaviour instead of the internals, which are quite
obvious from the code changes. Other cases in transformLockingClause()
don't mention XXX, they just have ereport(). Removed that comment.
Also massaged comments in the test - we usually don't describe the
expected behaviour or error when it is obvious from the output.
Attached revised patch.

Committed.

I also modified the comment that said "ignore JOIN, SPECIAL, ..." to
just say "ignore all other RTE kinds", because keeping that kind of list
up to date is silly. (RTE_SPECIAL hasn't existed in a long time.)

On Thu, Jul 9, 2026 at 1:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 10.06.26 14:49, Ashutosh Bapat wrote:

Attached is v2-0001, rebased on master and addressing
the comments you added in case we plan on parking the feature
work for once v20 opens.

Thanks. I revised it a bit. Reworded the commit message to describe
user facing behaviour instead of the internals, which are quite
obvious from the code changes. Other cases in transformLockingClause()
don't mention XXX, they just have ereport(). Removed that comment.
Also massaged comments in the test - we usually don't describe the
expected behaviour or error when it is obvious from the output.
Attached revised patch.

Committed.

Thanks.

I also modified the comment that said "ignore JOIN, SPECIAL, ..." to
just say "ignore all other RTE kinds", because keeping that kind of list
up to date is silly. (RTE_SPECIAL hasn't existed in a long time.)

That comment is useful since it confirms that the omission is
intentional; however I agree that keeping it up to date is cumbersome.
Maybe we need some way to tie the two switch cases so that
unintentional omissions can be easily caught.

--
Best Wishes,
Ashutosh Bapat