From: Tatsuo Ishii <ishii@postgresql.org>
To: li.evan.chao@gmail.com
Cc: vik@postgresfriends.org
Cc: ishii@postgresql.org
Cc: david.g.johnston@gmail.com
Cc: jacob.champion@enterprisedb.com
Cc: er@xs4all.nl
Cc: peter@eisentraut.org
Cc: pgsql-hackers@postgresql.org
Subject: Re: Row pattern recognition
Date: Fri, 21 Nov 2025 14:57:08 +0900 (JST)
Message-ID: <20251121.145708.931561343800399405.ishii@postgresql.org> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ishii@postgresql.org, li.evan.chao@gmail.com, vik@postgresfriends.org, david.g.johnston@gmail.com, jacob.champion@enterprisedb.com, er@xs4all.nl, peter@eisentraut.org
Subject: Re: Row pattern recognition
In-Reply-To: <20251121.145708.931561343800399405.ishii@postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox