Обсуждение: Re: vacuumdb changes for stats import/export

On Wed, Feb 5, 2025 at 4:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> [v2]

I started looking just at 0001, and it seems like a fairly
straightforward rearrangement. I found this comment quite hard to
read:

+ * 'found_objs' should be a fully qualified list of objects to process, as
+ * returned by a previous call to vacuum_one_database().  If *found_objs is
+ * NULL, it is set to the results of the catalog query discussed below.  If
+ * found_objs is NULL, the results of the catalog query are not returned.
+ *
+ * If *found_objs is NULL, this function performs a catalog query to retrieve
+ * the list of tables to process.  When 'objects' is NULL, all tables in the

I had to read it several times before I noticed the difference between
"* found_objs" and "*found_objs". Maybe some extra spacing and breaks
would help, or other reorganization.

--
John Naylor
Amazon Web Services

On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote:
> On Wed, Feb 5, 2025 at 4:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> [v2]
> 
> I started looking just at 0001, and it seems like a fairly
> straightforward rearrangement.

Thanks for taking a look.

> I found this comment quite hard to read:
> 
> + * 'found_objs' should be a fully qualified list of objects to process, as
> + * returned by a previous call to vacuum_one_database().  If *found_objs is
> + * NULL, it is set to the results of the catalog query discussed below.  If
> + * found_objs is NULL, the results of the catalog query are not returned.
> + *
> + * If *found_objs is NULL, this function performs a catalog query to retrieve
> + * the list of tables to process.  When 'objects' is NULL, all tables in the
> 
> I had to read it several times before I noticed the difference between
> "* found_objs" and "*found_objs". Maybe some extra spacing and breaks
> would help, or other reorganization.

Yeah, it's pretty atrocious.  I think the main problem is that the
interface is just too complicated, so I'll take a step back and see if I
can make it more understandable to humans.  In the meantime, here's an
attempt at adjusting the comment:

 * found_objs is a double pointer to a fully qualified list of objects to
 * process, as returned by a previous call to vacuum_one_database().  If
 * *found_objs (the once-dereferenced double pointer) is NULL, it is set to the
 * results of the catalog query discussed below.  If found_objs (the double
 * pointer) is NULL, the results of the catalog query are not returned.
 *
 * If *found_objs (the once-dereferenced double pointer) is NULL, this function
 * performs a catalog query to retrieve the list of tables to process.  When
 * "objects" is NULL, all tables in the database are processed.  Otherwise, the
 * catalog query performs a lookup for the objects listed in "objects".

-- 
nathan

On Sat, Mar 1, 2025 at 3:42 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote:

> > I had to read it several times before I noticed the difference between
> > "* found_objs" and "*found_objs". Maybe some extra spacing and breaks
> > would help, or other reorganization.
>
> Yeah, it's pretty atrocious.  I think the main problem is that the
> interface is just too complicated, so I'll take a step back and see if I
> can make it more understandable to humans.

The interface is awkward, but on the other hand only a small part has
to really know about it. It's worth trying to make it more readable if
you can.

> In the meantime, here's an
> attempt at adjusting the comment:

That's better, and if we end up with this interface, we'll want quotes
around the names so the eye can tell where the "*" belong.

--
John Naylor
Amazon Web Services

On Mon, Mar 03, 2025 at 05:58:43PM +0700, John Naylor wrote:
> On Sat, Mar 1, 2025 at 3:42 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote:
>> > I had to read it several times before I noticed the difference between
>> > "* found_objs" and "*found_objs". Maybe some extra spacing and breaks
>> > would help, or other reorganization.
>>
>> Yeah, it's pretty atrocious.  I think the main problem is that the
>> interface is just too complicated, so I'll take a step back and see if I
>> can make it more understandable to humans.
> 
> The interface is awkward, but on the other hand only a small part has
> to really know about it. It's worth trying to make it more readable if
> you can.

True.  One small thing we could do is to require "found_objs" (the double
pointer) to always be non-NULL, but that just compels some callers to
provide otherwise-unused variables.  I've left the interface alone for now.

>> In the meantime, here's an attempt at adjusting the comment:
> 
> That's better, and if we end up with this interface, we'll want quotes
> around the names so the eye can tell where the "*" belong.

I did that in v3.  I also tried to break up this comment into bullet points
for better separation and logical flow.

-- 
nathan

On Mon, Mar 3, 2025 at 11:21 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Mar 03, 2025 at 05:58:43PM +0700, John Naylor wrote:
> True.  One small thing we could do is to require "found_objs" (the double
> pointer) to always be non-NULL, but that just compels some callers to
> provide otherwise-unused variables.  I've left the interface alone for now.

One thing to consider is that a pointer named "dummy" is self-documenting.

> > That's better, and if we end up with this interface, we'll want quotes
> > around the names so the eye can tell where the "*" belong.
>
> I did that in v3.  I also tried to break up this comment into bullet points
> for better separation and logical flow.

That's much easier to follow, thanks.

--
John Naylor
Amazon Web Services

On Tue, Mar 04, 2025 at 01:05:17PM +0700, John Naylor wrote:
> On Mon, Mar 3, 2025 at 11:21 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I did that in v3.  I also tried to break up this comment into bullet points
>> for better separation and logical flow.
> 
> That's much easier to follow, thanks.

Thanks for looking.  I'll probably commit 0001 sooner than later so that we
can focus our attention on 0002.

-- 
nathan

On Wed, Mar 5, 2025 at 12:13 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Tue, Mar 04, 2025 at 01:05:17PM +0700, John Naylor wrote:
> > On Mon, Mar 3, 2025 at 11:21 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >> I did that in v3.  I also tried to break up this comment into bullet points
> >> for better separation and logical flow.
> >
> > That's much easier to follow, thanks.
>
> Thanks for looking.  I'll probably commit 0001 sooner than later so that we
> can focus our attention on 0002.

+1

On 0002:

+        This option can only be used in conjunction with
+        <option>--analyze-only</option> and
<option>--analyze-in-stages</option>.

+ /* Prohibit --missing-only without --analyze-only or --analyze-in-stages */
+ if (vacopts.missing_only && !vacopts.analyze_only)
+ pg_fatal("cannot use the \"%s\" option without \"%s\" or \"%s\"",
+ "missing-only", "analyze-only", "analyze-in-stages");

The first is slightly ambiguous, so maybe "or" is better throughout.

+ " CROSS JOIN LATERAL (SELECT c.relkind IN ('p', 'I')) as p (inherited)\n"

Looking elsewhere in this file, I think we prefer something like
"(c.relkind OPERATOR(pg_catalog.=) ANY (array["
  CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
  CppAsString2(RELKIND_PARTITIONED_INDEX) "]) as p (inherited)\n"

+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
+ " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
+ " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
+ " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");

IIUC correctly, pg_statistic doesn't store stats on itself, so this
causes the query result to always contain pg_statistic -- does that
get removed elsewhere?

--
John Naylor
Amazon Web Services

On Thu, Mar 06, 2025 at 06:30:59PM +0700, John Naylor wrote:
> +        This option can only be used in conjunction with
> +        <option>--analyze-only</option> and
> <option>--analyze-in-stages</option>.
> 
> + /* Prohibit --missing-only without --analyze-only or --analyze-in-stages */
> + if (vacopts.missing_only && !vacopts.analyze_only)
> + pg_fatal("cannot use the \"%s\" option without \"%s\" or \"%s\"",
> + "missing-only", "analyze-only", "analyze-in-stages");
> 
> The first is slightly ambiguous, so maybe "or" is better throughout.

Agreed.

> + " CROSS JOIN LATERAL (SELECT c.relkind IN ('p', 'I')) as p (inherited)\n"
> 
> Looking elsewhere in this file, I think we prefer something like
> "(c.relkind OPERATOR(pg_catalog.=) ANY (array["
>   CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
>   CppAsString2(RELKIND_PARTITIONED_INDEX) "]) as p (inherited)\n"

Fixed.

> + " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
> + " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
> + " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
> + " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
> 
> IIUC correctly, pg_statistic doesn't store stats on itself, so this
> causes the query result to always contain pg_statistic -- does that
> get removed elsewhere?

Good catch.  I think the current behavior is to call ANALYZE on
pg_statistic, too, but that should be mostly harmless (analyze_rel()
refuses to process it).  I suppose we could try to avoid returning
pg_statistic from the catalog query, but we don't bother doing that for any
other vacuumdb modes, so I'm tempted to leave it alone.

-- 
nathan

On Fri, Mar 7, 2025 at 4:47 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Thu, Mar 06, 2025 at 06:30:59PM +0700, John Naylor wrote:

> > IIUC correctly, pg_statistic doesn't store stats on itself, so this
> > causes the query result to always contain pg_statistic -- does that
> > get removed elsewhere?
>
> Good catch.  I think the current behavior is to call ANALYZE on
> pg_statistic, too, but that should be mostly harmless (analyze_rel()
> refuses to process it).  I suppose we could try to avoid returning
> pg_statistic from the catalog query, but we don't bother doing that for any
> other vacuumdb modes, so I'm tempted to leave it alone.

Okay, thanks for confirming. I have no further comments.

--
John Naylor
Amazon Web Services

On Mon, Mar 10, 2025 at 12:35:22PM +0700, John Naylor wrote:
> I have no further comments.

Thanks.  I'll give this a little more time for review before committing.

We'll still need to update the recommendation in pg_upgrade's
documentation.  I'm going to keep that separate because the stats
import/export work is still settling.

-- 
nathan

On Mon, Mar 10, 2025 at 10:08:49AM -0500, Nathan Bossart wrote:
> On Mon, Mar 10, 2025 at 12:35:22PM +0700, John Naylor wrote:
>> I have no further comments.
> 
> Thanks.  I'll give this a little more time for review before committing.

I realized that we could limit the catalog query reuse to only when
--missing-only is specified, so I've updated 0001 and 0002 accordingly.
This avoids changing any existing behavior.

> We'll still need to update the recommendation in pg_upgrade's
> documentation.  I'm going to keep that separate because the stats
> import/export work is still settling.

0003 is a first attempt at this.  Unless I am missing something, there's
really not much to update.

-- 
nathan

On Wed, Mar 12, 2025 at 12:00 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> On Mon, Mar 10, 2025 at 10:08:49AM -0500, Nathan Bossart wrote:
> > On Mon, Mar 10, 2025 at 12:35:22PM +0700, John Naylor wrote:
> >> I have no further comments.
> >
> > Thanks.  I'll give this a little more time for review before committing.
>
> I realized that we could limit the catalog query reuse to only when
> --missing-only is specified, so I've updated 0001 and 0002 accordingly.
> This avoids changing any existing behavior.
>
> > We'll still need to update the recommendation in pg_upgrade's
> > documentation.  I'm going to keep that separate because the stats
> > import/export work is still settling.
>
> 0003 is a first attempt at this.  Unless I am missing something, there's
> really not much to update.

The change here seems fine. My only quibble is that this sentence now
seems out of place: "Option --analyze-in-stages can be used to
generate minimal statistics quickly." I'm thinking we should make a
clearer separation for with and without the  --no-statistics option
specified.


--
John Naylor
Amazon Web Services

On Wed, Mar 12, 2025 at 01:56:04PM +0700, John Naylor wrote:
> The change here seems fine. My only quibble is that this sentence now
> seems out of place: "Option --analyze-in-stages can be used to
> generate minimal statistics quickly." I'm thinking we should make a
> clearer separation for with and without the  --no-statistics option
> specified.

I think the recommendation stays the same regardless of whether
--no-statistics was used.  --missing-only should do the right think either
way.  But I do agree that this paragraph feels a bit haphazard.  I modified
it to call out the full command for both --analyze-only and
--analyze-in-stages, and I moved the note about --jobs to its own sentence
and made it clear that it is applicable to both of the suggested vacuumdb
commands.

-- 
nathan

Out of curiosity, I generated many relations with the following command
(stolen from [0]):

do $$
begin
for i in 1..100000 loop
  execute format('create table t%s (f1 int unique, f2 int unique);', i);
  execute format('insert into t%s select x, x from generate_series(1,1000) x',
                 i);
  if i % 100 = 0 then commit; end if;
end loop;
end
$$;

And then I ran a database-wide ANALYZE.  Without --missing-only, vacuumdb's
catalog query took 65 ms.  With --missing-only, it took 735 ms.  While
that's a big jump, this query will only run once for a given vacuumdb, and
--missing-only is likely to save a lot of time elsewhere.

If no feedback or objections materialize, I'm planning to commit these
early next week.

[0] https://postgr.es/m/3612876.1689443232%40sss.pgh.pa.us

-- 
nathan

On Fri, Mar 14, 2025 at 02:05:30PM -0500, Nathan Bossart wrote:
> If no feedback or objections materialize, I'm planning to commit these
> early next week.

While preparing this for commit, I noticed that the expression index part
of the query was disregarding attstattarget.  To fix, I've modified that
part to look at the index's pg_attribute entries.

-- 
nathan

Committed with the following small changes:

* I renamed the new option to --missing-stats-only, which I felt was more
  descriptive.

* I moved the new tests to the main vacuumdb test file and interspersed
  some --analyze-only uses.

* I fixed a couple of other things in the new parts of the catalog query
  that were not fully qualified.

-- 
nathan

On 3/18/25 22:37, Nathan Bossart wrote:
> Committed with the following small changes:

Hi, I don't really understand this sentence in 
doc/src/sgml/ref/vacuumdb.sgml:

 > This option prevents vacuumdb from deleting existing statistics so 
that the query optimizer's choices do not become transiently worse.

I thought that the point was to avoid unnecessary post-upgrade analyzes?

On Wed, Jul 23, 2025 at 02:45:10PM +0200, Frédéric Yhuel wrote:
> 
> 
> On 3/18/25 22:37, Nathan Bossart wrote:
> > Committed with the following small changes:
> 
> Hi, I don't really understand this sentence in
> doc/src/sgml/ref/vacuumdb.sgml:
> 
> > This option prevents vacuumdb from deleting existing statistics so that
> the query optimizer's choices do not become transiently worse.
> 
> I thought that the point was to avoid unnecessary post-upgrade analyzes?

So, the full paragraph is:

+        Only analyze relations that are missing statistics for a column, index
+        expression, or extended statistics object.  This option prevents
+        <application>vacuumdb</application> from deleting existing statistics
+        so that the query optimizer's choices do not become transiently worse.

What it is trying to say is that if you run vacuumedb without this
option, not only will it analyze all tables, including ones that already
have statistics, but will drop statistics on this tables that already
have statistics for a brief period while it installs new statistics. 
During that period, the optimizer will not have any statistics for the
table.  Is there a clearer way to state this?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Sat, Jul 26, 2025 at 07:47:47AM -0700, David G. Johnston wrote:
> On Sat, Jul 26, 2025, 07:23 Bruce Momjian <bruce@momjian.us> wrote:
> 
>     On Wed, Jul 23, 2025 at 02:45:10PM +0200, Frédéric Yhuel wrote:
>     >
>     >
>     > On 3/18/25 22:37, Nathan Bossart wrote:
>     > > Committed with the following small changes:
>     >
>     > Hi, I don't really understand this sentence in
>     > doc/src/sgml/ref/vacuumdb.sgml:
>     >
>     > > This option prevents vacuumdb from deleting existing statistics so that
>     > the query optimizer's choices do not become transiently worse.
>     >
>     > I thought that the point was to avoid unnecessary post-upgrade analyzes?
> 
>     So, the full paragraph is:
> 
>     +        Only analyze relations that are missing statistics for a column,
>     index
>     +        expression, or extended statistics object.  This option prevents
>     +        <application>vacuumdb</application> from deleting existing
>     statistics
>     +        so that the query optimizer's choices do not become transiently
>     worse.
> 
>     What it is trying to say is that if you run vacuumedb without this
>     option, not only will it analyze all tables, including ones that already
>     have statistics, but will drop statistics on this tables that already
>     have statistics for a brief period while it installs new statistics.
>     During that period, the optimizer will not have any statistics for the
>     table.  Is there a clearer way to state this?
> 
> Statistics are transactional.  Without this option specified are we really
> removing them and commiting prior to computing and saving new ones?  And we are
> opposed to just changing this behavior and instead prefer to add an option that
> at first glance seems like everyone should use?

Yes, I thought it was transactional too, but the doc patch suggests is
isn't, so maybe I am wrong.

> "If not specified the system will analyze all statistics-capable objects in
> alphabetical order.  Specifying this option then limits the result to only
> those objects that do not already have statistics.". That may not be how the
> feature strictly behaves but that would seem to be all one would expect it to
> do.

Yes, I would prefer the simpler text, if it is accurate.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On 7/26/25 21:38, Corey Huinker wrote:
>
>
> It is transactional, mostly.
>
> The attribute stats for the table being analyzed and all attribute stats
> for the dependent indexes that have at least one expression column, plus
> extended stats objects on that table, will be replaced in one atomic
> operation. The old stats were there, commit happens, now the new stats
> are there.
>
> The relation stats (pg_class) happen in-place, non-transactionally. So
> if you had an analyze that got canceled, and some of the relstats had
> been updated on the table or indexes, those would stay as-is.
>

Then it seems very unlikely that the query optimizer's choices become
transiently worse because of that, doesn't it? and this shouldn't be
used to justify this option, IMHO.

How about this? "This option is primarily intended to be used in
post-upgrade scripts to quickly generate minimal optimizer statistics"

> Having said all that, these are very nitpick details that may not need
> to be in our documentation.
>

+1

I've always supposed that the statistics were completely transactional,
I think most users do, and the difference with reality seems small enough.

>
>
>
> /me dons Hat of Pedantry (+2 against simple explanations)
>
> "Specifying this option then limits the result to only tables that are
> missing an attribute statistic, have an index that is missing an
> attribute statistic, or have an extended statistics object that is
> itself missing object statistics".
>
> /me removes hat
>
> "Specifying this option then limits the result to only those tables that
> do not already have complete statistics."
>
> The above phrase is asking the word "complete" to do a lot of heavy
> lifting, covering the dependent indexes and extended statistics objects.
> Specifying "table" makes it clear that any deficit in statistics results
> in the table getting analyzed, not just the single index or extended object.

FWIW I much prefer the last, simple phrase.

On Sun, Jul 27, 2025 at 12:46:44PM +0200, Frédéric Yhuel wrote:
> Then it seems very unlikely that the query optimizer's choices become
> transiently worse because of that, doesn't it? and this shouldn't be used to
> justify this option, IMHO.

I can't remember who wrote this line, but it was borrowed from the
--analyze-in-stages description.  The point is that if you use
--analyze-in-stages without --missing-stats-only, there will be a period
where existing statistics will be replaced with ones generated with lower
statistics targets.  Obviously, this wording isn't clear enough.  We might
need to either remove that sentence or add "When used in conjunction with
--analyze-in-stages..."

-- 
nathan

On 7/28/25 16:47, Nathan Bossart wrote:
> I can't remember who wrote this line, but it was borrowed from the
> --analyze-in-stages description.  The point is that if you use
> --analyze-in-stages without --missing-stats-only, there will be a period
> where existing statistics will be replaced with ones generated with lower
> statistics targets.

Aha, it makes sense now, thank you!

> Obviously, this wording isn't clear enough.  We might
> need to either remove that sentence or add "When used in conjunction with
> --analyze-in-stages..."

I vote for the second option.

On 7/28/25 16:47, Nathan Bossart wrote:

I can't remember who wrote this line, but it was borrowed from the
--analyze-in-stages description.  The point is that if you use
--analyze-in-stages without --missing-stats-only, there will be a period
where existing statistics will be replaced with ones generated with lower
statistics targets.

Aha, it makes sense now, thank you!

Obviously, this wording isn't clear enough.  We might
need to either remove that sentence or add "When used in conjunction with
--analyze-in-stages..."

I vote for the second option.

On Mon, Jul 28, 2025 at 09:22:29AM -0700, David G. Johnston wrote:
> Makes sense.  This does beg the question - what happens if a column is left
> with a lower statistics target than what would be applied during an
> analyze, but one is present?  I don´t see where the statistics target is
> saved anywhere.  Can we start recording that piece of data and teach
> analyze in stages to just never go backwards - reporting any it had to skip
> to adhere to that rule.  Seems like a better policy than missing-only.

That would involve changing the behavior of an existing option, which has
thus far been strongly opposed [0], for what strikes me as a reasonably
uncommon situation.  Plus, --missing-stats-only may be useful in other
situations.

[0] https://postgr.es/m/Z6KKHX9PZkB19lAK%40nathan

-- 
nathan

On Mon, Jul 28, 2025 at 09:22:29AM -0700, David G. Johnston wrote:
> On Monday, July 28, 2025, Frédéric Yhuel <frederic.yhuel@dalibo.com> wrote:
>> On 7/28/25 16:47, Nathan Bossart wrote:
>>> Obviously, this wording isn't clear enough.  We might need to either
>>> remove that sentence or add "When used in conjunction with
>>> --analyze-in-stages..."
>>
>> I vote for the second option.
>
> Makes sense. 

It seems like there is some support for adding "When used in conjunction
with --analyze in stages..." to the beginning of the sentence.  I'll give
it another day or two for any further discussion before committing.

-- 
nathan

On Mon, Jul 28, 2025 at 01:46:34PM -0500, Nathan Bossart wrote:
> On Mon, Jul 28, 2025 at 09:22:29AM -0700, David G. Johnston wrote:
>> On Monday, July 28, 2025, Frédéric Yhuel <frederic.yhuel@dalibo.com> wrote:
>>> On 7/28/25 16:47, Nathan Bossart wrote:
>>>> Obviously, this wording isn't clear enough.  We might need to either
>>>> remove that sentence or add "When used in conjunction with
>>>> --analyze-in-stages..."
>>>
>>> I vote for the second option.
>>
>> Makes sense. 
> 
> It seems like there is some support for adding "When used in conjunction
> with --analyze in stages..." to the beginning of the sentence.  I'll give
> it another day or two for any further discussion before committing.

Here is what I have staged for commit.

-- 
nathan

On Wed, 2025-07-30 at 12:21 -0500, Nathan Bossart wrote:
> Here is what I have staged for commit.

That's more clear to me. I also like that it shows that the options
work well together, because that was not obvious before.

Regards,
    Jeff Davis

Committed.

-- 
nathan