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: <20251118.113320.572888636385647268.ishii@postgresql.org>
Date: Tue, 18 Nov 2025 13:03:08 +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: <DBA30B4B-F611-46AC-807D-027D3CBEDEC9@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>
To: Tatsuo Ishii <ishii@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/DBA30B4B-F611-46AC-807D-027D3CBEDEC9%40gmail.com>
Precedence: bulk

Hi Tatsuo-san,

I just reviewed 0006 docs changes and 0001. I plan to slowly review the =
patch, maybe one commit a day.=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>

I got a few comments, maybe just questions:

1 - 0001 - kwlist.h
```
+PG_KEYWORD("define", DEFINE, RESERVED_KEYWORD, BARE_LABEL)
```

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>
```

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)
```

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

So I just want to clarify.

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; }
		;
```

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.

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 */
```

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?

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