Thread: 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 timezone,
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
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
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
Attachment
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
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
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