MIME-Version: 1.0
References: 
 <CAMbWs48XHJEK1Q1CzAQ7L9sTANTs9W1cepXu8=Kc0quUL+tg4Q@mail.gmail.com>
 <CAApHDvoTyLG35Rm26F7-MFmX3kv-uAs9FD4XF7fqy-B0W6o_UQ@mail.gmail.com>
 <412552.1706203379@sss.pgh.pa.us>
In-Reply-To: <412552.1706203379@sss.pgh.pa.us>
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 26 Jan 2024 13:38:53 +0800
Message-ID: 
 <CAMbWs48d4JLv2uQsvK0EfqeNRACiyLT9yTUyu_4oSGxvKSR96g@mail.gmail.com>
Subject: Re: A performance issue with Memoize
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <dgrowleyml@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000cb0c8f060fd2b6d2"
Archived-At: 
 <https://www.postgresql.org/message-id/CAMbWs48d4JLv2uQsvK0EfqeNRACiyLT9yTUyu_4oSGxvKSR96g%40mail.gmail.com>
Precedence: bulk

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

On Fri, Jan 26, 2024 at 1:22=E2=80=AFAM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Apologies for not having noticed this thread before.  I'm taking
> a look at it now.  However, while sniffing around this I found
> what seems like an oversight in paramassign.c's
> assign_param_for_var(): it says it should compare all the same
> fields as _equalVar except for varlevelsup, but it's failing to
> compare varnullingrels.  Is that a bug?  It's conceivable that
> it's not possible to get here with varnullingrels different and
> all else the same, but I don't feel good about that proposition.
>
> I tried adding
>
> @@ -91,7 +91,10 @@ assign_param_for_var(PlannerInfo *root, Var *var)
>                  pvar->vartype =3D=3D var->vartype &&
>                  pvar->vartypmod =3D=3D var->vartypmod &&
>                  pvar->varcollid =3D=3D var->varcollid)
> +            {
> +                Assert(bms_equal(pvar->varnullingrels,
> var->varnullingrels));
>                  return pitem->paramId;
> +            }
>          }
>      }


Yeah, I think it should be safe to assert that the varnullingrels is
equal here.  The Var is supposed to be an upper-level Var, and two same
such Vars should not have different varnullingrels at this point,
although the varnullingrels might be adjusted later in
identify_current_nestloop_params according to which form of identity 3
we end up applying.

Thanks
Richard

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Fri, Jan 26, 2024 at 1:22=E2=80=AFAM T=
om Lane &lt;<a href=3D"mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>&gt; =
wrote:</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Apologies for not having noticed this thread before.=C2=A0 I&#39;m taking<b=
r>
a look at it now.=C2=A0 However, while sniffing around this I found<br>
what seems like an oversight in paramassign.c&#39;s<br>
assign_param_for_var(): it says it should compare all the same<br>
fields as _equalVar except for varlevelsup, but it&#39;s failing to<br>
compare varnullingrels.=C2=A0 Is that a bug?=C2=A0 It&#39;s conceivable tha=
t<br>
it&#39;s not possible to get here with varnullingrels different and<br>
all else the same, but I don&#39;t feel good about that proposition.<br>
<br>
I tried adding<br>
<br>
@@ -91,7 +91,10 @@ assign_param_for_var(PlannerInfo *root, Var *var)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pvar-&gt;vart=
ype =3D=3D var-&gt;vartype &amp;&amp;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pvar-&gt;vart=
ypmod =3D=3D var-&gt;vartypmod &amp;&amp;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pvar-&gt;varc=
ollid =3D=3D var-&gt;varcollid)<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(bms_equal(p=
var-&gt;varnullingrels, var-&gt;varnullingrels));<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return pitem-=
&gt;paramId;<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}<br>
=C2=A0 =C2=A0 =C2=A0}</blockquote><div><br></div><div>Yeah, I think it shou=
ld be safe to assert that the varnullingrels is<br>equal here.=C2=A0 The Va=
r is supposed to be an upper-level Var, and two same<br>such Vars should no=
t have different varnullingrels at this point,<br>although the varnullingre=
ls might be adjusted later in<br>identify_current_nestloop_params according=
 to which form of identity 3<br>we end up applying.<br><br>Thanks<br>Richar=
d<br></div></div></div>

--000000000000cb0c8f060fd2b6d2--