MIME-Version: 1.0
References: 
 <CAE+cgNgSG6oiAXT=FL+gK71Squ_eacyTCxSVXaAUTwODrkzqFg@mail.gmail.com>
 <20260318.204139.106185544494720182.ishii@postgresql.org>
 <CAN4CZFOw4X27zgYdUgbVMmT3x_22a9kdGnTe6U+2TV1jJv-1ow@mail.gmail.com>
 <20260319.103930.390354862957599874.ishii@postgresql.org>
In-Reply-To: <20260319.103930.390354862957599874.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Thu, 19 Mar 2026 11:28:29 +0900
Message-ID: 
 <CAAAe_zBz7mjNVms-ooF79+p_7eydkONBmz8YCk=oS0+WOfj3fQ@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>, zsolt.parragi@percona.com
Cc: 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
Content-Type: multipart/alternative; boundary="000000000000a24cda064d5753cd"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zBz7mjNVms-ooF79%2Bp_7eydkONBmz8YCk%3DoS0%2BWOfj3fQ%40mail.gmail.com>
Precedence: bulk

--000000000000a24cda064d5753cd
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo, Zsolt,

I just confirmed the crash.
>
> Henson, What do you think?
>

Good catch by Zsolt. Your careful review is really helping a lot.

I've fixed both the prefix merge and suffix merge paths in
mergeGroupPrefixSuffix() to also increment child->max. I'll
include the fix with a regression test in the next patch series.

Regarding the suggested fix:

    if (child->max != RPR_QUANTITY_INF)
        child->max += 1;

While the quantifier value is int (INT32_MAX = RPR_QUANTITY_INF),
making such repetition counts practically impossible, the +1
approach is not semantically equivalent -- it could silently turn
a finite max into infinity. Instead, I took a fallback approach:
skip the merge entirely when min or max would reach
RPR_QUANTITY_INF after increment, consistent with the overflow
checks in mergeConsecutiveVars/Groups.


> Good point. You are right, the plan cache should be read only.
> However, Henson is working on a different approach and the code will
> not be used any more...
>

Yes, I'm currently working on a slot-based approach (1-slot
PREV/NEXT) that won't need attno_map, so the plan cache mutation
issue will be gone.


> I forggot to include the data file. Please apply attached patch on top
> of v45.
>

Got it, thanks. By the way, how about splitting the test patch
into two -- one for sql + data files and another for expected
output files? The single test patch is getting quite large and
cfbot often fails to apply it.

Right. Maybe ERRCODE_TOO_MANY_ARGUMENTS?
>

 I'm currently reworking PREV/NEXT to accept 2 arguments, so this
error handling will change. I'll take care of the proper error
codes along with the GROUPS typo and ERRCODE_WINDOWING_ERROR change.

Best regards,
Henson

--000000000000a24cda064d5753cd
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo, Zsolt,<br></div><br><div class=
=3D"gmail_quote gmail_quote_container"><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:sol=
id;border-left-color:rgb(204,204,204);padding-left:1ex">I just confirmed th=
e crash.<br><br>
Henson, What do you think?=C2=A0<br></blockquote><div><br></div>Good catch =
by Zsolt. Your careful review is really helping a lot.<br><br>I&#39;ve fixe=
d both the prefix merge and suffix merge paths in<br>mergeGroupPrefixSuffix=
() to also increment child-&gt;max. I&#39;ll<br>include the fix with a regr=
ession test in the next patch series.<br><br>Regarding the suggested fix:<b=
r><br><font face=3D"monospace">=C2=A0 =C2=A0 if (child-&gt;max !=3D RPR_QUA=
NTITY_INF)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 child-&gt;max +=3D 1;<br></font><=
br>While the quantifier value is int (INT32_MAX =3D RPR_QUANTITY_INF),<br>m=
aking such repetition counts practically impossible, the +1<br>approach is =
not semantically equivalent -- it could silently turn<br>a finite max into =
infinity. Instead, I took a fallback approach:<br>skip the merge entirely w=
hen min or max would reach<br>RPR_QUANTITY_INF after increment, consistent =
with the overflow<br><div>checks in mergeConsecutiveVars/Groups.=C2=A0</div=
><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px=
 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:=
rgb(204,204,204);padding-left:1ex">Good point. You are right, the plan cach=
e should be read only.<br>
However, Henson is working on a different approach and the code will<br>
not be used any more...<br></blockquote><div><br></div><div>Yes, I&#39;m cu=
rrently working on a slot-based approach (1-slot<br>PREV/NEXT) that won&#39=
;t need attno_map, so the plan cache mutation<br>issue will be gone.<br></d=
iv><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0=
px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-colo=
r:rgb(204,204,204);padding-left:1ex">
I forggot to include the data file. Please apply attached patch on top<br>
of v45.<br></blockquote><div>=C2=A0</div><div>Got it, thanks. By the way, h=
ow about splitting the test patch<br>into two -- one for sql + data files a=
nd another for expected<br>output files? The single test patch is getting q=
uite large and<br>cfbot often fails to apply it.<br></div><div><br></div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);pad=
ding-left:1ex">Right. Maybe ERRCODE_TOO_MANY_ARGUMENTS?<br></blockquote><di=
v><br></div><div>=C2=A0I&#39;m currently reworking PREV/NEXT to accept 2 ar=
guments, so this</div>error handling will change. I&#39;ll take care of the=
 proper error<br>codes along with the GROUPS typo and ERRCODE_WINDOWING_ERR=
OR change.<br><br>Best regards,<br>Henson<br></div></div>

--000000000000a24cda064d5753cd--