Hi Chao,
>> On Nov 21, 2025, at 13:25, Chao Li <[email protected]> wrote:
>>
>>
>> Okay, I’d stop here and continue to review 0006 next week.
>>
>
> I just finished reviewing 0006, and see some problems:
>
> 15 - 0006 - select.sgml
> ```
> +[ <replaceable class="parameter">row_pattern_common_syntax</replaceable> ]
> ```
>
> row_pattern_common_syntax doesn’t look like a good name. I searched over the
> docs, and couldn't find a name suffixed by “_syntax”. I think we can just
> name it as “row_pattern_recognition_clause” or a shorter name just
> “row_pattern”.
I believe these names are based on the SQL standard. All syntax
components do not necessary be suffixed by "clause". For example
in select.sgml,
[ WITH [ RECURSIVE ] <replaceable class="parameter">with_query</replaceable> [,
...] ]
SELECT [ ALL | DISTINCT [ ON ( <replaceable
class="parameter">expression</replaceable> [, ...] ) ] ]
[ { * | <replaceable class="parameter">expression</replaceable> [ [ AS ]
<replaceable class="parameter">output_name</replaceable> ] } [, ...] ]
[ FROM <replaceable class="parameter">from_item</replaceable> [, ...] ]
[ WHERE <replaceable class="parameter">condition</replaceable> ]
[ GROUP BY { ALL | [ ALL | DISTINCT ] <replaceable
class="parameter">grouping_element</replaceable> [, ...] } ]
[ HAVING <replaceable class="parameter">condition</replaceable> ]
[ WINDOW <replaceable class="parameter">window_name</replaceable> AS (
<replaceable class="parameter">window_definition</replaceable> ) [, ...] ]
"window_definition" comes from the standard and does not suffixed by
"clause". If you look into the window clause definition in the
standard, you will see:
<window clause> ::=
WINDOW <window definition list>
<window definition list> ::=
<window definition> [ { <comma> <window definition> }... ]
So the usage of terms in our document matches the standard. Likewise,
we see the definition of row pattern common syntax in the stabdard:
<window clause> ::=
WINDOW <window definition list>
<window definition list> ::=
<window definition> [ { <comma> <window definition> }... ]
<window definition> ::=
<new window name> AS <window specification>
<new window name> ::=
<window name>
<window specification> ::=
<left paren> <window specification details> <right paren>
<window specification details> ::=
[ <existing window name> ]
[ <window partition clause> ]
[ <window order clause> ]
[ <window frame clause> ]
:
:
<window frame clause> ::=
[ <row pattern measures> ]
<window frame units> <window frame extent>
[ <window frame exclusion> ]
[ <row pattern common syntax> ]
So I think "row pattern common syntax" is perfectly appropriate
name. If we change it to “row_pattern_recognition_clause”, it will
just bring confusion. In the standard “row pattern recognition
clause” is defined RPR in FROM clause.
SELECT ... FROM table MATCH RECOGNIZE (....);
Here MATCH RECOGNIZE (....) is the “row pattern recognition clause”.
> 16 - 0006 - select.sgml
> ```
> + <synopsis>
> + [ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
> ``
>
> I think the two values are mutually exclusive, so curly braces should added
> surround them:
>
> [ { AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW } ]
>
> [] means optional, and {} means choose one from all alternatives.
Agreed. Will fix.
> 17 - 0006 - select.sgml
> ```
> PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+]
> [, ...]
> ```
>
> PATTERN definition should be placed inside (), here you missed ()
Good catch. Will fix.
> 18 - 0006 - select.sgml
> The same code as 17, <replaceable
> class="parameter">pattern_variable_name</replaceable>[+], do you only put “+”
> here, I think SQL allows a lot of pattern quantifiers. From 0001, gram.y,
> looks like +, * and ? Are supported in this patch. So, maybe we can do:
>
> ```
> PATTERN ( pattern_element, [pattern_element …] )
>
> and pattern_element = variable name optionally followed by quantifier
> (reference to somewhere introducing pattern quantifier).
> ```
Currently only *, + and ? are supported and I don't think it's worth
to invent "pattern_element". (Actually the standard defines much more
complex syntax for PATTERN. I think we can revisit this once the basic
support for quantifier *,+ and ? are brought in the core PostgreSQL
code.
> 19 - 0006 - select.sgml
>
> I don’t see INITIAL and SEEK are described. Even if SEEK is not supported
> currently, it’s worthy mentioning that.
Agreed. Will fix.
> 20 - 0006 - select.sgml
> ```
> + after a match found. With <literal>AFTER MATCH SKIP PAST LAST
> + ROW</literal> (the default) next row position is next to the last row of
> ```
>
> 21 - 0006 - select.sgml
> ```
> [ <replaceable class="parameter">frame_clause</replaceable> ]
> +[ <replaceable class="parameter">row_pattern_common_syntax</replaceable> ]
> ```
>
> Looks like row_pattern_common_syntax belongs to frame_clause, and you have
> lately added row_pattern_common_syntax to frame_clause:
> ```
> <synopsis>
> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [
> <replaceable>frame_exclusion</replaceable> ]
> -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND
> <replaceable>frame_end</replaceable> [
> <replaceable>frame_exclusion</replaceable> ]
> +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [
> <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
> +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND
> <replaceable>frame_end</replaceable> [
> <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
> </synopsis>
> ```
>
> So I guess row_pattern_common_syntax after frame_clause should not be added.
Yes, row_pattern_common_syntax belongs to frame_clause. Will fix.
> 22 - 0006 - advance.sgml
> ```
> <programlisting>
> DEFINE
> LOWPRICE AS price <= 100,
> UP AS price > PREV(price),
> DOWN AS price < PREV(price)
> </programlisting>
>
> Note that <function>PREV</function> returns the price column in the
> ```
>
> In the “Note” line, “price” refers to a column from above example, so I think
> it should be tagged by <literal>. This comment applies to multiple places.
You are right. Will fix.
> I will proceed 0007 tomorrow.
Thanks!
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
