Re: Possible Bug in pg_upgrade

Beginning with commit 002c105a0706bd1c1e939fe0f47ecdceeae6c52d
pg_upgrade will fail if there are orphaned temp tables in the current
database with the message 'old and new databases "postgres" have a
different number of relations'

On line 41 of pg_upgrade/info.c pg_upgrade checks that the number of
relations are the same but includes orphaned temp tables in the comparison.

Is this expected behavior? If so is there a more detailed error message
that can be added explain the cause of the failure? It wasn't evident
until modified pg_upgrade to show the missing oid's and recognized them
as temp tables with oid2name.

Dave Byrne

Phone: (310) 526-5000
Direct: (310) 526-5021
Email: DByrne(at)mdb(dot)com<mailto:DByrne(at)mdb(dot)com>

MDB CAPITAL GROUP LLC
Seeing Value Others Do Not, Creating Value Others Can Not.

401 Wilshire Boulevard, Suite 1020
Santa Monica, California 90401
www.mdb.com<http://www.mdb.com>

[MDB Capital Group]<http://www.mdb.com>
The IP Investment Bank

Dave Byrne <dbyrne(at)mdb(dot)com> writes:
> Beginning with commit 002c105a0706bd1c1e939fe0f47ecdceeae6c52d
> pg_upgrade will fail if there are orphaned temp tables in the current
> database with the message 'old and new databases "postgres" have a
> different number of relations'

> On line 41 of pg_upgrade/info.c pg_upgrade checks that the number of
> relations are the same but includes orphaned temp tables in the comparison.

> Is this expected behavior?

Seems like an oversight.

regards, tom lane

Attached is a patch that skips orphaned temporary relations in pg_upgrade if they are lingering around. It works for 9.0 -> 9.1 upgrades, however I wasn't able to tell when pg_class.relistemp was added so if it was unavailable in versions prior to 9.0 an additional check will have to be added.

Thanks
Dave Byrne

-----Original Message-----
From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
Sent: Wednesday, August 10, 2011 12:29 PM
To: Dave Byrne
Cc: pgsql-hackers(at)postgresql(dot)org; Bruce Momjian
Subject: Re: [HACKERS] Possible Bug in pg_upgrade

Dave Byrne <dbyrne(at)mdb(dot)com> writes:
> Beginning with commit 002c105a0706bd1c1e939fe0f47ecdceeae6c52d
> pg_upgrade will fail if there are orphaned temp tables in the current
> database with the message 'old and new databases "postgres" have a
> different number of relations'

> On line 41 of pg_upgrade/info.c pg_upgrade checks that the number of
> relations are the same but includes orphaned temp tables in the comparison.

> Is this expected behavior?

Seems like an oversight.

regards, tom lane

Dave Byrne <dbyrne(at)mdb(dot)com> writes:
> Attached is a patch that skips orphaned temporary relations in pg_upgrade if they are lingering around. It works for 9.0 -> 9.1 upgrades, however I wasn't able to tell when pg_class.relistemp was added so if it was unavailable in versions prior to 9.0 an additional check will have to be added.

I'm inclined to think the correct fix is to revert the assumption that
the old and new databases contain exactly the same number of tables ...
that seems to have a lot of potential failure modes besides this one.

regards, tom lane

On ons, 2011-08-10 at 18:53 -0400, Tom Lane wrote:
> Dave Byrne <dbyrne(at)mdb(dot)com> writes:
> > Attached is a patch that skips orphaned temporary relations in pg_upgrade if they are lingering around. It works for 9.0 -> 9.1 upgrades, however I wasn't able to tell when pg_class.relistemp was added so if it was unavailable in versions prior to 9.0 an additional check will have to be added.
>
> I'm inclined to think the correct fix is to revert the assumption that
> the old and new databases contain exactly the same number of tables ...
> that seems to have a lot of potential failure modes besides this one.

It's basically checking whether pg_dump -s worked. That doesn't seem
like a good use of time.

On 08/11/2011 12:02 AM, Peter Eisentraut wrote:
> On ons, 2011-08-10 at 18:53 -0400, Tom Lane wrote:
>> Dave Byrne<dbyrne(at)mdb(dot)com> writes:
>>> Attached is a patch that skips orphaned temporary relations in pg_upgrade if they are lingering around. It works for 9.0 -> 9.1 upgrades, however I wasn't able to tell when pg_class.relistemp was added so if it was unavailable in versions prior to 9.0 an additional check will have to be added.
>> I'm inclined to think the correct fix is to revert the assumption that
>> the old and new databases contain exactly the same number of tables ...
>> that seems to have a lot of potential failure modes besides this one.
> It's basically checking whether pg_dump -s worked. That doesn't seem
> like a good use of time.
>
If anyone has a suggestion for a better approach I'm happy to work on it
and amend the patch. It is certainly is a corner case but it bit me when
preparing for the upcoming 9.1 release. I would imagine others will hit
it as well.

Dave Byrne wrote:
> Beginning with commit 002c105a0706bd1c1e939fe0f47ecdceeae6c52d
> pg_upgrade will fail if there are orphaned temp tables in the current
> database with the message 'old and new databases "postgres" have a
> different number of relations'
>
> On line 41 of pg_upgrade/info.c pg_upgrade checks that the number of
> relations are the same but includes orphaned temp tables in the comparison.
>
> Is this expected behavior? If so is there a more detailed error message
> that can be added explain the cause of the failure? It wasn't evident
> until modified pg_upgrade to show the missing oid's and recognized them
> as temp tables with oid2name.

Thanks for your report and patch.

Let me give some background on pg_upgrade to explain what is happening.
Pg_upgrade uses two C arrays to store information about tables and
indexes for the old and new clusters. It is not possible to store this
information in a database because both clusters are down when pg_upgrade
needs to use this information.

In pre-9.1 pg_upgrade, pg_upgrade did a sequential scan of the arrays
looking for a match between old and new cluster objects. This was
reported as slow for databases with many objects, and I could reproduce
the slowness. I added some caching in 9.0 but the real solution for 9.1
was to assume a one-to-one mapping between the old and new C arrays,
i.e. the 5th entry in the old cluster array is the same as the 5th
element in the new cluster array.

I knew this was risky but was the right solution so it doesn't surprise
me you found a failure. pg_upgrade checks that the size of the two
arrays in the same and also checks that each element matches --- the
former is what generated your error.

Now, about the cause. I had not anticipated that orphaned temp objects
could exist in either cluster. In fact, this case would have generated
an error in 9.0 as well, but with a different error message.

Looking futher, pg_upgrade has to use the same object filter as
pg_dump, and pg_dump uses this C test:

pg_dump.c: else if (strncmp(nsinfo->dobj.name, "pg_", 3) == 0 ||

pg_dumpall uses this filter:

"WHERE spcname !~ '^pg_' "

The problem is that the filter used by pg_upgrade only excluded
pg_catalog, not pg_temp* as well.

I have developed the attached two patches, one for 9.0, and the second
for 9.1 and 9.2 which will make pg_upgrade now match the pg_dump
filtering and produce proper results for orphaned temp tables by
filtering them.

As far as unlogged tables, those are dumped by 9.1/9.2, so there is no
need to check relpersistence in this patch. pg_dump doesn't check
relistemp either.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Dave Byrne wrote:
> > Beginning with commit 002c105a0706bd1c1e939fe0f47ecdceeae6c52d
> > pg_upgrade will fail if there are orphaned temp tables in the current
> > database with the message 'old and new databases "postgres" have a
> > different number of relations'
> >
> > On line 41 of pg_upgrade/info.c pg_upgrade checks that the number of
> > relations are the same but includes orphaned temp tables in the comparison.
> >
> > Is this expected behavior? If so is there a more detailed error message
> > that can be added explain the cause of the failure? It wasn't evident
> > until modified pg_upgrade to show the missing oid's and recognized them
> > as temp tables with oid2name.
>
> Thanks for your report and patch.
>
> Let me give some background on pg_upgrade to explain what is happening.
> Pg_upgrade uses two C arrays to store information about tables and
> indexes for the old and new clusters. It is not possible to store this
> information in a database because both clusters are down when pg_upgrade
> needs to use this information.
>
> In pre-9.1 pg_upgrade, pg_upgrade did a sequential scan of the arrays
> looking for a match between old and new cluster objects. This was
> reported as slow for databases with many objects, and I could reproduce
> the slowness. I added some caching in 9.0 but the real solution for 9.1
> was to assume a one-to-one mapping between the old and new C arrays,
> i.e. the 5th entry in the old cluster array is the same as the 5th
> element in the new cluster array.
>
> I knew this was risky but was the right solution so it doesn't surprise
> me you found a failure. pg_upgrade checks that the size of the two
> arrays in the same and also checks that each element matches --- the
> former is what generated your error.
>
> Now, about the cause. I had not anticipated that orphaned temp objects
> could exist in either cluster. In fact, this case would have generated
> an error in 9.0 as well, but with a different error message.
>
> Looking futher, pg_upgrade has to use the same object filter as
> pg_dump, and pg_dump uses this C test:
>
> pg_dump.c: else if (strncmp(nsinfo->dobj.name, "pg_", 3) == 0 ||
>
> pg_dumpall uses this filter:
>
> "WHERE spcname !~ '^pg_' "
>
> The problem is that the filter used by pg_upgrade only excluded
> pg_catalog, not pg_temp* as well.
>
> I have developed the attached two patches, one for 9.0, and the second
> for 9.1 and 9.2 which will make pg_upgrade now match the pg_dump
> filtering and produce proper results for orphaned temp tables by
> filtering them.
>
> As far as unlogged tables, those are dumped by 9.1/9.2, so there is no
> need to check relpersistence in this patch. pg_dump doesn't check
> relistemp either.
>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> new file mode 100644
> index 567c64e..ca357e7
> *** a/contrib/pg_upgrade/info.c
> --- b/contrib/pg_upgrade/info.c
> *************** get_rel_infos(migratorContext *ctx, cons
> *** 326,332 ****
> " ON c.relnamespace = n.oid "
> " LEFT OUTER JOIN pg_catalog.pg_tablespace t "
> " ON c.reltablespace = t.oid "
> ! "WHERE (( n.nspname NOT IN ('pg_catalog', 'information_schema') "
> " AND c.oid >= %u "
> " ) OR ( "
> " n.nspname = 'pg_catalog' "
> --- 326,335 ----
> " ON c.relnamespace = n.oid "
> " LEFT OUTER JOIN pg_catalog.pg_tablespace t "
> " ON c.reltablespace = t.oid "
> ! "WHERE (( "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' "
> ! " AND n.nspname != 'information_schema' "
> " AND c.oid >= %u "
> " ) OR ( "
> " n.nspname = 'pg_catalog' "
> diff --git a/contrib/pg_upgrade/version_old_8_3.c b/contrib/pg_upgrade/version_old_8_3.c
> new file mode 100644
> index 6ca266c..930f76d
> *** a/contrib/pg_upgrade/version_old_8_3.c
> --- b/contrib/pg_upgrade/version_old_8_3.c
> *************** old_8_3_check_for_name_data_type_usage(m
> *** 61,67 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.name'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 61,68 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.name'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> *************** old_8_3_check_for_tsquery_usage(migrator
> *** 151,157 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsquery'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 152,159 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsquery'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> *************** old_8_3_rebuild_tsvector_tables(migrator
> *** 250,256 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> /*
> --- 252,259 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> /*
> *************** old_8_3_rebuild_tsvector_tables(migrator
> *** 268,274 ****
> " NOT a.attisdropped AND " \
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND " \
> " c.relnamespace = n.oid AND " \
> ! " n.nspname != 'pg_catalog' AND " \
> " n.nspname != 'information_schema') "
>
> ntups = PQntuples(res);
> --- 271,277 ----
> " NOT a.attisdropped AND " \
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND " \
> " c.relnamespace = n.oid AND " \
> ! " n.nspname !~ '^pg_' AND " \
> " n.nspname != 'information_schema') "
>
> ntups = PQntuples(res);
> *************** old_8_3_create_sequence_script(migratorC
> *** 638,644 ****
> " pg_catalog.pg_namespace n "
> "WHERE c.relkind = 'S' AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 641,648 ----
> " pg_catalog.pg_namespace n "
> "WHERE c.relkind = 'S' AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> new file mode 100644
> index 72bc489..3ef3429
> *** a/contrib/pg_upgrade/info.c
> --- b/contrib/pg_upgrade/info.c
> *************** get_rel_infos(ClusterInfo *cluster, DbIn
> *** 266,272 ****
> " LEFT OUTER JOIN pg_catalog.pg_tablespace t "
> " ON c.reltablespace = t.oid "
> "WHERE relkind IN ('r','t', 'i'%s) AND "
> ! " ((n.nspname NOT IN ('pg_catalog', 'information_schema', 'binary_upgrade') AND "
> " c.oid >= %u) "
> " OR (n.nspname = 'pg_catalog' AND "
> " relname IN ('pg_largeobject', 'pg_largeobject_loid_pn_index'%s) )) "
> --- 266,274 ----
> " LEFT OUTER JOIN pg_catalog.pg_tablespace t "
> " ON c.reltablespace = t.oid "
> "WHERE relkind IN ('r','t', 'i'%s) AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " ((n.nspname !~ '^pg_' AND "
> ! " n.nspname NOT IN ('information_schema', 'binary_upgrade') AND "
> " c.oid >= %u) "
> " OR (n.nspname = 'pg_catalog' AND "
> " relname IN ('pg_largeobject', 'pg_largeobject_loid_pn_index'%s) )) "
> diff --git a/contrib/pg_upgrade/version_old_8_3.c b/contrib/pg_upgrade/version_old_8_3.c
> new file mode 100644
> index 43bfdc1..1c736d2
> *** a/contrib/pg_upgrade/version_old_8_3.c
> --- b/contrib/pg_upgrade/version_old_8_3.c
> *************** old_8_3_check_for_name_data_type_usage(C
> *** 59,65 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.name'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 59,66 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.name'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> *************** old_8_3_check_for_tsquery_usage(ClusterI
> *** 148,154 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsquery'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 149,156 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsquery'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> *************** old_8_3_rebuild_tsvector_tables(ClusterI
> *** 245,251 ****
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> /*
> --- 247,254 ----
> " NOT a.attisdropped AND "
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> /*
> *************** old_8_3_rebuild_tsvector_tables(ClusterI
> *** 263,269 ****
> " NOT a.attisdropped AND " \
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND " \
> " c.relnamespace = n.oid AND " \
> ! " n.nspname != 'pg_catalog' AND " \
> " n.nspname != 'information_schema') "
>
> ntups = PQntuples(res);
> --- 266,272 ----
> " NOT a.attisdropped AND " \
> " a.atttypid = 'pg_catalog.tsvector'::pg_catalog.regtype AND " \
> " c.relnamespace = n.oid AND " \
> ! " n.nspname !~ '^pg_' AND " \
> " n.nspname != 'information_schema') "
>
> ntups = PQntuples(res);
> *************** old_8_3_create_sequence_script(ClusterIn
> *** 616,622 ****
> " pg_catalog.pg_namespace n "
> "WHERE c.relkind = 'S' AND "
> " c.relnamespace = n.oid AND "
> ! " n.nspname != 'pg_catalog' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);
> --- 619,626 ----
> " pg_catalog.pg_namespace n "
> "WHERE c.relkind = 'S' AND "
> " c.relnamespace = n.oid AND "
> ! /* exclude pg_catalog and pg_temp_ (could be orphaned tables) */
> ! " n.nspname !~ '^pg_' AND "
> " n.nspname != 'information_schema'");
>
> ntups = PQntuples(res);

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +