From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
cc: cam.daniel@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16655: pg_dump segfault when excluding postgis table
In-reply-to: <1909344.1602003707@sss.pgh.pa.us>
References: <16655-5c92d6b3a9438137@postgresql.org>
 <1819571.1601937068@sss.pgh.pa.us>
 <20201006163318.GX3063@tamriel.snowman.net>
 <1909344.1602003707@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 06 Oct 2020 13:01:47 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1995163.1602011154.0@sss.pgh.pa.us>
Date: Tue, 06 Oct 2020 15:06:58 -0400
Message-ID: <1995213.1602011218@sss.pgh.pa.us>
Precedence: bulk

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

I wrote:
> So I think the basic problem here is that checkExtensionMembership and
> processExtensionTables are not on the same page.  We can't have
> interesting = false for a table that any of the dobj.dump bits are set
> for.

After studying this further, I've concluded that there are two independent
issues here.  I was wrong to imagine that we can't get to dumpTableSchema
for tables that we're not going to dump; the actual intent of the code is
that it's going to run through that code anyway but not emit the
ArchiveEntry unless the DUMP_COMPONENT_DEFINITION flag is set.  (This
supports cases where we only want to dump comments, for example.)  So
dumpTableSchema really needs to cope with cases where getTableAttrs
has not loaded constraint info.  There are a lot of other loops in the
code that expect that checkexprs[] is populated when ncheck > 0, too.
Maybe none of them are reachable for a not-interesting table, but
I don't think that's the way to bet.  I think the right answer is to
redefine ncheck as being zero if we haven't loaded checkexprs[], as
per 0001 below.

0001 is sufficient to get past the proposed test case in 0002.  However,
I think processExtensionTables is also buggy here.  It does need to set
the "interesting" flag to true if it decides we need to dump the table's
data, because we have to have the per-attribute data to support that,
and "interesting" might not ever get set true on the table otherwise.
But *it is not OK to set "interesting" to false just because we don't
decide to dump the data*.  If it's been set true for some other reason,
that other reason didn't just vanish.  So I think we also need 0003.

BTW, the reason we can't get rid of the "interesting" flag is that
we need to set it on parent tables of tables we need to dump,
even if we're not going to dump the parents themselves (hence their
dobj.dump is zero).  Otherwise we won't collect the subsidiary data
we need to figure out which parts of the child's definition are
inherited.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0001-postpone-setting-ncheck.patch";
	charset="us-ascii"
Content-ID: <1995163.1602011154.2@sss.pgh.pa.us>
Content-Description: 0001-postpone-setting-ncheck.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 76320468ba..445057a00e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6772,7 +6772,7 @@ getTables(Archive *fout, int *numTables)
 			tblinfo[i].reloftype =3D NULL;
 		else
 			tblinfo[i].reloftype =3D pg_strdup(PQgetvalue(res, i, i_reloftype));
-		tblinfo[i].ncheck =3D atoi(PQgetvalue(res, i, i_relchecks));
+		tblinfo[i].relchecks =3D atoi(PQgetvalue(res, i, i_relchecks));
 		if (PQgetisnull(res, i, i_owning_tab))
 		{
 			tblinfo[i].owning_tab =3D InvalidOid;
@@ -8728,7 +8728,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int=
 numTables)
 		 * Get info about table CHECK constraints.  This is skipped for a
 		 * data-only dump, as it is only needed for table schemas.
 		 */
-		if (!dopt->dataOnly && tbinfo->ncheck > 0)
+		if (!dopt->dataOnly && tbinfo->relchecks > 0)
 		{
 			ConstraintInfo *constrs;
 			int			numConstrs;
@@ -8780,17 +8780,18 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, i=
nt numTables)
 			res =3D ExecuteSqlQuery(fout, q->data, PGRES_TUPLES_OK);
 =

 			numConstrs =3D PQntuples(res);
-			if (numConstrs !=3D tbinfo->ncheck)
+			if (numConstrs !=3D tbinfo->relchecks)
 			{
 				pg_log_error(ngettext("expected %d check constraint on table \"%s\" b=
ut found %d",
 									  "expected %d check constraints on table \"%s\" but found %d",
-									  tbinfo->ncheck),
-							 tbinfo->ncheck, tbinfo->dobj.name, numConstrs);
+									  tbinfo->relchecks),
+							 tbinfo->relchecks, tbinfo->dobj.name, numConstrs);
 				pg_log_error("(The system catalogs might be corrupted.)");
 				exit_nicely(1);
 			}
 =

 			constrs =3D (ConstraintInfo *) pg_malloc(numConstrs * sizeof(Constrain=
tInfo));
+			tbinfo->ncheck =3D numConstrs;
 			tbinfo->checkexprs =3D constrs;
 =

 			for (int j =3D 0; j < numConstrs; j++)
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index e0b42e8391..192388aacf 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -281,7 +281,7 @@ typedef struct _tableInfo
 	Oid			toast_oid;		/* toast table's OID, or 0 if none */
 	uint32		toast_frozenxid;	/* toast table's relfrozenxid, if any */
 	uint32		toast_minmxid;	/* toast table's relminmxid */
-	int			ncheck;			/* # of CHECK expressions */
+	int			relchecks;		/* # of CHECK expressions per pg_class */
 	char	   *reloftype;		/* underlying type for typed table */
 	Oid			foreign_server; /* foreign server oid, if applicable */
 	/* these two are set only if table is a sequence owned by a column: */
@@ -319,6 +319,7 @@ typedef struct _tableInfo
 	bool	   *notnull;		/* NOT NULL constraints on attributes */
 	bool	   *inhNotNull;		/* true if NOT NULL is inherited */
 	struct _attrDefInfo **attrdefs; /* DEFAULT expressions */
+	int			ncheck;			/* # of CHECK constraints in checkexprs[] */
 	struct _constraintInfo *checkexprs; /* CHECK constraints */
 	char	   *partkeydef;		/* partition key definition */
 	char	   *partbound;		/* partition bound definition */

------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0002-regression-test-case.patch";
	charset="us-ascii"
Content-ID: <1995163.1602011154.3@sss.pgh.pa.us>
Content-Description: 0002-regression-test-case.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/module=
s/test_pg_dump/t/001_base.pl
index 78aa07ce51..714ce2368c 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -135,6 +135,13 @@ my %pgdump_runs =3D (
 			"$tempdir/defaults_tar_format.tar",
 		],
 	},
+	exclude_table =3D> {
+		dump_cmd =3D> [
+			'pg_dump',   '--exclude-table=3Dregress_table_dumpable',
+			'--inserts', "--file=3D$tempdir/exclude_table.sql",
+			'postgres',
+		],
+	},
 	extension_schema =3D> {
 		dump_cmd =3D> [
 			'pg_dump', '--schema=3Dpublic', '--inserts',
@@ -225,6 +232,7 @@ my %full_runs =3D (
 	clean_if_exists =3D> 1,
 	createdb        =3D> 1,
 	defaults        =3D> 1,
+	exclude_table   =3D> 1,
 	no_privs        =3D> 1,
 	no_owner        =3D> 1,);
 =

@@ -317,7 +325,8 @@ my %tests =3D (
 		regexp =3D> qr/^
 			\QCREATE TABLE public.regress_pg_dump_table (\E
 			\n\s+\Qcol1 integer NOT NULL,\E
-			\n\s+\Qcol2 integer\E
+			\n\s+\Qcol2 integer,\E
+			\n\s+\QCONSTRAINT regress_pg_dump_table_col2_check CHECK ((col2 > 0))\=
E
 			\n\);\n/xm,
 		like =3D> { binary_upgrade =3D> 1, },
 	},
@@ -443,7 +452,8 @@ my %tests =3D (
 		regexp =3D> qr/^
 			\QCREATE TABLE regress_pg_dump_schema.test_table (\E
 			\n\s+\Qcol1 integer,\E
-			\n\s+\Qcol2 integer\E
+			\n\s+\Qcol2 integer,\E
+			\n\s+\QCONSTRAINT test_table_col2_check CHECK ((col2 > 0))\E
 			\n\);\n/xm,
 		like =3D> { binary_upgrade =3D> 1, },
 	},
@@ -589,6 +599,9 @@ my %tests =3D (
 		like =3D> {
 			extension_schema =3D> 1,
 		},
+		unlike =3D> {
+			exclude_table =3D> 1,
+		},
 	},);
 =

 #########################################
diff --git a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql b/src/tes=
t/modules/test_pg_dump/test_pg_dump--1.0.sql
index 90e461ed35..c7a35c3afa 100644
--- a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
+++ b/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
@@ -5,7 +5,7 @@
 =

 CREATE TABLE regress_pg_dump_table (
 	col1 serial,
-	col2 int
+	col2 int check (col2 > 0)
 );
 =

 CREATE SEQUENCE regress_pg_dump_seq;
@@ -14,7 +14,7 @@ CREATE SEQUENCE regress_seq_dumpable;
 SELECT pg_catalog.pg_extension_config_dump('regress_seq_dumpable', '');
 =

 CREATE TABLE regress_table_dumpable (
-	col1 int
+	col1 int check (col1 > 0)
 );
 SELECT pg_catalog.pg_extension_config_dump('regress_table_dumpable', '');
 =

@@ -34,7 +34,7 @@ CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER =
bthandler;
 -- this extension.
 CREATE TABLE regress_pg_dump_schema.test_table (
 	col1 int,
-	col2 int
+	col2 int check (col2 > 0)
 );
 GRANT SELECT ON regress_pg_dump_schema.test_table TO regress_dump_test_ro=
le;
 =


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0003-fix-interesting.patch";
	charset="us-ascii"
Content-ID: <1995163.1602011154.4@sss.pgh.pa.us>
Content-Description: 0003-fix-interesting.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 76320468ba..e3d61eafd1 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -17905,6 +17906,7 @@ processExtensionTables(Archive *fout, ExtensionInf=
o extinfo[],
 =

 				if (dumpobj)
 				{
+					configtbl->interesting =3D true;
 					makeTableDataInfo(dopt, configtbl);
 					if (configtbl->dataObj !=3D NULL)
 					{
@@ -17912,8 +17914,6 @@ processExtensionTables(Archive *fout, ExtensionInf=
o extinfo[],
 							configtbl->dataObj->filtercond =3D pg_strdup(extconditionarray[j])=
;
 					}
 				}
-
-				configtbl->interesting =3D dumpobj;
 			}
 		}
 		if (extconfigarray)

------- =_aaaaaaaaaa0--