Re: Extended Statistics set/restore/clear functions.

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Corey Huinker <[email protected]>
Cc: Tender Wang <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Extended Statistics set/restore/clear functions.
Date: Thu, 15 Jan 2026 16:57:00 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CADkLM=fnhZQzrRRB4ig5FvDfZ0trrYugLk2_R6qyzDPG4ESLpg@mail.gmail.com>
	<[email protected]>
	<CADkLM=cFrhE76jnqPZz4pBjyA97qfYkDqWUpw1PK7HEfWB+H4w@mail.gmail.com>
	<CADkLM=dNUxRonk5KUaUpnq-QoiwcoptZKMzqxMLOD=z4NeCZkw@mail.gmail.com>
	<[email protected]>
	<CAHewXNmV6odpdLNYeSaU3RDwzzC5ndbMw2mxuXX74c+8PReYrQ@mail.gmail.com>
	<CAHewXNku-jz-FPKeJVk25fZ1pV2buYh5vpeqGDOB=bFQhKxXhw@mail.gmail.com>
	<[email protected]>
	<CADkLM=eKxxhXeYsHH7fbLf1waTdAyKeH+A=n039c8Ht+O7JKbA@mail.gmail.com>
	<[email protected]>



> On Jan 15, 2026, at 16:09, Michael Paquier <[email protected]> wrote:
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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 
> useful on its own, also because its relation lookup logic is the same
> as the restore function.
> 
> 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);
> 
> 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 
> 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.
> 
> 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.
> 
> 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.
> 
> 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 = PG_GETARG_NAME(INHERITED_ARG);
```

Should this use PG_GETARG_BOOL()?

2
```
  proparallel => 'u', prorettype => 'void', proargtypes => 'text text text text bool’,
```

Should we define proargtypes as “name name name name bool”? As the doc change mentions type “name” for the first 4 parameters. Maybe you want to avoid that because Name is a structure. Anyway, I don’t 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 “2025-2026”, you will get a bunch of files, they should be created in 2025.

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

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

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Extended Statistics set/restore/clear functions.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox