pg_dump with tables created in schemas created by extensions
Hi,
About a month or two ago I reported a pg_dump bug regarding tables (and
other objects) created inside a schema from an extension.
Objects created by the extensions are not dumped, as they will be
created once again with the CREATE EXTENSION call, but and other objects
which might live inside an object created by the extension should be
dumped so they get created inside the same schema.
The problem showed up when dumping a DB with PgQ installed as an
extension. Check here:
/messages/by-id/d86dd685-1870-cfa0-e5e4-def1f918bec9@2ndquadrant.com
and here:
/messages/by-id/409fe594-f4cc-89f5-c0d2-0a921987a864@2ndquadrant.com
Some discussion came up on the bugs list on how to fix the issue, and
the fact the new tests were needed.
I'm attaching a patch to provide such test, which if applied now,
returns failure on a number of runs, all expected due to the bug we have
at hand.
I believe the fix will be simple after the back and forth mails with
Michael, Stephen and Tom. I will work on that later, but preferred to
have the tests the show the problem which will also make testing the fix
easier.
Thoughts?
Regards,
--
Mart�n Marqu�s http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
add_pg_dump_tests_for_tables_in_schemas_from_extensions.difftext/x-diff; name=add_pg_dump_tests_for_tables_in_schemas_from_extensions.diffDownload
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 fb4f573..6086317
*** a/src/test/modules/test_pg_dump/t/001_base.pl
--- b/src/test/modules/test_pg_dump/t/001_base.pl
*************** my %tests = (
*** 283,288 ****
--- 283,313 ----
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
+ 'CREATE TABLE regress_test_schema_table' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.test_schema_table (
+ col1 serial primary key,
+ CHECK (col1 <= 1000)
+ );',
+ regexp => qr/^
+ \QCREATE TABLE test_schema_table (\E
+ \n\s+\Qcol1 integer NOT NULL,\E
+ \n\s+\QCONSTRAINT test_table_col1_check CHECK \E
+ \Q((col1 <= 1000))\E
+ \n\);/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
'CREATE ACCESS METHOD regress_test_am' => {
regexp => qr/^
\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
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 c2fe90d..3f88e6c
*** 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
*************** CREATE TABLE regress_pg_dump_table (
*** 10,15 ****
--- 10,24 ----
CREATE SEQUENCE regress_pg_dump_seq;
+ -- We want to test that schemas and objects created in the schema by the
+ -- extension are not dumped, yet other objects created afterwards will be
+ -- dumped.
+ CREATE SCHEMA regress_pg_dump_schema
+ CREATE TABLE regress_pg_dump_schema_table (
+ col1 serial,
+ col2 int
+ );
+
GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
I believe the fix will be simple after the back and forth mails with
Michael, Stephen and Tom. I will work on that later, but preferred to
have the tests the show the problem which will also make testing the fix
easier.Thoughts?
It seems to me that we are going to need a bit more coverage for more
object types depending on the code paths that are being changed. For
example, sequences or functions should be checked for as well, and not
only tables. By the way, do you need help to build a patch or should I
jump in?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Michael,
2016-08-23 5:02 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
I believe the fix will be simple after the back and forth mails with
Michael, Stephen and Tom. I will work on that later, but preferred to
have the tests the show the problem which will also make testing the fix
easier.Thoughts?
It seems to me that we are going to need a bit more coverage for more
object types depending on the code paths that are being changed. For
example, sequences or functions should be checked for as well, and not
only tables. By the way, do you need help to build a patch or should I
jump in?
I wanted to test what I had in mind with one object, and then see if
any replication is needed for other objects.
I was struggling the last days as what I was reading in my patched
pg_dump.c had to work as expected, and dump the tables not created by
the test_pg_dump extension but inside the schema
regress_pg_dump_schema.
Today I decided to go over the test I wrote, and found a bug there,
reason why I couldn't get a successful make check.
Here go 2 patches. One is a fix for the test I sent earlier. The other
is the proposed idea Tom had using the dump_contains that Stephan
committed on 9.6.
So far I've checked that it fixes the dumpable for Tables, but I think
it should work for all other objects as well, as all this patch does
is leave execution of checkExtensionMembership at the end of
selectDumpableNamespace, leaving the dump_contains untouched.
Checks pass ok.
I will add tests for sequence and functions as you mention and test again.
Then I'll check if other tests should be added as well.
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-New-tests-for-pg_dump-which-make-sure-that-tables-fr.patchtext/x-patch; charset=US-ASCII; name=0001-New-tests-for-pg_dump-which-make-sure-that-tables-fr.patchDownload
From 6cbba9e87d1733ccb03d3d705e135e607be63cba Mon Sep 17 00:00:00 2001
From: Martin <martin@2ndquadrant.com>
Date: Tue, 23 Aug 2016 16:17:38 -0300
Subject: [PATCH 1/2] New tests for pg_dump which make sure that tables from a
schema depending on an extension will get dumped when they don't depend on
the extension.
The only objects we shouldn't dump are the ones created by the extension.
We will provide a patch that fixes this bug.
---
src/test/modules/test_pg_dump/t/001_base.pl | 25 ++++++++++++++++++++++
.../modules/test_pg_dump/test_pg_dump--1.0.sql | 9 ++++++++
2 files changed, 34 insertions(+)
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index fb4f573..7bb92e7 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -283,6 +283,31 @@ my %tests = (
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
+ 'CREATE TABLE regress_test_schema_table' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.test_schema_table (
+ col1 serial primary key,
+ CHECK (col1 <= 1000)
+ );',
+ regexp => qr/^
+ \QCREATE TABLE test_schema_table (\E
+ \n\s+\Qcol1 integer NOT NULL,\E
+ \n\s+\QCONSTRAINT test_schema_table_col1_check CHECK \E
+ \Q((col1 <= 1000))\E
+ \n\);/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
'CREATE ACCESS METHOD regress_test_am' => {
regexp => qr/^
\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
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
index c2fe90d..3f88e6c 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
@@ -10,6 +10,15 @@ CREATE TABLE regress_pg_dump_table (
CREATE SEQUENCE regress_pg_dump_seq;
+-- We want to test that schemas and objects created in the schema by the
+-- extension are not dumped, yet other objects created afterwards will be
+-- dumped.
+CREATE SCHEMA regress_pg_dump_schema
+ CREATE TABLE regress_pg_dump_schema_table (
+ col1 serial,
+ col2 int
+ );
+
GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
--
2.5.5
0002-This-patch-fixes-a-bug-reported-against-pg_dump-and-.patchtext/x-patch; charset=US-ASCII; name=0002-This-patch-fixes-a-bug-reported-against-pg_dump-and-.patchDownload
From 0dfe2224fae86ab2b6f8da4fc25b96fb13ca0f6c Mon Sep 17 00:00:00 2001
From: Martin <martin@2ndquadrant.com>
Date: Tue, 23 Aug 2016 16:23:44 -0300
Subject: [PATCH 2/2] This patch fixes a bug reported against pg_dump, and
makes pg_dump dump the objects contained in schemas depending on an
extension.
Schema will not be dumped, as it will be created by the extension, but
any other tables created outside the extension's SQL will be dumped.
We need to provide further patches for other objects like sequences,
functions, etc.
---
src/bin/pg_dump/pg_dump.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4ee10fc..13b61bd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1338,9 +1338,6 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout)
static void
selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
{
- if (checkExtensionMembership(&nsinfo->dobj, fout))
- return; /* extension membership overrides all else */
-
/*
* If specific tables are being dumped, do not dump any complete
* namespaces. If specific namespaces are being dumped, dump just those
@@ -1377,6 +1374,13 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
simple_oid_list_member(&schema_exclude_oids,
nsinfo->dobj.catId.oid))
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
+
+ /*
+ * Extension membership overrides all else, with the exception of objects
+ * dump_contains.
+ */
+ checkExtensionMembership(&nsinfo->dobj, fout);
+
}
/*
--
2.5.5
Hi,
2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
I will add tests for sequence and functions as you mention and test again.
Then I'll check if other tests should be added as well.
I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.pl
I can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).
Other things to check:
CREATE AGGREGATE
CREATE DOMAIN
CREATE FUNCTION
CREATE TYPE
CREATE MATERIALIZED VIEW
CREATE POLICY
Maybe even CREATE INDEX over a table created in the schema.
Also, ACLs have to be tested against objects in the schema.
I hope I didn't miss anything there.
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 8/23/16 3:34 PM, Martín Marqués wrote:
I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.plI can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).
At one point I had some code that understood what object names (ie:
AGGREGATE, TABLE, etc) went with what catalog tables, what ones lived
inside a schema (as opposed to globally), and which ones were
shared/global (cross-database). I think I needed this for some automatic
handling of comments, but it's been a while. Maybe something like that
would help reduce the duplication...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
Hi,
2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
I will add tests for sequence and functions as you mention and test again.
Then I'll check if other tests should be added as well.
I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.plI can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).Other things to check:
CREATE AGGREGATE
CREATE DOMAIN
CREATE FUNCTION
CREATE TYPE
CREATE MATERIALIZED VIEW
CREATE POLICYMaybe even CREATE INDEX over a table created in the schema.
Also, ACLs have to be tested against objects in the schema.
I hope I didn't miss anything there.
I'll take a look at that soon and review your patch as well as your
tests. For now I have added an entry in the CF app to not lose track
of it:
https://commitfest.postgresql.org/10/736/
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 24, 2016 at 9:07 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
Hi,
2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
I will add tests for sequence and functions as you mention and test again.
Then I'll check if other tests should be added as well.
I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.plI can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).Other things to check:
CREATE AGGREGATE
CREATE FUNCTION
Agreed on those two.
CREATE DOMAIN
CREATE TYPE
Those two overlap, so adding just a type would be fine.
CREATE MATERIALIZED VIEW
This overlaps with normal relations.
CREATE POLICY
Policies are not schema-qualified.
Maybe even CREATE INDEX over a table created in the schema.
Yes, we can do something fancy things here... But see below as pg_dump
is broken even in this case...
Also, ACLs have to be tested against objects in the schema.
I hope I didn't miss anything there.
So I have reviewed the patch for master, did some wordsmithing and
added more tests. Particularly, I have added tests for a couple of
objects created in the extension, as well as objects that are not part
of the extension but make use of the schema of the extension. Even
with that, I am still seeing two problems:
- ACLs assigned to an aggregate in an extension are not getting dumped
in a binary upgrade, and I think that they should be. The aggregate
definition gets correctly handled though.
- if an index is defined on a table part of an extension it will not
be dumped. We can fix the problem of this thread and the one I just
found separately though.
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.
--
Michael
Attachments:
pgdump-extension-v2.patchinvalid/octet-stream; name=pgdump-extension-v2.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a5c2d09..63ced1c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1338,9 +1338,6 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout)
static void
selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
{
- if (checkExtensionMembership(&nsinfo->dobj, fout))
- return; /* extension membership overrides all else */
-
/*
* If specific tables are being dumped, do not dump any complete
* namespaces. If specific namespaces are being dumped, dump just those
@@ -1377,6 +1374,12 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
simple_oid_list_member(&schema_exclude_oids,
nsinfo->dobj.catId.oid))
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
+
+ /*
+ * Extension membership overrides all else, and this is done last
+ * to be sure that dump_contains is in a correct state.
+ */
+ (void) checkExtensionMembership(&nsinfo->dobj, fout);
}
/*
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index f02beb3..0adccb1 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -283,6 +283,31 @@ my %tests = (
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
+ 'CREATE TABLE regress_test_schema_table' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.test_schema_table (
+ col1 serial primary key,
+ CHECK (col1 <= 1000)
+ );',
+ regexp => qr/^
+ \QCREATE TABLE test_schema_table (\E
+ \n\s+\Qcol1 integer NOT NULL,\E
+ \n\s+\QCONSTRAINT test_schema_table_col1_check CHECK \E
+ \Q((col1 <= 1000))\E
+ \n\);/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
'CREATE ACCESS METHOD regress_test_am' => {
regexp => qr/^
\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
@@ -429,7 +454,231 @@ my %tests = (
unlike => {
no_privs => 1,
pg_dumpall_globals => 1,
- section_post_data => 1, }, },);
+ section_post_data => 1, }, },
+ # Objects included in extension part of a schema created by this extension */
+ 'CREATE TABLE regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QCREATE TABLE test_table (\E
+ \n\s+\Qcol1 integer,\E
+ \n\s+\Qcol2 integer\E
+ \n\);$/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, }, },
+ 'GRANT SELECT ON regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT SELECT ON TABLE test_table TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE SEQUENCE regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QCREATE SEQUENCE test_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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT USAGE ON SEQUENCE test_seq TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE TYPE regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QCREATE TYPE test_type AS (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON TYPE test_type TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE FUNCTION regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QCREATE FUNCTION test_func() RETURNS integer\E
+ \n\s+\QLANGUAGE sql\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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_func() TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE AGGREGATE regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QCREATE AGGREGATE test_agg(smallint) (\E
+ \n\s+\QSFUNC = int2_sum,\E
+ \n\s+\QSTYPE = bigint\E
+ \n\);$/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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ # Objects not included in extension, part of schema created by extension
+ 'CREATE TABLE regress_pg_dump_schema.external_tab' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.external_tab
+ (col1 int);',
+ regexp => qr/^
+ \QCREATE TABLE external_tab (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
+ 'CREATE INDEX regress_pg_dump_schema.test_index' => {
+ create_order => 6,
+ create_sql => 'CREATE INDEX test_index
+ ON regress_pg_dump_schema.test_table (col1);',
+ regexp => qr/^
+ \QCREATE INDEX test_index ON test_table USING btree (col1);\E
+ $/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, }, );
#########################################
# Create a PG instance to test actually dumping from
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
index c2fe90d..ca9fb18 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
@@ -10,6 +10,8 @@ CREATE TABLE regress_pg_dump_table (
CREATE SEQUENCE regress_pg_dump_seq;
+CREATE SCHEMA regress_pg_dump_schema;
+
GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
@@ -19,3 +21,25 @@ GRANT SELECT(col2) ON regress_pg_dump_table TO regress_dump_test_role;
REVOKE SELECT(col2) ON regress_pg_dump_table FROM regress_dump_test_role;
CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;
+
+-- Create a set of objects that are part of the schema created by
+-- this extension.
+CREATE TABLE regress_pg_dump_schema.test_table (
+ col1 int,
+ col2 int
+);
+GRANT SELECT ON regress_pg_dump_schema.test_table TO regress_dump_test_role;
+
+CREATE SEQUENCE regress_pg_dump_schema.test_seq;
+GRANT USAGE ON regress_pg_dump_schema.test_seq TO regress_dump_test_role;
+
+CREATE TYPE regress_pg_dump_schema.test_type AS (col1 int);
+GRANT USAGE ON TYPE regress_pg_dump_schema.test_type TO regress_dump_test_role;
+
+CREATE FUNCTION regress_pg_dump_schema.test_func () RETURNS int
+AS 'SELECT 1;' LANGUAGE SQL;
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_func() TO regress_dump_test_role;
+
+CREATE AGGREGATE regress_pg_dump_schema.test_agg(int2)
+(SFUNC = int2_sum, STYPE = int8);
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_agg(int2) TO regress_dump_test_role;
Michael,
* Michael Paquier (michael.paquier@gmail.com) wrote:
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.
Are you sure you have the tests correct..? At least for testagg(), it
looks like you're testing for:
GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
but what's in the dump is (equivilantly):
GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
I've not looked into all the failures, but at least this one seems like
an issue in the test, not an issue in pg_dump.
Thanks!
Stephen
2016-08-24 11:15 GMT-03:00 Stephen Frost <sfrost@snowman.net>:
Michael,
* Michael Paquier (michael.paquier@gmail.com) wrote:
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.Are you sure you have the tests correct..? At least for testagg(), it
looks like you're testing for:GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
but what's in the dump is (equivilantly):
GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
Yes, that was the problem there.
I've not looked into all the failures, but at least this one seems like
an issue in the test, not an issue in pg_dump.
I see the other 12 failures regarding the CREATE INDEX that Michael
reported but can't quite find where it's originated. (or actually
where the problem is)
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-24 17:01 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
2016-08-24 11:15 GMT-03:00 Stephen Frost <sfrost@snowman.net>:
Michael,
* Michael Paquier (michael.paquier@gmail.com) wrote:
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.Are you sure you have the tests correct..? At least for testagg(), it
looks like you're testing for:GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
but what's in the dump is (equivilantly):
GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
Yes, that was the problem there.
I've not looked into all the failures, but at least this one seems like
an issue in the test, not an issue in pg_dump.I see the other 12 failures regarding the CREATE INDEX that Michael
reported but can't quite find where it's originated. (or actually
where the problem is)
OK, I see where the problem is.
Indexes don't have a selectDumpableIndex() function to see if we dump
it or not. We just don't gather indexes from tables for which we are
dumping their definition:
/* Ignore indexes of tables whose definitions are not
to be dumped */
if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
This means we have to perform the same change we did on
selectDumpableNamespace for selectDumpableTable, and also assign the
correct value to dump_contains, which is not set there.
The problem will come when we have to decide on which indexes were
created by the extension (primary key indexes, other indexes created
by the extension) and which were created afterwards over a table which
depends on the extension (the test_table from the extension).
Right now, I'm in an intermediate state, where I got getIndexes() to
query for the indexes of these tables, but dumpIndexes is not dumping
the indexes that were queried.
I wonder if we should have a selectDumpableIndexes to set the
appropriate dobj.dump for the Indexes.
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 24, 2016 at 11:15 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Michael Paquier (michael.paquier@gmail.com) wrote:
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.Are you sure you have the tests correct..? At least for testagg(), it
looks like you're testing for:GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
but what's in the dump is (equivilantly):
GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
I've not looked into all the failures, but at least this one seems like
an issue in the test, not an issue in pg_dump.
Yes, you are right. If I look at the diffs this morning I am seeing
the ACLs being dumped for this aggregate. So we could just fix the
test and be done with it. I did not imagine the index issue though,
and this is broken for some time, so that's not exclusive to 9.6 :)
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
Yes, you are right. If I look at the diffs this morning I am seeing
the ACLs being dumped for this aggregate. So we could just fix the
test and be done with it. I did not imagine the index issue though,
and this is broken for some time, so that's not exclusive to 9.6 :)
Hi Michael,
Do you see any easier way than what I mentioned earlier (adding a
selectDumpableIndex() function) to fix the index dumping issue?
Regards,
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
Yes, you are right. If I look at the diffs this morning I am seeing
the ACLs being dumped for this aggregate. So we could just fix the
test and be done with it. I did not imagine the index issue though,
and this is broken for some time, so that's not exclusive to 9.6 :)Do you see any easier way than what I mentioned earlier (adding a
selectDumpableIndex() function) to fix the index dumping issue?
Yes, we are going to need something across those lines. And my guess
is that this is going to be rather close to getOwnedSeqs() in terms of
logic.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
2016-08-25 8:10 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
Yes, you are right. If I look at the diffs this morning I am seeing
the ACLs being dumped for this aggregate. So we could just fix the
test and be done with it. I did not imagine the index issue though,
and this is broken for some time, so that's not exclusive to 9.6 :)Do you see any easier way than what I mentioned earlier (adding a
selectDumpableIndex() function) to fix the index dumping issue?Yes, we are going to need something across those lines. And my guess
is that this is going to be rather close to getOwnedSeqs() in terms of
logic.
I was able to get this fixed without any further new functions (just
using the dump/dump_contains and applying the same fix on
selectDumpableTable).
Main problem relied here in getIndexes()
@@ -6158,7 +6167,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[],
int numTables)
continue;
/* Ignore indexes of tables whose definitions are not
to be dumped */
- if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
+ if (!(tbinfo->dobj.dump_contains & DUMP_COMPONENT_DEFINITION))
continue;if (g_verbose)
But we have to set dump_contains with correct values.
There's still one issue, which I'll add a test for as well, which is
that if the index was created by the extension, it will be dumped
anyway. I'll ave a look at that as well.
One other thing I found was that one of the CREATE INDEX tests had
incorrectly set like and unlike for pre_data and post_data. (indexes
are dumped in section post_data)
That's been fixes as well.
I've cleaned up the patch a bit, so this is v3 with all checks passing.
I'll add that new test regarding dumping an index created by the
extension (which will fail) and look for ways to fix it.
Regards,
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
pgdump-extension-v3.patchinvalid/octet-stream; name=pgdump-extension-v3.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a5c2d09..f98bc52 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1338,9 +1338,6 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout)
static void
selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
{
- if (checkExtensionMembership(&nsinfo->dobj, fout))
- return; /* extension membership overrides all else */
-
/*
* If specific tables are being dumped, do not dump any complete
* namespaces. If specific namespaces are being dumped, dump just those
@@ -1377,6 +1374,12 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
simple_oid_list_member(&schema_exclude_oids,
nsinfo->dobj.catId.oid))
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
+
+ /*
+ * Extension membership overrides all else, and this is done last
+ * to be sure that dump_contains is in a correct state.
+ */
+ (void) checkExtensionMembership(&nsinfo->dobj, fout);
}
/*
@@ -1386,9 +1389,6 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
static void
selectDumpableTable(TableInfo *tbinfo, Archive *fout)
{
- if (checkExtensionMembership(&tbinfo->dobj, fout))
- return; /* extension membership overrides all else */
-
/*
* If specific tables are being dumped, dump just those tables; else, dump
* according to the parent namespace's dump flag.
@@ -1407,6 +1407,14 @@ selectDumpableTable(TableInfo *tbinfo, Archive *fout)
simple_oid_list_member(&table_exclude_oids,
tbinfo->dobj.catId.oid))
tbinfo->dobj.dump = DUMP_COMPONENT_NONE;
+
+ tbinfo->dobj.dump_contains = tbinfo->dobj.dump;
+
+ /*
+ * Extension membership overrides all else, and this is done last
+ * to be sure that dump_contains is in a correct state.
+ */
+ (void) checkExtensionMembership(&tbinfo->dobj, fout);
}
/*
@@ -6157,8 +6165,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
if (!tbinfo->hasindex)
continue;
- /* Ignore indexes of tables whose definitions are not to be dumped */
- if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
+ /* Ignore indexes of tables whose definitions contents are not to be dumped */
+ if (!(tbinfo->dobj.dump_contains & DUMP_COMPONENT_DEFINITION))
continue;
if (g_verbose)
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index f02beb3..8853af3 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -283,6 +283,31 @@ my %tests = (
schema_only => 1,
section_pre_data => 1,
section_post_data => 1, }, },
+ 'CREATE TABLE regress_test_schema_table' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.test_schema_table (
+ col1 serial primary key,
+ CHECK (col1 <= 1000)
+ );',
+ regexp => qr/^
+ \QCREATE TABLE test_schema_table (\E
+ \n\s+\Qcol1 integer NOT NULL,\E
+ \n\s+\QCONSTRAINT test_schema_table_col1_check CHECK \E
+ \Q((col1 <= 1000))\E
+ \n\);/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_privs => 1,
+ no_owner => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
'CREATE ACCESS METHOD regress_test_am' => {
regexp => qr/^
\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
@@ -429,7 +454,231 @@ my %tests = (
unlike => {
no_privs => 1,
pg_dumpall_globals => 1,
- section_post_data => 1, }, },);
+ section_post_data => 1, }, },
+ # Objects included in extension part of a schema created by this extension */
+ 'CREATE TABLE regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QCREATE TABLE test_table (\E
+ \n\s+\Qcol1 integer,\E
+ \n\s+\Qcol2 integer\E
+ \n\);$/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, }, },
+ 'GRANT SELECT ON regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT SELECT ON TABLE test_table TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE SEQUENCE regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QCREATE SEQUENCE test_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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT USAGE ON SEQUENCE test_seq TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE TYPE regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QCREATE TYPE test_type AS (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON TYPE test_type TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE FUNCTION regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QCREATE FUNCTION test_func() RETURNS integer\E
+ \n\s+\QLANGUAGE sql\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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_func() TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE AGGREGATE regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QCREATE AGGREGATE test_agg(smallint) (\E
+ \n\s+\QSFUNC = int2_sum,\E
+ \n\s+\QSTYPE = bigint\E
+ \n\);$/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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ # Objects not included in extension, part of schema created by extension
+ 'CREATE TABLE regress_pg_dump_schema.external_tab' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.external_tab
+ (col1 int);',
+ regexp => qr/^
+ \QCREATE TABLE external_tab (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, },
+ 'CREATE INDEX regress_pg_dump_schema.test_index' => {
+ create_order => 6,
+ create_sql => 'CREATE INDEX test_index
+ ON regress_pg_dump_schema.test_table (col1);',
+ regexp => qr/^
+ \QCREATE INDEX test_index ON test_table USING btree (col1);\E
+ $/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ schema_only => 1,
+ section_post_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_pre_data => 1, }, }, );
#########################################
# Create a PG instance to test actually dumping from
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
index c2fe90d..ca9fb18 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
@@ -10,6 +10,8 @@ CREATE TABLE regress_pg_dump_table (
CREATE SEQUENCE regress_pg_dump_seq;
+CREATE SCHEMA regress_pg_dump_schema;
+
GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
@@ -19,3 +21,25 @@ GRANT SELECT(col2) ON regress_pg_dump_table TO regress_dump_test_role;
REVOKE SELECT(col2) ON regress_pg_dump_table FROM regress_dump_test_role;
CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;
+
+-- Create a set of objects that are part of the schema created by
+-- this extension.
+CREATE TABLE regress_pg_dump_schema.test_table (
+ col1 int,
+ col2 int
+);
+GRANT SELECT ON regress_pg_dump_schema.test_table TO regress_dump_test_role;
+
+CREATE SEQUENCE regress_pg_dump_schema.test_seq;
+GRANT USAGE ON regress_pg_dump_schema.test_seq TO regress_dump_test_role;
+
+CREATE TYPE regress_pg_dump_schema.test_type AS (col1 int);
+GRANT USAGE ON TYPE regress_pg_dump_schema.test_type TO regress_dump_test_role;
+
+CREATE FUNCTION regress_pg_dump_schema.test_func () RETURNS int
+AS 'SELECT 1;' LANGUAGE SQL;
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_func() TO regress_dump_test_role;
+
+CREATE AGGREGATE regress_pg_dump_schema.test_agg(int2)
+(SFUNC = int2_sum, STYPE = int8);
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_agg(int2) TO regress_dump_test_role;
Hi,
2016-08-26 10:53 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
There's still one issue, which I'll add a test for as well, which is
that if the index was created by the extension, it will be dumped
anyway. I'll have a look at that as well.
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension. I don't know if
it's deliberate or an overlook.
The thing is that we can't check pg_depend for dependency of an index
and the extension that creates it.
I was talking with other developers, and we kind of agree this is a
bug, for 2 reasons we thought of:
*) If the extension only creates an index over an existing table, a
drop extension will not drop that index
*) We need to have the dependency for this patch as well, else we'll
end up with an inconsistent dump, or at least one that could restore
with a != 0 return error code.
I'll open a separate bug report for this.
Regards,
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?= <martin@2ndquadrant.com> writes:
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.
Surely the index has a dependency on a table, which depends on the
extension?
If you mean that you want an extension to create an index on a table that
doesn't belong to it, but it's assuming pre-exists, I think that's just
stupid and we need not support it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/27/2016 12:37 AM, Tom Lane wrote:
=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?= <martin@2ndquadrant.com> writes:
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.Surely the index has a dependency on a table, which depends on the
extension?If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.
I don't see why that would be stupid (and I'm not sure it's up to us to
just decide it's stupid).
Imagine you use extensions to break the application into modules. You
have a "basic" extension, with the table, and a "search" extension
implementing fulltext search on top of that table. Isn't it natural to
keep the gin indexes in the search extension?
So the basic--1.0.sql will do something like
CREATE TABLE x ( ...)
and search--1.0.sql would do
CREATE INDEX y ON x USING gin (z);
CREATE FUNCTION search_objects(..) ...
because the index and function doing the search are somewhat tightly
coupled. I admit this is just an example and I don't know how many
people use extensions this way, but I don't see why this would be
inherently stupid pattern?
I see the current behavior is documented, and I do understand why global
objects can't be part of the extension, but for indexes it seems to
violate POLA a bit.
Is there a reason why we don't want the extension/index dependencies?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
On 08/27/2016 12:37 AM, Tom Lane wrote:
If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.
I don't see why that would be stupid (and I'm not sure it's up to us to
just decide it's stupid).
Well, think about it.
1. Let's say user A owns the pre-existing table and user B owns the
extension. Who owns the index?
2. Generally speaking, if an object is part of an extension then you
can't drop the object without dropping the whole extension. This means
that either user A can't drop his own table, or he can but that causes
dropping the whole extension (which he does not own), not to mention
whatever else depends on it (which he owns even less).
3. Can user A do something that would mutate the index? (For instance,
ALTER COLUMN TYPE on one of the columns it's on.) Now is it still a
member of the extension? If user A can't, can user B? What if either of
them mutated the column to a datatype that belongs to some extension that
has a dependency on the original one? Now you've got a circular
dependency loop, what fun --- especially at dump/reload time, where
pg_dump has no hope of breaking the circularity.
4. If we're going to support indexes as independent members of extensions,
why not foreign-key constraints, or check constraints? Maybe check
constraints on domain types that don't belong to the extension? Heck,
why not allow an extension to do ALTER TABLE ADD COLUMN? (I think
BTW that several of these cases would allow creation of circular
extension dependencies even without any ALTER COLUMN TYPE shenanigans.)
None of this makes any sense, because these things are not stand-alone
objects in any sense of the word. They are attributes of tables (or
domain types, for that case) and there are good reasons why we don't
consider such attributes to have ownership independent of the object
they are part of. I do not think it is sensible for an extension to "own"
objects that don't also have, or could potentially have [1]weasel wording for cases like casts, which we don't consider to have owners, though I'm not sure why not., independent
ownership in the sense of an owning user.
Imagine you use extensions to break the application into modules.
I do not think that extensions as we currently understand them are a
suitable basis for slicing an application in the way you suggest.
I'm fine with an extension owning a whole table, but the rest of this
is just crazy. And I sure as hell do not want to put it in as part
of a bug fix for existing releases.
regards, tom lane
[1]: weasel wording for cases like casts, which we don't consider to have owners, though I'm not sure why not.
to have owners, though I'm not sure why not.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/26/2016 04:15 PM, Tomas Vondra wrote:
On 08/27/2016 12:37 AM, Tom Lane wrote:
=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?= <martin@2ndquadrant.com> writes:
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.Surely the index has a dependency on a table, which depends on the
extension?If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.I don't see why that would be stupid (and I'm not sure it's up to us to
just decide it's stupid).
+1, there are a lot of workflow patterns that make sense and don't make
sense depending on your perspective, consider safe mode with MySQL. I
personally think it is stupid too but it also obviously has a huge
useful user base.
Imagine you use extensions to break the application into modules. You
have a "basic" extension, with the table, and a "search" extension
implementing fulltext search on top of that table. Isn't it natural to
keep the gin indexes in the search extension?So the basic--1.0.sql will do something like
CREATE TABLE x ( ...)
and search--1.0.sql would do
CREATE INDEX y ON x USING gin (z);
CREATE FUNCTION search_objects(..) ...because the index and function doing the search are somewhat tightly
coupled. I admit this is just an example and I don't know how many
people use extensions this way, but I don't see why this would be
inherently stupid pattern?
It isn't and in fact shows that as we extend Pg, the more we allow
people flexibility in developing WITHIN the database, the more we will
be able to influence them to do so. That is a good thing.
Or maybe we should just tell everyone, use an ORM it will solve all your
problems. (/sarcasm)
Sincerely,
JD
--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-26 19:37 GMT-03:00 Tom Lane <tgl@sss.pgh.pa.us>:
=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?= <martin@2ndquadrant.com> writes:
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.Surely the index has a dependency on a table, which depends on the
extension?If you mean that you want an extension to create an index on a table that
doesn't belong to it, but it's assuming pre-exists, I think that's just
stupid and we need not support it.
Well, there's still the second pattern I mentioned before (which
actually came up while trying to fix this patch).
Extension creates a table and an index over one of the columns:
CREATE TABLE regress_pg_dump_schema.test_table (
col1 int,
col2 int
);
CREATE INDEX test_extension_index ON regress_pg_dump_schema.test_table (col2);
Later, some application (or a user, doesn't matter really) creates a
second index over col1:
CREATE INDEX test_index ON regress_pg_dump_schema.test_table (col1);
What we are doing (or at least it's what I understand from the code)
is checking if the table depends on an extension, and so we don't dump
it.
We should be able to use the same procedure (and reuse the code we
already have) to decide if an index should be dumped or not. But we
are missing the dependency, and so it's not possible to know that
regress_pg_dump_schema.test_extension_index depends on the extension
and regress_pg_dump_schema.test_index doesn't.
Or is this something we shouldn't support (in that case we should document it).
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Aug 27, 2016 at 8:15 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
On 08/27/2016 12:37 AM, Tom Lane wrote:
=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?= <martin@2ndquadrant.com> writes:
Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.Surely the index has a dependency on a table, which depends on the
extension?If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.I don't see why that would be stupid (and I'm not sure it's up to us to just
decide it's stupid).
Like Tomas, I am not really getting this opposition..
I see the current behavior is documented, and I do understand why global
objects can't be part of the extension, but for indexes it seems to violate
POLA a bit.Is there a reason why we don't want the extension/index dependencies?
I think that we could do a better effort for indexes at least, in the
same way as we do for sequences as both are referenced in pg_class. I
don't know the effort to get that done for < 9.6, but if we can do it
at least for 9.6 and 10, which is where pg_dump is a bit smarter in
the way it deals with dependencies, we should do it.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-29 4:51 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
I see the current behavior is documented, and I do understand why global
objects can't be part of the extension, but for indexes it seems to violate
POLA a bit.Is there a reason why we don't want the extension/index dependencies?
I think that we could do a better effort for indexes at least, in the
same way as we do for sequences as both are referenced in pg_class. I
don't know the effort to get that done for < 9.6, but if we can do it
at least for 9.6 and 10, which is where pg_dump is a bit smarter in
the way it deals with dependencies, we should do it.
ATM I don't have a strong opinion one way or the other regarding the
dependency of indexes and extensions. I believe we have to put more
thought into it, and at the end we might just leave it as it is.
What I do believe is that this requires a separate thread, and if
agreed, a separate patch from this issue.
I'm going to prepare another patch where I'm going to strip the tests
for external indexes which are failing now. They actually fail
correctly as the table they depend on will not be dumped, so it's the
developer/DB designer who has to take care of these things.
If in the near or not so near future we provide a patch to deal with
these missing dependencies, we can easily patch pg_dump so it deals
with this correctly.
Regards,
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.
I stripped off the external index created from the tests as that index
shouldn't be dumped (table it belongs to isn't dumped, so neither
should the index). I also took off a test which was duplicated.
I think this patch is a very good first approach. Future improvements
can be made for indexes, but we need to get the extension dependencies
right first. That could be done later, on a different patch.
Thoughts?
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
pgdump-extension-v4.patchinvalid/octet-stream; name=pgdump-extension-v4.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a5c2d09..63ced1c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1338,9 +1338,6 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout)
static void
selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
{
- if (checkExtensionMembership(&nsinfo->dobj, fout))
- return; /* extension membership overrides all else */
-
/*
* If specific tables are being dumped, do not dump any complete
* namespaces. If specific namespaces are being dumped, dump just those
@@ -1377,6 +1374,12 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
simple_oid_list_member(&schema_exclude_oids,
nsinfo->dobj.catId.oid))
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
+
+ /*
+ * Extension membership overrides all else, and this is done last
+ * to be sure that dump_contains is in a correct state.
+ */
+ (void) checkExtensionMembership(&nsinfo->dobj, fout);
}
/*
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index f02beb3..55f0eb4 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -429,7 +429,211 @@ my %tests = (
unlike => {
no_privs => 1,
pg_dumpall_globals => 1,
- section_post_data => 1, }, },);
+ section_post_data => 1, }, },
+ # Objects included in extension part of a schema created by this extension */
+ 'CREATE TABLE regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QCREATE TABLE test_table (\E
+ \n\s+\Qcol1 integer,\E
+ \n\s+\Qcol2 integer\E
+ \n\);$/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, }, },
+ 'GRANT SELECT ON regress_pg_dump_schema.test_table' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT SELECT ON TABLE test_table TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE SEQUENCE regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QCREATE SEQUENCE test_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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_seq' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT USAGE ON SEQUENCE test_seq TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE TYPE regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QCREATE TYPE test_type AS (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/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, }, },
+ 'GRANT USAGE ON regress_pg_dump_schema.test_type' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON TYPE test_type TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE FUNCTION regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QCREATE FUNCTION test_func() RETURNS integer\E
+ \n\s+\QLANGUAGE sql\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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_func' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_func() TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ 'CREATE AGGREGATE regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QCREATE AGGREGATE test_agg(smallint) (\E
+ \n\s+\QSFUNC = int2_sum,\E
+ \n\s+\QSTYPE = bigint\E
+ \n\);$/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, }, },
+ 'GRANT ALL ON regress_pg_dump_schema.test_agg' => {
+ regexp => qr/^
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\E\n
+ \QGRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;\E\n
+ \QSELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\E
+ $/xms,
+ like => { binary_upgrade => 1, },
+ unlike => {
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ pg_dumpall_globals => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ section_post_data => 1, }, },
+ # Objects not included in extension, part of schema created by extension
+ 'CREATE TABLE regress_pg_dump_schema.external_tab' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE regress_pg_dump_schema.external_tab
+ (col1 int);',
+ regexp => qr/^
+ \QCREATE TABLE external_tab (\E
+ \n\s+\Qcol1 integer\E
+ \n\);$/xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ no_owner => 1,
+ no_privs => 1,
+ schema_only => 1,
+ section_pre_data => 1, },
+ unlike => {
+ pg_dumpall_globals => 1,
+ section_post_data => 1, }, }, );
#########################################
# Create a PG instance to test actually dumping from
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
index c2fe90d..ca9fb18 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
@@ -10,6 +10,8 @@ CREATE TABLE regress_pg_dump_table (
CREATE SEQUENCE regress_pg_dump_seq;
+CREATE SCHEMA regress_pg_dump_schema;
+
GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
@@ -19,3 +21,25 @@ GRANT SELECT(col2) ON regress_pg_dump_table TO regress_dump_test_role;
REVOKE SELECT(col2) ON regress_pg_dump_table FROM regress_dump_test_role;
CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;
+
+-- Create a set of objects that are part of the schema created by
+-- this extension.
+CREATE TABLE regress_pg_dump_schema.test_table (
+ col1 int,
+ col2 int
+);
+GRANT SELECT ON regress_pg_dump_schema.test_table TO regress_dump_test_role;
+
+CREATE SEQUENCE regress_pg_dump_schema.test_seq;
+GRANT USAGE ON regress_pg_dump_schema.test_seq TO regress_dump_test_role;
+
+CREATE TYPE regress_pg_dump_schema.test_type AS (col1 int);
+GRANT USAGE ON TYPE regress_pg_dump_schema.test_type TO regress_dump_test_role;
+
+CREATE FUNCTION regress_pg_dump_schema.test_func () RETURNS int
+AS 'SELECT 1;' LANGUAGE SQL;
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_func() TO regress_dump_test_role;
+
+CREATE AGGREGATE regress_pg_dump_schema.test_agg(int2)
+(SFUNC = int2_sum, STYPE = int8);
+GRANT EXECUTE ON FUNCTION regress_pg_dump_schema.test_agg(int2) TO regress_dump_test_role;
On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.I stripped off the external index created from the tests as that index
shouldn't be dumped (table it belongs to isn't dumped, so neither
should the index). I also took off a test which was duplicated.I think this patch is a very good first approach. Future improvements
can be made for indexes, but we need to get the extension dependencies
right first. That could be done later, on a different patch.Thoughts?
Let's do as you suggest then, and just focus on the schema issue. I
just had an extra look at the patch and it looks fine to me. So the
patch is now switched as ready for committer.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-30 2:02 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.I stripped off the external index created from the tests as that index
shouldn't be dumped (table it belongs to isn't dumped, so neither
should the index). I also took off a test which was duplicated.I think this patch is a very good first approach. Future improvements
can be made for indexes, but we need to get the extension dependencies
right first. That could be done later, on a different patch.Thoughts?
Let's do as you suggest then, and just focus on the schema issue. I
just had an extra look at the patch and it looks fine to me. So the
patch is now switched as ready for committer.
That's great. Thanks for all Michael
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.
Let's do as you suggest then, and just focus on the schema issue. I
just had an extra look at the patch and it looks fine to me. So the
patch is now switched as ready for committer.
Pushed with cosmetic adjustments (mainly, I thought the comment needed
to be much more explicit).
I'm not sure whether we want to try to fix this in older branches.
It would be a significantly larger patch, and maybe more of a behavior
change than we want to make in stable branches. So I'm inclined to call
it done at this point.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers