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>
 <20260505.090124.365339750969814137.ishii@postgresql.org>
In-Reply-To: <20260505.090124.365339750969814137.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Tue, 5 May 2026 13:26:56 +0900
Message-ID: 
 <CAAAe_zCwY4Lt_OS44DMuqua-szeDVVTgJDeH=c-d5Qco5v2sOQ@mail.gmail.com>
Subject: Re: Row pattern recognition
To: 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="000000000000c7027706510a75dc"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zCwY4Lt_OS44DMuqua-szeDVVTgJDeH%3Dc-d5Qco5v2sOQ%40mail.gmail.com>
Precedence: bulk

--000000000000c7027706510a75dc
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo,

Thanks for the close look.  Inline below, then a sketch of what I
plan to put in v48 so you can pick what to pull first.

v47 starts to check whether range variable qualified expressions are
> used in DEFINE clause. If used, raise an error. This is good because
> we don't support the syntax yet (pattern variable range var), or it's
> prohibited (from clause range var). However, the error message may not
> be appropreate for the case when complex data type is involved.
>
> CREATE TEMP TABLE item (name TEXT, amount INT);
> CREATE TABLE
> CREATE TEMP TABLE sales(items item);
> CREATE TABLE
> SELECT (items).name, (items).amount, count(*) OVER w
> FROM sales WINDOW w AS (
>     ORDER BY (items).name
>     ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
>     AFTER MATCH SKIP PAST LAST ROW
>     PATTERN (A+)
>     DEFINE A AS (A.items).amount > 10
> );
> ERROR:  pattern variable qualified column reference "a.items" is not
> supported in DEFINE clause
> LINE 7:     DEFINE A AS (A.items).amount > 10
>                          ^
> If I change the DEFINE clause to:
>
>     DEFINE A AS (sales.items).amount > 10
>
> I get:
>
> ERROR:  range variable qualified column reference "sales.items" is not
> allowed in DEFINE clause
> LINE 8:     DEFINE A AS (sales.items).amount > 10
>                          ^
>
> In both cases, "a.items" or "sales.items" in the error messages are
> not column names, therefore the wording "column reference" in the
> error messages are not appropriate.
>

Agreed.  Even for the simple non-composite cases the noun is fuzzy
once A_Indirection enters the picture.  "expression" reads naturally
in both cases.


> In order to fix the issue, I think we need to add code to understand
> range var qualification form "A.item" or "sales.items" in complex data
> types case. But is it worth the trouble? The reword is just nicer
> error messages. If we support MEASURES, the code is useful. But so far
> we have decided to not support it in the first cut of RPR.
>

Not now.  Teaching the pre-check about the surrounding A_Indirection
duplicates work that MEASURES will need anyway, and the only visible
gain before MEASURES is a more accurate echo of the source text.

Instead I'll mark the limitation in three places so future MEASURES
work can't miss it: a block comment above the pre-check that names
the limitation and the revisit point, an XXX cross-reference in
parse_rpr.c pointing back to the pre-check, and composite-type cases
in rpr_base whose expected output quotes only the ColumnRef portion
-- so when indirection-aware quoting lands, those outputs churn as
a tripwire.


> Maybe we should just change the error messages something like below
> for now?
>
> ERROR:  pattern variable qualified expression "a.items" is not supported
> in DEFINE clause
> ERROR:  range variable qualified expression "sales.items" is not allowed
> in DEFINE clause


Yes -- planned for v48.  While I'm in that path I'll also clean up
two adjacent issues:

  - the pre-check is binary, so an unknown qualifier (typo,
    undefined name) is misreported as a range-variable reference
    with SYNTAX_ERROR instead of falling through to the standard
    "column does not exist" / "missing FROM-clause entry"
    diagnostic;

  - parse_rpr.c and the SELECT docs claim DEFINE-name "collection"
    and "filtering during planning"; the actual behavior is
    validate-and-reject at parse analysis.


------------------------------------------------------------------
v48 follow-up plan
------------------------------------------------------------------

The items below are independent, so they can ship as separate
patches or as a single batched posting -- whichever you prefer.
Order is just for narrative; nothing depends on anything else.

  [A] Sizable refactor: collapse the four DEFINE walkers across
      parser/planner/executor into a single phase-tagged
      traversal.  On that base, reject volatile / NextValueExpr
      in DEFINE (the NFA may re-evaluate predicates during
      backtracking; STABLE / IMMUTABLE remain accepted), and
      bundle a STABLE/IMMUTABLE baseline test as a guard against
      accidental over-rejection.

  [B] Make the empty-match path observable in tests: replace the
      stale XXX comments in rpr_nfa with the actual behavior, and
      add EXPLAIN ANALYZE coverage in rpr_explain that surfaces
      "NFA: N matched (len 0/0/0.0)" so the NFA-found-but-window-
      empty case is regression-visible.

  [C] Trim the per-advance NFA visited-bitmap reset to a high-water
      mark range instead of the full bitmap.  Tradeoff: two int16
      comparisons added per visit, paying off for larger NFAs but
      added overhead for single-word bitmaps; semantics unchanged.
      I'll leave the decision to apply to your judgment.

  [D] DEFINE qualifier diagnostic: tri-classify (pattern var /
      range var / fall-through), reword to "expression", add
      unknown-qualifier and composite-type tests, and sync the
      adjacent stale comments and SELECT doc.

If any of [A]..[D] looks misjudged or you'd prefer a different
slicing, I'll reshape before posting.

Best,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo,<br><br>Thanks for the close lo=
ok.=C2=A0 Inline below, then a sketch of what I<br>plan to put in v48 so yo=
u can pick what to pull first.<br><br></div><div class=3D"gmail_quote gmail=
_quote_container"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px=
 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">v47 sta=
rts to check whether range variable qualified expressions are<br>
used in DEFINE clause. If used, raise an error. This is good because<br>
we don&#39;t support the syntax yet (pattern variable range var), or it&#39=
;s<br>
prohibited (from clause range var). However, the error message may not<br>
be appropreate for the case when complex data type is involved.<br>
<br>
CREATE TEMP TABLE item (name TEXT, amount INT);<br>
CREATE TABLE<br>
CREATE TEMP TABLE sales(items item);<br>
CREATE TABLE<br>
SELECT (items).name, (items).amount, count(*) OVER w<br>
FROM sales WINDOW w AS (<br>
=C2=A0 =C2=A0 ORDER BY (items).name<br>
=C2=A0 =C2=A0 ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING<br>
=C2=A0 =C2=A0 AFTER MATCH SKIP PAST LAST ROW<br>
=C2=A0 =C2=A0 PATTERN (A+)<br>
=C2=A0 =C2=A0 DEFINE A AS (A.items).amount &gt; 10<br>
);<br>
ERROR:=C2=A0 pattern variable qualified column reference &quot;a.items&quot=
; is not supported in DEFINE clause<br>
LINE 7:=C2=A0 =C2=A0 =C2=A0DEFINE A AS (A.items).amount &gt; 10<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0^<br>
If I change the DEFINE clause to:<br>
<br>
=C2=A0 =C2=A0 DEFINE A AS (sales.items).amount &gt; 10<br>
<br>
I get:<br>
<br>
ERROR:=C2=A0 range variable qualified column reference &quot;sales.items&qu=
ot; is not allowed in DEFINE clause<br>
LINE 8:=C2=A0 =C2=A0 =C2=A0DEFINE A AS (sales.items).amount &gt; 10<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0^<br>
<br>
In both cases, &quot;a.items&quot; or &quot;sales.items&quot; in the error =
messages are<br>
not column names, therefore the wording &quot;column reference&quot; in the=
<br>
error messages are not appropriate.<br></blockquote><div><br></div><div>Agr=
eed.=C2=A0 Even for the simple non-composite cases the noun is fuzzy<br>onc=
e A_Indirection enters the picture. =C2=A0&quot;expression&quot; reads natu=
rally<br>in both cases.<br></div><div>=C2=A0</div><blockquote class=3D"gmai=
l_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,20=
4,204);padding-left:1ex">In order to fix the issue, I think we need to add =
code to understand<br>
range var qualification form &quot;A.item&quot; or &quot;sales.items&quot; =
in complex data<br>
types case. But is it worth the trouble? The reword is just nicer<br>
error messages. If we support MEASURES, the code is useful. But so far<br>
we have decided to not support it in the first cut of RPR.<br></blockquote>=
<div><br></div>Not now.=C2=A0 Teaching the pre-check about the surrounding =
A_Indirection<br>duplicates work that MEASURES will need anyway, and the on=
ly visible<br>gain before MEASURES is a more accurate echo of the source te=
xt.<br><br>Instead I&#39;ll mark the limitation in three places so future M=
EASURES<br>work can&#39;t miss it: a block comment above the pre-check that=
 names<br>the limitation and the revisit point, an XXX cross-reference in<b=
r>parse_rpr.c pointing back to the pre-check, and composite-type cases<br>i=
n rpr_base whose expected output quotes only the ColumnRef portion<br>-- so=
 when indirection-aware quoting lands, those outputs churn as<br>a tripwire=
.=C2=A0<br><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e=
x">
Maybe we should just change the error messages something like below<br>
for now?<br>
<br>
ERROR:=C2=A0 pattern variable qualified expression &quot;a.items&quot; is n=
ot supported in DEFINE clause<br>
ERROR:=C2=A0 range variable qualified expression &quot;sales.items&quot; is=
 not allowed in DEFINE clause</blockquote><br>Yes -- planned for v48.=C2=A0=
 While I&#39;m in that path I&#39;ll also clean up<br>two adjacent issues:<=
br><br>=C2=A0 - the pre-check is binary, so an unknown qualifier (typo,<br>=
=C2=A0 =C2=A0 undefined name) is misreported as a range-variable reference<=
br>=C2=A0 =C2=A0 with SYNTAX_ERROR instead of falling through to the standa=
rd<br>=C2=A0 =C2=A0 &quot;column does not exist&quot; / &quot;missing FROM-=
clause entry&quot;<br>=C2=A0 =C2=A0 diagnostic;<br><br>=C2=A0 - parse_rpr.c=
 and the SELECT docs claim DEFINE-name &quot;collection&quot;<br>=C2=A0 =C2=
=A0 and &quot;filtering during planning&quot;; the actual behavior is<br>=
=C2=A0 =C2=A0 validate-and-reject at parse analysis.<br><br><br>-----------=
-------------------------------------------------------<br>v48 follow-up pl=
an<br>------------------------------------------------------------------<br=
><br>The items below are independent, so they can ship as separate<br>patch=
es or as a single batched posting -- whichever you prefer.<br>Order is just=
 for narrative; nothing depends on anything else.<br><br>=C2=A0 [A] Sizable=
 refactor: collapse the four DEFINE walkers across<br>=C2=A0 =C2=A0 =C2=A0 =
parser/planner/executor into a single phase-tagged<br>=C2=A0 =C2=A0 =C2=A0 =
traversal.=C2=A0 On that base, reject volatile / NextValueExpr<br>=C2=A0 =
=C2=A0 =C2=A0 in DEFINE (the NFA may re-evaluate predicates during<br>=C2=
=A0 =C2=A0 =C2=A0 backtracking; STABLE / IMMUTABLE remain accepted), and<br=
>=C2=A0 =C2=A0 =C2=A0 bundle a STABLE/IMMUTABLE baseline test as a guard ag=
ainst<br>=C2=A0 =C2=A0 =C2=A0 accidental over-rejection.<br><br>=C2=A0 [B] =
Make the empty-match path observable in tests: replace the<br>=C2=A0 =C2=A0=
 =C2=A0 stale XXX comments in rpr_nfa with the actual behavior, and<br>=C2=
=A0 =C2=A0 =C2=A0 add EXPLAIN ANALYZE coverage in rpr_explain that surfaces=
<br>=C2=A0 =C2=A0 =C2=A0 &quot;NFA: N matched (len 0/0/0.0)&quot; so the NF=
A-found-but-window-<br>=C2=A0 =C2=A0 =C2=A0 empty case is regression-visibl=
e.<br><br>=C2=A0 [C] Trim the per-advance NFA visited-bitmap reset to a hig=
h-water<br>=C2=A0 =C2=A0 =C2=A0 mark range instead of the full bitmap.=C2=
=A0 Tradeoff: two int16<br>=C2=A0 =C2=A0 =C2=A0 comparisons added per visit=
, paying off for larger NFAs but<br>=C2=A0 =C2=A0 =C2=A0 added overhead for=
 single-word bitmaps; semantics unchanged.<br>=C2=A0 =C2=A0 =C2=A0 I&#39;ll=
 leave the decision to apply to your judgment.<br><br>=C2=A0 [D] DEFINE qua=
lifier diagnostic: tri-classify (pattern var /<br>=C2=A0 =C2=A0 =C2=A0 rang=
e var / fall-through), reword to &quot;expression&quot;, add<br>=C2=A0 =C2=
=A0 =C2=A0 unknown-qualifier and composite-type tests, and sync the<br>=C2=
=A0 =C2=A0 =C2=A0 adjacent stale comments and SELECT doc.<br><br>If any of =
[A]..[D] looks misjudged or you&#39;d prefer a different<br>slicing, I&#39;=
ll reshape before posting.<br><br>Best,<br>Henson=C2=A0</div></div>

--000000000000c7027706510a75dc--