MIME-Version: 1.0
References: <20260330.133428.1197197778850463943.ishii@postgresql.org>
 <CAAAe_zCB31g2bkRAWhDZVaegx+Z2JnF-zBxfXD7nunWttYi6Gg@mail.gmail.com>
 <CAAAe_zDEDtJEf6aO=x3qhPOs8kXmsha7Q8ROJXE8VZNp+Dxe2A@mail.gmail.com>
 <20260410.101847.595894887122872457.ishii@postgresql.org>
In-Reply-To: <20260410.101847.595894887122872457.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Fri, 10 Apr 2026 17:51:54 +0900
Message-ID: 
 <CAAAe_zCCnCJYd2aovB1wf1BFXCSR1raFmVZS1dZjj76_Vtyt-Q@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
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
Content-Type: multipart/alternative; boundary="00000000000082e52c064f173f17"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zCCnCJYd2aovB1wf1BFXCSR1raFmVZS1dZjj76_Vtyt-Q%40mail.gmail.com>
Precedence: bulk

--00000000000082e52c064f173f17
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo,

Thank you for the careful review.

Before this, there's a check in the function:
>
>                 if (tle->ressortgroupref || tle->resjunk)
>                         continue;
>
> Below is the query in the regression test that is suppoed to trigger
> the error.  In this case column "i" in the target list should have
> resjunk flag to be set to true. So I thought the if statenment above
> becomes true and the column i is not removed from subquery's target
> list. Am I missing something?


That is a very good observation. For the test query you cited, the
resjunk check does indeed protect column "i", as you noted.

The allpaths.c guard is needed for a different scenario: when a Var
already exists in the inner SELECT as a non-resjunk entry, but the
outer query does not reference it. For example:

  SELECT count(*) FROM (
    SELECT i, count(*) OVER w FROM generate_series(1,10) i
    --     ^ in SELECT -> resjunk=false
    WINDOW w AS (
      ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
      PATTERN (A+)
      DEFINE A AS i > PREV(i)
    )
  ) t;
  -- outer uses only count(*) -> i is not in attrs_used

In this case, the Var addition logic in parse_rpr.c (pull_var_clause
+ dup check) finds that "i" already exists in the targetlist, so it
does not add a new resjunk entry. Since "i" is resjunk=false and the
outer query does not reference it, remove_unused_subquery_outputs()
would replace it with NULL, breaking DEFINE evaluation.

Changing the existing entry to resjunk=true is not an option either,
since it would alter the subquery's output schema -- the outer query
may reference that column, and at parse time we do not yet know
whether it will. The existing protection mechanisms (resjunk,
ressortgroupref, attrs_used, volatile check) cannot express
"referenced by DEFINE", since DEFINE is a new column reference path
that did not exist in PostgreSQL before. The allpaths.c guard
follows the same pattern as the existing SRF and volatile guards in
that function.

Also I think removal of the WindowAgg node is fine here because it's
> not necessary to calculate the result of the sub query at
> all. Actually the query above runs fine on v46. I guess some of the
> incremental patches caused this to stop working. Can you elaborate?
>

I agree -- for this specific query, removing the WindowAgg does not
change the count(*) result, since window functions do not change the
number of rows.

Regarding v46: the two bugs were actually latent from the initial
implementation, not caused by incremental patches. The old code
added the entire DEFINE expression (e.g., "i > PREV(i)") to the
query targetlist via findTargetlistEntrySQL99(). In most cases,
DEFINE conditions involve comparison operators, so the resulting
OpExpr did not match existing Var entries in the targetlist, and a
new resjunk entry was created. This accidentally prevented removal.
(In fact, a bare boolean DEFINE such as "DEFINE A AS flag" would
have exhibited the same removal bug even in v46, since
findTargetlistEntrySQL99 would reuse the existing entry instead
of creating a new resjunk one -- confirming that the issue was
latent in the original implementation, not introduced by recent
patches.)

However, that same approach caused the SIGSEGV (Bug 1): when RPR
and non-RPR windows coexist, the non-RPR WindowAgg inherits
targetlist entries containing RPRNavExpr nodes that it cannot
evaluate.

The two bugs are two sides of the same coin -- the old approach
prevented removal but caused SIGSEGV. Switching to Var-only
addition fixed the SIGSEGV but required the allpaths.c guard to
prevent removal.

As for more precise optimization (allowing removal when all
WindowFuncs for an RPR clause are unused), it would require
structural changes to the loop in remove_unused_subquery_outputs(),
which currently evaluates entries individually. Since this function
runs for all subqueries, not just RPR, such a change would need
broader testing.

The patch addresses two distinct issues: the parse_rpr.c change
(Var-only addition) prevents RPRNavExpr from propagating to
non-RPR WindowAgg nodes, while the allpaths.c guard prevents
incorrect column removal. Since both are needed, the guard
cannot simply be reverted. Would it make sense to keep it as-is
for now, and explore the more precise optimization as a separate
follow-up patch?

Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo,<br><br>Thank =
you for the careful review.<br></div><div class=3D"gmail_quote"><div dir=3D=
"ltr" class=3D"gmail_attr"><br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">
Before this, there&#39;s a check in the function:<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tle-&gt;ressort=
groupref || tle-&gt;resjunk)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 continue;<br>
<br>
Below is the query in the regression test that is suppoed to trigger<br>
the error.=C2=A0 In this case column &quot;i&quot; in the target list shoul=
d have<br>
resjunk flag to be set to true. So I thought the if statenment above<br>
becomes true and the column i is not removed from subquery&#39;s target<br>
list. Am I missing something?=C2=A0</blockquote><div><br></div><div>That is=
 a very good observation. For the test query you cited, the<br>resjunk chec=
k does indeed protect column &quot;i&quot;, as you noted.<br><br>The allpat=
hs.c guard is needed for a different scenario: when a Var<br>already exists=
 in the inner SELECT as a non-resjunk entry, but the<br>outer query does no=
t reference it. For example:<br><br>=C2=A0 SELECT count(*) FROM (<br>=C2=A0=
 =C2=A0 SELECT i, count(*) OVER w FROM generate_series(1,10) i<br>=C2=A0 =
=C2=A0 -- =C2=A0 =C2=A0 ^ in SELECT -&gt; resjunk=3Dfalse<br>=C2=A0 =C2=A0 =
WINDOW w AS (<br>=C2=A0 =C2=A0 =C2=A0 ROWS BETWEEN CURRENT ROW AND UNBOUNDE=
D FOLLOWING<br>=C2=A0 =C2=A0 =C2=A0 PATTERN (A+)<br>=C2=A0 =C2=A0 =C2=A0 DE=
FINE A AS i &gt; PREV(i)<br>=C2=A0 =C2=A0 )<br>=C2=A0 ) t;<br>=C2=A0 -- out=
er uses only count(*) -&gt; i is not in attrs_used<br><br>In this case, the=
 Var addition logic in parse_rpr.c (pull_var_clause<br>+ dup check) finds t=
hat &quot;i&quot; already exists in the targetlist, so it<br>does not add a=
 new resjunk entry. Since &quot;i&quot; is resjunk=3Dfalse and the<br>outer=
 query does not reference it, remove_unused_subquery_outputs()<br>would rep=
lace it with NULL, breaking DEFINE evaluation.<br><br>Changing the existing=
 entry to resjunk=3Dtrue is not an option either,<br>since it would alter t=
he subquery&#39;s output schema -- the outer query<br>may reference that co=
lumn, and at parse time we do not yet know<br>whether it will. The existing=
 protection mechanisms (resjunk,<br>ressortgroupref, attrs_used, volatile c=
heck) cannot express<br>&quot;referenced by DEFINE&quot;, since DEFINE is a=
 new column reference path<br>that did not exist in PostgreSQL before. The =
allpaths.c guard<br>follows the same pattern as the existing SRF and volati=
le guards in<br>that function.</div><div><br></div><blockquote class=3D"gma=
il_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,2=
04,204);padding-left:1ex">
Also I think removal of the WindowAgg node is fine here because it&#39;s<br=
>
not necessary to calculate the result of the sub query at<br>
all. Actually the query above runs fine on v46. I guess some of the<br>
incremental patches caused this to stop working. Can you elaborate?<br></bl=
ockquote><div><br></div>I agree -- for this specific query, removing the Wi=
ndowAgg does not<br>change the count(*) result, since window functions do n=
ot change the<br>number of rows.<br><br>Regarding v46: the two bugs were ac=
tually latent from the initial<br>implementation, not caused by incremental=
 patches. The old code<br>added the entire DEFINE expression (e.g., &quot;i=
 &gt; PREV(i)&quot;) to the<br>query targetlist via findTargetlistEntrySQL9=
9(). In most cases,<br>DEFINE conditions involve comparison operators, so t=
he resulting<br>OpExpr did not match existing Var entries in the targetlist=
, and a<br>new resjunk entry was created. This accidentally prevented remov=
al.<br>(In fact, a bare boolean DEFINE such as &quot;DEFINE A AS flag&quot;=
 would<br>have exhibited the same removal bug even in v46, since<br>findTar=
getlistEntrySQL99 would reuse the existing entry instead<br>of creating a n=
ew resjunk one -- confirming that the issue was<br>latent in the original i=
mplementation, not introduced by recent<br>patches.)<br><br>However, that s=
ame approach caused the SIGSEGV (Bug 1): when RPR<br>and non-RPR windows co=
exist, the non-RPR WindowAgg inherits<br>targetlist entries containing RPRN=
avExpr nodes that it cannot<br>evaluate.<br><br>The two bugs are two sides =
of the same coin -- the old approach<br>prevented removal but caused SIGSEG=
V. Switching to Var-only<br>addition fixed the SIGSEGV but required the all=
paths.c guard to<br>prevent removal.<br><br>As for more precise optimizatio=
n (allowing removal when all<br>WindowFuncs for an RPR clause are unused), =
it would require<br>structural changes to the loop in remove_unused_subquer=
y_outputs(),<br>which currently evaluates entries individually. Since this =
function<br>runs for all subqueries, not just RPR, such a change would need=
<br>broader testing.<br><br>The patch addresses two distinct issues: the pa=
rse_rpr.c change<br>(Var-only addition) prevents RPRNavExpr from propagatin=
g to<br>non-RPR WindowAgg nodes, while the allpaths.c guard prevents<br>inc=
orrect column removal. Since both are needed, the guard<br>cannot simply be=
 reverted. Would it make sense to keep it as-is<br>for now, and explore the=
 more precise optimization as a separate<br>follow-up patch?<br><br>Best re=
gards,<br><div>Henson=C2=A0</div></div></div>
</div>

--00000000000082e52c064f173f17--