Re: [HACKERS] [COMMITTERS] pgsql: Add pg_sequence system catalog

[Peter Eisentraut]

On 1/24/17 10:23 AM, Stephen Frost wrote:
> It wouldn't hurt to add a comment as to why things are so different in
> PG10 than other versions, ie:
> 
> /*
>  * In PG10, sequence meta-data was moved into pg_sequence, so switch to
>  * the pg_catalog schema instead of operating in a user schema and pull
>  * the necessary meta-data from there.
>  */

I've committed this, but I've put the comment in the old sections and
explained how they are different from the new style, instead of vice versa.

-- 
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] [COMMITTERS] pgsql: Add pg_sequence system catalog

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 1/19/17 11:03 AM, Stephen Frost wrote:
> > I'd suggest using our usual approach in pg_dump, which is matching based
> > on the OID, like so:
> > 
> > WHERE c.oid = '%u'::oid
> > 
> > The OID is in: tbinfo->dobj.catId.oid
> > 
> > Also, you should move the selectSourceSchema() into the per-version
> > branches and set it to 'pg_catalog' for PG10 and up, which would allow
> > you to avoid having to qualify the table names, et al.
> 
> Does the attached patch look correct to you?

On a one-over, yes, that looks correct and avoids the issue of running
in a user's schema.

It wouldn't hurt to add a comment as to why things are so different in
PG10 than other versions, ie:

/*
 * In PG10, sequence meta-data was moved into pg_sequence, so switch to
 * the pg_catalog schema instead of operating in a user schema and pull
 * the necessary meta-data from there.
 */

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Add pg_sequence system catalog

[Peter Eisentraut]

On 1/19/17 11:03 AM, Stephen Frost wrote:
> I'd suggest using our usual approach in pg_dump, which is matching based
> on the OID, like so:
> 
> WHERE c.oid = '%u'::oid
> 
> The OID is in: tbinfo->dobj.catId.oid
> 
> Also, you should move the selectSourceSchema() into the per-version
> branches and set it to 'pg_catalog' for PG10 and up, which would allow
> you to avoid having to qualify the table names, et al.

Does the attached patch look correct to you?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 96820dd460e9fe842a608d99e7a1da2f8b9ce67d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 24 Jan 2017 10:03:25 -0500
Subject: [PATCH] pg_dump: Fix some schema issues when dumping sequences

In the new code for selecting sequence data from pg_sequence, set the
schema to pg_catalog instead of the sequences own schema, and refer to
the sequence by OID instead of name, which was missing a schema
qualification.

Reported-by: Stephen Frost 
---
 src/bin/pg_dump/pg_dump.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e3cca62bf7..b28b7e42d4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15873,14 +15873,14 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
 	PQExpBuffer delqry = createPQExpBuffer();
 	PQExpBuffer labelq = createPQExpBuffer();
 
-	/* Make sure we are in proper schema */
-	selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
-
 	snprintf(bufm, sizeof(bufm), INT64_FORMAT, PG_INT64_MIN);
 	snprintf(bufx, sizeof(bufx), INT64_FORMAT, PG_INT64_MAX);
 
 	if (fout->remoteVersion >= 10)
 	{
+		/* Make sure we are in proper schema */
+		selectSourceSchema(fout, "pg_catalog");
+
 		appendPQExpBuffer(query,
 		  "SELECT seqstart, seqincrement, "
 		  "CASE WHEN seqincrement > 0 AND seqmax = %s THEN NULL "
@@ -15894,12 +15894,15 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
 		  "seqcache, seqcycle "
 		  "FROM pg_class c "
 		  "JOIN pg_sequence s ON (s.seqrelid = c.oid) "
-		  "WHERE relname = ",
-		  bufx, bufm);
-		appendStringLiteralAH(query, tbinfo->dobj.name, fout);
+		  "WHERE c.oid = '%u'::oid",
+		  bufx, bufm,
+		  tbinfo->dobj.catId.oid);
 	}
 	else if (fout->remoteVersion >= 80400)
 	{
+		/* Make sure we are in proper schema */
+		selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
+
 		appendPQExpBuffer(query,
 		  "SELECT start_value, increment_by, "
    "CASE WHEN increment_by > 0 AND max_value = %s THEN NULL "
@@ -15916,6 +15919,9 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
 	}
 	else
 	{
+		/* Make sure we are in proper schema */
+		selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
+
 		appendPQExpBuffer(query,
 		  "SELECT 0 AS start_value, increment_by, "
    "CASE WHEN increment_by > 0 AND max_value = %s THEN NULL "
-- 
2.11.0


-- 
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] [COMMITTERS] pgsql: Add pg_sequence system catalog

[Stephen Frost]

Peter, all,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Peter Eisentraut (pete...@gmx.net) wrote:
> > Add pg_sequence system catalog
> 
> The query this added to dumpSequence() seems to think that sequence
> names are unique across databases:

Just FYI, I've added this to the PG10 open items list so it doesn't get
forgotten (and so I can swap it out of my head and into a wiki):

https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Add pg_sequence system catalog

[Stephen Frost]

Peter,

* Peter Eisentraut (pete...@gmx.net) wrote:
> Add pg_sequence system catalog

The query this added to dumpSequence() seems to think that sequence
names are unique across databases:

appendPQExpBuffer(query,
  "SELECT seqstart, seqincrement, "
  "CASE WHEN seqincrement > 0 AND seqmax = %s THEN NULL "
  " WHEN seqincrement < 0 AND seqmax = -1 THEN NULL "
  " ELSE seqmax "
  "END AS seqmax, "
  "CASE WHEN seqincrement > 0 AND seqmin = 1 THEN NULL "
  " WHEN seqincrement < 0 AND seqmin = %s THEN NULL "
  " ELSE seqmin "
  "END AS seqmin, "
  "seqcache, seqcycle "
  "FROM pg_class c "
  "JOIN pg_sequence s ON (s.seqrelid = c.oid) "
  "WHERE relname = ",
  bufx, bufm);
appendStringLiteralAH(query, tbinfo->dobj.name, fout);

Note that tbinfo->dobj.name contains just the name, and trying to match
based on just 'relname' isn't going to work since sequences with the
same name can exist in difference schemas.

I'd suggest using our usual approach in pg_dump, which is matching based
on the OID, like so:

WHERE c.oid = '%u'::oid

The OID is in: tbinfo->dobj.catId.oid

Also, you should move the selectSourceSchema() into the per-version
branches and set it to 'pg_catalog' for PG10 and up, which would allow
you to avoid having to qualify the table names, et al.  As is, this
query will also fail if a user has created a 'pg_class' or
'pg_sequence' table in their schema.

Thanks!

Stephen


signature.asc
Description: Digital signature