dump difference between 9.3 and master after upgrade
As I was updating my cross version upgrade tester to include support for
the 9.3 branch, I noted this dump difference between the dump of the
original 9.3 database and the dump of the converted database, which
looks odd. Is it correct?
cheers
andrew
--- /home/bf/bfr/root/upgrade/HEAD/origin-REL9_3_STABLE.sql 2013-06-18
08:45:22.518761202 -0400
+++ /home/bf/bfr/root/upgrade/HEAD/converted-REL9_3_STABLE-to-HEAD.sql
2013-06-18 08:46:01.648782111 -0400
@@ -385,6 +385,7 @@
-- CREATE FOREIGN TABLE ft1 (
+ "........pg.dropped.1........" integer,
c1 integer NOT NULL,
c2 integer NOT NULL,
c3 text,
@@ -413,6 +414,7 @@
CREATE FOREIGN TABLE ft2 (
c1 integer NOT NULL,
c2 integer NOT NULL,
+ "........pg.dropped.3........" integer,
c3 text,
c4 timestamp with time zone,
c5 timestamp without time zone,--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 18, 2013 at 12:18 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
As I was updating my cross version upgrade tester to include support for the
9.3 branch, I noted this dump difference between the dump of the original
9.3 database and the dump of the converted database, which looks odd. Is it
correct?
It looks sketchy to me, but I'm not sure exactly what you're comparing.
--
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
On 06/20/2013 10:43 AM, Robert Haas wrote:
On Tue, Jun 18, 2013 at 12:18 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
As I was updating my cross version upgrade tester to include support for the
9.3 branch, I noted this dump difference between the dump of the original
9.3 database and the dump of the converted database, which looks odd. Is it
correct?It looks sketchy to me, but I'm not sure exactly what you're comparing.
Essentially, cross version upgrade testing runs pg_dumpall from the new
version on the old cluster, runs pg_upgrade, and then runs pg_dumpall on
the upgraded cluster, and compares the two outputs. This is what we get
when the new version is HEAD and the old version is 9.3.
The reason this hasn't been caught by the standard same-version upgrade
tests is that this module uses a more extensive cluster, which has had
not only the core regression tests run but also all the contrib and pl
regression tests, and this problem seems to be exposed by the
postgres_fdw tests.
At first glance to me like pg_dump in binary-upgrade mode is not
suppressing something that it should be suppressing.
The
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/20/2013 11:16 AM, I wrote:
On 06/20/2013 10:43 AM, Robert Haas wrote:
On Tue, Jun 18, 2013 at 12:18 PM, Andrew Dunstan
<andrew@dunslane.net> wrote:As I was updating my cross version upgrade tester to include support
for the
9.3 branch, I noted this dump difference between the dump of the
original
9.3 database and the dump of the converted database, which looks
odd. Is it
correct?It looks sketchy to me, but I'm not sure exactly what you're comparing.
Essentially, cross version upgrade testing runs pg_dumpall from the
new version on the old cluster, runs pg_upgrade, and then runs
pg_dumpall on the upgraded cluster, and compares the two outputs. This
is what we get when the new version is HEAD and the old version is 9.3.The reason this hasn't been caught by the standard same-version
upgrade tests is that this module uses a more extensive cluster, which
has had not only the core regression tests run but also all the
contrib and pl regression tests, and this problem seems to be exposed
by the postgres_fdw tests.At first glance to me like pg_dump in binary-upgrade mode is not
suppressing something that it should be suppressing.
Yeah, after examination I don't see why we should output anything for
dropped columns of a foreign table in binary upgrade mode. This looks to
me like it's been a bug back to 9.1 where we introduced foreign tables.
I think something like the attached should fix it, although I'm not sure
if that's the right place for the fix.
cheers
andrew
Attachments:
upgrade-foreign-fix.patchtext/x-patch; name=upgrade-foreign-fix.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ec956ad..b25b475 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6670,7 +6670,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* such a column it will mistakenly get pg_attribute.attislocal set to true.)
* However, in binary_upgrade mode, we must print all such columns anyway and
* fix the attislocal/attisdropped state later, so as to keep control of the
- * physical column order.
+ * physical column order, unless it's a Foreign Table.
*
* This function exists because there are scattered nonobvious places that
* must be kept in sync with this decision.
@@ -6679,7 +6679,7 @@ bool
shouldPrintColumn(TableInfo *tbinfo, int colno)
{
if (binary_upgrade)
- return true;
+ return (tbinfo->relkind != RELKIND_FOREIGN_TABLE || !tbinfo->attisdropped[colno]);
return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
}
On Sat, Jun 22, 2013 at 3:55 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
Essentially, cross version upgrade testing runs pg_dumpall from the new
version on the old cluster, runs pg_upgrade, and then runs pg_dumpall on the
upgraded cluster, and compares the two outputs. This is what we get when the
new version is HEAD and the old version is 9.3.The reason this hasn't been caught by the standard same-version upgrade
tests is that this module uses a more extensive cluster, which has had not
only the core regression tests run but also all the contrib and pl
regression tests, and this problem seems to be exposed by the postgres_fdw
tests.At first glance to me like pg_dump in binary-upgrade mode is not
suppressing something that it should be suppressing.Yeah, after examination I don't see why we should output anything for
dropped columns of a foreign table in binary upgrade mode. This looks to me
like it's been a bug back to 9.1 where we introduced foreign tables.I think something like the attached should fix it, although I'm not sure if
that's the right place for the fix.
We probably do need to preserve attribute numbers across an upgrade,
even for foreign tables. I think those could make their way into
other places. Consider:
rhaas=# create foreign data wrapper dummy;
CREATE FOREIGN DATA WRAPPER
rhaas=# create server s1 foreign data wrapper dummy;
CREATE SERVER
rhaas=# create foreign table ft1 (a int, b text) server s1;
CREATE FOREIGN TABLE
rhaas=# create table sam (x ft1);
CREATE TABLE
If the new and old clusters don't agree on the attribute numbers for
ft1, post-upgrade attempts to access sam.x will likely crash the
server.
--
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
Robert Haas <robertmhaas@gmail.com> writes:
We probably do need to preserve attribute numbers across an upgrade,
even for foreign tables. I think those could make their way into
other places.
Hm ... this argument would also apply to composite types; do we get it
right for those?
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 06/24/2013 03:39 PM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
We probably do need to preserve attribute numbers across an upgrade,
even for foreign tables. I think those could make their way into
other places.Hm ... this argument would also apply to composite types; do we get it
right for those?
Yes we do. It's handled at pg_dump.c starting about line 8936.
It looks like we need to add cases of foreign tables to the block that
starts around line 13117.
We should also have a test of this in the core regression tests so that
doing the wrong thing might be caught by regular upgrade testing.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers