Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81.1.4\))
Subject: Re: Extended Statistics set/restore/clear functions.
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <aWigpNk8CsZtn-P7@paquier.xyz>
Date: Thu, 15 Jan 2026 16:57:00 +0800
Cc: Corey Huinker <corey.huinker@gmail.com>,
 Tender Wang <tndrwang@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <58FB2F62-66DD-495E-83A6-607903834272@gmail.com>
References: <aTJT2arh0B6dWLci@paquier.xyz>
 <CADkLM=fnhZQzrRRB4ig5FvDfZ0trrYugLk2_R6qyzDPG4ESLpg@mail.gmail.com>
 <aTYpGfoSuRGTjJnG@paquier.xyz>
 <CADkLM=cFrhE76jnqPZz4pBjyA97qfYkDqWUpw1PK7HEfWB+H4w@mail.gmail.com>
 <CADkLM=dNUxRonk5KUaUpnq-QoiwcoptZKMzqxMLOD=z4NeCZkw@mail.gmail.com>
 <aUzYfkMELU5X1tFk@paquier.xyz>
 <CAHewXNmV6odpdLNYeSaU3RDwzzC5ndbMw2mxuXX74c+8PReYrQ@mail.gmail.com>
 <CAHewXNku-jz-FPKeJVk25fZ1pV2buYh5vpeqGDOB=bFQhKxXhw@mail.gmail.com>
 <aU2_GiTD3dXmxN1c@paquier.xyz>
 <CADkLM=eKxxhXeYsHH7fbLf1waTdAyKeH+A=n039c8Ht+O7JKbA@mail.gmail.com>
 <aWigpNk8CsZtn-P7@paquier.xyz>
To: Michael Paquier <michael@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/58FB2F62-66DD-495E-83A6-607903834272%40gmail.com>
Precedence: bulk


> On Jan 15, 2026, at 16:09, Michael Paquier <michael@paquier.xyz> =
wrote:
>=20
> On Wed, Jan 14, 2026 at 12:57:45PM -0500, Corey Huinker wrote:
>> Some of the header cleanup work required adding utils/typcache.h to
>> extended_stats.c.
>=20
> Thanks for sending a rebased set.  It is a large patch, but most of
> the changes are super mechanical, making it sort of "simpler" to think
> about the whole.
>=20
> Anyway, I have begun a review of the backend changes, and found one
> independent piece that was useful, leading to 32e27bd32082 that I have
> just applied after some renames and a couple of fixes.
>=20
> Then I have spent a good portion of today looking at the backend
> changes, with first a focus to extract the "clear" function as it is=20=

> useful on its own, also because its relation lookup logic is the same
> as the restore function.
>=20
> And then, this block of code has been giving me a long pause, because
> it was fishy, and clearly wrong:
> +  /* no need to fetch reloid, we already have it */
> +  RangeVarGetRelidExtended(makeRangeVar(nspname,
> +       RelationGetRelationName(rel), -1),
> +       ShareUpdateExclusiveLock, 0,
> +       RangeVarCallbackForStats, &locked_table);
>=20
> A shocking thing here is that we build a RangeVar for the relation of
> the extended stats object based on the *extstat namespace*, not the
> *relation namespace*, and that we do so *after* opening the extstat
> catalog.  Anyway, I think that we need to redesign that, and for
> consistency's sake do a RangeVar lookup *before* even trying to touch=20=

> the stats data or open its catalogs.  That would be beneficial on
> consistency ground, and one piece where I think that patch is designed
> wrongly is that we should give in input of the new clear and restore
> functions the *schema* and *relation* names of the table we expect to
> find, so as we can enforce first a clean RangeVar lookup, then
> manipulate the stats data.  This relates to 688dc6299a5b, as well,
> except that we would be stuck with an incorrect design after release
> if we are not careful because the function schema would be stuck in
> time.  This has to be right from the start.
>=20
> All these changes have given me the attached patch for the clear()
> function.  Another piece is that we did not really check that a role
> has the MAINTAIN rights of the relation where we want to manipulate
> the extended stats.
>=20
> A final thing is the location of the new SQL function code, let's put
> that into a new file, named extended_stats_funcs.c in the patch.
> There is nothing in the new restore and clear functions that require
> extended_stats.c.
>=20
> Finally, for the clear() part, the attached version addresses
> everything I have found during my review.  We will need to make the
> restore() part follow the same design model with the Rangevar lookups
> of the parent relation, or we'll have trouble waiting ahead.
> --
> Michael
> <v24-0001-Add-pg_clear_extended_stats.patch>

Hi Michael,

Here are some comments for v24:

1.
```
+	inherited =3D PG_GETARG_NAME(INHERITED_ARG);
```

Should this use PG_GETARG_BOOL()?

2
```
  proparallel =3D> 'u', prorettype =3D> 'void', proargtypes =3D> 'text =
text text text bool=E2=80=99,
```

Should we define proargtypes as =E2=80=9Cname name name name bool=E2=80=9D=
? As the doc change mentions type =E2=80=9Cname=E2=80=9D for the first 4 =
parameters. Maybe you want to avoid that because Name is a structure. =
Anyway, I don=E2=80=99t have a strong opinion here.

3 - extended_stats_funcs.c
```
 * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
```

This is a brand new file, so the copyright year should be just 2026. If =
you search for =E2=80=9C2025-2026=E2=80=9D, you will get a bunch of =
files, they should be created in 2025.

4 - Given comment 1, no test fails. I think that=E2=80=99s because all =
test cases use inherited =3D false. Maybe we need a test case for =
inherited =3D true.=20

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/