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>
In-Reply-To: 
 <CA+TgmoaVcgZJpwxyi7HEfNMJhmm7BXkTQ468jvxkW2QzQZRkvg@mail.gmail.com>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Tue, 16 Jun 2026 18:50:20 -0400
Message-ID: 
 <CADkLM=dy18dDJ+j3-_Ofn11vt5J7zF33oTWnE6N=ePc83uS91A@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="0000000000003492c0065466c58f"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3Ddy18dDJ%2Bj3-_Ofn11vt5J7zF33oTWnE6N%3DePc83uS91A%40mail.gmail.com>
Precedence: bulk

--0000000000003492c0065466c58f
Content-Type: text/plain; charset="UTF-8"

>
> >
> > Would you be ok with a function like attribute_statistics_update, but
> purely with cstring args? Obviously the callers would have to modify their
> calls each time a new stat param is added, but that is seemingly preferable
> for you than the current situation.
>
> I feel pretty strongly that we want a way to identify the relation by
> OID rather than by schema and name.


Previous versions of pg_set_attribute_stats() were like that, but it went
away as the use-cases folded into pg_restore_attribute_stats(). So it's
do-able, but will make sharing code with the existing function harder in
the near term. We may have to settle for something that takes the oid, but
then plugs into the existing functions until such time as we can refactor
the existing callers.


> I'm not sure whether attributes
> should be identified by name or by number in this context. Apart from
> those things, if passing strings and leaving it up to the called
> function to interpret them makes most sense, that's OK with me.
>

Identifying attributes by attnum is mostly for index expressions, so
attname would be the preference here.


> I agree with you that getting rid of SPI is a much higher priority
> than getting rid of the FunctionCallInfo stuff altogether. I could
> live with DirectFunctionCall if we don't have a better option.
>

Ok, good to know that I have that as a fallback position.

I think the right first step is to create the functions
import_relation_stats() and import_attribute_stats() which take the oid of
the destination relation and the rest of the parameters are const char
pointers, which is the ideal case for a bunch of values being fetched from
PQgetValue(). Those functions will for now just make the call to
attribute_statistics_update() / relation_statistics_update(), but at least
it will be localized inside relation_stats.c and attribute_stats.c where we
can make internal refactors without disturbing anyone else.

Now, we *could* make these 2 new functions take the oid/oid+attname
followed by a variadic array of keyname+value string pairs like the
existing pg_restore_*_stats() functions, so the functions call signature
would be somewhat future-proofed. Let me know if that violates your vision
of what this should be.


>  I'm a
> bit suspicious that this will lead to code bloat in postgres_fdw or
> elsewhere, but maybe not.
>

postgres_fdw will get slimmer for not having SPI in it. relation_stats.c
and attribute_stats.c will have some temporary bloat until things can be
refactored.

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

<div dir=3D"ltr"><div class=3D"gmail_quote gmail_quote_container"><blockquo=
te class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px =
solid rgb(204,204,204);padding-left:1ex">&gt;<br>
&gt; Would you be ok with a function like attribute_statistics_update, but =
purely with cstring args? Obviously the callers would have to modify their =
calls each time a new stat param is added, but that is seemingly preferable=
 for you than the current situation.<br>
<br>
I feel pretty strongly that we want a way to identify the relation by<br>
OID rather than by schema and name.</blockquote><div><br></div><div>Previou=
s versions of pg_set_attribute_stats() were like that, but it went away as =
the use-cases folded into pg_restore_attribute_stats(). So it&#39;s do-able=
, but will make sharing code with the existing function harder in the near =
term. We may have to settle for something that takes the oid, but then plug=
s into the existing functions until such time as we can refactor the existi=
ng callers.</div><div></div><div>=C2=A0</div><blockquote class=3D"gmail_quo=
te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204=
);padding-left:1ex"> I&#39;m not sure whether attributes<br>
should be identified by name or by number in this context. Apart from<br>
those things, if passing strings and leaving it up to the called<br>
function to interpret them makes most sense, that&#39;s OK with me.<br></bl=
ockquote><div><br></div><div>Identifying attributes by attnum is mostly for=
 index expressions, so attname would be the preference here.</div><div></di=
v><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0p=
x 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

I agree with you that getting rid of SPI is a much higher priority<br>
than getting rid of the FunctionCallInfo stuff altogether. I could<br>
live with DirectFunctionCall if we don&#39;t have a better option.<br></blo=
ckquote><div><br></div><div>Ok, good to know that I have that as a fallback=
 position.<br><br></div><div>I think the right first step is to create the =
functions import_relation_stats() and import_attribute_stats() which take t=
he oid of the destination relation and the rest of the parameters are const=
 char pointers, which is the ideal case for a bunch of values being fetched=
 from PQgetValue(). Those functions will for now just make the call to attr=
ibute_statistics_update() / relation_statistics_update(), but at least it w=
ill be localized inside relation_stats.c and attribute_stats.c where we can=
 make internal refactors without disturbing anyone else.<br><br></div><div>=
Now, we *could* make these 2 new functions take the oid/oid+attname followe=
d by a variadic array of keyname+value string pairs like the existing pg_re=
store_*_stats() functions, so the functions call signature would be somewha=
t future-proofed. Let me know if that violates your vision of what this sho=
uld be.</div><div></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" =
style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);pa=
dding-left:1ex">=C2=A0I&#39;m a<br>
bit suspicious that this will lead to code bloat in postgres_fdw or<br>
elsewhere, but maybe not.<br></blockquote><div><br></div><div>postgres_fdw =
will get slimmer for not having SPI in it.=C2=A0relation_stats.c and attrib=
ute_stats.c will have some temporary bloat until things can be refactored.<=
/div></div></div>

--0000000000003492c0065466c58f--