Date: Fri, 21 Nov 2025 14:57:08 +0900 (JST)
Message-Id: <20251121.145708.931561343800399405.ishii@postgresql.org>
To: li.evan.chao@gmail.com
Cc: vik@postgresfriends.org, ishii@postgresql.org,
 david.g.johnston@gmail.com, jacob.champion@enterprisedb.com, er@xs4all.nl,
 peter@eisentraut.org, pgsql-hackers@postgresql.org
Subject: Re: Row pattern recognition
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: <66415FA2-B539-40C1-A58D-B28E373BDB26@gmail.com>
References: <DBA30B4B-F611-46AC-807D-027D3CBEDEC9@gmail.com>
	<59fe567f-ba41-4cc2-b04a-a830288ea929@postgresfriends.org>
	<66415FA2-B539-40C1-A58D-B28E373BDB26@gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=iso-2022-jp
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20251121.145708.931561343800399405.ishii%40postgresql.org>
Precedence: bulk

Hi Chao,

>> On Nov 18, 2025, at 19:19, Vik Fearing <vik@postgresfriends.org> wrote:
>> 
>> 
>> Because of position. Without making DEFINE a reserved keyword, how do you know that it isn't another variable in the PATTERN clause?
>> 
> 
> Ah, thanks for the clarification. Now I got it.
> 
> I’m continue to review 0002.

Thanks for the review!

> 5 - 0002 - parse_clause.c
> ```
> +		ereport(ERROR,
> +				(errcode(ERRCODE_SYNTAX_ERROR),
> +				 errmsg("FRAME must start at current row when row patttern recognition is used")));
> ```
> 
> Nit typo: patttern -> pattern

Will fix (this involves regression test change because this changes
the ERROR messages in the expected file).

> 6 - 0002 - parse_clause.c
> ```
> +	/* DEFINE variable name initials */
> +	static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz”;
> ```
> 
> This string can also be const, so “static const char *”

Agreed. Will fix.

> 7 - 0002 - parse_clause.c
> ```
> +	/*
> +	 * Create list of row pattern DEFINE variable name's initial. We assign
> +	 * [a-z] to them (up to 26 variable names are allowed).
> +	 */
> +	restargets = NIL;
> +	i = 0;
> +	initialLen = strlen(defineVariableInitials);
> +
> +	foreach(lc, windef->rpCommonSyntax->rpDefs)
> +	{
> +		char		initial[2];
> +
> +		restarget = (ResTarget *) lfirst(lc);
> +		name = restarget->name;
> ```
> 
> Looks like “name” is not used inside “foreach”.

Oops. Will fix.

> 8 - 0002 - parse_clause.c
> ```
> +	/*
> +	 * Create list of row pattern DEFINE variable name's initial. We assign
> +	 * [a-z] to them (up to 26 variable names are allowed).
> +	 */
> +	restargets = NIL;
> +	i = 0;
> +	initialLen = strlen(defineVariableInitials);
> +
> +	foreach(lc, windef->rpCommonSyntax->rpDefs)
> +	{
> +		char		initial[2];
> ```
> 
> I guess this “foreach” for build initial list can be combined into the previous foreach loop of checking duplication. If an def has no dup, then assign an initial to it.

You are right. Will change.

> 9 - 0002 - parse_clause.c
> ```
> +static void
> +transformPatternClause(ParseState *pstate, WindowClause *wc,
> +					   WindowDef *windef)
> +{
> +	ListCell   *lc;
> +
> +	/*
> +	 * Row Pattern Common Syntax clause exists?
> +	 */
> +	if (windef->rpCommonSyntax == NULL)
> +		return;
> ```
> 
> Checking  (windef->rpCommonSyntax == NULL) seems a duplicate, because transformPatternClause() is only called by transformRPR(), and transformRPR() has done the check.

Yeah. transformDefineClause() already does the similar check using
Assert. What about using Assert in transPatternClause() as well?

	Assert(windef->rpCommonSyntax != NULL);

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp