Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit d8c2a21

Browse files
committed
Rethink recent fix for pg_dump's handling of extension config tables.
Commit 3eb3d3e was a few bricks shy of a load: while it correctly set the table's "interesting" flag when deciding to dump the data of an extension config table, it was not correct to clear that flag if we concluded we shouldn't dump the data. This led to the crash reported in bug #16655, because in fact we'll traverse dumpTableSchema anyway for all extension tables (to see if they have user-added seclabels or RLS policies). The right thing to do is to force "interesting" true in makeTableDataInfo, and otherwise leave the flag alone. (Doing it there is more future-proof in case additional calls are added, and it also avoids setting the flag unnecessarily if that function decides the table is non-dumpable.) This investigation also showed that while only the --inserts code path had an obvious failure in the case considered by 3eb3d3e, the COPY code path also has a problem with not having loaded table subsidiary data. That causes fmtCopyColumnList to silently return an empty string instead of the correct column list. That accidentally mostly works, which perhaps is why we didn't notice this before. It would only fail if the restore column order is different from the dump column order, which only happens in weird inheritance cases, so it's not surprising nobody had hit the case with an extension config table. Nonetheless, it's a bug, and it goes a long way back, not just to v12 where the --inserts code path started to have a problem with this. In hopes of catching such cases a bit sooner in future, add some Asserts that "interesting" has been set in both dumpTableData and dumpTableSchema. Adjust the test case added by 3eb3d3e so that it checks the COPY rather than INSERT form of that bug, allowing it to detect the longer-standing symptom. Per bug #16655 from Cameron Daniel. Back-patch to all supported branches. Discussion: https://postgr.es/m/16655-5c92d6b3a9438137@postgresql.org Discussion: https://postgr.es/m/18048b44-3414-b983-8c7c-9165b177900d@2ndQuadrant.com
1 parent 77971bc commit d8c2a21

File tree

3 files changed

+42
-22
lines changed

3 files changed

+42
-22
lines changed

src/bin/pg_dump/pg_dump.c

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2040,8 +2040,6 @@ dumpTableData_insert(Archive *fout, void *dcontext)
20402040
if (nfields == 0)
20412041
continue;
20422042

2043-
Assert(tbinfo->attgenerated);
2044-
20452043
/* Emit a row heading */
20462044
if (rows_per_statement == 1)
20472045
archputs(" (", fout);
@@ -2202,6 +2200,9 @@ dumpTableData(Archive *fout, TableDataInfo *tdinfo)
22022200
char *copyStmt;
22032201
const char *copyFrom;
22042202

2203+
/* We had better have loaded per-column details about this table */
2204+
Assert(tbinfo->interesting);
2205+
22052206
if (!dopt->dump_inserts)
22062207
{
22072208
/* Dump/restore using COPY */
@@ -2390,6 +2391,9 @@ makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo)
23902391
addObjectDependency(&tdinfo->dobj, tbinfo->dobj.dumpId);
23912392

23922393
tbinfo->dataObj = tdinfo;
2394+
2395+
/* Make sure that we'll collect per-column info for this table. */
2396+
tbinfo->interesting = true;
23932397
}
23942398

23952399
/*
@@ -15651,10 +15655,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
1565115655
int j,
1565215656
k;
1565315657

15658+
/* We had better have loaded per-column details about this table */
15659+
Assert(tbinfo->interesting);
15660+
1565415661
qrelname = pg_strdup(fmtId(tbinfo->dobj.name));
1565515662
qualrelname = pg_strdup(fmtQualifiedDumpable(tbinfo));
1565615663

15657-
1565815664
if (tbinfo->hasoids)
1565915665
pg_log_warning("WITH OIDS is not supported anymore (table \"%s\")",
1566015666
qrelname);
@@ -18016,8 +18022,6 @@ processExtensionTables(Archive *fout, ExtensionInfo extinfo[],
1801618022
configtbl->dataObj->filtercond = pg_strdup(extconditionarray[j]);
1801718023
}
1801818024
}
18019-
18020-
configtbl->interesting = dumpobj;
1802118025
}
1802218026
}
1802318027
if (extconfigarray)

src/test/modules/test_pg_dump/t/001_base.pl

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,17 @@
135135
"$tempdir/defaults_tar_format.tar",
136136
],
137137
},
138+
exclude_table => {
139+
dump_cmd => [
140+
'pg_dump',
141+
'--exclude-table=regress_table_dumpable',
142+
"--file=$tempdir/exclude_table.sql",
143+
'postgres',
144+
],
145+
},
138146
extension_schema => {
139147
dump_cmd => [
140-
'pg_dump', '--schema=public', '--inserts',
148+
'pg_dump', '--schema=public',
141149
"--file=$tempdir/extension_schema.sql", 'postgres',
142150
],
143151
},
@@ -225,6 +233,7 @@
225233
clean_if_exists => 1,
226234
createdb => 1,
227235
defaults => 1,
236+
exclude_table => 1,
228237
no_privs => 1,
229238
no_owner => 1,);
230239

@@ -317,11 +326,28 @@
317326
regexp => qr/^
318327
\QCREATE TABLE public.regress_pg_dump_table (\E
319328
\n\s+\Qcol1 integer NOT NULL,\E
320-
\n\s+\Qcol2 integer\E
329+
\n\s+\Qcol2 integer,\E
330+
\n\s+\QCONSTRAINT regress_pg_dump_table_col2_check CHECK ((col2 > 0))\E
321331
\n\);\n/xm,
322332
like => { binary_upgrade => 1, },
323333
},
324334
335+
'COPY public.regress_table_dumpable (col1)' => {
336+
regexp => qr/^
337+
\QCOPY public.regress_table_dumpable (col1) FROM stdin;\E
338+
\n/xm,
339+
like => {
340+
%full_runs,
341+
data_only => 1,
342+
section_data => 1,
343+
extension_schema => 1,
344+
},
345+
unlike => {
346+
binary_upgrade => 1,
347+
exclude_table => 1,
348+
},
349+
},
350+
325351
'CREATE ACCESS METHOD regress_test_am' => {
326352
regexp => qr/^
327353
\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
@@ -443,7 +469,8 @@
443469
regexp => qr/^
444470
\QCREATE TABLE regress_pg_dump_schema.test_table (\E
445471
\n\s+\Qcol1 integer,\E
446-
\n\s+\Qcol2 integer\E
472+
\n\s+\Qcol2 integer,\E
473+
\n\s+\QCONSTRAINT test_table_col2_check CHECK ((col2 > 0))\E
447474
\n\);\n/xm,
448475
like => { binary_upgrade => 1, },
449476
},
@@ -578,17 +605,6 @@
578605
schema_only => 1,
579606
section_pre_data => 1,
580607
},
581-
},
582-
583-
# Dumpable object inside specific schema
584-
'INSERT INTO public.regress_table_dumpable VALUES (1);' => {
585-
create_sql => 'INSERT INTO public.regress_table_dumpable VALUES (1);',
586-
regexp => qr/^
587-
\QINSERT INTO public.regress_table_dumpable VALUES (1);\E
588-
\n/xm,
589-
like => {
590-
extension_schema => 1,
591-
},
592608
},);
593609
594610
#########################################

src/test/modules/test_pg_dump/test_pg_dump--1.0.sql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
CREATE TABLE regress_pg_dump_table (
77
col1 serial,
8-
col2 int
8+
col2 int check (col2 > 0)
99
);
1010

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

1616
CREATE TABLE regress_table_dumpable (
17-
col1 int
17+
col1 int check (col1 > 0)
1818
);
1919
SELECT pg_catalog.pg_extension_config_dump('regress_table_dumpable', '');
2020

@@ -34,7 +34,7 @@ CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;
3434
-- this extension.
3535
CREATE TABLE regress_pg_dump_schema.test_table (
3636
col1 int,
37-
col2 int
37+
col2 int check (col2 > 0)
3838
);
3939
GRANT SELECT ON regress_pg_dump_schema.test_table TO regress_dump_test_role;
4040

0 commit comments

Comments
 (0)