Re: Row pattern recognition

Hi Henson,

I looked into 0051 and have some suggestions.

If you like, can you please included the changes in v49?
I am going to release v48 soon.

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

> index 1a754bcdac5..7ba0b6df849 100644
> --- a/src/backend/commands/explain.c
> +++ b/src/backend/commands/explain.c

> +static int
> +deparse_rpr_node(RPRPattern *pattern, int i, int limit, StringInfo buf)

One letter function argument name "i" looks unusual and better to be
named more meaningful one, like idx?

> /*
> - * Process an ALT element: adjust depth parens and register separator positions.
> + * Find the END that closes the group opened by the BEGIN at beginIdx: the
> + * first END at the same depth scanning forward.
> */
> -static void
> -deparse_rpr_alt(RPRPattern *pattern, int *idx, StringInfoData *buf,
> - RPRDepth *prevDepth, bool *needSpace, List **altSeps)
> +static int
> +rpr_match_end(RPRPattern *pattern, int beginIdx)
> {
> - RPRPatternElement *elem = &pattern->elements[*idx];
> -
> - /* Close parens for depth decrease */
> - while (*prevDepth > elem->depth)
> - {
> - appendStringInfoChar(buf, ')');
> - (*prevDepth)--;
> - *needSpace = true;
> - }
> -
> - /* Open parens up to ALT's depth */
> - while (*prevDepth < elem->depth)
> - {
> - if (*needSpace)
> - appendStringInfoChar(buf, ' ');
> - appendStringInfoChar(buf, '(');
> - (*prevDepth)++;
> - *needSpace = false;
> - }
> + RPRDepth d = pattern->elements[beginIdx].depth;
> + int j;

If we change function arugument to other than "i", "j" here better to
be named "i".

> /*
> - * Process a VAR element: adjust depth parens and output variable name.
> + * Scope end of the construct at index i: the first following element whose
> + * depth is no greater than i's own. For an ALT marker this is the index just
> + * past its last branch, since depth stays constant across branch boundaries.
> + * FIN sits at depth 0, so a top-level ALT stops there.
> */
> -static void
> -deparse_rpr_var(RPRPattern *pattern, int *idx, StringInfoData *buf,
> - RPRDepth *prevDepth, bool *needSpace, List **altSeps)
> +static int
> +rpr_alt_scope_end(RPRPattern *pattern, int i)

One letter argument name "i" looks unusual and better to be named more
meaningful one, like idx?

> {
> - RPRPatternElement *elem = &pattern->elements[*idx];
> -
> - /* Open parens for depth increase */
> - while (*prevDepth < elem->depth)
> - {
> - if (*needSpace)
> - appendStringInfoChar(buf, ' ');
> - appendStringInfoChar(buf, '(');
> - (*prevDepth)++;
> - *needSpace = false;
> - }
> + RPRDepth d = pattern->elements[i].depth;
> + int k;

If we change function arugument to other than "i", "j" here better to
be named "i".

> +/*
> + * Boundary of the alternation branch starting at b (i.e. the start of the next
> + * branch, or altEnd if b is the last branch).
> + *
> + * The branch-start element's jump points at the next branch when this is not
> + * the last branch. jump is overloaded (a group BEGIN also uses it for its
> + * skip path), so confirm a real branch boundary with the relative test
> + * elem[j-1].next != j: at a true boundary the preceding branch's tail has its
> + * next redirected past the alternation, so it does not point at j.
> + */
> +static int
> +rpr_next_branch(RPRPattern *pattern, int b, int altEnd)

One letter function argument name "b" looks unusual and better to be
named more meaningful one, like bno or bid?

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