Date: Sat, 13 Jun 2026 11:48:31 +0900 (JST)
Message-Id: <20260613.114831.102648086806206558.ishii@postgresql.org>
To: assam258@gmail.com
Cc: jian.universality@gmail.com, ishii@postgresql.org,
 zsolt.parragi@percona.com, sjjang112233@gmail.com,
 vik@postgresfriends.org, er@xs4all.nl, jacob.champion@enterprisedb.com,
 david.g.johnston@gmail.com, peter@eisentraut.org, li.evan.chao@gmail.com,
 pgsql-hackers@postgresql.org
Subject: Re: Row pattern recognition
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAAAe_zDYxq0d3exCDwvKncD0kaL2uehDir6HXo4r5DXMitKrSg@mail.gmail.com>
References: 
 <CACJufxHJFNBJ0vHCCLestWV5b7DF5e4VzfmovqGKBGgqg+rcGA@mail.gmail.com>
	<CAAAe_zBY0rrgf+tKXMUc-Y3nDDD69hddRBKopEKAZobhY=Cy-Q@mail.gmail.com>
	<CAAAe_zDYxq0d3exCDwvKncD0kaL2uehDir6HXo4r5DXMitKrSg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260613.114831.102648086806206558.ishii%40postgresql.org>
Precedence: bulk

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