Subject: Re: Statistics Import and Export

From: Corey Huinker <[email-masked]>
Date: Fri, 4 Apr 2025 19:32:48 -0400

On Fri, Apr 4, 2025 at 6:25 PM Nathan Bossart <[email-masked]>
wrote:

> On Fri, Apr 04, 2025 at 03:58:53PM -0500, Nathan Bossart wrote:
> > I pushed commit 8ec0aae to fix this.
>
> And now I'm seeing cross-version test failures due to our use of WITH
> ORDINALITY, which wasn't added until v9.4.  Looking into it...
>
>
>
This patch shrinks the array size to 1 for versions < 9.4, which keeps the
modern code fairly elegant.
From fe551ab55622f95d84ac4c4d79fba898c6b60057 Mon Sep 17 00:00:00 2001
From: Corey Huinker <[email-masked]>
Date: Fri, 4 Apr 2025 19:30:00 -0400
Subject: [PATCH v1] Fall back to single attribute stat fetching for versions <
 9.4

Existing attribute statistics batch fetching query relies on the
existence of WITH ORDINALTIY, as well as multi-parameter unnest() calls.
Without those, we have no choice but to fall back to single relation
fetching.

Preserve the existing array building infrastructure, and drop the array
size to 1 for older versions.
---
 src/bin/pg_dump/pg_dump.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 0e915432e77..b88188448b0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -10571,6 +10571,14 @@ fetchAttributeStats(Archive *fout)
 	PGresult   *res = NULL;
 	static TocEntry *te;
 	static bool restarted;
+	int			max_rels = MAX_ATTR_STATS_RELS;
+
+	/*
+	 * Versions prior to 9.4 lack the unnest() WITH ORDINALITY feature
+	 * that we need to keep the relation batches in order.
+	 */
+	if (fout->remoteVersion < 90400)
+		max_rels = 1;
 
 	/* If we're just starting, set our TOC pointer. */
 	if (!te)
@@ -10596,7 +10604,7 @@ fetchAttributeStats(Archive *fout)
 	 * This is perhaps not the sturdiest assumption, so we verify it matches
 	 * reality in dumpRelationStats_dumper().
 	 */
-	for (; te != AH->toc && count < MAX_ATTR_STATS_RELS; te = te->next)
+	for (; te != AH->toc && count < max_rels; te = te->next)
 	{
 		if ((te->reqs & REQ_STATS) != 0 &&
 			strcmp(te->desc, "STATISTICS DATA") == 0)
@@ -10709,14 +10717,25 @@ dumpRelationStats_dumper(Archive *fout, const void *userArg, const TocEntry *te)
 		 * sufficient to convince the planner to use
 		 * pg_class_relname_nsp_index, which avoids a full scan of pg_stats.
 		 * This may not work for all versions.
+		 *
+		 * WITH ORDINALITY was introduced in 9.4, and multi-argument unnest()
+		 * was introduced in 9.3. Rather than create a bunch of corner-cases,
+		 * we simply fall back to fetching a single relation per call.
 		 */
-		appendPQExpBufferStr(query,
-							 "FROM pg_catalog.pg_stats s "
-							 "JOIN unnest($1, $2) WITH ORDINALITY AS u (schemaname, tablename, ord) "
-							 "ON s.schemaname = u.schemaname "
-							 "AND s.tablename = u.tablename "
-							 "WHERE s.tablename = ANY($2) "
-							 "ORDER BY u.ord, s.attname, s.inherited");
+		if (fout->remoteVersion >= 90400)
+			appendPQExpBufferStr(query,
+								 "FROM pg_catalog.pg_stats s "
+								 "JOIN unnest($1, $2) WITH ORDINALITY AS u (schemaname, tablename, ord) "
+								 "ON s.schemaname = u.schemaname "
+								 "AND s.tablename = u.tablename "
+								 "WHERE s.tablename = ANY($2) "
+								 "ORDER BY u.ord, s.attname, s.inherited");
+		else
+			appendPQExpBufferStr(query,
+								 "FROM pg_catalog.pg_stats s "
+								 "WHERE s.schemaname = ($1::text[])[1] "
+								 "AND s.tablename = ($2::text[])[1] "
+								 "ORDER BY s.attname, s.inherited");
 
 		ExecuteSqlStatement(fout, query->data);

base-commit: 0f43083d16f4be7c01efa80d05d0eef5e5ff69d3
-- 
2.49.0

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 4 Apr 2025 21:06:38 -0500

> modern code fairly elegant.

Committed.

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Tue, 13 May 2025 17:01:02 -0700

We found a minor issue when testing statistics import with upgrading from
versions older than v14. (We have VACUUM and ANALYZE disabled)
3d351d916b20534f973eda760cde17d96545d4c4
<https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d351d916b20534f973eda760cde17d96545d4c4>
changed
the default value for reltuples from 0 to -1. So when such tables are
imported they get the pg13 default of 0 which in pg18 is treated
as "vacuumed and seen to be empty" instead of "never yet vacuumed". The
planner then proceeds to pick seq scans even if there are indexes for these
tables.
This is a very narrow edge case and the next VACUUM or ANALYZE will fix it
but the perf of these tables immediately after the upgrade is considerably
affected.

Can we instead use -1 if the version is older than 14, and reltuples is 0?
This will have the unintended consequence of treating a truly empty table
as "never yet vacuumed", but that should be fine as empty tables are going
to be fast regardless of the plan picked.

PS: This is my first patch, so apologies for any issues with the patch.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 14 May 2025 11:53:08 -0400

> but the perf of these tables immediately after the upgrade is considerably

There was a similar report for vacuumdb's new --missing-stats-only option.
We fixed that in commit 9879105 by removing the check for reltuples != 0,
which means that --missing-stats-only will process empty tables.

> to be fast regardless of the plan picked.

I'm inclined to agree that we should do this.  Even if it's much more
likely that 0 means empty versus not-yet-processed, the one-time cost of
processing some empty tables doesn't sound too bad.  In any case, since
this only applies to upgrades from <v14, that trade-off should dissipate
over time.

> PS: This is my first patch, so apologies for any issues with the patch.

It needs a comment, but otherwise it looks generally reasonable to me after
a quick glance.

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Wed, 14 May 2025 13:30:48 -0700

Thanks Nathan.
Here is the patch with a comment.

On Wed, May 14, 2025 at 8:53 AM Nathan Bossart <[email-masked]>
wrote:

> > <
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d351d916b20534f973eda760cde17d96545d4c4
> >
> > changed

---

From: Hari Krishna Sunder <[email-masked]>
Date: Fri, 16 May 2025 11:47:12 -0700

Gentle ping on this.

---
Hari Krishna Sunder

---

From: Michael Paquier <[email-masked]>
Date: Mon, 19 May 2025 10:52:15 +0900

> Gentle ping on this.

Most of the major PostgreSQL developers were at pgconf.dev held in
Montreal last week, explaining a reduction in the activity of the
mailing lists.

Your initial report was on Monday the 14th, with this ping being on
Friday the 16th, both happening during the conference.  I suspect that
that there was just no time for folks of this thread to be able to
provide feedback for your patch, so please be a bit more patient.

Thanks!
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 19 May 2025 11:51:32 -0500

> Here is the patch with a comment.

Thanks.

>> which means that --missing-stats-only will process empty tables.

I'm wondering if we should revert commit 9879105 if we take this change,
which solves the --missing-stats-only problem in a different way.  My
current thinking is that we should just leave it in place, if for no other
reason than analyzing some empty tables seems unlikely to cause too much
trouble.  Thoughts?

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Mon, 19 May 2025 14:13:45 -0700

Sorry didn't know about the conference.

I think it would be better to revert 9879105 since there can be a
considerable number of true empty tables that we don’t need to process.

---
Hari Krishna Sunder

On Mon, May 19, 2025 at 9:51 AM Nathan Bossart <[email-masked]>
wrote:

> > On Wed, May 14, 2025 at 8:53 AM Nathan Bossart <[email-masked]
> >
> > wrote:

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 19 May 2025 16:31:57 -0500

On Mon, May 19, 2025 at 02:13:45PM -0700, Hari Krishna Sunder wrote:
> considerable number of true empty tables that we don´t need to process.

I'm not sure that's a use-case we really need to optimize.  Even with
100,000 empty tables, "vacuumdb --analyze-only --missing-stats-only --jobs
64" completes in ~5.5 seconds on my laptop.  Plus, even if reltuples is 0,
there might actually be rows in the table, in which case analyzing it will
produce rows in pg_statistic.

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Tue, 20 May 2025 10:32:39 -0700

Ah ya, forgot that reltuples are not always accurate. This sounds
reasonable to me.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 21 May 2025 11:08:23 -0500

> Ah ya, forgot that reltuples are not always accurate. This sounds

Cool.  Here is what I have staged for commit, which I am planning to do
shortly.

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Wed, 21 May 2025 12:33:03 -0700

Looks good to me.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 21 May 2025 14:14:55 -0700

> Cool.  Here is what I have staged for commit, which I am planning to

Originally, one of the reasons we added a version field during dump is
so that some future version could reinterpret stats in older dump files
during import.

This patch is using a newer version of pg_dump to interpret stats from
older versions during export. That might be fine, but it would be good
to understand where the line is between things we should reinterpret
during export vs things we should reinterpret during import.

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 21 May 2025 16:29:56 -0500

> during export vs things we should reinterpret during import.

I don't know precisely where that line might be, but in this case, the
dumped stats have no hope of restoring into anything older than v18 (since
the stats import functions won't exist), which is well past the point where
we started using -1 for reltuples.  If we could dump the stats from v13 and
restore them into v13, then I think there would be a reasonably strong
argument for dumping it as-is and reinterpreting as necessary during
import.  But I see no particular benefit from moving the complexity to the
import side here.

Does that seem like a reasonable position to you?  Is there anything else
we should consider?

-- 
nathan

---

From: Corey Huinker <[email-masked]>
Date: Wed, 21 May 2025 19:11:09 -0400

> import.  But I see no particular benefit from moving the complexity to the

Definitely keep complexity on the export-side.

Mapping reltuples 0 -> -1 if system version < 14 like the original patch
did makes the most sense to me. That allows vacuumdb to go back to ignoring
tables that are seemingly empty while still vacuuming the tables that had
the pre-14 suspicious 0 reltuples value.

> Does that seem like a reasonable position to you?  Is there anything else

Automatically vacuuming tables that purport to be empty may not take much
time, but it may alarm users using --missing-only, wondering why so many
tables didn't get stats imported, especially if we introduce a --dry-run
parameter which would answer for a user the question "what tables does
vacuumdb think are missing statistics?".

---

From: Jeff Davis <[email-masked]>
Date: Wed, 21 May 2025 16:53:17 -0700

On Wed, 2025-05-21 at 16:29 -0500, Nathan Bossart wrote:
> dumped stats have no hope of restoring into anything older than
> v18... But I see no particular benefit from moving the complexity
> to the
> import side here.

That's fine with me. Perhaps we should just say that pre-18 behavior
differences can be fixed up during export, and post-18 behavior
differences are fixed up during import?

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 22 May 2025 10:25:03 -0500

> differences are fixed up during import?

WFM.  I've committed the patch.

-- 
nathan

---

From: Hari Krishna Sunder <[email-masked]>
Date: Thu, 22 May 2025 12:25:29 -0700

Thanks for the help. This has unblocked us!