public inbox for [email protected]  

help / color / mirror / Atom feed

From: WangYu <[email protected]>
To: Corey Huinker <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: jian he <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re:Re: Extended Statistics set/restore/clear functions.
Date: Thu, 4 Dec 2025 08:52:16 +0800 (CST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CADkLM=ckVb6AvfTsN=W3Z+Hqy9bzhh-quRsJOy_tzY-zUqWb9A@mail.gmail.com>
References: <CADkLM=d_mmTn3+ChL-z_ZomwOKT-BssQU+=6_a8WDv6A4b7uAA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CACJufxGGzkb58BU+YyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A@mail.gmail.com>
	<[email protected]>
	<CADkLM=czQ0UUyBn3AiHZfJJWRwXH5e2xn056yG0mpuXpW09RSw@mail.gmail.com>
	<CACJufxHVKiVEvCsc11oxOM=0GgjqChFwi-ii4RJuw95ZFB_fHQ@mail.gmail.com>
	<CADkLM=eRop0t==hg0TU3KuDH6DgnUxQ6vZxV9th=6vW1JG36MQ@mail.gmail.com>
	<CACJufxGgEnOzFq0HjmkKBTnEUgToGFC0g0A4wPWoJMzOuoWUhw@mail.gmail.com>
	<CADkLM=crdyK3w7-EDEdDfXHGidxm2ms+bQYNFOZUD4eOLuLXEQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CADkLM=coABiYY0+fW7JzKFb-UJOXYXLDYPFYcJbHck2FniFMLw@mail.gmail.com>
	<CADkLM=ckVb6AvfTsN=W3Z+Hqy9bzhh-quRsJOy_tzY-zUqWb9A@mail.gmail.com>

Hi Corey
I was reviewing the recent patch v19-0003-Include-Extended-Statistics-in-pg_dump.patch and noticed a couple of small typo issues in the explanatory comments — nothing that affects the functionality.


Here are the two minor fixes I’d suggest:
1. “ndistintinct” should be “ndistinct”.
2. “depdendencies” should be “dependencies”.


Best regards,
Yu







在 2025-12-04 08:46:29，"Corey Huinker" <[email protected]> 写道：





On Tue, Nov 25, 2025 at 11:14 PM Corey Huinker <[email protected]> wrote:


I don’t see any of my comments are addressed in v18.




My apologies. My v17 focused entirely on the input functions, as those were receiving the vast majority of the attention. Now that those are out of the way (Thanks Michael!) I can address those issues.


Paraphrasing the "quotes" here for brevity...


> Several functions are made external visible, they are all renamed with adding a prefix “statatt_”, why text_to_stavalues is an exception?


Michael had specifically said that one didn't need to be renamed. I suppose statatt_import_stavalues() might be a good name for it. It *is* specific to attribute stats, though that definition also applies to the attribute stats nested in the stxdexprs of extended stats. I have no strong opinion on the matter.


> This MVDependency * can be const.

+1

> static void
> upsert_pg_statistic_ext_data(Datum *values, bool *nulls, bool *replaces)
> {
> ```

> This function pass values, nulls and replaces to heap_modify_tuple() and heap_form_tuple(),
> the both functions take all const pointers as parameters.
> So, here values, nulls and replaces can all be const.
I find your argument here persuasive enough to override Michael's previously stated non-excitement.


> ...the two NULL_FRAC questions


Yes, fixed.

> import_mcvlist is declared twice, looks like a copy-paste mistake.


That or a rebase/apply gone wrong. Fixed.


> PREPQUERY_DUMPEXTSTATSSTATS weird, suggest PREPQUERY_DUMPEXTSTATSDATA


Awkward names are almost inevitable when talking about the statistics associated with an object of type "statistics".


There was a debate about whether statistics were data or not, and I'd rather not restart that, so I went with PREPQUERY_DUMPEXTSTATSOBJSTATS for now.


 Incorporated these fixes, and some other lessons learned.

view thread (91+ 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], [email protected], [email protected], [email protected]
  Subject: Re: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