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
PATTERNclause?
>>
>
> 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.
Ifan 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
