Michael, (dropping -general, not sure why that list was included...)
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Wed, Jul 27, 2016 at 8:07 AM, Stephen Frost <sfr...@snowman.net> wrote: > > That'd be great. It's definitely on my list of things to look into, but > > I'm extremely busy this week. I hope to look into it on Friday, would > > be great to see what you find. > > Sequences that are directly defined in extensions do not get dumped, > and sequences that are part of a serial column in an extension are > getting dumped. Looking into this problem, getOwnedSeqs() is visibly > doing an incorrect assumption: sequences owned by table columns are > dumped unconditionally, but this is not true for sequences that are > part of extensions. More precisely, dobj->dump is being enforced to > DUMP_COMPONENT_ALL, which makes the sequence definition to be dumped. > Oops. Right, it should be set to the same value as the table which is being dumped rather than DUMP_COMPONENT_ALL. Moreover, the owning_tab->dobj.dump check should explicitly check against DUMP_COMPONENT_NONE, but that's just to be neat since that's '0'. > The patch attached fixes the problem for me. I have added as well > tests in test_pg_dump in the shape of sequences defined in an > extension, and sequences that are part of a serial column. This patch > is also able to work in the case where a sequence is created as part > of a serial column, and gets removed after, say with ALTER EXTENSION > DROP SEQUENCE. The behavior for sequences and serial columns that are > not part of extensions is unchanged. This isn't correct as the dump components which are independent of the extension (ACLs, security labels, policies) won't be dumped. Consider, for example, what happens if the user runs: GRANT USAGE ON SEQUENCE <extension_sequence> TO role; This wouldn't get dumped out with your patch, since we would decide that, because the sequence is a member of the extension, that nothing about it should be dumped, which isn't correct. > Stephen, it would be good if you could check the correctness of this > patch as you did all this refactoring of pg_dump to support catalog > ACLs. I am sure by the way that checking for (owning_tab->dobj.dump && > DUMP_COMPONENT_DEFINITION) != 0 is not good because of for example the > case of a serial column created in an extension where the sequence is > dropped from the extension afterwards. If the sequence is dropped from the extension then the sequence will be ignored by checkExtensionMembership() during selectDumpableObject() and the regular selectDumpableObject() code will handle marking the sequence appropriately. What we need to be doing here is combining the set of components that the sequence has been marked with and the set of components that the table has been marked with, not trying to figure out if the sequence is a member of an extension or not and changing what to do in those cases, that's checkExtensionMembership()'s job, really. Attached is a patch implementing this and which passes the regression tests you added and a couple that I added for the non-extension case. I'm going to look at adding a few more regression tests and if I don't come across any issues then I'll likely push the fix sometime tomorrow. Comments welcome, of course. Thanks! Stephen
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
new file mode 100644
index 08c2b0c..333e628
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** getOwnedSeqs(Archive *fout, TableInfo tb
*** 6035,6048 ****
if (!OidIsValid(seqinfo->owning_tab))
continue; /* not an owned sequence */
! if (seqinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
! continue; /* no need to search */
owning_tab = findTableByOid(seqinfo->owning_tab);
! if (owning_tab && owning_tab->dobj.dump)
! {
seqinfo->interesting = true;
- seqinfo->dobj.dump = DUMP_COMPONENT_ALL;
- }
}
}
--- 6035,6061 ----
if (!OidIsValid(seqinfo->owning_tab))
continue; /* not an owned sequence */
!
owning_tab = findTableByOid(seqinfo->owning_tab);
!
! /*
! * We need to dump the components that are being dumped for the table
! * and any components which the sequence is explicitly marked with.
! *
! * We can't simply use the set of components which are being dumped for
! * the table as the table might be in an extension (and only the
! * non-extension components, eg: ACLs if changed, security labels, and
! * policies, are being dumped) while the sequence is not (and therefore
! * the definition and other components should also be dumped).
! *
! * If the sequence is part of the extension then it should be properly
! * marked by checkExtensionMembership() and this will be a no-op as the
! * table will be equivilantly marked.
! */
! seqinfo->dobj.dump = seqinfo->dobj.dump | owning_tab->dobj.dump;
!
! if (seqinfo->dobj.dump != DUMP_COMPONENT_NONE)
seqinfo->interesting = true;
}
}
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
new file mode 100644
index 1d82bfd..5a042b8
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** my %tests = (
*** 361,366 ****
--- 361,416 ----
only_dump_test_schema => 1,
only_dump_test_table => 1,
test_schema_plus_blobs => 1, }, },
+ 'ALTER SEQUENCE test_table_col1_seq' => {
+ regexp => qr/^
+ \QALTER SEQUENCE test_table_col1_seq OWNED BY test_table.col1;\E
+ /xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ exclude_test_table_data => 1,
+ no_privs => 1,
+ no_owner => 1,
+ only_dump_test_schema => 1,
+ only_dump_test_table => 1,
+ pg_dumpall_dbprivs => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ test_schema_plus_blobs => 1, },
+ unlike => {
+ exclude_test_table => 1,
+ exclude_dump_test_schema => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ section_post_data => 1, }, },
+ 'ALTER SEQUENCE test_third_table_col1_seq' => {
+ regexp => qr/^
+ \QALTER SEQUENCE test_third_table_col1_seq OWNED BY test_third_table.col1;\E
+ /xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ exclude_dump_test_schema => 1,
+ exclude_test_table => 1,
+ exclude_test_table_data => 1,
+ no_privs => 1,
+ no_owner => 1,
+ pg_dumpall_dbprivs => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ only_dump_test_schema => 1,
+ only_dump_test_table => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ section_post_data => 1,
+ test_schema_plus_blobs => 1, }, },
'ALTER TABLE ONLY test_table ADD CONSTRAINT ... PRIMARY KEY' => {
regexp => qr/^
\QALTER TABLE ONLY test_table\E \n^\s+
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
new file mode 100644
index fd9c37f..fc93e1b
*** a/src/test/modules/test_pg_dump/t/001_base.pl
--- b/src/test/modules/test_pg_dump/t/001_base.pl
*************** my %tests = (
*** 223,232 ****
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
'CREATE TABLE regress_pg_dump_table' => {
regexp => qr/^
\QCREATE TABLE regress_pg_dump_table (\E
! \n\s+\Qcol1 integer,\E
\n\s+\Qcol2 integer\E
\n\);$/xm,
like => { binary_upgrade => 1, },
--- 223,274 ----
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
+ 'CREATE SEQUENCE regress_pg_dump_table_col1_seq' => {
+ regexp => qr/^
+ \QCREATE SEQUENCE regress_pg_dump_table_col1_seq\E
+ \n\s+\QSTART WITH 1\E
+ \n\s+\QINCREMENT BY 1\E
+ \n\s+\QNO MINVALUE\E
+ \n\s+\QNO MAXVALUE\E
+ \n\s+\QCACHE 1;\E
+ $/xm,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE SEQUENCE regress_pg_dump_seq' => {
+ regexp => qr/^
+ \QCREATE SEQUENCE regress_pg_dump_seq\E
+ \n\s+\QSTART WITH 1\E
+ \n\s+\QINCREMENT BY 1\E
+ \n\s+\QNO MINVALUE\E
+ \n\s+\QNO MAXVALUE\E
+ \n\s+\QCACHE 1;\E
+ $/xm,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
'CREATE TABLE regress_pg_dump_table' => {
regexp => qr/^
\QCREATE TABLE regress_pg_dump_table (\E
! \n\s+\Qcol1 integer NOT NULL,\E
\n\s+\Qcol2 integer\E
\n\);$/xm,
like => { binary_upgrade => 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.sql
new file mode 100644
index 5fe6063..93de2c5
*** 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
***************
*** 4,13 ****
\echo Use "CREATE EXTENSION test_pg_dump" to load this file. \quit
CREATE TABLE regress_pg_dump_table (
! col1 int,
col2 int
);
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
GRANT SELECT(col1) ON regress_pg_dump_table TO public;
--- 4,15 ----
\echo Use "CREATE EXTENSION test_pg_dump" to load this file. \quit
CREATE TABLE regress_pg_dump_table (
! col1 serial,
col2 int
);
+ CREATE SEQUENCE regress_pg_dump_seq;
+
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
GRANT SELECT(col1) ON regress_pg_dump_table TO public;
signature.asc
Description: Digital signature
