MIME-Version: 1.0
References: 
 <CA+TgmoYqMtWb4zLUkT98oFnEkJ=Wz0Pw-ggDJrp9wnSXPzUaeQ@mail.gmail.com>
 <CAPmGK161TJ4G01eptv3sBqgxa4SNHDE9VRqoC6jy5Y6G3D8yDw@mail.gmail.com>
 <CA+TgmoZpwfTGqqp=0efV99e_BMYWdLFq2XKRWVSDcukifkZMeA@mail.gmail.com>
 <CADkLM=dxe8NbQu4RNi2OFXpaRKdpS0-CHGm6Mqtf_rW155BM4g@mail.gmail.com>
 <CADkLM=cV=uezAKyZdyyxnZOBRhGLUSbeYfDj5_jbDCLP9LP24Q@mail.gmail.com>
 <CA+TgmoaVcgZJpwxyi7HEfNMJhmm7BXkTQ468jvxkW2QzQZRkvg@mail.gmail.com>
 <CADkLM=dy18dDJ+j3-_Ofn11vt5J7zF33oTWnE6N=ePc83uS91A@mail.gmail.com>
 <CAPmGK15MfVT2H+PUvA_nAJvySGr_pAVkVx3KcnVXWGO36acWLA@mail.gmail.com>
 <CADkLM=dr5bfB8YN-bV1U0NDvyVO7a6uuz0BsAi89a1Md4A==8g@mail.gmail.com>
 <CAPmGK14P2GRe78hp6fZ3vExU_9RRDjMsYVOx7AALvUQ-hKTUiQ@mail.gmail.com>
 <CADkLM=cL9ZGvO-72QWyXA5Wqv-4P3T1fp+VFJy5e6k3Hf=POMw@mail.gmail.com>
 <CA+TgmoZdTVeXRQo__cEr_1y+mUO9nubOX=6PEfw7Cr0UfZQn6w@mail.gmail.com>
 <CADkLM=fe=ycPdwT2wE_iYwK8e8viMbWvwWhXR9vDe+55KKF-KQ@mail.gmail.com>
 <CADkLM=dWJaw04u9FU=WkykOy7bu7B5eKmoDkfVz6JXh7mdemkw@mail.gmail.com>
 <CA+TgmoY3a_S0munZ0wg4hQpMKVMyM_jpi6aGhFR0gAm1jLukag@mail.gmail.com>
 <CADkLM=cdKjgphHpFW8kKukHF+DVYKogHcqLXO=BA8MGfiRFqQA@mail.gmail.com>
 <CADkLM=c9BEuA9vPjE9Wco6+Lz-HSnnmBZrFhi=-yNi0K8tmZug@mail.gmail.com>
 <CAPmGK17W1sUB3JnkRoBpgO0_iSH5HRcc0_nxk8Us4KYsQhuQxg@mail.gmail.com>
 <CA+TgmoZd5MDhcx6uav6MWxhM42yWcfaRAHYKNtNkuog=yrKAgw@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoZd5MDhcx6uav6MWxhM42yWcfaRAHYKNtNkuog=yrKAgw@mail.gmail.com>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Mon, 29 Jun 2026 21:10:41 -0400
Message-ID: 
 <CADkLM=f_c7Xoq-bSzw28LgvxPa+OQ3rVow=xwkTyjz1J0=pMdw@mail.gmail.com>
Subject: Re: use of SPI by postgresImportForeignStatistics
To: Robert Haas <robertmhaas@gmail.com>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000046913a06556e3fff"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3Df_c7Xoq-bSzw28LgvxPa%2BOQ3rVow%3DxwkTyjz1J0%3DpMdw%40mail.gmail.com>
Precedence: bulk

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

On Mon, Jun 29, 2026 at 2:17=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:

> On Mon, Jun 29, 2026 at 7:50=E2=80=AFAM Etsuro Fujita <etsuro.fujita@gmai=
l.com>
> wrote:
> > I like this refactoring, but while it's rather mechanical, it's pretty
> > large, so I think it's too late to do the refactoring at this time
> > just before the beta 2 release.  So I'd vote for going with your
> > v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
> > Robert, I don't think it's good to call LOCAL_FCINFO() in
> > import_relation_statistics() and import_attribute_statistics() to call
> > the guts of those functions either, but that is *consistent* with the
> > existing way pg_restore_relation_stats() and
> > pg_restore_attribute_stats() do that, so that is actually not that
> > bad.  Also, as you mentioned above, it's inefficient for the new API
> > functions to lock an already-locked relation, and validate an
> > already-validated attname/attnum, but I think it would be negligible.


> Fujita-san, is it then your plan to get those two patches committed?


Are we ok with changing the functions that postgres_fdw makes from v19 to
v20? Because what I put in the v2 patch does not match the v1 patch. If we
are NOT ok with that, then we'd need to:

1. rename the two relation_stats_argnum -> relation_args_argnum and
attribute_stats_argnum -> attribute_args_argnum out of the way of the ones
created in relation_stats.h and attribute_stats.h. The actual enumeration
values can stay the same, as there are no conflicts there, just ambiguity
about which set of values they belong to.
2. rename the existing static functions relation_statistics_update ->
update_relstats and attribute_statistics_update -> update_relstats to make
way for the public functions of the same names.
3. Create new relation_statistics_update and attribute_statistics_update,
with the isnull/values, and have those fcinfo-invoke the respective
pg_restore functions just like v1 does.

That would at least make the user API consistent in v19 vs v20.

I'm on vacation this week, but time is short for beta2, so I'll make an
exception to get the above into beta2. Let me know if you want me to write
that up.

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

<div dir=3D"ltr"><div dir=3D"ltr">On Mon, Jun 29, 2026 at 2:17=E2=80=AFPM R=
obert Haas &lt;<a href=3D"mailto:robertmhaas@gmail.com">robertmhaas@gmail.c=
om</a>&gt; wrote:</div><div class=3D"gmail_quote gmail_quote_container"><bl=
ockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-lef=
t:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Jun 29, 2026 at 7:50=
=E2=80=AFAM Etsuro Fujita &lt;<a href=3D"mailto:etsuro.fujita@gmail.com" ta=
rget=3D"_blank">etsuro.fujita@gmail.com</a>&gt; wrote:<br>
&gt; I like this refactoring, but while it&#39;s rather mechanical, it&#39;=
s pretty<br>
&gt; large, so I think it&#39;s too late to do the refactoring at this time=
<br>
&gt; just before the beta 2 release.=C2=A0 So I&#39;d vote for going with y=
our<br>
&gt; v1-0001 and v1-0002 and doing the refactoring in v20.=C2=A0 As mention=
ed by<br>
&gt; Robert, I don&#39;t think it&#39;s good to call LOCAL_FCINFO() in<br>
&gt; import_relation_statistics() and import_attribute_statistics() to call=
<br>
&gt; the guts of those functions either, but that is *consistent* with the<=
br>
&gt; existing way pg_restore_relation_stats() and<br>
&gt; pg_restore_attribute_stats() do that, so that is actually not that<br>
&gt; bad.=C2=A0 Also, as you mentioned above, it&#39;s inefficient for the =
new API<br>
&gt; functions to lock an already-locked relation, and validate an<br>
&gt; already-validated attname/attnum, but I think it would be negligible.<=
/blockquote><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Fujita-san, is it then your plan to get those two patches committed?</block=
quote><div><br></div><div>Are we ok with changing the functions that postgr=
es_fdw makes from v19 to v20? Because what I put in the v2 patch does not m=
atch the v1 patch. If we are NOT ok with that, then we&#39;d need to:<br><b=
r></div><div>1. rename the two=C2=A0relation_stats_argnum -&gt; relation_ar=
gs_argnum and attribute_stats_argnum -&gt; attribute_args_argnum out of the=
 way of the ones created in relation_stats.h and attribute_stats.h. The act=
ual enumeration values can stay the same, as there are no conflicts there, =
just ambiguity about which set of values they belong to.</div><div>2. renam=
e the existing static functions relation_statistics_update -&gt; update_rel=
stats and attribute_statistics_update -&gt; update_relstats to make way for=
 the public functions of the same names.</div><div>3. Create new relation_s=
tatistics_update and attribute_statistics_update, with the isnull/values, a=
nd have those fcinfo-invoke the respective pg_restore functions just like v=
1 does.<br><br></div><div>That would at least make the user API consistent =
in v19 vs v20.</div><div><br></div><div>I&#39;m on vacation this week, but =
time is short for beta2, so I&#39;ll make an exception to get the above int=
o beta2. Let me know if you want me to write that up.=C2=A0</div></div></di=
v>

--00000000000046913a06556e3fff--