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>
In-Reply-To: 
 <CADkLM=c9BEuA9vPjE9Wco6+Lz-HSnnmBZrFhi=-yNi0K8tmZug@mail.gmail.com>
From: Etsuro Fujita <etsuro.fujita@gmail.com>
Date: Mon, 29 Jun 2026 20:50:45 +0900
Message-ID: 
 <CAPmGK17W1sUB3JnkRoBpgO0_iSH5HRcc0_nxk8Us4KYsQhuQxg@mail.gmail.com>
Subject: Re: use of SPI by postgresImportForeignStatistics
To: Corey Huinker <corey.huinker@gmail.com>
Cc: Robert Haas <robertmhaas@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/CAPmGK17W1sUB3JnkRoBpgO0_iSH5HRcc0_nxk8Us4KYsQhuQxg%40mail.gmail.com>
Precedence: bulk

On Mon, Jun 29, 2026 at 5:14=E2=80=AFPM Corey Huinker <corey.huinker@gmail.=
com> wrote:

>> Requiring Datums simplifies things greatly inside the existing functions=
, but pushes that complexity to the caller. My first proposal was to keep c=
omplexity 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.
>
>
> I've had some time to look this over, and it's pretty clear that we don't=
 actually need the whole positional FunctionCallInfo, we just need the fcin=
fo->args of it. We also need the ability to pass statistical values along w=
ith the values that comprise the key of the relation/attribute/object to be=
 modified.
>
> There are important differences in the parameters needed by the different=
 types of functions. The pg_restore_*() SQL function calls need to identify=
 the schema+relname of the relation being modified, and already have all of=
 the values as typed Datums, whereas the internal API calls already have an=
 identified and locked open Relation, but all of the statistical parameters=
 are just cstrings.
>
> The solution I chose was to create common functions that take a relation =
object and an array of just the statistical parameters. This requires the p=
g_restore_* functions to resolve and lock the relation themselves, and then=
 carry forward just the subset of parameters that are statistics (right typ=
es, but wrong number of arguments). Conversely, the internal API functions =
need to map their array of cstring values to the correctly typed Datums (wr=
ong types, but right number of arguments in the right order).
>
> Attached is v2. The patches are broken down into very small bites to aid =
digestion and to confirm that tests pass after each comparatively minor cha=
nge.

Thanks for doing that work!

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.

Best regards,
Etsuro Fujita