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