Re: Row pattern recognition

Hi Tatsuo,

Thank you very much for taking the time to start the review.  I will reply
in step, one patch at a time, from 0001 up to wherever your review has
reached -- so far that is 0001..0004 -- and I will keep following along as
you continue.

For convenience I have attached just the four patches you have reviewed,
keeping their original v50 names but renamed to nocfbot-* so that cfbot does
not pick them up:

  nocfbot-0001  Remove blank-line changes unrelated to row pattern
recognition
  nocfbot-0002  Remove unnecessary includes from the row pattern
recognition patch
  nocfbot-0003  Recognize row pattern navigation operations by name in
DEFINE
  nocfbot-0004  Use a dedicated ExprContext for RPR DEFINE clause evaluation

0001, 0002 and 0004 are unchanged from what you reviewed; I include them
only so this reply stands on its own for the reviewed range.  0003 folds in
the three points you raised below.

> 0001 and 0002 look good to me.

Thank you for confirming -- I have left them as they are.

>>   v50-0003  Recognize row pattern navigation operations by name in DEFINE
>
> + if (argnames != NIL)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("row pattern navigation operations cannot use named arguments"),
> + parser_errposition(pstate, location)));
>
> Shouldn't the error code for the named arguments case be
> ERRCODE_SYNTAX_ERROR? As far as I know the SQL standard does not allow
> to use named arguments with row pattern navigation operations. So it's
> not a required feature.

You are right, thank you.  The standard does not provide for named arguments
here at all, so this is a syntax violation rather than an unimplemented
feature.  I have changed the errcode to ERRCODE_SYNTAX_ERROR, which also
makes it consistent with the VARIADIC and argument-count checks alongside
it.

> Also errmsg is different from surroundings. Probably "cannot use named
> arguments with row pattern navigation function %s" is better?

Thank you for the suggested wording; I have adopted it as is:

    errmsg("cannot use named arguments with row pattern navigation function
%s",
           navname)

It now reads in the same "cannot use X with row pattern navigation
function %s" shape as the VARIADIC message just above it.  I have also
updated the expected output in rpr.sql to match (ERROR:  cannot use named
arguments with row pattern navigation function PREV).

> + *     * offset_arg / compound_offset_arg must not contain column refs
> + *       or nested navigation operations
>
> Needs '*' in front of "or nested....".

Thank you for catching this.  The last line is a continuation, but the '*'
bullets ran together with the comment's own '*' and hid that.  I switched
the bullets to '-' to match the sibling rule list in this file, so the
continuation now reads clearly:

     * ... enforces, for each outer RPRNavExpr (per ISO/IEC 19075-5 5.6.4
     * nesting rules):
     *   - arg must contain at least one column reference
     *   - PREV/NEXT wrapping FIRST/LAST flattens to a compound kind
     *   - Other nestings are rejected (FIRST(PREV()), PREV(PREV()), ...)
     *   - offset_arg / compound_offset_arg must not contain column refs
     *     or nested navigation operations

I hope this addresses what you had in mind; please let me know if you would
prefer a different form.

>>   v50-0004  Use a dedicated ExprContext for RPR DEFINE clause evaluation
>
> Looks good to me.

Thank you for confirming -- left as is.

> Will continue reviewes tomorrow.

Thank you, I look forward to it.  I will respond to each further patch as
your review reaches it, in the same one-at-a-time fashion.

Best regards,
Henson