Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Row pattern recognition
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <DBA30B4B-F611-46AC-807D-027D3CBEDEC9@gmail.com>
Date: Tue, 18 Nov 2025 13:05:52 +0800
Cc: david.g.johnston@gmail.com,
 vik@postgresfriends.org,
 jacob.champion@enterprisedb.com,
 er@xs4all.nl,
 peter@eisentraut.org,
 pgsql-hackers@postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <BAB55CE0-200D-4698-A3ED-CC3BF2D23140@gmail.com>
References: <20250816.174552.1094782787078410357.ishii@postgresql.org>
 <20250924.193523.694296225366690354.ishii@postgresql.org>
 <20251117.155715.178271279022552982.ishii@postgresql.org>
 <20251118.113320.572888636385647268.ishii@postgresql.org>
 <DBA30B4B-F611-46AC-807D-027D3CBEDEC9@gmail.com>
To: Tatsuo Ishii <ishii@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/BAB55CE0-200D-4698-A3ED-CC3BF2D23140%40gmail.com>
Precedence: bulk


> On Nov 18, 2025, at 13:03, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
> Hi Tatsuo-san,
>=20
> I just reviewed 0006 docs changes and 0001. I plan to slowly review =
the patch, maybe one commit a day.=20
>=20
>> On Nov 18, 2025, at 10:33, Tatsuo Ishii <ishii@postgresql.org> wrote:
>>=20
>> Attached are the v35 patches for Row pattern recognition.  In =
v34-0001
>> gram.y patch, %type for RPR were misplaced. v35-0001 fixes this. =
Other
>> patches are not changed.
>>=20
>> Best regards,
>> --
>> Tatsuo Ishii
>> SRA OSS K.K.
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp
>> =
<v35-0001-Row-pattern-recognition-patch-for-raw-parser.patch><v35-0002-Row=
-pattern-recognition-patch-parse-analysis.patch><v35-0003-Row-pattern-reco=
gnition-patch-rewriter.patch><v35-0004-Row-pattern-recognition-patch-plann=
er.patch><v35-0005-Row-pattern-recognition-patch-executor.patch><v35-0006-=
Row-pattern-recognition-patch-docs.patch><v35-0007-Row-pattern-recognition=
-patch-tests.patch><v35-0008-Row-pattern-recognition-patch-typedefs.list.p=
atch>
>=20
> I got a few comments, maybe just questions:
>=20
> 1 - 0001 - kwlist.h
> ```
> +PG_KEYWORD("define", DEFINE, RESERVED_KEYWORD, BARE_LABEL)
> ```
>=20
> Why do we add =E2=80=9Cdefine=E2=80=9D as a reserved keyword? =46rom =
the SQL example you put in 0006:
> ```
> <programlisting>
> SELECT company, tdate, price,
> first_value(price) OVER w,
> max(price) OVER w,
> count(price) OVER w
> FROM stock
> WINDOW w AS (
> PARTITION BY company
> ORDER BY tdate
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> AFTER MATCH SKIP PAST LAST ROW
> INITIAL
> PATTERN (LOWPRICE UP+ DOWN+)
> DEFINE
>  LOWPRICE AS price &lt;=3D 100,
>  UP AS price &gt; PREV(price),
>  DOWN AS price &lt; PREV(price)
> );
> </programlisting>
> ```
>=20
> PARTITION is at the same level as DEFINE, but it=E2=80=99s not defined =
as a reserved keyword:
> ```
> PG_KEYWORD("partition", PARTITION, UNRESERVED_KEYWORD, BARE_LABEL)
> ```
>=20
> Even in this patch,=E2=80=9Dinitial=E2=80=9D,=E2=80=9Dpast=E2=80=9D, =
=E2=80=9Cpattern=E2=80=9D and =E2=80=9Cseek=E2=80=9D are defined as =
unreserved, why? =20
>=20
> So I just want to clarify.
>=20
> 2 - 0001 - gram.y
> ```
> opt_row_pattern_initial_or_seek:
> INITIAL { $$ =3D true; }
> | SEEK
> {
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("SEEK is not supported"),
> errhint("Use INITIAL instead."),
> parser_errposition(@1)));
> }
> | /*EMPTY*/ { $$ =3D true; }
> ;
> ```
>=20
> As SEEK is specially listed here, I guess it might be supported in =
future. If that is true, would it be better to defer the semantic check =
to later parse phase, which would future work easier.
>=20
> 3 - 0001 - parsenodes.h
> ```
> +/*
> + * RowPatternCommonSyntax - raw representation of row pattern common =
syntax
> + *
> + */
> +typedef struct RPCommonSyntax
> +{
> + NodeTag type;
> + RPSkipTo rpSkipTo; /* Row Pattern AFTER MATCH SKIP type */
> + bool initial; /* true if <row pattern initial or seek> is
> + * initial */
> + List   *rpPatterns; /* PATTERN variables (list of A_Expr) */
> + List   *rpDefs; /* row pattern definitions clause (list of
> + * ResTarget) */
> +} RPCommonSyntax;
> +
> /*
>  * WindowDef - raw representation of WINDOW and OVER clauses
>  *
> @@ -593,6 +618,7 @@ typedef struct WindowDef
> char   *refname; /* referenced window name, if any */
> List   *partitionClause; /* PARTITION BY expression list */
> List   *orderClause; /* ORDER BY (list of SortBy) */
> + RPCommonSyntax *rpCommonSyntax; /* row pattern common syntax */
> ```
>=20
> RP fields are directly defined in WindowClause, then why do we need a =
wrapper of RPCommonSyntax? Can we directly define RP fields in WindowRef =
as well?
>=20

4 - 0001 - parsenodes.h
```
+	/* Row Pattern AFTER MACH SKIP clause */
```

Typo: MACH -> MATCH

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/