From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
Cc: cam.daniel@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16655: pg_dump segfault when excluding postgis table
Date: Tue, 06 Oct 2020 15:06:58 -0400
Message-ID: <1995213.1602011218@sss.pgh.pa.us> (raw)
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>
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
Attachments:download all (3 files, .tar.gz)0001-postpone-setting-ncheck.patch (text/x-diff, 2.9K) downloaddiff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 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 = NULL;
else
tblinfo[i].reloftype = pg_strdup(PQgetvalue(res, i, i_reloftype));
- tblinfo[i].ncheck = atoi(PQgetvalue(res, i, i_relchecks));+ tblinfo[i].relchecks = atoi(PQgetvalue(res, i, i_relchecks));
if (PQgetisnull(res, i, i_owning_tab))
{
tblinfo[i].owning_tab = 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, int numTables)
res = ExecuteSqlQuery(fout, q->data, PGRES_TUPLES_OK);
numConstrs = PQntuples(res);
- if (numConstrs != tbinfo->ncheck)+ if (numConstrs != tbinfo->relchecks)
{
pg_log_error(ngettext("expected %d check constraint on table \"%s\" but 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 = (ConstraintInfo *) pg_malloc(numConstrs * sizeof(ConstraintInfo));
+ tbinfo->ncheck = numConstrs;
tbinfo->checkexprs = constrs;
for (int j = 0; j < numConstrs; j++)
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.hindex 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 */
0002-regression-test-case.patch (text/x-diff, 2.5K) downloaddiff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.plindex 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 = (
"$tempdir/defaults_tar_format.tar",
],
},
+ exclude_table => {+ dump_cmd => [+ 'pg_dump', '--exclude-table=regress_table_dumpable',+ '--inserts', "--file=$tempdir/exclude_table.sql",+ 'postgres',+ ],+ },
extension_schema => {
dump_cmd => [
'pg_dump', '--schema=public', '--inserts',
@@ -225,6 +232,7 @@ my %full_runs = (
clean_if_exists => 1,
createdb => 1,
defaults => 1,
+ exclude_table => 1,
no_privs => 1,
no_owner => 1,);
@@ -317,7 +325,8 @@ my %tests = (
regexp => 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 => { binary_upgrade => 1, },
},
@@ -443,7 +452,8 @@ my %tests = (
regexp => 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 => { binary_upgrade => 1, },
},
@@ -589,6 +599,9 @@ my %tests = (
like => {
extension_schema => 1,
},
+ unlike => {+ exclude_table => 1,+ },
},);
#########################################
diff --git 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.sqlindex 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_role;
=
0003-fix-interesting.patch (text/x-diff, 656B) downloaddiff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 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, ExtensionInfo extinfo[],
if (dumpobj)
{
+ configtbl->interesting = true;
makeTableDataInfo(dopt, configtbl);
if (configtbl->dataObj != NULL)
{
@@ -17912,8 +17914,6 @@ processExtensionTables(Archive *fout, ExtensionInfo extinfo[],
configtbl->dataObj->filtercond = pg_strdup(extconditionarray[j]);
}
}
-- configtbl->interesting = dumpobj;
}
}
if (extconfigarray)
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: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, sfrost@snowman.net, cam.daniel@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16655: pg_dump segfault when excluding postgis table
In-Reply-To: <1995213.1602011218@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox