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>
In-Reply-To: 
 <CA+TgmoY3a_S0munZ0wg4hQpMKVMyM_jpi6aGhFR0gAm1jLukag@mail.gmail.com>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Tue, 23 Jun 2026 11:53:22 -0400
Message-ID: 
 <CADkLM=cdKjgphHpFW8kKukHF+DVYKogHcqLXO=BA8MGfiRFqQA@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="000000000000e6a15a0654edc2b5"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3DcdKjgphHpFW8kKukHF%2BDVYKogHcqLXO%3DBA8MGfiRFqQA%40mail.gmail.com>
Precedence: bulk

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

On Tue, Jun 23, 2026 at 8:11=E2=80=AFAM Robert Haas <robertmhaas@gmail.com>=
 wrote:

> On Mon, Jun 22, 2026 at 3:08=E2=80=AFPM Corey Huinker <corey.huinker@gmai=
l.com>
> wrote:
> > The first bit of dissonance comes from the SQL-level functions having
> schemaname+relname parameters, and the reloid is resolved via
> RangeVarGetRelidExtended() which has a callback to check for correct
> permissions and setting the proper lock level. However, the C-caller woul=
d
> either have the reloid already, or an already open Relation, but no
> assurance that the caller has the correct permissions for that table or t=
he
> correct lock level on the table. So either we make an equivalent to
> RangeVarGetRelidExtended() that takes an oid, or the C-caller has to deri=
ve
> a RangeVar, call the existing RangeVarGetRelidExtended() function, and
> verify the result reloid against the supplied parameter. I went with
> deriving the RangeVar and putting an Assert on the before/after reloids,
> but perhaps the smarter play is to make a function that checks for
> ShareUpdateExclusiveLock on the Relation, and then does the equivalent of
> RangeVarCallbackForStats().
>
> I don't think this is really a problem. If the caller is specifying
> the OID, they should have called RangeVarGetRelidExtended themselves.
> Permissions-checking, locking, and opening the relation should all
> happen simultaneously, and the logic shouldn't be duplicated later.
>

It seems dangerous to me to provide an externally visible function that
assumes the caller has already self-verified that they have permission to
modify stats for an object.

It's possible that the existing analyze() code already has verified these
permissions, but at this moment I'm unsure.


> > A smaller bit of dissonance was with RecoveryInProgress(), which if I
> recall we're checking before RangeVarGetRelidExtended() to avoid trying t=
o
> take a lock that will fail. That check might not be meaningful if the C
> call takes a relation, thus ensuring that some level of locking worked,
> thus we aren't in recovery.
>
> I don't quite follow this part.
>

I mean that I don't know if that check is required in situations where the
caller already has an open relation with the right lock, and if we can
avoid the call by having the API take a Relation rather than an Oid, then
that would be preferable.


>
> > Next is the existing validation functions stats_check_required_arg(),
> stats_check_arg_array(), and stats_check_arg_pairs() all work on values
> indexed by the positional functioncallinfo and the corresponding
> relstatsinfo/attstatsinfo structure, and this makes a lot of type checkin=
g
> and value checking compact and uniform.  If we want to keep this sort of
> uniformity, the resulting StatsData structure will end up looking a lot
> like the FunctionCallInfo that we already had.
>
> Yeah, this is worth thinking about. You could consider putting an
> array inside the struct and use #defines for the indexes. And instead
> of having a separate Boolean for each index, you could use the same
> index to reference the N'th bit of a single integer flag variable.


That's basically what I was trying, though I wasn't brave enough to use
bitflags in a v1 patch.

The #defines already exist now in the form of enums that act as indexes
into the FunctionCallInfo array. But if my data structure is an array of
Datums with an array of is-not-nulls, then I'm just not that far away from
the FunctionCallInfo structure we already have.


>
> > Next is the fact that the end-destination for every value passed in is =
a
> Datum for a pg_statistic heaptuple. Most Datum values are checked only fo=
r
> their null-ness and if they're the correct type, so the value itself is
> usually just passed directly from fcinfo into the heap tuple values[]
> array. The float[] values are checked for number of elements and whether
> any elements are NULL, but that is done via array functions that take a
> Datum input. Only in a few cases do we actually look at the actual intern=
al
> value of the Datum (reltuples, attname, attnum, the anyarrays), so there'=
s
> little to gain there.
>
> Right, so the question is whether it makes more sense to pass down C
> strings or Datums.
>

Requiring Datums simplifies things greatly inside the existing functions,
but pushes that complexity to the caller. My first proposal was to keep
complexity to an absolute minimum on the caller side, but your comments
make me think there's considerable tolerance for more complexity on the
caller side.


> > There's some additional hassle in the fact that
> pg_restore_attribute_stats() can take an attnum parameter OR an attname
> parameter, but not both. I was able to resolve that in a semi-elegant
> fashion, but the other issues have convinced me that we're probably bette=
r
> off continuing to use the FunctionCallInfo version of
> attribute_stats_update(), though perhaps with a different name, allowing =
us
> to use that name for the new API call instead of
> import_attribute_statistics().
>
> On that particular point, I think I'm still unconvinced, but I also
> haven't looked deeply into this just yet, so maybe I'm wrong.
>

The issue becomes moot if we require the caller to construct their own
isnull and values arrays as was suggested above.

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

<div dir=3D"ltr"><div class=3D"gmail_quote gmail_quote_container"><div dir=
=3D"ltr" class=3D"gmail_attr">On Tue, Jun 23, 2026 at 8:11=E2=80=AFAM Rober=
t Haas &lt;<a href=3D"mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</=
a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p=
x 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On=
 Mon, Jun 22, 2026 at 3:08=E2=80=AFPM Corey Huinker &lt;<a href=3D"mailto:c=
orey.huinker@gmail.com" target=3D"_blank">corey.huinker@gmail.com</a>&gt; w=
rote:<br>
&gt; The first bit of dissonance comes from the SQL-level functions having =
schemaname+relname parameters, and the reloid is resolved via RangeVarGetRe=
lidExtended() which has a callback to check for correct permissions and set=
ting the proper lock level. However, the C-caller would either have the rel=
oid already, or an already open Relation, but no assurance that the caller =
has the correct permissions for that table or the correct lock level on the=
 table. So either we make an equivalent to RangeVarGetRelidExtended() that =
takes an oid, or the C-caller has to derive a RangeVar, call the existing R=
angeVarGetRelidExtended() function, and verify the result reloid against th=
e supplied parameter. I went with deriving the RangeVar and putting an Asse=
rt on the before/after reloids, but perhaps the smarter play is to make a f=
unction that checks for ShareUpdateExclusiveLock on the Relation, and then =
does the equivalent of RangeVarCallbackForStats().<br>
<br>
I don&#39;t think this is really a problem. If the caller is specifying<br>
the OID, they should have called RangeVarGetRelidExtended themselves.<br>
Permissions-checking, locking, and opening the relation should all<br>
happen simultaneously, and the logic shouldn&#39;t be duplicated later.<br>=
</blockquote><div><br></div><div>It seems dangerous to me to provide an ext=
ernally visible function that assumes the caller has already self-verified =
that they have permission to modify stats for an object.</div><div><br></di=
v><div>It&#39;s possible that the existing analyze() code already has verif=
ied these permissions, but at this moment I&#39;m unsure.</div><div></div><=
div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0=
px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

&gt; A smaller bit of dissonance was with RecoveryInProgress(), which if I =
recall we&#39;re checking before RangeVarGetRelidExtended() to avoid trying=
 to take a lock that will fail. That check might not be meaningful if the C=
 call takes a relation, thus ensuring that some level of locking worked, th=
us we aren&#39;t in recovery.<br>
<br>
I don&#39;t quite follow this part.<br></blockquote><div><br></div><div>I m=
ean that I don&#39;t know if that check is required in situations where the=
 caller already has an open relation with the right lock, and if we can avo=
id the call by having the API take a Relation rather than an Oid, then that=
 would be preferable.</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(2=
04,204,204);padding-left:1ex">
<br>
&gt; Next is the existing validation functions stats_check_required_arg(), =
stats_check_arg_array(), and stats_check_arg_pairs() all work on values ind=
exed by the positional functioncallinfo and the corresponding relstatsinfo/=
attstatsinfo structure, and this makes a lot of type checking and value che=
cking compact and uniform.=C2=A0 If we want to keep this sort of uniformity=
, the resulting StatsData structure will end up looking a lot like the Func=
tionCallInfo that we already had.<br>
<br>
Yeah, this is worth thinking about. You could consider putting an<br>
array inside the struct and use #defines for the indexes. And instead<br>
of having a separate Boolean for each index, you could use the same<br>
index to reference the N&#39;th bit of a single integer flag variable.</blo=
ckquote><div><br></div><div>That&#39;s basically what I was trying, though =
I wasn&#39;t brave enough to use bitflags in a v1 patch.</div><div><br></di=
v><div>The #defines already exist now in the form of enums that act as inde=
xes into the FunctionCallInfo array. But if my data structure is an array o=
f Datums with an array of is-not-nulls, then I&#39;m just not that far away=
 from the FunctionCallInfo structure we already have.</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);padding-left:1ex">
<br>
&gt; Next is the fact that the end-destination for every value passed in is=
 a Datum for a pg_statistic heaptuple. Most Datum values are checked only f=
or their null-ness and if they&#39;re the correct type, so the value itself=
 is usually just passed directly from fcinfo into the heap tuple values[] a=
rray. The float[] values are checked for number of elements and whether any=
 elements are NULL, but that is done via array functions that take a Datum =
input. Only in a few cases do we actually look at the actual internal value=
 of the Datum (reltuples, attname, attnum, the anyarrays), so there&#39;s l=
ittle to gain there.<br>
<br>
Right, so the question is whether it makes more sense to pass down C<br>
strings or Datums.<br></blockquote><div><br></div><div>Requiring Datums sim=
plifies things greatly inside the existing functions, but pushes that compl=
exity to the caller. My first proposal was to keep complexity to an absolut=
e minimum on the caller side, but your comments make me think there&#39;s c=
onsiderable tolerance for more complexity on the caller side.</div><div></d=
iv><div><br></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">
<br>
&gt; There&#39;s some additional hassle in the fact that pg_restore_attribu=
te_stats() can take an attnum parameter OR an attname parameter, but not bo=
th. I was able to resolve that in a semi-elegant fashion, but the other iss=
ues have convinced me that we&#39;re probably better off continuing to use =
the FunctionCallInfo version of attribute_stats_update(), though perhaps wi=
th a different name, allowing us to use that name for the new API call inst=
ead of import_attribute_statistics().<br>
<br>
On that particular point, I think I&#39;m still unconvinced, but I also<br>
haven&#39;t looked deeply into this just yet, so maybe I&#39;m wrong.<br></=
blockquote><div><br></div><div>The issue becomes moot if we require the cal=
ler to construct their own isnull and values arrays as was suggested above.=
</div></div></div>

--000000000000e6a15a0654edc2b5--