Date: Thu, 02 Jul 2026 09:48:13 +0900 (JST)
Message-Id: <20260702.094813.2267011850936872788.ishii@postgresql.org>
To: assam258@gmail.com
Cc: jian.universality@gmail.com, pgsql-hackers@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
Subject: Re: Row pattern recognition
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAAAe_zAwu0nuwXmEt-mnjV+J-BVLpJu64p-kRPyj8itbpriJhA@mail.gmail.com>
References: <20260629.183730.1799088184925453299.ishii@postgresql.org>
	<20260701.211805.2273383346456030984.ishii@postgresql.org>
	<CAAAe_zAwu0nuwXmEt-mnjV+J-BVLpJu64p-kRPyj8itbpriJhA@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/20260702.094813.2267011850936872788.ishii%40postgresql.org>
Precedence: bulk

Hi Henson,

> Hi Tatsuo,
> 
> Thanks for the review. I agree with the direction -- the (void) is
> confusing. I did try your exact suggestion and the regression tests pass
> unchanged, but I would keep one guard, for the following reason.
> 
>> Is there any reason that we call row_is_in_reduced_frame() here,
>> instead of something like:
>>
>>    update_frameheadpos(winstate);
>>    update_reduced_frame(winobj, pos);
> 
> row_is_in_reduced_frame() guards the update:
> 
>     state = get_reduced_frame_status(winstate, pos);
>     if (state == RF_NOT_DETERMINED)
>     {
>         update_frameheadpos(winstate);
>         update_reduced_frame(winobj, pos);
>     }
> 
> Calling update_reduced_frame() unconditionally drops that guard. When
> currentpos is already determined -- e.g. an interior row of a previous
> match under AFTER MATCH SKIP PAST LAST ROW (RF_SKIPPED) -- it takes an
> O(1) early return and overwrites the shared rpr_match_* record, silently
> reclassifying a SKIPPED row as UNMATCHED.
> 
> This is not observable today -- I confirmed with a guarded-vs-unguarded
> differential (patterns, both skip modes, various aggregates: identical
> output), because both paths end up with an empty reduced frame. But it
> does change the state semantics and leans on that convergence, so I would
> rather keep the guard. Two ways, both of which remove the (void):

Thanks for the explanation. That makese sense.

>   A) Keep the guard inline at the call site.
> 
>   B) Factor it into a small helper shared by the nav-driving call site
>      and row_is_in_reduced_frame():
> 
>         static void
>         ensure_reduced_frame(WindowObject winobj, int64 pos)
>         {
>             WindowAggState *winstate = winobj->winstate;
> 
>             if (get_reduced_frame_status(winstate, pos) ==
> RF_NOT_DETERMINED)
>             {
>                 update_frameheadpos(winstate);
>                 update_reduced_frame(winobj, pos);
>             }
>         }
> 
> I lean towards B (no duplicated guard, and the name states the intent),
> but A is the smaller diff. Which do you prefer?

I like B too.

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