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>
In-Reply-To: 
 <CADkLM=dWJaw04u9FU=WkykOy7bu7B5eKmoDkfVz6JXh7mdemkw@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Tue, 23 Jun 2026 08:10:52 -0400
Message-ID: 
 <CA+TgmoY3a_S0munZ0wg4hQpMKVMyM_jpi6aGhFR0gAm1jLukag@mail.gmail.com>
Subject: Re: use of SPI by postgresImportForeignStatistics
To: Corey Huinker <corey.huinker@gmail.com>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmoY3a_S0munZ0wg4hQpMKVMyM_jpi6aGhFR0gAm1jLukag%40mail.gmail.com>
Precedence: bulk

On Mon, Jun 22, 2026 at 3:08=E2=80=AFPM Corey Huinker <corey.huinker@gmail.=
com> wrote:
> The first bit of dissonance comes from the SQL-level functions having sch=
emaname+relname parameters, and the reloid is resolved via RangeVarGetRelid=
Extended() which has a callback to check for correct permissions and settin=
g the proper lock level. However, the C-caller would either have the reloid=
 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 ta=
ble. So either we make an equivalent to RangeVarGetRelidExtended() that tak=
es an oid, or the C-caller has to derive a RangeVar, call the existing Rang=
eVarGetRelidExtended() function, and verify the result reloid against the s=
upplied 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 func=
tion that checks for ShareUpdateExclusiveLock on the Relation, and then doe=
s 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.

> A smaller bit of dissonance was with RecoveryInProgress(), which if I rec=
all we're checking before RangeVarGetRelidExtended() to avoid trying to tak=
e a lock that will fail. That check might not be meaningful if the C call t=
akes a relation, thus ensuring that some level of locking worked, thus we a=
ren't in recovery.

I don't quite follow this part.

> Next is the existing validation functions stats_check_required_arg(), sta=
ts_check_arg_array(), and stats_check_arg_pairs() all work on values indexe=
d by the positional functioncallinfo and the corresponding relstatsinfo/att=
statsinfo structure, and this makes a lot of type checking and value checki=
ng compact and uniform.  If we want to keep this sort of uniformity, the re=
sulting StatsData structure will end up looking a lot like the FunctionCall=
Info 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.

> 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 for =
their null-ness and if they're the correct type, so the value itself is usu=
ally just passed directly from fcinfo into the heap tuple values[] array. T=
he float[] values are checked for number of elements and whether any elemen=
ts 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's little to ga=
in there.

Right, so the question is whether it makes more sense to pass down C
strings or Datums.

> There's some additional hassle in the fact that pg_restore_attribute_stat=
s() can take an attnum parameter OR an attname parameter, but not both. I w=
as able to resolve that in a semi-elegant fashion, but the other issues hav=
e convinced me that we're probably better off continuing to use the Functio=
nCallInfo version of attribute_stats_update(), though perhaps with a differ=
ent name, allowing us to use that name for the new API call instead of impo=
rt_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.

--=20
Robert Haas
EDB: http://www.enterprisedb.com