MIME-Version: 1.0
References: <20260427.174220.1939160662649810289.ishii@postgresql.org>
 <CAAAe_zBdwAUDNs_WFdLkFF=ewhkDkv-AqizVEVzhsfremGFb4w@mail.gmail.com>
 <20260502.140304.670813149418899420.ishii@postgresql.org>
 <20260531.213313.1224322053367141508.ishii@postgresql.org>
In-Reply-To: <20260531.213313.1224322053367141508.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Mon, 1 Jun 2026 10:16:54 +0900
Message-ID: 
 <CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>, jian he <jian.universality@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, li.evan.chao@gmail.com, pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="000000000000e9a1ee065326f341"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog%40mail.gmail.com>
Precedence: bulk

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

Hi Tatsuo,

> I accidentaly noticed that v47-0002 changes findTargetlistEntrySQL99
> from static to extern.
> [...]
> I think this is not necessary anymore since findTargetlistEntrySQL99
> is not used outside parse_clause.c.

Good catch -- you're right, and I'll revert it to static in v48. I
double-checked the current tree: the only callers left are inside
parse_clause.c (the findTargetlistEntry wrapper, plus the GROUP BY and ORDER
BY paths), so the extern prototype in parse_clause.h is now unused. The
revert is just dropping that prototype and restoring the static qualifier
with the in-file forward declaration it used to have.

It's worth saying why it went extern in the first place, since the reason is
no longer visible in the tree:

The DEFINE clause needs its referenced columns present in the plan's
targetlist to be evaluable at run time. The original implementation did
that from the RPR side, in parse_rpr.c, by calling
findTargetlistEntrySQL99()
with resjunk = true to add the missing entry -- and since that function was
static in parse_clause.c, reaching it across files is what required exposing
it as extern.

That approach added the whole DEFINE expression to the targetlist, and that
turned out to be the source of a SIGSEGV: when an RPR window and a plain
window coexist, the non-RPR WindowAgg inherited targetlist entries carrying
RPRNavExpr nodes it has no way to evaluate. The fix was to add only the Vars
a DEFINE references (with a guard in allpaths.c to keep those columns from
being pruned), and that is what removed the cross-file call. So the extern
has simply outlived its caller -- exactly as you spotted.

The one loose end there is an optimization, not a correctness issue: the
allpaths.c guard is deliberately coarse, so it also blocks removing a
WindowAgg whose RPR WindowFuncs are all unused. Doing that precisely means
restructuring remove_unused_subquery_outputs(), which runs for every
subquery and not just RPR -- broad enough that I'm treating it as a
longer-term item rather than part of this work. It doesn't bring the
external call back -- the Var-only path stays -- so the revert to static is
safe independently of it.

I'll fold the static revert into v48.

Thanks,
Henson

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

<div dir=3D"ltr">Hi Tatsuo,<br><br>&gt; I accidentaly noticed that v47-0002=
 changes findTargetlistEntrySQL99<br>&gt; from static to extern.<br>&gt; [.=
..]<br>&gt; I think this is not necessary anymore since findTargetlistEntry=
SQL99<br>&gt; is not used outside parse_clause.c.<br><br>Good catch -- you&=
#39;re right, and I&#39;ll revert it to static in v48. I<br>double-checked =
the current tree: the only callers left are inside<br>parse_clause.c (the f=
indTargetlistEntry wrapper, plus the GROUP BY and ORDER<br>BY paths), so th=
e extern prototype in parse_clause.h is now unused. The<br>revert is just d=
ropping that prototype and restoring the static qualifier<br>with the in-fi=
le forward declaration it used to have.<br><br>It&#39;s worth saying why it=
 went extern in the first place, since the reason is<br>no longer visible i=
n the tree:<br><br>The DEFINE clause needs its referenced columns present i=
n the plan&#39;s<br>targetlist to be evaluable at run time. The original im=
plementation did<br>that from the RPR side, in parse_rpr.c, by calling find=
TargetlistEntrySQL99()<br>with resjunk =3D true to add the missing entry --=
 and since that function was<br>static in parse_clause.c, reaching it acros=
s files is what required exposing<br>it as extern.<br><br>That approach add=
ed the whole DEFINE expression to the targetlist, and that<br>turned out to=
 be the source of a SIGSEGV: when an RPR window and a plain<br>window coexi=
st, the non-RPR WindowAgg inherited targetlist entries carrying<br>RPRNavEx=
pr nodes it has no way to evaluate. The fix was to add only the Vars<br>a D=
EFINE references (with a guard in allpaths.c to keep those columns from<br>=
being pruned), and that is what removed the cross-file call. So the extern<=
br>has simply outlived its caller -- exactly as you spotted.<br><br>The one=
 loose end there is an optimization, not a correctness issue: the<br>allpat=
hs.c guard is deliberately coarse, so it also blocks removing a<br>WindowAg=
g whose RPR WindowFuncs are all unused. Doing that precisely means<br>restr=
ucturing remove_unused_subquery_outputs(), which runs for every<br>subquery=
 and not just RPR -- broad enough that I&#39;m treating it as a<br>longer-t=
erm item rather than part of this work. It doesn&#39;t bring the<br>externa=
l call back -- the Var-only path stays -- so the revert to static is<br>saf=
e independently of it.<br><br>I&#39;ll fold the static revert into v48.<br>=
<br>Thanks,<br>Henson<br></div>

--000000000000e9a1ee065326f341--