Date: Fri, 17 Apr 2026 17:10:22 +0900 (JST)
Message-Id: <20260417.171022.397021509290810631.ishii@postgresql.org>
To: assam258@gmail.com
Cc: 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,
 pgsql-hackers@postgresql.org
Subject: Re: Row pattern recognition
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAAAe_zB7rAEJtT6hXgF85=_Tj8Nti45ZHbQw26gxTF2DBs3hJw@mail.gmail.com>
References: 
 <CAAAe_zCCnCJYd2aovB1wf1BFXCSR1raFmVZS1dZjj76_Vtyt-Q@mail.gmail.com>
	<20260410.183515.1561517749674643173.ishii@postgresql.org>
	<CAAAe_zB7rAEJtT6hXgF85=_Tj8Nti45ZHbQw26gxTF2DBs3hJw@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/20260417.171022.397021509290810631.ishii%40postgresql.org>
Precedence: bulk

Here is a review for 0008.

The patch does not apply using git apply.
$ git apply /usr/local/src/pgsql/RPR/make_rpr_patches/v47/input_patches/nocfbot-0008-Replace-reduced-frame-map-with-single-match-result.txt 
error: patch failed: src/backend/executor/nodeWindowAgg.c:247
error: src/backend/executor/nodeWindowAgg.c: patch does not apply
error: patch failed: src/include/nodes/execnodes.h:2694
error: src/include/nodes/execnodes.h: patch does not apply

I applied it using patch command.

$ patch -p1 < /usr/local/src/pgsql/RPR/make_rpr_patches/v47/input_patches/nocfbot-0008-Replace-reduced-frame-map-with-single-match-result.txt 
patching file src/backend/executor/execRPR.c
patching file src/backend/executor/nodeWindowAgg.c
Hunk #1 succeeded at 235 with fuzz 2 (offset -12 lines).
Hunk #2 succeeded at 1017 (offset -15 lines).
Hunk #3 succeeded at 1041 (offset -15 lines).
Hunk #4 succeeded at 1060 (offset -15 lines).
Hunk #5 succeeded at 1340 (offset 4 lines).
Hunk #6 succeeded at 1571 (offset 11 lines).
Hunk #7 succeeded at 2355 (offset 11 lines).
Hunk #8 succeeded at 2464 (offset 11 lines).
Hunk #9 succeeded at 2991 (offset 33 lines).
Hunk #10 succeeded at 3600 (offset -37 lines).
Hunk #11 succeeded at 3900 (offset -37 lines).
Hunk #12 succeeded at 3914 (offset -37 lines).
Hunk #13 succeeded at 3925 (offset -37 lines).
Hunk #14 succeeded at 3938 (offset -37 lines).
Hunk #15 succeeded at 3948 (offset -37 lines).
Hunk #16 succeeded at 4044 (offset -37 lines).
Hunk #17 succeeded at 4065 (offset -37 lines).
Hunk #18 succeeded at 4141 (offset -37 lines).
Hunk #19 succeeded at 4657 (offset -35 lines).
patching file src/include/nodes/execnodes.h
Hunk #2 succeeded at 2698 with fuzz 2 (offset 2 lines).
patching file src/test/regress/expected/rpr_explain.out

From: Henson Choi <assam258@gmail.com>
Subject: Re: Row pattern recognition
Date: Sun, 12 Apr 2026 16:27:26 +0900
Message-ID: <CAAAe_zB7rAEJtT6hXgF85=_Tj8Nti45ZHbQw26gxTF2DBs3hJw@mail.gmail.com>

> From 59e99c6b9322f402df560bc693863491487e12db Mon Sep 17 00:00:00 2001
> From: Henson Choi <assam258@gmail.com>
> Date: Thu, 2 Apr 2026 14:09:12 +0900
> Subject: [PATCH] Replace reduced frame map with single match result
> 
> The reduced frame map was a per-row byte array tracking match status.
> Since rows are processed sequentially and only one match is active
> at a time, replace it with four scalar fields: valid, matched,
> start, and length.

You are right, we don't need to have the map as an array.

> Also distinguish empty matches (FIN reached with zero rows consumed)
> from unmatched rows via RF_EMPTY_MATCH, counted as matched in NFA
> statistics.

Good enhancement.

> Widen row_is_in_reduced_frame() return type from int to int64,
> since it returns rpr_match_length which is int64.

Good point.

> ---
>  src/backend/executor/execRPR.c            |  56 +++---
>  src/backend/executor/nodeWindowAgg.c      | 233 +++++++++-------------
>  src/include/nodes/execnodes.h             |  21 +-
>  src/test/regress/expected/rpr_explain.out |   8 +-
>  4 files changed, 132 insertions(+), 186 deletions(-)
> 
> diff --git a/src/backend/executor/execRPR.c b/src/backend/executor/execRPR.c
> index 58f9da0b814..1cbe8e14780 100644
> --- a/src/backend/executor/execRPR.c
> +++ b/src/backend/executor/execRPR.c
> @@ -549,7 +549,7 @@
>   *
>   *   (1) Find or create a context for the target row
>   *   (2) Enter the row processing loop
> - *   (3) After the loop ends, record the result in reduced_frame_map
> + *   (3) After the loop ends, record the match result
>   *
>   * Pseudocode of the row processing loop:
>   *
> @@ -923,18 +923,19 @@
>   * Chapter X  Match Result Processing
>   * ============================================================================
>   *
> - * X-1. Reduced Frame Map
> + * X-1. Match Result
>   *
> - * RPR match results are recorded in a byte array called reduced_frame_map.
> - * One byte is allocated per row, and the value is one of the following:
> + * RPR tracks the current match result as a single entry in WindowAggState
> + * with four fields: rpr_match_valid, rpr_match_matched, rpr_match_start,
> + * and rpr_match_length.  When valid is true, the entry describes the

"When valid is true" sounds a little bit confusing for readers. I
think "When rpr_match_valid is true" is cleaner, although it adds more
characters. Same thing can be said to "matchded", "valid" and "length".

> + * match result for the position at rpr_match_start: matched indicates
> + * success or failure, and length gives the number of rows consumed.
> + * A match with length 0 represents an empty match (pattern matched but
> + * consumed no rows).  When valid is false, the position has not been
> + * evaluated yet (RF_NOT_DETERMINED).
>   *
> - *   RF_NOT_DETERMINED (0)  Not yet processed
> - *   RF_FRAME_HEAD     (1)  Start row of the match
> - *   RF_SKIPPED        (2)  Interior row of the match (skipped in frame)
> - *   RF_UNMATCHED      (3)  Match failure
> - *
> - * The window function references this map to determine frame inclusion for
> - * each row.
> + * The window function calls get_reduced_frame_status() to look up a
> + * row's status against the current match result.

This may be misleading since get_reduced_frame_status() is not an
exposed public API to a window function. What about something like
"Row's status against the current match result can be obtained by
calling get_reduced_frame_status()"?

In execRPR.c, documentation occupies 1445 lines out of 3052 lines. I
don't see any other .[ch] files, half of it is taken up by
documentation. I suggest to create something like "README.rpr" and
move the documentation part to it.

> diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
> index aed7cbef99a..dca2de570e8 100644
> --- a/src/backend/executor/nodeWindowAgg.c
> +++ b/src/backend/executor/nodeWindowAgg.c

No comment to nodeWindowAgg.c patch. This part looks good to me.

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