MIME-Version: 1.0
References: 
 <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
 <CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
 <20260604.132108.405136284364833955.ishii@postgresql.org>
 <20260609.171307.1883356507067957349.ishii@postgresql.org>
 <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
 <CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com>
 <CACJufxHJFNBJ0vHCCLestWV5b7DF5e4VzfmovqGKBGgqg+rcGA@mail.gmail.com>
 <CAAAe_zBY0rrgf+tKXMUc-Y3nDDD69hddRBKopEKAZobhY=Cy-Q@mail.gmail.com>
 <CAAAe_zDYxq0d3exCDwvKncD0kaL2uehDir6HXo4r5DXMitKrSg@mail.gmail.com>
 <CACJufxG57=ddtbN=5RZCzhxWDYXvocKmB7NtZy+DoqZuhxb_DA@mail.gmail.com>
 <CAAAe_zB4TMHnpaOrwYp7dKs553q2474ZXRytGfYOfYp4DdrgiQ@mail.gmail.com>
 <CACJufxFAQhbOD9EVCTAy-VwDbG4446N10GsxCcgdpFnjHO1Efw@mail.gmail.com>
 <CACJufxG6rKfd0WtsVZgzcoh8vOEwo_8UDWkuOd888ATavNv_uw@mail.gmail.com>
In-Reply-To: 
 <CACJufxG6rKfd0WtsVZgzcoh8vOEwo_8UDWkuOd888ATavNv_uw@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 17 Jun 2026 16:33:26 +0900
Message-ID: 
 <CAAAe_zDfrGSTqhgy8vOC1K6mzDsqVJGEmMy5N84G6=5EJT6--g@mail.gmail.com>
Subject: Re: Row pattern recognition
To: jian he <jian.universality@gmail.com>, Tatsuo Ishii <ishii@postgresql.org>
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/alternative; boundary="00000000000004032d06546e14d1"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zDfrGSTqhgy8vOC1K6mzDsqVJGEmMy5N84G6%3D5EJT6--g%40mail.gmail.com>
Precedence: bulk

--00000000000004032d06546e14d1
Content-Type: text/plain; charset="UTF-8"

Hi Jian and Tatsuo,

Thanks for the patch and the careful review.

Tatsuo, item 1 below (attribute notation inside a DEFINE clause) is a
question for you; the rest is feedback on Jian's patch.

> /* arity: a value expression and an optional offset */
> Typo: arity

"arity" may be an unfamiliar term, so I'll reword the comment in plainer
language ("takes a value expression and an optional offset").

> Simplify ParseFuncOrColumn:
> It now routes to ParseRPRNavCall exclusively when ParseExprKind is
> EXPR_KIND_RPR_DEFINE and not column projection and list_length(funcname)
> == 1. Original behavior is preserved otherwise.
> Centralize error handling:
> Treat RPR navigation as FUNCDETAIL_NORMAL to reuse the common error
> handling in ParseFuncOrColumn, effectively stripping redundant error
> checks from ParseRPRNavCall.

I'd take this structural part -- it's a clear cleanup: ParseRPRNavCall
drops the duplicated decoration checks while the common path gives the
identical messages, with no change in behavior or output.

Two user-visible changes in the patch I'd rather settle on their own
before taking them:

1. Attribute notation inside a DEFINE clause, e.g. (f).prev.

   The guard this change removes is one I deliberately left undecided
   during development (hence the XXX comment), so I'd keep it for now and
   ask here.  Without it, (f).prev with no such field gives a generic
   "column \"prev\" not found ..." instead of the dedicated "cannot use
   row pattern navigation function PREV in attribute notation".  Three
   options:

   (a) Treat (f).prev as an ordinary function (prev(f)), the same as
       outside a DEFINE clause -- which is what the patch does.

   (b) Treat (f).prev as the navigation function -- read the attribute
       notation as navigation.  An ordinary function of that name is still
       reachable as public.prev(...).

   (c) Reject the ambiguous (f).prev with a dedicated error (what is
       currently committed), rather than resolving it one way or the
       other.

   My own leaning is actually (a) -- it keeps attribute notation behaving
   the same inside and outside a DEFINE clause.  (c) is what's in the tree
   now, and either way it changes the user-visible error and SQLSTATE, so
   I'd rather settle this explicitly than let the refactor decide it
   silently.  Tatsuo, what do you think?

2. The offset type-mismatch message.

   The patch rephrases

     offset argument of %s must be type bigint, not type %s
   to
     %s offset argument of type %s cannot be coerced to the expected
     bigint   (+ a hint)

   The behavior is identical, so I'd keep the original wording.  "argument
   ... must be type X, not type Y" is the established phrasing -- it's what
   coerce_to_boolean() produces, and the non-boolean DEFINE error in this
   same feature already uses it.  Rewording only the offset message would
   be inconsistent with that, for no behavioral gain.

One small process note: for patches that aren't really being proposed to
the list yet -- ones still under review, or throwaway implementations
written just to analyze a problem -- could you send them off-list or as a
pull request instead?  That keeps the thread focused on what's actually
being proposed for the patch.

Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi Jian and Tatsuo,<br><br>Thanks for the=
 patch and the careful review.<br><br>Tatsuo, item 1 below (attribute notat=
ion inside a DEFINE clause) is a<br>question for you; the rest is feedback =
on Jian&#39;s patch.<br><br>&gt; /* arity: a value expression and an option=
al offset */<br>&gt; Typo: arity<br><br>&quot;arity&quot; may be an unfamil=
iar term, so I&#39;ll reword the comment in plainer<br>language (&quot;take=
s a value expression and an optional offset&quot;).<br><br>&gt; Simplify Pa=
rseFuncOrColumn:<br>&gt; It now routes to ParseRPRNavCall exclusively when =
ParseExprKind is<br>&gt; EXPR_KIND_RPR_DEFINE and not column projection and=
 list_length(funcname)<br>&gt; =3D=3D 1. Original behavior is preserved oth=
erwise.<br>&gt; Centralize error handling:<br>&gt; Treat RPR navigation as =
FUNCDETAIL_NORMAL to reuse the common error<br>&gt; handling in ParseFuncOr=
Column, effectively stripping redundant error<br>&gt; checks from ParseRPRN=
avCall.<br><br>I&#39;d take this structural part -- it&#39;s a clear cleanu=
p: ParseRPRNavCall<br>drops the duplicated decoration checks while the comm=
on path gives the<br>identical messages, with no change in behavior or outp=
ut.<br><br>Two user-visible changes in the patch I&#39;d rather settle on t=
heir own<br>before taking them:<br><br>1. Attribute notation inside a DEFIN=
E clause, e.g. (f).prev.<br><br>=C2=A0 =C2=A0The guard this change removes =
is one I deliberately left undecided<br>=C2=A0 =C2=A0during development (he=
nce the XXX comment), so I&#39;d keep it for now and<br>=C2=A0 =C2=A0ask he=
re.=C2=A0 Without it, (f).prev with no such field gives a generic<br>=C2=A0=
 =C2=A0&quot;column \&quot;prev\&quot; not found ...&quot; instead of the d=
edicated &quot;cannot use<br>=C2=A0 =C2=A0row pattern navigation function P=
REV in attribute notation&quot;.=C2=A0 Three<br>=C2=A0 =C2=A0options:<br><b=
r>=C2=A0 =C2=A0(a) Treat (f).prev as an ordinary function (prev(f)), the sa=
me as<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0outside a DEFINE clause -- which is wha=
t the patch does.<br><br>=C2=A0 =C2=A0(b) Treat (f).prev as the navigation =
function -- read the attribute<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0notation as na=
vigation.=C2=A0 An ordinary function of that name is still<br>=C2=A0 =C2=A0=
 =C2=A0 =C2=A0reachable as public.prev(...).<br><br>=C2=A0 =C2=A0(c) Reject=
 the ambiguous (f).prev with a dedicated error (what is<br>=C2=A0 =C2=A0 =
=C2=A0 =C2=A0currently committed), rather than resolving it one way or the<=
br>=C2=A0 =C2=A0 =C2=A0 =C2=A0other.<br><br>=C2=A0 =C2=A0My own leaning is =
actually (a) -- it keeps attribute notation behaving<br>=C2=A0 =C2=A0the sa=
me inside and outside a DEFINE clause. =C2=A0(c) is what&#39;s in the tree<=
br>=C2=A0 =C2=A0now, and either way it changes the user-visible error and S=
QLSTATE, so<br>=C2=A0 =C2=A0I&#39;d rather settle this explicitly than let =
the refactor decide it<br>=C2=A0 =C2=A0silently.=C2=A0 Tatsuo, what do you =
think?<br><br>2. The offset type-mismatch message.<br><br>=C2=A0 =C2=A0The =
patch rephrases<br><br>=C2=A0 =C2=A0 =C2=A0offset argument of %s must be ty=
pe bigint, not type %s<br>=C2=A0 =C2=A0to<br>=C2=A0 =C2=A0 =C2=A0%s offset =
argument of type %s cannot be coerced to the expected<br>=C2=A0 =C2=A0 =C2=
=A0bigint =C2=A0 (+ a hint)<br><br>=C2=A0 =C2=A0The behavior is identical, =
so I&#39;d keep the original wording. =C2=A0&quot;argument<br>=C2=A0 =C2=A0=
... must be type X, not type Y&quot; is the established phrasing -- it&#39;=
s what<br>=C2=A0 =C2=A0coerce_to_boolean() produces, and the non-boolean DE=
FINE error in this<br>=C2=A0 =C2=A0same feature already uses it.=C2=A0 Rewo=
rding only the offset message would<br>=C2=A0 =C2=A0be inconsistent with th=
at, for no behavioral gain.<br><br>One small process note: for patches that=
 aren&#39;t really being proposed to<br>the list yet -- ones still under re=
view, or throwaway implementations<br>written just to analyze a problem -- =
could you send them off-list or as a<br>pull request instead?=C2=A0 That ke=
eps the thread focused on what&#39;s actually<br>being proposed for the pat=
ch.<br><br>Best regards,<br>Henson</div></div>

--00000000000004032d06546e14d1--