MIME-Version: 1.0
References: 
 <CAHg+QDcE9wp6nOEC3SCRQ90nrCO=QF+OZq1MG8Qc6hnusmogqw@mail.gmail.com>
 <CAExHW5sQ5CfQeO4QXQaArRpD5FrUK_ndDAqHiQFg04DaC-+NaQ@mail.gmail.com>
 <CAJTYsWX8J2cUTCnVRwks0gJw4wiMCv_qVSxshFC9f8oh7Ch6JA@mail.gmail.com>
In-Reply-To: 
 <CAJTYsWX8J2cUTCnVRwks0gJw4wiMCv_qVSxshFC9f8oh7Ch6JA@mail.gmail.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 10 Jun 2026 18:19:49 +0530
Message-ID: 
 <CAExHW5tGQCjWEdyhOHGMj32NAMObH--eZDGeoxKRS9O3Wv_7wQ@mail.gmail.com>
Subject: Re: [Bug] Add the missing RTE_GRAPH_TABLE case to
 transformLockingClause()
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>,
	SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="00000000000098c53e0653e5aeae"
Archived-At: 
 <https://www.postgresql.org/message-id/CAExHW5tGQCjWEdyhOHGMj32NAMObH--eZDGeoxKRS9O3Wv_7wQ%40mail.gmail.com>
Precedence: bulk

--00000000000098c53e0653e5aeae
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 10, 2026 at 1:03=E2=80=AFAM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
>
> Hi,
>
> On Wed, 13 May 2026 at 10:57, Ashutosh Bapat <ashutosh.bapat.oss@gmail.co=
m> 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 ... c=
annot be
>> > applied to a GRAPH_TABLE" with a position pointer, matching the conven=
tion 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.

--=20
Best Wishes,
Ashutosh Bapat

--00000000000098c53e0653e5aeae
Content-Type: text/x-patch; charset="US-ASCII";
	name="v20260610-0009-Prohibit-Locking-Clauses-on-GRAPH_TABLE.patch"
Content-Disposition: attachment;
	filename="v20260610-0009-Prohibit-Locking-Clauses-on-GRAPH_TABLE.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mq81s36b0>
X-Attachment-Id: f_mq81s36b0

RnJvbSBhMmYyYmYyZGFkZDY3MGIzY2RmNDUyYWJjZWQxY2JiZDRjOTliMzczIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBeXVzaCBUaXdhcmkgPGF5dXNodGl3YXJpLnNsZzAxQGdtYWls
LmNvbT4KRGF0ZTogVHVlLCA5IEp1biAyMDI2IDIwOjE0OjEzICswNTMwClN1YmplY3Q6IFtQQVRD
SCB2MjAyNjA2MTAgOS85XSBQcm9oaWJpdCBMb2NraW5nIENsYXVzZXMgb24gR1JBUEhfVEFCTEUK
ClNwZWNpZnlpbmcgYSBsb2NraW5nIGNsYXVzZSAoRk9SIFVQREFURS9TSEFSRSkgdGhhdCBuYW1l
cyBhIEdSQVBIX1RBQkxFCmFsaWFzIGN1cnJlbnRseSByZXN1bHRzIGluIGFuIHVuaGVscGZ1bCAi
dW5yZWNvZ25pemVkIFJURSB0eXBlOiA4IgplcnJvci4gVGhpcyBjb21taXQgZXhwbGljaXRseSBw
cm9oaWJpdHMgc3BlY2lmeWluZyBhIGxvY2tpbmcgY2xhdXNlIG9uIGEKR1JBUEhfVEFCTEUgYWxp
YXMsIHJhaXNpbmcgYSBtb3JlIHVzZXItZnJpZW5kbHkgIkZPUiAuLi4gY2Fubm90IGJlCmFwcGxp
ZWQgdG8gYSBHUkFQSF9UQUJMRSIgZXJyb3IgaW5zdGVhZC4KCkF1dGhvcjogU0FUWUFOQVJBWUFO
QSBOQVJMQVBVUkFNIDxzYXR5YW5hcmxhcHVyYW1AZ21haWwuY29tPgpBdXRob3I6IEF5dXNoIFRp
d2FyaSA8YXl1c2h0aXdhcmkuc2xnMDFAZ21haWwuY29tPgpSZXBvcnRlZC1ieTogU0FUWUFOQVJB
WUFOQSBOQVJMQVBVUkFNIDxzYXR5YW5hcmxhcHVyYW1AZ21haWwuY29tPgpSZXZpZXdlZC1ieTog
QXNodXRvc2ggQmFwYXQgPGFzaHV0b3NoLmJhcGF0Lm9zc0BnbWFpbC5jb20+CkRpc2N1c3Npb246
IGh0dHBzOi8vcG9zdGdyLmVzL20vQ0FIZyUyQlFEY0U5d3A2bk9FQzNTQ1JROTBuckNPJTNEUUYl
MkJPWnExTUc4UWM2aG51c21vZ3F3JTQwbWFpbC5nbWFpbC5jb20KLS0tCiBzcmMvYmFja2VuZC9w
YXJzZXIvYW5hbHl6ZS5jICAgICAgICAgICAgICB8IDE1ICsrKysrKysrKysrKysrLQogc3JjL3Rl
c3QvcmVncmVzcy9leHBlY3RlZC9ncmFwaF90YWJsZS5vdXQgfCAxMyArKysrKysrKysrKysrCiBz
cmMvdGVzdC9yZWdyZXNzL3NxbC9ncmFwaF90YWJsZS5zcWwgICAgICB8ICA0ICsrKysKIDMgZmls
ZXMgY2hhbmdlZCwgMzEgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBh
L3NyYy9iYWNrZW5kL3BhcnNlci9hbmFseXplLmMgYi9zcmMvYmFja2VuZC9wYXJzZXIvYW5hbHl6
ZS5jCmluZGV4IDkzZmE2NmFlNTdjLi5mNzYyYjY1M2U3NyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tl
bmQvcGFyc2VyL2FuYWx5emUuYworKysgYi9zcmMvYmFja2VuZC9wYXJzZXIvYW5hbHl6ZS5jCkBA
IC0zODY2LDcgKzM4NjYsMTEgQEAgdHJhbnNmb3JtTG9ja2luZ0NsYXVzZShQYXJzZVN0YXRlICpw
c3RhdGUsIFF1ZXJ5ICpxcnksIExvY2tpbmdDbGF1c2UgKmxjLAogCQkJCQkJCQkJCSAgIGFsbHJl
bHMsIHRydWUpOwogCQkJCQlicmVhazsKIAkJCQlkZWZhdWx0OgotCQkJCQkvKiBpZ25vcmUgSk9J
TiwgU1BFQ0lBTCwgRlVOQ1RJT04sIFZBTFVFUywgQ1RFIFJURXMgKi8KKworCQkJCQkvKgorCQkJ
CQkgKiBpZ25vcmUgSk9JTiwgU1BFQ0lBTCwgRlVOQ1RJT04sIFZBTFVFUywgQ1RFLAorCQkJCQkg
KiBHUkFQSF9UQUJMRQorCQkJCQkgKi8KIAkJCQkJYnJlYWs7CiAJCQl9CiAJCX0KQEAgLTQwMDEs
NiArNDAwNSwxNSBAQCB0cmFuc2Zvcm1Mb2NraW5nQ2xhdXNlKFBhcnNlU3RhdGUgKnBzdGF0ZSwg
UXVlcnkgKnFyeSwgTG9ja2luZ0NsYXVzZSAqbGMsCiAJCQkJCQkJCQkJCUxDU19hc1N0cmluZyhs
Yy0+c3RyZW5ndGgpKSwKIAkJCQkJCQkJCSBwYXJzZXJfZXJycG9zaXRpb24ocHN0YXRlLCB0aGlz
cmVsLT5sb2NhdGlvbikpKTsKIAkJCQkJCQlicmVhazsKKwkJCQkJCWNhc2UgUlRFX0dSQVBIX1RB
QkxFOgorCQkJCQkJCWVyZXBvcnQoRVJST1IsCisJCQkJCQkJCQkoZXJyY29kZShFUlJDT0RFX0ZF
QVRVUkVfTk9UX1NVUFBPUlRFRCksCisJCQkJCQkJLyotLS0tLS0KKwkJCQkJCQkgIHRyYW5zbGF0
b3I6ICVzIGlzIGEgU1FMIHJvdyBsb2NraW5nIGNsYXVzZSBzdWNoIGFzIEZPUiBVUERBVEUgKi8K
KwkJCQkJCQkJCSBlcnJtc2coIiVzIGNhbm5vdCBiZSBhcHBsaWVkIHRvIGEgR1JBUEhfVEFCTEUi
LAorCQkJCQkJCQkJCQlMQ1NfYXNTdHJpbmcobGMtPnN0cmVuZ3RoKSksCisJCQkJCQkJCQkgcGFy
c2VyX2VycnBvc2l0aW9uKHBzdGF0ZSwgdGhpc3JlbC0+bG9jYXRpb24pKSk7CisJCQkJCQkJYnJl
YWs7CiAKIAkJCQkJCQkvKiBTaG91bGRuJ3QgYmUgcG9zc2libGUgdG8gc2VlIFJURV9SRVNVTFQg
aGVyZSAqLwogCmRpZmYgLS1naXQgYS9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL2dyYXBoX3Rh
YmxlLm91dCBiL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvZ3JhcGhfdGFibGUub3V0CmluZGV4
IGMyMjkyMTJjYjUxLi5jMDYyMmFiNzIzNCAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvcmVncmVzcy9l
eHBlY3RlZC9ncmFwaF90YWJsZS5vdXQKKysrIGIvc3JjL3Rlc3QvcmVncmVzcy9leHBlY3RlZC9n
cmFwaF90YWJsZS5vdXQKQEAgLTExMjksNCArMTEyOSwxNyBAQCBTRUxFQ1Qgc3JjLnZuYW1lLCBj
b3VudCgqKSBGUk9NIHYxIEFTIHNyYwogIHYxMyAgIHwgICAgIDEKICgzIHJvd3MpCiAKKy0tIExv
Y2tpbmcgY2xhdXNlIG9uIEdSQVBIX1RBQkxFCitTRUxFQ1QgKiBGUk9NIEdSQVBIX1RBQkxFIChn
MSBNQVRDSCAoc3JjIElTIHZsMSkgQ09MVU1OUyAoc3JjLnZuYW1lKSkgZ3QgRk9SIFVQREFURSBP
RiBndDsgIC0tIG5vdCBzdXBwb3J0ZWQKK0VSUk9SOiAgRk9SIFVQREFURSBjYW5ub3QgYmUgYXBw
bGllZCB0byBhIEdSQVBIX1RBQkxFCitMSU5FIDE6IC4uLk1BVENIIChzcmMgSVMgdmwxKSBDT0xV
TU5TIChzcmMudm5hbWUpKSBndCBGT1IgVVBEQVRFIE9GIGd0OworICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBeCitTRUxF
Q1QgKiBGUk9NIEdSQVBIX1RBQkxFIChnMSBNQVRDSCAoc3JjIElTIHZsMSkgQ09MVU1OUyAoc3Jj
LnZuYW1lKSkgZ3QgRk9SIFVQREFURTsgLS0gaWdub3JlZAorIHZuYW1lIAorLS0tLS0tLQorIHYx
MQorIHYxMgorIHYxMworKDMgcm93cykKKwogLS0gbGVhdmUgdGhlIG9iamVjdHMgYmVoaW5kIGZv
ciBwZ191cGdyYWRlL3BnX2R1bXAgdGVzdHMKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlZ3Jlc3Mv
c3FsL2dyYXBoX3RhYmxlLnNxbCBiL3NyYy90ZXN0L3JlZ3Jlc3Mvc3FsL2dyYXBoX3RhYmxlLnNx
bAppbmRleCBlMzBjOTA4ZGNjYy4uNDM3ZmIzYjAxZjIgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L3Jl
Z3Jlc3Mvc3FsL2dyYXBoX3RhYmxlLnNxbAorKysgYi9zcmMvdGVzdC9yZWdyZXNzL3NxbC9ncmFw
aF90YWJsZS5zcWwKQEAgLTYzNyw0ICs2MzcsOCBAQCBTRUxFQ1Qgc3JjLnZuYW1lLCBjb3VudCgq
KSBGUk9NIHYxIEFTIHNyYwogICBIQVZJTkcgY291bnQoKikgPj0gKFNFTEVDVCBjb3VudCgqKSBG
Uk9NIEdSQVBIX1RBQkxFIChnMSBNQVRDSCAoYSBJUyB2bDEgfCB2bDIpIENPTFVNTlMgKGEudm5h
bWUgQVMgbikpIFdIRVJFIG4gPSBzcmMudm5hbWUpCiAgIE9SREVSIEJZIHZuYW1lOwogCistLSBM
b2NraW5nIGNsYXVzZSBvbiBHUkFQSF9UQUJMRQorU0VMRUNUICogRlJPTSBHUkFQSF9UQUJMRSAo
ZzEgTUFUQ0ggKHNyYyBJUyB2bDEpIENPTFVNTlMgKHNyYy52bmFtZSkpIGd0IEZPUiBVUERBVEUg
T0YgZ3Q7ICAtLSBub3Qgc3VwcG9ydGVkCitTRUxFQ1QgKiBGUk9NIEdSQVBIX1RBQkxFIChnMSBN
QVRDSCAoc3JjIElTIHZsMSkgQ09MVU1OUyAoc3JjLnZuYW1lKSkgZ3QgRk9SIFVQREFURTsgLS0g
aWdub3JlZAorCiAtLSBsZWF2ZSB0aGUgb2JqZWN0cyBiZWhpbmQgZm9yIHBnX3VwZ3JhZGUvcGdf
ZHVtcCB0ZXN0cwotLSAKMi4zNC4xCgo=
--00000000000098c53e0653e5aeae--