Re: [HACKERS] Possible Bug in pg_upgrade

2011-08-15 Thread Bruce Momjian

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  br...@momjian.ushttp://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 
 ! 

Re: [HACKERS] Possible Bug in pg_upgrade

2011-08-14 Thread Bruce Momjian
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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
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 
  			

Re: [HACKERS] Possible Bug in pg_upgrade

2011-08-12 Thread Dave Byrne

On 08/11/2011 12:02 AM, Peter Eisentraut wrote:

On ons, 2011-08-10 at 18:53 -0400, Tom Lane wrote:

Dave Byrnedby...@mdb.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.





--
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] Possible Bug in pg_upgrade

2011-08-11 Thread Peter Eisentraut
On ons, 2011-08-10 at 18:53 -0400, Tom Lane wrote:
 Dave Byrne dby...@mdb.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.


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


[HACKERS] Possible Bug in pg_upgrade

2011-08-10 Thread Dave Byrne

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:  dby...@mdb.commailto:dby...@mdb.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.comhttp://www.mdb.com


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

--
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] Possible Bug in pg_upgrade

2011-08-10 Thread Tom Lane
Dave Byrne dby...@mdb.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

-- 
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] Possible Bug in pg_upgrade

2011-08-10 Thread Dave Byrne

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:t...@sss.pgh.pa.us] 
Sent: Wednesday, August 10, 2011 12:29 PM
To: Dave Byrne
Cc: pgsql-hackers@postgresql.org; Bruce Momjian
Subject: Re: [HACKERS] Possible Bug in pg_upgrade 

Dave Byrne dby...@mdb.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


pg_upgrade_tmp.patch
Description: pg_upgrade_tmp.patch

-- 
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] Possible Bug in pg_upgrade

2011-08-10 Thread Tom Lane
Dave Byrne dby...@mdb.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

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