MIME-Version: 1.0
References: <20260427.174220.1939160662649810289.ishii@postgresql.org>
 <CAAAe_zBdwAUDNs_WFdLkFF=ewhkDkv-AqizVEVzhsfremGFb4w@mail.gmail.com>
 <20260502.140304.670813149418899420.ishii@postgresql.org>
 <20260524.171139.1720602000749196206.ishii@postgresql.org>
In-Reply-To: <20260524.171139.1720602000749196206.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 27 May 2026 07:31:02 +0900
Message-ID: 
 <CAAAe_zD7vCLCb+vxpO3P-NsDUZ=JcN8EGCV0dz0BNgBKsbOGcQ@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>, jian he <jian.universality@gmail.com>
Cc: zsolt.parragi@percona.com, sjjang112233@gmail.com,
 vik@postgresfriends.org,
	er@xs4all.nl, jacob.champion@enterprisedb.com, david.g.johnston@gmail.com,
	peter@eisentraut.org, li.evan.chao@gmail.com, pgsql-hackers@postgresql.org
Content-Type: multipart/mixed; boundary="000000000000813d2e0652c00de8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zD7vCLCb%2BvxpO3P-NsDUZ%3DJcN8EGCV0dz0BNgBKsbOGcQ%40mail.gmail.com>
Precedence: bulk

--000000000000813d2e0652c00de8
Content-Type: multipart/alternative; boundary="000000000000813d2d0652c00de6"

--000000000000813d2d0652c00de6
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo, jian,

While looking into v47, I noticed that
> raw_expression_tree_walker_impl() lacks tracking RPCommonSyntax and
> its children nodes. Probably this does nothing wrong with RPR
> functionalities but just for completeness I created a patch on top of
> v47.


Thanks for catching this. I applied the patch and the RPR regress passes
cleanly. I also re-ran it with debug_raw_expression_coverage_test turned
on (on an assert-enabled build), and the full regress is green as well.

One observation while testing: the GUC catches missing case handlers
once the walker actually reaches a node, but it cannot flag a missing
WALK on its own -- if no caller drives the walker into a subtree, the
omission stays silent. So your inspection was the part that found the
gap; the GUC just confirms the patch closes it. With the patch in, RPR
raw subtrees are on the safety net for any future node-type additions.

I'll include the patch in v48 as nocfbot-0015. My suggestion would
be to defer the fold until the jian-response patches (numbered from
0016 onward, which I'll be sending shortly) have also gone through a
review round, so the whole bundle can land together in one pass.
Patch attached below for convenience.


jian -- thanks for the thorough review. It covers a lot of ground,
and I'm still working through it. Current expectation is that most
items will be accepted; a few smaller ones may end up with a
different conclusion than your suggestion, and those are still under
analysis on my side.

The plan is to turn the responses into a patch series and send them
out for another round of review. More to follow once the batch is in
shape.


Thanks,
Henson

--000000000000813d2d0652c00de6
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo, jian,<br></div><div dir=3D"ltr=
"><br></div><div class=3D"gmail_quote gmail_quote_container"><blockquote cl=
ass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid=
 rgb(204,204,204);padding-left:1ex">While looking into v47, I noticed that<=
br>
raw_expression_tree_walker_impl() lacks tracking RPCommonSyntax and<br>
its children nodes. Probably this does nothing wrong with RPR<br>
functionalities but just for completeness I created a patch on top of<br>
v47.</blockquote><div><br></div><div>Thanks for catching this. I applied th=
e patch and the RPR regress passes<br>cleanly. I also re-ran it with debug_=
raw_expression_coverage_test turned<br>on (on an assert-enabled build), and=
 the full regress is green as well.<br><br>One observation while testing: t=
he GUC catches missing case handlers<br>once the walker actually reaches a =
node, but it cannot flag a missing<br>WALK on its own -- if no caller drive=
s the walker into a subtree, the<br>omission stays silent. So your inspecti=
on was the part that found the<br>gap; the GUC just confirms the patch clos=
es it. With the patch in, RPR<br>raw subtrees are on the safety net for any=
 future node-type additions.<br><br>I&#39;ll include the patch in v48 as no=
cfbot-0015. My suggestion would<br>be to defer the fold until the jian-resp=
onse patches (numbered from<br>0016 onward, which I&#39;ll be sending short=
ly) have also gone through a<br>review round, so the whole bundle can land =
together in one pass.<br>Patch attached below for convenience.<br><br><br>j=
ian -- thanks for the thorough review. It covers a lot of ground,<br>and I&=
#39;m still working through it. Current expectation is that most<br>items w=
ill be accepted; a few smaller ones may end up with a<br>different conclusi=
on than your suggestion, and those are still under<br>analysis on my side.<=
br><br>The plan is to turn the responses into a patch series and send them<=
br>out for another round of review. More to follow once the batch is in<br>=
shape.<br></div><br><br>Thanks,<br>Henson<br></div></div>

--000000000000813d2d0652c00de6--

--000000000000813d2e0652c00de8
Content-Type: text/plain; charset="US-ASCII";
 name="nocfbot-0015-RPR-raw-walker-coverage.txt"
Content-Disposition: attachment;
	filename="nocfbot-0015-RPR-raw-walker-coverage.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_mpn78e1i1>
X-Attachment-Id: f_mpn78e1i1

RnJvbSBkYmVmZGQ0N2FiNjNjMDk5YjhjYjllN2E3YWYzOTA3ZWQyMjEyZGNkIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBIZW5zb24gQ2hvaSA8YXNzYW0yNThAZ21haWwuY29tPgpEYXRl
OiBXZWQsIDI3IE1heSAyMDI2IDA3OjAyOjM4ICswOTAwClN1YmplY3Q6IFtQQVRDSF0gQWRkIHJh
d19leHByZXNzaW9uX3RyZWVfd2Fsa2VyIGNvdmVyYWdlIGZvciBSUFIgcmF3IG5vZGVzCgpXaW5k
b3dEZWYucnBDb21tb25TeW50YXggd2FzIG5vdCB3YWxrZWQsIGFuZCB0aGVyZSB3ZXJlIG5vIGNh
c2UKYXJtcyBmb3IgVF9SUENvbW1vblN5bnRheCBvciBUX1JQUlBhdHRlcm5Ob2RlLiAgUlBSIGNv
cmUgd2FzCnVuYWZmZWN0ZWQgLS0gY29udGFpbl9ycHJfd2Fsa2VyKCkgaW4gcGFyc2VfY3RlLmMg
aW50ZXJjZXB0cwpXaW5kb3dEZWYgYmVmb3JlIGRlbGVnYXRpbmcgLS0gYnV0IGRlYnVnX3Jhd19l
eHByZXNzaW9uX2NvdmVyYWdlX3Rlc3QKc2lsZW50bHkgc2tpcHBlZCB0aGVzZSBzdWJ0cmVlcywg
bGVhdmluZyBhbnkgZnV0dXJlIHJhdy1ub2RlCm9taXNzaW9uIG9uIHRoZSBSUFIgc2lkZSB1bmRl
dGVjdGVkLgotLS0KIHNyYy9iYWNrZW5kL25vZGVzL25vZGVGdW5jcy5jIHwgMjAgKysrKysrKysr
KysrKysrKysrKysKIDEgZmlsZSBjaGFuZ2VkLCAyMCBpbnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0
IGEvc3JjL2JhY2tlbmQvbm9kZXMvbm9kZUZ1bmNzLmMgYi9zcmMvYmFja2VuZC9ub2Rlcy9ub2Rl
RnVuY3MuYwppbmRleCA3MzRiYjA1NTRmZS4uMTAxYzAzYjZhZTggMTAwNjQ0Ci0tLSBhL3NyYy9i
YWNrZW5kL25vZGVzL25vZGVGdW5jcy5jCisrKyBiL3NyYy9iYWNrZW5kL25vZGVzL25vZGVGdW5j
cy5jCkBAIC00NjQxLDYgKzQ2NDEsOCBAQCByYXdfZXhwcmVzc2lvbl90cmVlX3dhbGtlcl9pbXBs
KE5vZGUgKm5vZGUsCiAJCQkJCXJldHVybiB0cnVlOwogCQkJCWlmIChXQUxLKHdkLT5lbmRPZmZz
ZXQpKQogCQkJCQlyZXR1cm4gdHJ1ZTsKKwkJCQlpZiAoV0FMSyh3ZC0+cnBDb21tb25TeW50YXgp
KQorCQkJCQlyZXR1cm4gdHJ1ZTsKIAkJCX0KIAkJCWJyZWFrOwogCQljYXNlIFRfUmFuZ2VTdWJz
ZWxlY3Q6CkBAIC00ODk2LDYgKzQ4OTgsMjQgQEAgcmF3X2V4cHJlc3Npb25fdHJlZV93YWxrZXJf
aW1wbChOb2RlICpub2RlLAogCQkJCQlyZXR1cm4gdHJ1ZTsKIAkJCX0KIAkJCWJyZWFrOworCQlj
YXNlIFRfUlBDb21tb25TeW50YXg6CisJCQl7CisJCQkJUlBDb21tb25TeW50YXggKnJjID0gKFJQ
Q29tbW9uU3ludGF4ICopIG5vZGU7CisKKwkJCQlpZiAoV0FMSyhyYy0+cnBQYXR0ZXJuKSkKKwkJ
CQkJcmV0dXJuIHRydWU7CisJCQkJaWYgKFdBTEsocmMtPnJwRGVmcykpCisJCQkJCXJldHVybiB0
cnVlOworCQkJfQorCQkJYnJlYWs7CisJCWNhc2UgVF9SUFJQYXR0ZXJuTm9kZToKKwkJCXsKKwkJ
CQlSUFJQYXR0ZXJuTm9kZSAqcnAgPSAoUlBSUGF0dGVybk5vZGUgKikgbm9kZTsKKworCQkJCWlm
IChXQUxLKHJwLT5jaGlsZHJlbikpCisJCQkJCXJldHVybiB0cnVlOworCQkJfQorCQkJYnJlYWs7
CiAJCWRlZmF1bHQ6CiAJCQllbG9nKEVSUk9SLCAidW5yZWNvZ25pemVkIG5vZGUgdHlwZTogJWQi
LAogCQkJCSAoaW50KSBub2RlVGFnKG5vZGUpKTsKLS0gCjIuNTAuMSAoQXBwbGUgR2l0LTE1NSkK
Cg==
--000000000000813d2e0652c00de8--