Re: [HACKERS] PUBLICATIONS and pg_dump

2017-03-21 Thread Peter Eisentraut 
On 2/26/17 14:25, Petr Jelinek wrote:
> Yeah that was oversight in initial patch, publications and their
> membership was supposed to be dumped only when table filter is not used.
> I mistakenly made it check for data_only instead of using the
> selectDumpableObject machinery.

Committed.

-- 
Peter Eisentraut  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

Re: [HACKERS] PUBLICATIONS and pg_dump

2017-02-26 Thread Petr Jelinek 
On 08/02/17 05:02, Stephen Frost wrote:
> Peter,
> 
> On Tue, Feb 7, 2017 at 22:49 Peter Eisentraut
>  > wrote:
> 
> On 2/7/17 3:19 PM, Stephen Frost wrote:
> > I understand that this is a bit complicated, but I would have thought
> > we'd do something similar to what is done for DEFAULT PRIVILEGES,
> where
> > we include the "global" default privileges when we are doing a dump of
> > "everything", but if we're dumping a specific schema then we only
> > include the default privileges directly associated with that schema.
> >
> > Perhaps we need to include publications which are specific to a
> > particular table, but the current logic of, essentially, "always
> include
> > all publications" does not seem to make a lot of sense to me.
> 
> I think it would be sensible to refine it along those lines.
> 
> 
> Great!  I've added it to the open items list for PG10. 
> 

Yeah that was oversight in initial patch, publications and their
membership was supposed to be dumped only when table filter is not used.
I mistakenly made it check for data_only instead of using the
selectDumpableObject machinery.

Fix attached.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 907dcca71712558ba954a28cdf65ccdd77473bfe Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sun, 26 Feb 2017 20:12:26 +0100
Subject: [PATCH] Don't dump publications with pg_dump -t

---
 src/bin/pg_dump/pg_dump.c| 29 +
 src/bin/pg_dump/t/002_pg_dump.pl | 17 +
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7273ec8..8f7245d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1613,6 +1613,23 @@ selectDumpableExtension(ExtensionInfo *extinfo, DumpOptions *dopt)
 }
 
 /*
+ * selectDumpablePublicationTable: policy-setting subroutine
+ *		Mark a publication table as to be dumped or not
+ *
+ * Publication tables have schemas but those should be ignored in decitions
+ * making as publications are only dumped when we are dumping everything.
+ */
+static void
+selectDumpablePublicationTable(DumpableObject *dobj, Archive *fout)
+{
+	if (checkExtensionMembership(dobj, fout))
+		return;	/* extension membership overrides all else */
+
+	dobj->dump = fout->dopt->include_everything ?
+		DUMP_COMPONENT_ALL : DUMP_COMPONENT_NONE;
+}
+
+/*
  * selectDumpableObject: policy-setting subroutine
  *		Mark a generic dumpable object as to be dumped or not
  *
@@ -3389,6 +3406,9 @@ getPublications(Archive *fout)
 		if (strlen(pubinfo[i].rolname) == 0)
 			write_msg(NULL, "WARNING: owner of publication \"%s\" appears to be invalid\n",
 	  pubinfo[i].dobj.name);
+
+		/* Decide whether we want to dump it */
+		selectDumpableObject(&(pubinfo[i].dobj), fout);
 	}
 	PQclear(res);
 
@@ -3402,11 +3422,10 @@ getPublications(Archive *fout)
 static void
 dumpPublication(Archive *fout, PublicationInfo *pubinfo)
 {
-	DumpOptions *dopt = fout->dopt;
 	PQExpBuffer delq;
 	PQExpBuffer query;
 
-	if (dopt->dataOnly)
+	if (!(pubinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
 		return;
 
 	delq = createPQExpBuffer();
@@ -3534,6 +3553,9 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
 			pubrinfo[j].dobj.namespace = tbinfo->dobj.namespace;
 			pubrinfo[j].pubname = pg_strdup(PQgetvalue(res, j, i_pubname));
 			pubrinfo[j].pubtable = tbinfo;
+
+			/* Decide whether we want to dump it */
+			selectDumpablePublicationTable(&(pubrinfo[j].dobj), fout);
 		}
 		PQclear(res);
 	}
@@ -3547,12 +3569,11 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
 static void
 dumpPublicationTable(Archive *fout, PublicationRelInfo *pubrinfo)
 {
-	DumpOptions *dopt = fout->dopt;
 	TableInfo  *tbinfo = pubrinfo->pubtable;
 	PQExpBuffer query;
 	char	   *tag;
 
-	if (dopt->dataOnly)
+	if (!(pubrinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
 		return;
 
 	tag = psprintf("%s %s", pubrinfo->pubname, tbinfo->dobj.name);
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index f73bf89..40509a4 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2242,14 +2242,14 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 			exclude_test_table   => 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_post_data=> 1,
-			test_schema_plus_blobs   => 1, },
+			section_post_data=> 1, },
 		unlike => {
 			section_pre_data => 1,
+			only_dump_test_table => 1,
+			test_schema_plus_blobs   => 1,
+

Re: [HACKERS] PUBLICATIONS and pg_dump

2017-02-07 Thread Stephen Frost 
Peter,

On Tue, Feb 7, 2017 at 22:49 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2/7/17 3:19 PM, Stephen Frost wrote:
> > I understand that this is a bit complicated, but I would have thought
> > we'd do something similar to what is done for DEFAULT PRIVILEGES, where
> > we include the "global" default privileges when we are doing a dump of
> > "everything", but if we're dumping a specific schema then we only
> > include the default privileges directly associated with that schema.
> >
> > Perhaps we need to include publications which are specific to a
> > particular table, but the current logic of, essentially, "always include
> > all publications" does not seem to make a lot of sense to me.
>
> I think it would be sensible to refine it along those lines.


Great!  I've added it to the open items list for PG10.

Thanks!

Stephen

Re: [HACKERS] PUBLICATIONS and pg_dump

2017-02-07 Thread Peter Eisentraut 
On 2/7/17 3:19 PM, Stephen Frost wrote:
> I understand that this is a bit complicated, but I would have thought
> we'd do something similar to what is done for DEFAULT PRIVILEGES, where
> we include the "global" default privileges when we are doing a dump of
> "everything", but if we're dumping a specific schema then we only
> include the default privileges directly associated with that schema.
> 
> Perhaps we need to include publications which are specific to a
> particular table, but the current logic of, essentially, "always include
> all publications" does not seem to make a lot of sense to me.

I think it would be sensible to refine it along those lines.

-- 
Peter Eisentraut  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

[HACKERS] PUBLICATIONS and pg_dump

2017-02-07 Thread Stephen Frost 
Peter,

* Peter Eisentraut (pete...@gmx.net) wrote:
> Logical replication
> 
> - Add PUBLICATION catalogs and DDL
> - Add SUBSCRIPTION catalog and DDL
> - Define logical replication protocol and output plugin
> - Add logical replication workers

I think we need to have a bit more discussion regarding where
publications (and maybe subscriptions... not sure on that though) fit
when it comes to pg_dump.

In particular, I'm trying to clean up the pg_dump TAP tests and am
finding things I wouldn't have expected.  For example, publications
appear to be included in pretty much every pg_dump output, no matter if
a specific schema or even table was explicitly called for, or if that
publication or subscription was explicitly associated with that table.

The example I'm playing with is:

CREATE PUBLICATION pub2 WITH (PUBLISH INSERT, PUBLISH UPDATE, PUBLISH
DELETE);

and a simple:

pg_dump -n public -t t1

Will end up including the CREATE PUBLICATION command.

In fact, I'm not entirely sure how to have it not included in pg_dump's
output.

I understand that this is a bit complicated, but I would have thought
we'd do something similar to what is done for DEFAULT PRIVILEGES, where
we include the "global" default privileges when we are doing a dump of
"everything", but if we're dumping a specific schema then we only
include the default privileges directly associated with that schema.

Perhaps we need to include publications which are specific to a
particular table, but the current logic of, essentially, "always include
all publications" does not seem to make a lot of sense to me.

I'm happy to be corrected if I've grossly misunderstood something here,
of course.

Thanks!

Stephen


signature.asc
Description: Digital signature