Re: [HACKERS] insufficient qualification of some objects in dump files

[Peter Eisentraut]

On 04/05/2016 09:46 PM, Robert Haas wrote:

On Fri, Mar 18, 2016 at 11:22 AM, Robert Haas  wrote:

On Fri, Mar 18, 2016 at 2:13 AM, Michael Paquier
 wrote:

On Fri, Mar 18, 2016 at 5:38 AM, Tom Lane  wrote:

Given the lack of any other complaints about this, I'm okay with the
approach as presented.  (I haven't read the patch in detail, though.)


FWIW, I am still of the opinion that the last patch sent by Peter is
in a pretty good shape.


Great.  I've marked this patch as "Ready for Committer" in the
CommitFest application.


Peter, are you going to commit this?  Feature freeze is in just a few days.


done



--
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] insufficient qualification of some objects in dump files

[Robert Haas]

On Fri, Mar 18, 2016 at 11:22 AM, Robert Haas  wrote:
> On Fri, Mar 18, 2016 at 2:13 AM, Michael Paquier
>  wrote:
>> On Fri, Mar 18, 2016 at 5:38 AM, Tom Lane  wrote:
>>> Given the lack of any other complaints about this, I'm okay with the
>>> approach as presented.  (I haven't read the patch in detail, though.)
>>
>> FWIW, I am still of the opinion that the last patch sent by Peter is
>> in a pretty good shape.
>
> Great.  I've marked this patch as "Ready for Committer" in the
> CommitFest application.

Peter, are you going to commit this?  Feature freeze is in just a few days.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] insufficient qualification of some objects in dump files

[Tom Lane]

Peter Eisentraut  writes:
> On 2/26/16 1:30 AM, Tom Lane wrote:
>> As the patch is presented, I agree with Peter that it does not really
>> need a format number bump.  The question that has to be answered is
>> whether this solution is good enough?  You could not trust it for
>> automated processing of tags --- it's easy to think of cases in which the
>> schema/object name separation would be ambiguous.  So is the tag really
>> "strictly for human consumption"?  I'm not sure about that.

> Well what are those tags for?  They are not used by pg_restore, so they
> are for users.  My understanding is that the tags help in editing a TOC
> list for use by pg_restore.  What pg_restore actually reads are the
> OIDs, but the tags are there so users can edit the files.  The tags can
> also be used for ad hoc automatic processing.  They are not sufficiently
> delimited and escaped for robustness in all cases, but it can be done if
> you control the inputs and know what to expect.  But this is the same
> problem before and after my patch.

> Both of these cases are helped by my patch, and both of these cases were
> pretty broken (for the object classes in question) before my patch.

Given the lack of any other complaints about this, I'm okay with the
approach as presented.  (I haven't read the patch in detail, though.)

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

Re: [HACKERS] insufficient qualification of some objects in dump files

[Robert Haas]

On Fri, Mar 18, 2016 at 2:13 AM, Michael Paquier
 wrote:
> On Fri, Mar 18, 2016 at 5:38 AM, Tom Lane  wrote:
>> Given the lack of any other complaints about this, I'm okay with the
>> approach as presented.  (I haven't read the patch in detail, though.)
>
> FWIW, I am still of the opinion that the last patch sent by Peter is
> in a pretty good shape.

Great.  I've marked this patch as "Ready for Committer" in the
CommitFest application.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Mar 18, 2016 at 5:38 AM, Tom Lane  wrote:
> Given the lack of any other complaints about this, I'm okay with the
> approach as presented.  (I haven't read the patch in detail, though.)

FWIW, I am still of the opinion that the last patch sent by Peter is
in a pretty good shape.
-- 
Michael


-- 
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] insufficient qualification of some objects in dump files

[Peter Eisentraut]

On 2/26/16 1:30 AM, Tom Lane wrote:
> As the patch is presented, I agree with Peter that it does not really
> need a format number bump.  The question that has to be answered is
> whether this solution is good enough?  You could not trust it for
> automated processing of tags --- it's easy to think of cases in which the
> schema/object name separation would be ambiguous.  So is the tag really
> "strictly for human consumption"?  I'm not sure about that.

Well what are those tags for?  They are not used by pg_restore, so they
are for users.  My understanding is that the tags help in editing a TOC
list for use by pg_restore.  What pg_restore actually reads are the
OIDs, but the tags are there so users can edit the files.  The tags can
also be used for ad hoc automatic processing.  They are not sufficiently
delimited and escaped for robustness in all cases, but it can be done if
you control the inputs and know what to expect.  But this is the same
problem before and after my patch.

Both of these cases are helped by my patch, and both of these cases were
pretty broken (for the object classes in question) before my patch.



-- 
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] insufficient qualification of some objects in dump files

[David Steele]

Hi Peter,

On 2/26/16 1:30 AM, Tom Lane wrote:
> Michael Paquier  writes:
>> On Fri, Feb 26, 2016 at 9:47 AM, Peter Eisentraut  wrote:
>>> Tom thought this might require an archive version dump, but I'm not
>>> sure.  The tags are more of an informational string for human
>>> consumption, not strictly part of the archive format.
> 
>> Hm, the TOC entry, with its tag changed, is part of the dump, and this
>> is written in the archive, but the shape of TocEntry does not change
>> so this is really debatable.
> 
> I had in mind that we would add a separate field for tag's schema name to
> TocEntry, which surely would require an archive format number bump.
> As the patch is presented, I agree with Peter that it does not really
> need a format number bump.  The question that has to be answered is
> whether this solution is good enough?  You could not trust it for
> automated processing of tags --- it's easy to think of cases in which the
> schema/object name separation would be ambiguous.  So is the tag really
> "strictly for human consumption"?  I'm not sure about that.

It looks like there is still some discussion to be had here about
whether a "human readable" solution is enough.

Until that's resolved I've marked this patch "Waiting on Author".

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] insufficient qualification of some objects in dump files

[Tom Lane]

Michael Paquier  writes:
> On Fri, Feb 26, 2016 at 9:47 AM, Peter Eisentraut  wrote:
>> Tom thought this might require an archive version dump, but I'm not
>> sure.  The tags are more of an informational string for human
>> consumption, not strictly part of the archive format.

> Hm, the TOC entry, with its tag changed, is part of the dump, and this
> is written in the archive, but the shape of TocEntry does not change
> so this is really debatable.

I had in mind that we would add a separate field for tag's schema name to
TocEntry, which surely would require an archive format number bump.
As the patch is presented, I agree with Peter that it does not really
need a format number bump.  The question that has to be answered is
whether this solution is good enough?  You could not trust it for
automated processing of tags --- it's easy to think of cases in which the
schema/object name separation would be ambiguous.  So is the tag really
"strictly for human consumption"?  I'm not sure about that.

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

Re: [HACKERS] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Feb 26, 2016 at 9:47 AM, Peter Eisentraut  wrote:
> On 1/29/16 1:24 AM, Michael Paquier wrote:
>>> I think we should amend the archive tag for these kinds of objects to
>>> > include the table name, so it might look like:
>>> >
>>> > 2153; 2604 39696 DEFAULT public test a rolename
>>> >
>>> > Comments?
>> +1. I noticed that this limitation is present for triggers (as you
>> mentioned), constraints, fk constraints and RLS policies which should
>> be completed with a table name.
>
> Thank you for collecting this list.  Attached is a patch for this.

Visibly rules are on the stack as well, I forgot to mention them, and
your updated patch does the job correctly.

> Tom thought this might require an archive version dump, but I'm not
> sure.  The tags are more of an informational string for human
> consumption, not strictly part of the archive format.

Hm, the TOC entry, with its tag changed, is part of the dump, and this
is written in the archive, but the shape of TocEntry does not change
so this is really debatable. It is true that pg_restore -L is able to
work even if the tag output is changed, still now that I think about
that a version bump would be preferrable: what is generated by the
bump is changed after all. The previous version upgrades that are
K_VERS_1_11 or K_VERS_1_6 working on TOC did add new fields in the
structure TocEntry and influenced pg_restore.
-- 
Michael


-- 
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] insufficient qualification of some objects in dump files

[Peter Eisentraut]

On 1/29/16 1:24 AM, Michael Paquier wrote:
>> I think we should amend the archive tag for these kinds of objects to
>> > include the table name, so it might look like:
>> >
>> > 2153; 2604 39696 DEFAULT public test a rolename
>> >
>> > Comments?
> +1. I noticed that this limitation is present for triggers (as you
> mentioned), constraints, fk constraints and RLS policies which should
> be completed with a table name.

Thank you for collecting this list.  Attached is a patch for this.

Tom thought this might require an archive version dump, but I'm not
sure.  The tags are more of an informational string for human
consumption, not strictly part of the archive format.

From 44a23b4e960040f1e5de7a0ae0ecb6de432c4027 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 25 Feb 2016 18:56:37 -0500
Subject: [PATCH] pg_dump: Add table qualifications to some tags

Some object types have names that are only unique for one table.  But
for those we generally didn't put the table name into the dump TOC tag.
So it was impossible to identify these objects if the same name was used
for multiple tables.  This affects policies, column defaults,
constraints, triggers, and rules.

Fix by adding the table name to the TOC tag, so that it now reads
"$schema $table $object".
---
 src/bin/pg_dump/pg_dump.c | 42 ++
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 64c2673..8bb134d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -3057,6 +3057,7 @@ dumpPolicy(Archive *fout, PolicyInfo *polinfo)
 	PQExpBuffer query;
 	PQExpBuffer delqry;
 	const char *cmd;
+	char	   *tag;
 
 	if (dopt->dataOnly)
 		return;
@@ -3124,8 +3125,10 @@ dumpPolicy(Archive *fout, PolicyInfo *polinfo)
 	appendPQExpBuffer(delqry, "DROP POLICY %s", fmtId(polinfo->polname));
 	appendPQExpBuffer(delqry, " ON %s;\n", fmtId(tbinfo->dobj.name));
 
+	tag = psprintf("%s %s", tbinfo->dobj.name, polinfo->dobj.name);
+
 	ArchiveEntry(fout, polinfo->dobj.catId, polinfo->dobj.dumpId,
- polinfo->dobj.name,
+ tag,
  polinfo->dobj.namespace->dobj.name,
  NULL,
  tbinfo->rolname, false,
@@ -3134,6 +3137,7 @@ dumpPolicy(Archive *fout, PolicyInfo *polinfo)
  NULL, 0,
  NULL, NULL);
 
+	free(tag);
 	destroyPQExpBuffer(query);
 	destroyPQExpBuffer(delqry);
 }
@@ -14626,6 +14630,7 @@ dumpAttrDef(Archive *fout, AttrDefInfo *adinfo)
 	int			adnum = adinfo->adnum;
 	PQExpBuffer q;
 	PQExpBuffer delq;
+	char	   *tag;
 
 	/* Skip if table definition not to be dumped */
 	if (!tbinfo->dobj.dump || dopt->dataOnly)
@@ -14654,8 +14659,10 @@ dumpAttrDef(Archive *fout, AttrDefInfo *adinfo)
 	appendPQExpBuffer(delq, "ALTER COLUMN %s DROP DEFAULT;\n",
 	  fmtId(tbinfo->attnames[adnum - 1]));
 
+	tag = psprintf("%s %s", tbinfo->dobj.name, tbinfo->attnames[adnum - 1]);
+
 	ArchiveEntry(fout, adinfo->dobj.catId, adinfo->dobj.dumpId,
- tbinfo->attnames[adnum - 1],
+ tag,
  tbinfo->dobj.namespace->dobj.name,
  NULL,
  tbinfo->rolname,
@@ -14664,6 +14671,7 @@ dumpAttrDef(Archive *fout, AttrDefInfo *adinfo)
  NULL, 0,
  NULL, NULL);
 
+	free(tag);
 	destroyPQExpBuffer(q);
 	destroyPQExpBuffer(delq);
 }
@@ -14804,6 +14812,7 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 	TableInfo  *tbinfo = coninfo->contable;
 	PQExpBuffer q;
 	PQExpBuffer delq;
+	char	   *tag = NULL;
 
 	/* Skip if not to be dumped */
 	if (!coninfo->dobj.dump || dopt->dataOnly)
@@ -14897,8 +14906,10 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 		appendPQExpBuffer(delq, "DROP CONSTRAINT %s;\n",
 		  fmtId(coninfo->dobj.name));
 
+		tag = psprintf("%s %s", tbinfo->dobj.name, coninfo->dobj.name);
+
 		ArchiveEntry(fout, coninfo->dobj.catId, coninfo->dobj.dumpId,
-	 coninfo->dobj.name,
+	 tag,
 	 tbinfo->dobj.namespace->dobj.name,
 	 indxinfo->tablespace,
 	 tbinfo->rolname, false,
@@ -14930,8 +14941,10 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 		appendPQExpBuffer(delq, "DROP CONSTRAINT %s;\n",
 		  fmtId(coninfo->dobj.name));
 
+		tag = psprintf("%s %s", tbinfo->dobj.name, coninfo->dobj.name);
+
 		ArchiveEntry(fout, coninfo->dobj.catId, coninfo->dobj.dumpId,
-	 coninfo->dobj.name,
+	 tag,
 	 tbinfo->dobj.namespace->dobj.name,
 	 NULL,
 	 tbinfo->rolname, false,
@@ -14965,8 +14978,10 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 			appendPQExpBuffer(delq, "DROP CONSTRAINT %s;\n",
 			  fmtId(coninfo->dobj.name));
 
+			tag = psprintf("%s %s", tbinfo->dobj.name, coninfo->dobj.name);
+
 			ArchiveEntry(fout, coninfo->dobj.catId, coninfo->dobj.dumpId,
-		 coninfo->dobj.name,
+		 tag,
 		 tbinfo->dobj.namespace->dobj.name,
 		 NULL,
 		 tbinfo->rolname, false,
@@ -15001,8 +15016,10 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 			appendPQExpBuffer(delq, "DROP CONSTRAINT %s;\n",
 		

Re: [HACKERS] insufficient qualification of some objects in dump files

[Alvaro Herrera]

Peter Eisentraut wrote:
> Some dump objects whose names are not unique on a schema level have
> insufficient details in the dump TOC.  For example, a column default
> might have a TOC entry like this:
> 
> 2153; 2604 39696 DEFAULT public a rolename

> I think we should amend the archive tag for these kinds of objects to
> include the table name, so it might look like:
> 
> 2153; 2604 39696 DEFAULT public test a rolename
> 
> Comments?

If we're changing anything, I wonder if it makes sense to go for adding
the object identity there.  Perhaps that would make the whole thing be a
little more regular, rather than have to count elements depending on
object type.  So in this case it'd be

2153; 2604 39696 DEFAULT public.test.a rolename

-- 
Álvaro Herrerahttp://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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
> Some dump objects whose names are not unique on a schema level have
> insufficient details in the dump TOC.  For example, a column default
> might have a TOC entry like this:
>
> 2153; 2604 39696 DEFAULT public a rolename
>
> Note that this only contains the schema name and the column name, but
> not the table name.  So this makes it impossible to filter the TOC by
> text search in situations like this.
>
> It looks like other object types such as triggers have the same problem.
>
> I think we should amend the archive tag for these kinds of objects to
> include the table name, so it might look like:
>
> 2153; 2604 39696 DEFAULT public test a rolename
>
> Comments?

+1. I noticed that this limitation is present for triggers (as you
mentioned), constraints, fk constraints and RLS policies which should
be completed with a table name.
-- 
Michael


-- 
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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Jan 29, 2016 at 3:51 PM, Tom Lane  wrote:
>
> Michael Paquier  writes:
> > On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
> >> I think we should amend the archive tag for these kinds of objects to
> >> include the table name, so it might look like:
> >>
> >> 2153; 2604 39696 DEFAULT public test a rolename
>
> > +1. I noticed that this limitation is present for triggers (as you
> > mentioned), constraints, fk constraints and RLS policies which should
> > be completed with a table name.
>
> How can we do this without an archive format version bump ... or were
> you assuming that that would be an acceptable price?  (It's not like
> we haven't done those before, so maybe it is.)

Yes, I am assuming that's worth the price, many people run similar
relation schemas on the same database with different schema names. And
Peter has a point that the current format can be confusing for the
user. Sorry if I sounded like it was a bug that should be backpatched
or something similar, I don't mean that.
-- 
Michael


-- 
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] insufficient qualification of some objects in dump files

[Tom Lane]

Michael Paquier  writes:
> On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
>> I think we should amend the archive tag for these kinds of objects to
>> include the table name, so it might look like:
>> 
>> 2153; 2604 39696 DEFAULT public test a rolename

> +1. I noticed that this limitation is present for triggers (as you
> mentioned), constraints, fk constraints and RLS policies which should
> be completed with a table name.

How can we do this without an archive format version bump ... or were
you assuming that that would be an acceptable price?  (It's not like
we haven't done those before, so maybe it is.)

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