From: Tom Lane <tgl@sss.pgh.pa.us>
To: Rian McGuire <rian.mcguire@buildkite.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] pg_dump: Do not dump statistics for excluded tables
In-reply-to: <7075d3aa-3f05-44a5-b68f-47dc6a8a0550@buildkite.com>
References: <7075d3aa-3f05-44a5-b68f-47dc6a8a0550@buildkite.com>
Comments: In-reply-to Rian McGuire <rian.mcguire@buildkite.com>
	message dated "Wed, 27 Dec 2023 15:44:09 +1100"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2588574.1703696429.0@sss.pgh.pa.us>
Date: Wed, 27 Dec 2023 12:01:50 -0500
Message-ID: <2588766.1703696510@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2588766.1703696510%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2588574.1703696429.1@sss.pgh.pa.us>

Rian McGuire <rian.mcguire@buildkite.com> writes:
> I've attached a patch against master that addresses a small bug in pg_dump.
> Previously, pg_dump would include CREATE STATISTICS statements for
> tables that were excluded from the dump, causing reload to fail if any
> excluded tables had extended statistics.

I agree that's a bug ...

> The patch skips the creation of the StatsExtInfo if the associated
> table does not have the DUMP_COMPONENT_DEFINITION flag set. This is
> similar to how getPublicationTables behaves if a table is excluded.

... but I don't like the details of this patch (and I'm not too
thrilled with the implementation of getPublicationTables, either).
The style in pg_dump is to put such decisions into separate
policy-setting subroutines.  Also, skipping creation of the
DumpableObject altogether is the wrong thing because it'd prevent
pg_dump from tracing or reasoning about dependencies involving the
stats object, which can be relevant even if the object itself isn't
dumped --- this is why all the other data-collection subroutines
operate as they do.  getPublicationTables can probably get away
with its low-rent approach given that publication membership isn't
represented by pg_depend entries, but it's far from clear that it'll
never be an issue for stats.

So I think it needs to be more like the attached.
(I did use your test case verbatim.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name*0="v2-0001-pg_dump-Do-not-dump-statistics-for-excluded-tables.";
	name*1="patch"; charset="us-ascii"
Content-ID: <2588574.1703696429.2@sss.pgh.pa.us>
Content-Description: 
 v2-0001-pg_dump-Do-not-dump-statistics-for-excluded-tables.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8c0b5486b9..050a831226 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2046,6 +2046,26 @@ selectDumpablePublicationObject(DumpableObject *dob=
j, Archive *fout)
 		DUMP_COMPONENT_ALL : DUMP_COMPONENT_NONE;
 }
 =

+/*
+ * selectDumpableStatisticsObject: policy-setting subroutine
+ *		Mark an extended statistics object as to be dumped or not
+ *
+ * We dump an extended statistics object if the schema it's in and the ta=
ble
+ * it's for are being dumped.  (This'll need more thought if statistics
+ * objects ever support cross-table stats.)
+ */
+static void
+selectDumpableStatisticsObject(StatsExtInfo *sobj, Archive *fout)
+{
+	if (checkExtensionMembership(&sobj->dobj, fout))
+		return;					/* extension membership overrides all else */
+
+	sobj->dobj.dump =3D sobj->dobj.namespace->dobj.dump_contains;
+	if (sobj->stattable =3D=3D NULL ||
+		!(sobj->stattable->dobj.dump & DUMP_COMPONENT_DEFINITION))
+		sobj->dobj.dump =3D DUMP_COMPONENT_NONE;
+}
+
 /*
  * selectDumpableObject: policy-setting subroutine
  *		Mark a generic dumpable object as to be dumped or not
@@ -7291,6 +7311,7 @@ getExtendedStatistics(Archive *fout)
 	int			i_stxname;
 	int			i_stxnamespace;
 	int			i_stxowner;
+	int			i_stxrelid;
 	int			i_stattarget;
 	int			i;
 =

@@ -7302,11 +7323,11 @@ getExtendedStatistics(Archive *fout)
 =

 	if (fout->remoteVersion < 130000)
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
-							 "stxnamespace, stxowner, (-1) AS stxstattarget "
+							 "stxnamespace, stxowner, stxrelid, (-1) AS stxstattarget "
 							 "FROM pg_catalog.pg_statistic_ext");
 	else
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
-							 "stxnamespace, stxowner, stxstattarget "
+							 "stxnamespace, stxowner, stxrelid, stxstattarget "
 							 "FROM pg_catalog.pg_statistic_ext");
 =

 	res =3D ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -7318,6 +7339,7 @@ getExtendedStatistics(Archive *fout)
 	i_stxname =3D PQfnumber(res, "stxname");
 	i_stxnamespace =3D PQfnumber(res, "stxnamespace");
 	i_stxowner =3D PQfnumber(res, "stxowner");
+	i_stxrelid =3D PQfnumber(res, "stxrelid");
 	i_stattarget =3D PQfnumber(res, "stxstattarget");
 =

 	statsextinfo =3D (StatsExtInfo *) pg_malloc(ntups * sizeof(StatsExtInfo)=
);
@@ -7332,10 +7354,12 @@ getExtendedStatistics(Archive *fout)
 		statsextinfo[i].dobj.namespace =3D
 			findNamespace(atooid(PQgetvalue(res, i, i_stxnamespace)));
 		statsextinfo[i].rolname =3D getRoleName(PQgetvalue(res, i, i_stxowner))=
;
+		statsextinfo[i].stattable =3D
+			findTableByOid(atooid(PQgetvalue(res, i, i_stxrelid)));
 		statsextinfo[i].stattarget =3D atoi(PQgetvalue(res, i, i_stattarget));
 =

 		/* Decide whether we want to dump it */
-		selectDumpableObject(&(statsextinfo[i].dobj), fout);
+		selectDumpableStatisticsObject(&(statsextinfo[i]), fout);
 	}
 =

 	PQclear(res);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 2fe3cbed9a..673ca5c92d 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -423,7 +423,8 @@ typedef struct _indexAttachInfo
 typedef struct _statsExtInfo
 {
 	DumpableObject dobj;
-	const char *rolname;
+	const char *rolname;		/* owner */
+	TableInfo  *stattable;		/* link to table the stats are for */
 	int			stattarget;		/* statistics target */
 } StatsExtInfo;
 =

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_d=
ump.pl
index eb3ec534b4..a671603cd2 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3742,14 +3742,15 @@ my %tests =3D (
 	'CREATE STATISTICS extended_stats_no_options' =3D> {
 		create_order =3D> 97,
 		create_sql =3D> 'CREATE STATISTICS dump_test.test_ext_stats_no_options
-							ON col1, col2 FROM dump_test.test_fifth_table',
+							ON col1, col2 FROM dump_test.test_table',
 		regexp =3D> qr/^
-			\QCREATE STATISTICS dump_test.test_ext_stats_no_options ON col1, col2 =
FROM dump_test.test_fifth_table;\E
+			\QCREATE STATISTICS dump_test.test_ext_stats_no_options ON col1, col2 =
FROM dump_test.test_table;\E
 		    /xms,
 		like =3D>
 		  { %full_runs, %dump_test_schema_runs, section_post_data =3D> 1, },
 		unlike =3D> {
 			exclude_dump_test_schema =3D> 1,
+			exclude_test_table =3D> 1,
 			only_dump_measurement =3D> 1,
 		},
 	},

------- =_aaaaaaaaaa0--