Re: 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,

On Tue, Jun 18, 2013 at 12:18 PM, Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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

On Sat, Jun 22, 2013 at 3:55 PM, Andrew Dunstan <andrew(at)dunslane(dot)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(at)gmail(dot)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(at)gmail(dot)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