Обсуждение: Pg_upgrade and toast tables bug discovered

There have been periodic reports of pg_upgrade errors related to toast
tables.  The most recent one was from May of this year:
http://www.postgresql.org/message-id/flat/20140520202223.GB3701@momjian.us#20140520202223.GB3701@momjian.us

There error was:
Copying user relation files   /var/lib/postgresql/8.4/main/base/4275487/4278965Mismatch of relation OID in database
"FNBooking":old OID 4279499, new OID 19792Failure, exiting
 

and the fix is to add a dummy TEXT column to the table on the old
cluster to force a toast table, then drop the dummy column.

I have had trouble getting a table schema that is causing problems, but
received a report via EDB support recently that had a simple schema
(anonymized):
CREATE TABLE pg_upgrade_toast_test (    x1 numeric(15,0),    x2 numeric(15,0),    x3 character varying(15),    x4
charactervarying(60),    x5 numeric(15,0),    x6 numeric(15,0),    x7 character varying(15),    x8 character
varying(60),   x9 numeric(15,0),    x10 character varying(15),    x11 character varying(60),    x12 numeric(15,0),
x13numeric(15,0),    x14 character varying(15),    x15 character varying(60),    x16 numeric(15,0),    x17 character
varying(15),   x18 character varying(60),    x19 numeric(15,0),    x20 character varying(15),    x21 character
varying(60));

needs_toast_table() computes the length of this table as 2024 bytes in
9.0, and 2064 bytes on 9.1, with the TOAST threshold being 2032 bytes. 
It turns out it is this commit that causes the difference:
commit 97f38001acc61449f7ac09c539ccc29e40fecd26Author: Robert Haas <rhaas@postgresql.org>Date:   Wed Aug 4 17:33:09
2010+0000    Fix numeric_maximum_size() calculation.    The old computation can sometimes underestimate the necessary
space   by 2 bytes; however we're not back-patching this, because this result    isn't used for anything critical.  Per
discussionwith Tom Lane,    make the typmod test in this function match the ones in numeric()    and apply_typmod()
exactly.

It seems the impact of this patch on pg_upgrade wasn't considered, or
even realized until now.

Suggestions on a fix?  

My initial idea is to to allow for toast tables in the new cluster that
aren't in the old cluster by skipping over the extra toast tables.  This
would only be for pre-9.1 old clusters.  It would not involve adding
toast tables to the old cluster as pg_upgrade never modifies the old
cluster.  We already handle cases where the old cluster had toast tables
and the new cluster wouldn't ordinarily have them.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> I have had trouble getting a table schema that is causing problems, but
> received a report via EDB support recently that had a simple schema
> (anonymized):
> ...
> needs_toast_table() computes the length of this table as 2024 bytes in
> 9.0, and 2064 bytes on 9.1, with the TOAST threshold being 2032 bytes. 

> My initial idea is to to allow for toast tables in the new cluster that
> aren't in the old cluster by skipping over the extra toast tables.  This
> would only be for pre-9.1 old clusters.

TBH, it has never been more than the shakiest of assumptions that the new
version could not create toast tables where the old one hadn't.  I think
you should just allow for that case, independently of specific PG
versions.  Fortunately it seems easy enough, since you certainly don't
need to put any data into the new toast tables.
        regards, tom lane

On 2014-07-03 17:09:41 -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I have had trouble getting a table schema that is causing problems, but
> > received a report via EDB support recently that had a simple schema
> > (anonymized):
> > ...
> > needs_toast_table() computes the length of this table as 2024 bytes in
> > 9.0, and 2064 bytes on 9.1, with the TOAST threshold being 2032 bytes. 
> 
> > My initial idea is to to allow for toast tables in the new cluster that
> > aren't in the old cluster by skipping over the extra toast tables.  This
> > would only be for pre-9.1 old clusters.
> 
> TBH, it has never been more than the shakiest of assumptions that the new
> version could not create toast tables where the old one hadn't.  I think
> you should just allow for that case, independently of specific PG
> versions.  Fortunately it seems easy enough, since you certainly don't
> need to put any data into the new toast tables.

I don't think it's just that simple unfortunately. If pg_class entries
get created that didn't exist on the old server there's a chance for oid
conflicts. Consider

SELECT binary_upgrade.set_next_heap_pg_class_oid('17094'::pg_catalog.oid);
CREATE TABLE table_without_toast_in_old_server(...);
-- heap oid 17094, toast oid 16384

SELECT binary_upgrade.set_next_heap_pg_class_oid('16384'::pg_catalog.oid);
CREATE TABLE another_table(...);
ERROR: could not create file ...: File exists

I think we even had reports of such a problem.

The most robust, but not trivial, approach seems to be to prevent toast
table creation if there wasn't a set_next_toast_pg_class_oid(). Then,
after all relations are created, iterate over all pg_class entries that
possibly need toast tables and recheck if they now might need one.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Thu, Jul  3, 2014 at 05:09:41PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I have had trouble getting a table schema that is causing problems, but
> > received a report via EDB support recently that had a simple schema
> > (anonymized):
> > ...
> > needs_toast_table() computes the length of this table as 2024 bytes in
> > 9.0, and 2064 bytes on 9.1, with the TOAST threshold being 2032 bytes.
>
> > My initial idea is to to allow for toast tables in the new cluster that
> > aren't in the old cluster by skipping over the extra toast tables.  This
> > would only be for pre-9.1 old clusters.
>
> TBH, it has never been more than the shakiest of assumptions that the new
> version could not create toast tables where the old one hadn't.  I think
> you should just allow for that case, independently of specific PG
> versions.  Fortunately it seems easy enough, since you certainly don't
> need to put any data into the new toast tables.

OK, patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Thu, Jul  3, 2014 at 11:55:40PM +0200, Andres Freund wrote:
> I don't think it's just that simple unfortunately. If pg_class entries
> get created that didn't exist on the old server there's a chance for oid
> conflicts. Consider
> 
> SELECT binary_upgrade.set_next_heap_pg_class_oid('17094'::pg_catalog.oid);
> CREATE TABLE table_without_toast_in_old_server(...);
> -- heap oid 17094, toast oid 16384
> 
> SELECT binary_upgrade.set_next_heap_pg_class_oid('16384'::pg_catalog.oid);
> CREATE TABLE another_table(...);
> ERROR: could not create file ...: File exists
> 
> I think we even had reports of such a problem.

I had not considered this.

I don't remember ever seeing such a report.  We have had oid mismatch
reports, but we now know the cause of those.

> The most robust, but not trivial, approach seems to be to prevent toast
> table creation if there wasn't a set_next_toast_pg_class_oid(). Then,
> after all relations are created, iterate over all pg_class entries that
> possibly need toast tables and recheck if they now might need one.

Wow, that is going to be kind of odd in that there really isn't a good
way to create toast tables except perhaps add a dummy TEXT column and
remove it.  There also isn't an easy way to not create a toast table,
but also find out that one was needed.  I suppose we would have to
insert some dummy value in the toast pg_class column and come back later
to clean it up.

I am wondering what the probability of having a table that didn't need a
toast table in the old cluster, and needed one in the new cluster, and
there being an oid collision.

I think the big question is whether we want to go down that route.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Fri, Jul  4, 2014 at 12:01:37AM -0400, Bruce Momjian wrote:
> > The most robust, but not trivial, approach seems to be to prevent toast
> > table creation if there wasn't a set_next_toast_pg_class_oid(). Then,
> > after all relations are created, iterate over all pg_class entries that
> > possibly need toast tables and recheck if they now might need one.
>
> Wow, that is going to be kind of odd in that there really isn't a good
> way to create toast tables except perhaps add a dummy TEXT column and
> remove it.  There also isn't an easy way to not create a toast table,
> but also find out that one was needed.  I suppose we would have to
> insert some dummy value in the toast pg_class column and come back later
> to clean it up.
>
> I am wondering what the probability of having a table that didn't need a
> toast table in the old cluster, and needed one in the new cluster, and
> there being an oid collision.
>
> I think the big question is whether we want to go down that route.

Here is an updated patch.  It was a little tricky because if the
mismatched non-toast table is after the last old relation, you need to
test differently and emit a different error message as there is no old
relation to complain about.

As far as the reusing of oids, we don't set the oid counter until after
the restore, so any new unmatched toast table would given a very low
oid.  Since we restore in oid order, for an oid to be assigned that was
used in the old cluster, it would have to be a very early relation, so I
think that reduces the odds considerably.  I am not inclined to do
anything more to avoid this until I see an actual error report ---
trying to address it might be invasive and introduce new errors.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Fri, Jul 4, 2014 at 11:12 PM, Bruce Momjian <bruce@momjian.us> wrote:
> On Fri, Jul  4, 2014 at 12:01:37AM -0400, Bruce Momjian wrote:
>> > The most robust, but not trivial, approach seems to be to prevent toast
>> > table creation if there wasn't a set_next_toast_pg_class_oid(). Then,
>> > after all relations are created, iterate over all pg_class entries that
>> > possibly need toast tables and recheck if they now might need one.
>>
>> Wow, that is going to be kind of odd in that there really isn't a good
>> way to create toast tables except perhaps add a dummy TEXT column and
>> remove it.  There also isn't an easy way to not create a toast table,
>> but also find out that one was needed.  I suppose we would have to
>> insert some dummy value in the toast pg_class column and come back later
>> to clean it up.
>>
>> I am wondering what the probability of having a table that didn't need a
>> toast table in the old cluster, and needed one in the new cluster, and
>> there being an oid collision.
>>
>> I think the big question is whether we want to go down that route.
>
> Here is an updated patch.  It was a little tricky because if the
> mismatched non-toast table is after the last old relation, you need to
> test differently and emit a different error message as there is no old
> relation to complain about.
>
> As far as the reusing of oids, we don't set the oid counter until after
> the restore, so any new unmatched toast table would given a very low
> oid.  Since we restore in oid order, for an oid to be assigned that was
> used in the old cluster, it would have to be a very early relation, so I
> think that reduces the odds considerably.  I am not inclined to do
> anything more to avoid this until I see an actual error report ---
> trying to address it might be invasive and introduce new errors.

That sounds pretty shaky from where I sit.  I wonder if the
pg_upgrade_support module should have a function
create_toast_table_if_needed(regclass) or something like that.  Or
maybe just create_any_missing_toast_tables(), iterating over all
relations.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul  4, 2014 at 11:12:58PM -0400, Bruce Momjian wrote:
> On Fri, Jul  4, 2014 at 12:01:37AM -0400, Bruce Momjian wrote:
> > > The most robust, but not trivial, approach seems to be to prevent toast
> > > table creation if there wasn't a set_next_toast_pg_class_oid(). Then,
> > > after all relations are created, iterate over all pg_class entries that
> > > possibly need toast tables and recheck if they now might need one.
> > 
> > Wow, that is going to be kind of odd in that there really isn't a good
> > way to create toast tables except perhaps add a dummy TEXT column and
> > remove it.  There also isn't an easy way to not create a toast table,
> > but also find out that one was needed.  I suppose we would have to
> > insert some dummy value in the toast pg_class column and come back later
> > to clean it up.
> > 
> > I am wondering what the probability of having a table that didn't need a
> > toast table in the old cluster, and needed one in the new cluster, and
> > there being an oid collision.
> > 
> > I think the big question is whether we want to go down that route.
> 
> Here is an updated patch.  It was a little tricky because if the
> mismatched non-toast table is after the last old relation, you need to
> test differently and emit a different error message as there is no old
> relation to complain about.
> 
> As far as the reusing of oids, we don't set the oid counter until after
> the restore, so any new unmatched toast table would given a very low
> oid.  Since we restore in oid order, for an oid to be assigned that was
> used in the old cluster, it would have to be a very early relation, so I
> think that reduces the odds considerably.  I am not inclined to do
> anything more to avoid this until I see an actual error report ---
> trying to address it might be invasive and introduce new errors.

Patch applied back through 9.2.  9.1 and earlier had code that was
different enought that I thought it would cause too many problems, and I
doubt many users are doing major uprades to 9.1, and the bug is rare.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Mon, Jul  7, 2014 at 11:24:51AM -0400, Robert Haas wrote:
> > As far as the reusing of oids, we don't set the oid counter until after
> > the restore, so any new unmatched toast table would given a very low
> > oid.  Since we restore in oid order, for an oid to be assigned that was
> > used in the old cluster, it would have to be a very early relation, so I
> > think that reduces the odds considerably.  I am not inclined to do
> > anything more to avoid this until I see an actual error report ---
> > trying to address it might be invasive and introduce new errors.
> 
> That sounds pretty shaky from where I sit.  I wonder if the
> pg_upgrade_support module should have a function
> create_toast_table_if_needed(regclass) or something like that.  Or
> maybe just create_any_missing_toast_tables(), iterating over all
> relations.

Uh, I guess we could write some code that iterates over all tables and
finds the tables that should have TOAST tables, but don't (because
binary-upgrade backend mode suppressed their creation), and adds them.

However, that would be a lot of code and might be risky to backpatch. 
The error is so rare I am not sure it is worth it.  I tried to create
the failure case and it was very difficult, requiring me to create the
problem table first, then some dummy stuff to get the offsets right so
the oid would collide.

Based on the rareness of the bug, I am not sure it is worth it, but if
it is, it would be something only for 9.4 (maybe) and 9.5, not something
to backpatch.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Mon, Jul  7, 2014 at 01:44:59PM -0400, Bruce Momjian wrote:
> On Mon, Jul  7, 2014 at 11:24:51AM -0400, Robert Haas wrote:
> > > As far as the reusing of oids, we don't set the oid counter until after
> > > the restore, so any new unmatched toast table would given a very low
> > > oid.  Since we restore in oid order, for an oid to be assigned that was
> > > used in the old cluster, it would have to be a very early relation, so I
> > > think that reduces the odds considerably.  I am not inclined to do
> > > anything more to avoid this until I see an actual error report ---
> > > trying to address it might be invasive and introduce new errors.
> > 
> > That sounds pretty shaky from where I sit.  I wonder if the
> > pg_upgrade_support module should have a function
> > create_toast_table_if_needed(regclass) or something like that.  Or
> > maybe just create_any_missing_toast_tables(), iterating over all
> > relations.
> 
> Uh, I guess we could write some code that iterates over all tables and
> finds the tables that should have TOAST tables, but don't (because
> binary-upgrade backend mode suppressed their creation), and adds them.
> 
> However, that would be a lot of code and might be risky to backpatch. 
> The error is so rare I am not sure it is worth it.  I tried to create
> the failure case and it was very difficult, requiring me to create the
> problem table first, then some dummy stuff to get the offsets right so
> the oid would collide.
> 
> Based on the rareness of the bug, I am not sure it is worth it, but if
> it is, it would be something only for 9.4 (maybe) and 9.5, not something
> to backpatch.

Another idea would be to renumber empty toast tables that conflict
during new toast table creation, when in binary upgrade mode.  Since the
files are always empty in binary upgrade mode, you could just create a
new toast table, repoint the old table to use (because it didn't ask for
a specific toast table oid because it didn't need one), and then use
that one for the table that actually requested the oid.  However, if one
is a heap (zero size), and one is an index (8k size), it wouldn't work
and you would have to recreate the file system file too.

That seems a lot more localized than the other approaches.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

>> Uh, I guess we could write some code that iterates over all tables and
>> finds the tables that should have TOAST tables, but don't (because
>> binary-upgrade backend mode suppressed their creation), and adds them.
>>
>> However, that would be a lot of code and might be risky to backpatch.
>> The error is so rare I am not sure it is worth it.  I tried to create
>> the failure case and it was very difficult, requiring me to create the
>> problem table first, then some dummy stuff to get the offsets right so
>> the oid would collide.
>>
>> Based on the rareness of the bug, I am not sure it is worth it, but if
>> it is, it would be something only for 9.4 (maybe) and 9.5, not something
>> to backpatch.
>
> Another idea would be to renumber empty toast tables that conflict
> during new toast table creation, when in binary upgrade mode.  Since the
> files are always empty in binary upgrade mode, you could just create a
> new toast table, repoint the old table to use (because it didn't ask for
> a specific toast table oid because it didn't need one), and then use
> that one for the table that actually requested the oid.  However, if one
> is a heap (zero size), and one is an index (8k size), it wouldn't work
> and you would have to recreate the file system file too.
>
> That seems a lot more localized than the other approaches.

To me, that sounds vastly more complicated and error-prone than
forcing the TOAST tables to be added in a second pass as Andres
suggested.

But I just work here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul  9, 2014 at 10:13:17AM -0400, Robert Haas wrote:
> >> Uh, I guess we could write some code that iterates over all tables and
> >> finds the tables that should have TOAST tables, but don't (because
> >> binary-upgrade backend mode suppressed their creation), and adds them.
> >>
> >> However, that would be a lot of code and might be risky to backpatch.
> >> The error is so rare I am not sure it is worth it.  I tried to create
> >> the failure case and it was very difficult, requiring me to create the
> >> problem table first, then some dummy stuff to get the offsets right so
> >> the oid would collide.
> >>
> >> Based on the rareness of the bug, I am not sure it is worth it, but if
> >> it is, it would be something only for 9.4 (maybe) and 9.5, not something
> >> to backpatch.
> >
> > Another idea would be to renumber empty toast tables that conflict
> > during new toast table creation, when in binary upgrade mode.  Since the
> > files are always empty in binary upgrade mode, you could just create a
> > new toast table, repoint the old table to use (because it didn't ask for
> > a specific toast table oid because it didn't need one), and then use
> > that one for the table that actually requested the oid.  However, if one
> > is a heap (zero size), and one is an index (8k size), it wouldn't work
> > and you would have to recreate the file system file too.
> >
> > That seems a lot more localized than the other approaches.
> 
> To me, that sounds vastly more complicated and error-prone than
> forcing the TOAST tables to be added in a second pass as Andres
> suggested.
> 
> But I just work here.

Agreed.  I am now thinking we could harness the code that already exists
to optionally add a TOAST table as part of ALTER TABLE ADD COLUMN.  We
would just need an entry point to call it from pg_upgrade, either via an
SQL command that checks (and hopefully doesn't do anything else), or a C
function that does it, e.g. VACUUM would be trivial to run on every
database, but I don't think it tests that;  is _could_ in binary_upgrade
mode.  However, the idea of having a C function plug into the guts of
the server and call internal functions makes me uncomforable.

The code already called via pg_restore would only need to suppress TOAST
table creation if a heap oid is supplied but a TOAST table oid is not.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Wed, Jul 9, 2014 at 12:09 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> To me, that sounds vastly more complicated and error-prone than
>> forcing the TOAST tables to be added in a second pass as Andres
>> suggested.
>>
>> But I just work here.
>
> Agreed.  I am now thinking we could harness the code that already exists
> to optionally add a TOAST table as part of ALTER TABLE ADD COLUMN.  We
> would just need an entry point to call it from pg_upgrade, either via an
> SQL command that checks (and hopefully doesn't do anything else), or a C
> function that does it, e.g. VACUUM would be trivial to run on every
> database, but I don't think it tests that;  is _could_ in binary_upgrade
> mode.  However, the idea of having a C function plug into the guts of
> the server and call internal functions makes me uncomforable.

Well, pg_upgrade_support's charter is basically to provide access to
the guts of the server in ways we wouldn't normally allow; all that
next-OID stuff is basically exactly that.  So I don't think this is
such a big deal.  It needs to be properly commented, of course.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 10, 2014 at 10:46:30AM -0400, Robert Haas wrote:
> > Agreed.  I am now thinking we could harness the code that already exists
> > to optionally add a TOAST table as part of ALTER TABLE ADD COLUMN.  We
> > would just need an entry point to call it from pg_upgrade, either via an
> > SQL command that checks (and hopefully doesn't do anything else), or a C
> > function that does it, e.g. VACUUM would be trivial to run on every
> > database, but I don't think it tests that;  is _could_ in binary_upgrade
> > mode.  However, the idea of having a C function plug into the guts of
> > the server and call internal functions makes me uncomforable.
> 
> Well, pg_upgrade_support's charter is basically to provide access to
> the guts of the server in ways we wouldn't normally allow; all that
> next-OID stuff is basically exactly that.  So I don't think this is
> such a big deal.  It needs to be properly commented, of course.

If you look at how oid assignment is handled, it is done in a very
surgical way, i.e. pg_upgrade_support sets a global variable, and the
variable triggers different behavior in a CREATE command.  This change
would be far more invasive than that.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On 2014-07-10 16:33:40 -0400, Bruce Momjian wrote:
> On Thu, Jul 10, 2014 at 10:46:30AM -0400, Robert Haas wrote:
> > > Agreed.  I am now thinking we could harness the code that already exists
> > > to optionally add a TOAST table as part of ALTER TABLE ADD COLUMN.  We
> > > would just need an entry point to call it from pg_upgrade, either via an
> > > SQL command that checks (and hopefully doesn't do anything else), or a C
> > > function that does it, e.g. VACUUM would be trivial to run on every
> > > database, but I don't think it tests that;  is _could_ in binary_upgrade
> > > mode.  However, the idea of having a C function plug into the guts of
> > > the server and call internal functions makes me uncomforable.
> > 
> > Well, pg_upgrade_support's charter is basically to provide access to
> > the guts of the server in ways we wouldn't normally allow; all that
> > next-OID stuff is basically exactly that.  So I don't think this is
> > such a big deal.  It needs to be properly commented, of course.
> 
> If you look at how oid assignment is handled, it is done in a very
> surgical way, i.e. pg_upgrade_support sets a global variable, and the
> variable triggers different behavior in a CREATE command.  This change
> would be far more invasive than that.

Meh. It's only somewhat surgical because there's pg_upgrade specific
code sprinkled in the backend at strategic places. That's the contrary
of a clear abstraction barrier. And arguably a function call from a SQL
C function has a much clearer abstraction barrier.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Thu, Jul 10, 2014 at 11:11:19PM +0200, Andres Freund wrote:
> On 2014-07-10 16:33:40 -0400, Bruce Momjian wrote:
> > On Thu, Jul 10, 2014 at 10:46:30AM -0400, Robert Haas wrote:
> > > > Agreed.  I am now thinking we could harness the code that already exists
> > > > to optionally add a TOAST table as part of ALTER TABLE ADD COLUMN.  We
> > > > would just need an entry point to call it from pg_upgrade, either via an
> > > > SQL command that checks (and hopefully doesn't do anything else), or a C
> > > > function that does it, e.g. VACUUM would be trivial to run on every
> > > > database, but I don't think it tests that;  is _could_ in binary_upgrade
> > > > mode.  However, the idea of having a C function plug into the guts of
> > > > the server and call internal functions makes me uncomforable.
> > > 
> > > Well, pg_upgrade_support's charter is basically to provide access to
> > > the guts of the server in ways we wouldn't normally allow; all that
> > > next-OID stuff is basically exactly that.  So I don't think this is
> > > such a big deal.  It needs to be properly commented, of course.
> > 
> > If you look at how oid assignment is handled, it is done in a very
> > surgical way, i.e. pg_upgrade_support sets a global variable, and the
> > variable triggers different behavior in a CREATE command.  This change
> > would be far more invasive than that.
> 
> Meh. It's only somewhat surgical because there's pg_upgrade specific
> code sprinkled in the backend at strategic places. That's the contrary
> of a clear abstraction barrier. And arguably a function call from a SQL
> C function has a much clearer abstraction barrier.

Well, we are going to need to call internal C functions, often bypassing
their typical call sites and the assumption about locking, etc.  Perhaps
this could be done from a plpgsql function.  We could add and drop a
dummy column to force TOAST table creation --- we would then only need a
way to detect if a function _needs_ a TOAST table, which was skipped in
binary upgrade mode previously.

That might be a minimalistic approach.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Jul 10, 2014 at 06:17:14PM -0400, Bruce Momjian wrote:
> Well, we are going to need to call internal C functions, often bypassing
> their typical call sites and the assumption about locking, etc.  Perhaps
> this could be done from a plpgsql function.  We could add and drop a
> dummy column to force TOAST table creation --- we would then only need a
> way to detect if a function _needs_ a TOAST table, which was skipped in
> binary upgrade mode previously.
> 
> That might be a minimalistic approach.

I have thought some more on this.  I thought I would need to open
pg_class in C and do complex backend stuff, but I now realize I can do
it from libpq, and just call ALTER TABLE and I think that always
auto-checks if a TOAST table is needed.  All I have to do is query
pg_class from libpq, then construct ALTER TABLE commands for each item,
and it will optionally create the TOAST table if needed.  I just have to
use a no-op ALTER TABLE command, like SET STATISTICS.

I am in Asia the next two weeks but will work on it after I return.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:
> On Thu, Jul 10, 2014 at 06:17:14PM -0400, Bruce Momjian wrote:
> > Well, we are going to need to call internal C functions, often bypassing
> > their typical call sites and the assumption about locking, etc.  Perhaps
> > this could be done from a plpgsql function.  We could add and drop a
> > dummy column to force TOAST table creation --- we would then only need a
> > way to detect if a function _needs_ a TOAST table, which was skipped in
> > binary upgrade mode previously.
> >
> > That might be a minimalistic approach.
>
> I have thought some more on this.  I thought I would need to open
> pg_class in C and do complex backend stuff, but I now realize I can do
> it from libpq, and just call ALTER TABLE and I think that always
> auto-checks if a TOAST table is needed.  All I have to do is query
> pg_class from libpq, then construct ALTER TABLE commands for each item,
> and it will optionally create the TOAST table if needed.  I just have to
> use a no-op ALTER TABLE command, like SET STATISTICS.
>
> I am in Asia the next two weeks but will work on it after I return.

Attached is the backend part of the patch.  I will work on the
pg_upgrade/libpq/ALTER TABLE part later.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian wrote:
> On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:

> > I have thought some more on this.  I thought I would need to open
> > pg_class in C and do complex backend stuff, but I now realize I can do
> > it from libpq, and just call ALTER TABLE and I think that always
> > auto-checks if a TOAST table is needed.  All I have to do is query
> > pg_class from libpq, then construct ALTER TABLE commands for each item,
> > and it will optionally create the TOAST table if needed.  I just have to
> > use a no-op ALTER TABLE command, like SET STATISTICS.
> 
> Attached is the backend part of the patch.  I will work on the
> pg_upgrade/libpq/ALTER TABLE part later.

Uh, why does this need to be in ALTER TABLE?  Can't this be part of
table creation done by pg_dump?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jul 11, 2014 at 12:18:40AM -0400, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:
> 
> > > I have thought some more on this.  I thought I would need to open
> > > pg_class in C and do complex backend stuff, but I now realize I can do
> > > it from libpq, and just call ALTER TABLE and I think that always
> > > auto-checks if a TOAST table is needed.  All I have to do is query
> > > pg_class from libpq, then construct ALTER TABLE commands for each item,
> > > and it will optionally create the TOAST table if needed.  I just have to
> > > use a no-op ALTER TABLE command, like SET STATISTICS.
> > 
> > Attached is the backend part of the patch.  I will work on the
> > pg_upgrade/libpq/ALTER TABLE part later.
> 
> Uh, why does this need to be in ALTER TABLE?  Can't this be part of
> table creation done by pg_dump?

Uh, I think you need to read the thread.  We have to delay the toast
creation part so we don't use an oid that will later be required by
another table from the old cluster.  This has to be done after all
tables have been created.

We could have pg_dump spit out those ALTER lines at the end of the dump,
but it seems simpler to do it in pg_upgrade.

Even if we have pg_dump create all the tables that require pre-assigned
TOAST oids first, then the other tables that _might_ need a TOAST table,
those later tables might create a toast oid that matches a later
non-TOAST-requiring table, so I don't think that fixes the problem.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
> > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
> > table creation done by pg_dump?
> 
> Uh, I think you need to read the thread.  We have to delay the toast
> creation part so we don't use an oid that will later be required by
> another table from the old cluster.  This has to be done after all
> tables have been created.
> 
> We could have pg_dump spit out those ALTER lines at the end of the dump,
> but it seems simpler to do it in pg_upgrade.
> 
> Even if we have pg_dump create all the tables that require pre-assigned
> TOAST oids first, then the other tables that _might_ need a TOAST table,
> those later tables might create a toast oid that matches a later
> non-TOAST-requiring table, so I don't think that fixes the problem.

What would be nice is if I could mark just the tables that will need
toast tables created in that later phase (those tables that didn't have
a toast table in the old cluster, but need one in the new cluster). 
However, I can't see where to store that or how to pass that back into
pg_upgrade.  I don't see a logical place in pg_class to put it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Fri, Jul 11, 2014 at 9:55 AM, Bruce Momjian <bruce@momjian.us> wrote:
> On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
>> > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
>> > table creation done by pg_dump?
>>
>> Uh, I think you need to read the thread.  We have to delay the toast
>> creation part so we don't use an oid that will later be required by
>> another table from the old cluster.  This has to be done after all
>> tables have been created.
>>
>> We could have pg_dump spit out those ALTER lines at the end of the dump,
>> but it seems simpler to do it in pg_upgrade.
>>
>> Even if we have pg_dump create all the tables that require pre-assigned
>> TOAST oids first, then the other tables that _might_ need a TOAST table,
>> those later tables might create a toast oid that matches a later
>> non-TOAST-requiring table, so I don't think that fixes the problem.
>
> What would be nice is if I could mark just the tables that will need
> toast tables created in that later phase (those tables that didn't have
> a toast table in the old cluster, but need one in the new cluster).
> However, I can't see where to store that or how to pass that back into
> pg_upgrade.  I don't see a logical place in pg_class to put it.

reloptions?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Jul 14, 2014 at 12:26 PM, Robert Haas <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br />><br />> On Fri, Jul 11, 2014 at
9:55AM, Bruce Momjian <<a href="mailto:bruce@momjian.us">bruce@momjian.us</a>> wrote:<br /> > > On Fri, Jul
11,2014 at 09:48:06AM -0400, Bruce Momjian wrote:<br />> >> > Uh, why does this need to be in ALTER TABLE?
 Can'tthis be part of<br />> >> > table creation done by pg_dump?<br /> > >><br />> >>
Uh,I think you need to read the thread.  We have to delay the toast<br />> >> creation part so we don't use an
oidthat will later be required by<br />> >> another table from the old cluster.  This has to be done after
all<br/> > >> tables have been created.<br />> >><br />> >> We could have pg_dump spit out
thoseALTER lines at the end of the dump,<br />> >> but it seems simpler to do it in pg_upgrade.<br />>
>><br/> > >> Even if we have pg_dump create all the tables that require pre-assigned<br />> >>
TOASToids first, then the other tables that _might_ need a TOAST table,<br />> >> those later tables might
createa toast oid that matches a later<br /> > >> non-TOAST-requiring table, so I don't think that fixes the
problem.<br/>> ><br />> > What would be nice is if I could mark just the tables that will need<br />>
>toast tables created in that later phase (those tables that didn't have<br /> > > a toast table in the old
cluster,but need one in the new cluster).<br />> > However, I can't see where to store that or how to pass that
backinto<br />> > pg_upgrade.  I don't see a logical place in pg_class to put it.<br /> ><br />>
reloptions?<br/>><br /><br /></div><div class="gmail_extra">Is this another use case to "custom reloptions" idea?<br
/></div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br /></div><div class="gmail_extra"><br
/>--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

On Mon, Jul 14, 2014 at 11:26:19AM -0400, Robert Haas wrote:
> On Fri, Jul 11, 2014 at 9:55 AM, Bruce Momjian <bruce@momjian.us> wrote:
> > On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
> >> > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
> >> > table creation done by pg_dump?
> >>
> >> Uh, I think you need to read the thread.  We have to delay the toast
> >> creation part so we don't use an oid that will later be required by
> >> another table from the old cluster.  This has to be done after all
> >> tables have been created.
> >>
> >> We could have pg_dump spit out those ALTER lines at the end of the dump,
> >> but it seems simpler to do it in pg_upgrade.
> >>
> >> Even if we have pg_dump create all the tables that require pre-assigned
> >> TOAST oids first, then the other tables that _might_ need a TOAST table,
> >> those later tables might create a toast oid that matches a later
> >> non-TOAST-requiring table, so I don't think that fixes the problem.
> >
> > What would be nice is if I could mark just the tables that will need
> > toast tables created in that later phase (those tables that didn't have
> > a toast table in the old cluster, but need one in the new cluster).
> > However, I can't see where to store that or how to pass that back into
> > pg_upgrade.  I don't see a logical place in pg_class to put it.
> 
> reloptions?

Yes, that might work.  I thought about that but did not have time to see
if we can easily add/remove reloptions at the C level in that area of
the code.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On 2014-07-11 09:55:34 -0400, Bruce Momjian wrote:
> On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
> > > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
> > > table creation done by pg_dump?
> > 
> > Uh, I think you need to read the thread.  We have to delay the toast
> > creation part so we don't use an oid that will later be required by
> > another table from the old cluster.  This has to be done after all
> > tables have been created.
> > 
> > We could have pg_dump spit out those ALTER lines at the end of the dump,
> > but it seems simpler to do it in pg_upgrade.
> > 
> > Even if we have pg_dump create all the tables that require pre-assigned
> > TOAST oids first, then the other tables that _might_ need a TOAST table,
> > those later tables might create a toast oid that matches a later
> > non-TOAST-requiring table, so I don't think that fixes the problem.
> 
> What would be nice is if I could mark just the tables that will need
> toast tables created in that later phase (those tables that didn't have
> a toast table in the old cluster, but need one in the new cluster). 
> However, I can't see where to store that or how to pass that back into
> pg_upgrade.  I don't see a logical place in pg_class to put it.

This seems overengineered. Why not just do
SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r';

and in maintain_toast() just call AlterTableCreateToastTable()?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Mon, Jul 14, 2014 at 09:40:33PM +0200, Andres Freund wrote:
> On 2014-07-11 09:55:34 -0400, Bruce Momjian wrote:
> > On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
> > > > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
> > > > table creation done by pg_dump?
> > > 
> > > Uh, I think you need to read the thread.  We have to delay the toast
> > > creation part so we don't use an oid that will later be required by
> > > another table from the old cluster.  This has to be done after all
> > > tables have been created.
> > > 
> > > We could have pg_dump spit out those ALTER lines at the end of the dump,
> > > but it seems simpler to do it in pg_upgrade.
> > > 
> > > Even if we have pg_dump create all the tables that require pre-assigned
> > > TOAST oids first, then the other tables that _might_ need a TOAST table,
> > > those later tables might create a toast oid that matches a later
> > > non-TOAST-requiring table, so I don't think that fixes the problem.
> > 
> > What would be nice is if I could mark just the tables that will need
> > toast tables created in that later phase (those tables that didn't have
> > a toast table in the old cluster, but need one in the new cluster). 
> > However, I can't see where to store that or how to pass that back into
> > pg_upgrade.  I don't see a logical place in pg_class to put it.
> 
> This seems overengineered. Why not just do
> SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r';
> 
> and in maintain_toast() just call AlterTableCreateToastTable()?

I didn't think you could call into backend functions like that, and if I
did, it might break something in the future.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On 2014-07-16 10:19:05 -0400, Bruce Momjian wrote:
> > > What would be nice is if I could mark just the tables that will need
> > > toast tables created in that later phase (those tables that didn't have
> > > a toast table in the old cluster, but need one in the new cluster). 
> > > However, I can't see where to store that or how to pass that back into
> > > pg_upgrade.  I don't see a logical place in pg_class to put it.
> > 
> > This seems overengineered. Why not just do
> > SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r';
> > 
> > and in maintain_toast() just call AlterTableCreateToastTable()?
> 
> I didn't think you could call into backend functions like that, and if I
> did, it might break something in the future.

Why not? Pg_upgrade is already intimately linked into to the way the
backend works.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Wed, Jul 16, 2014 at 10:19 AM, Bruce Momjian <bruce@momjian.us> wrote:
> On Mon, Jul 14, 2014 at 09:40:33PM +0200, Andres Freund wrote:
>> On 2014-07-11 09:55:34 -0400, Bruce Momjian wrote:
>> > On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote:
>> > > > Uh, why does this need to be in ALTER TABLE?  Can't this be part of
>> > > > table creation done by pg_dump?
>> > >
>> > > Uh, I think you need to read the thread.  We have to delay the toast
>> > > creation part so we don't use an oid that will later be required by
>> > > another table from the old cluster.  This has to be done after all
>> > > tables have been created.
>> > >
>> > > We could have pg_dump spit out those ALTER lines at the end of the dump,
>> > > but it seems simpler to do it in pg_upgrade.
>> > >
>> > > Even if we have pg_dump create all the tables that require pre-assigned
>> > > TOAST oids first, then the other tables that _might_ need a TOAST table,
>> > > those later tables might create a toast oid that matches a later
>> > > non-TOAST-requiring table, so I don't think that fixes the problem.
>> >
>> > What would be nice is if I could mark just the tables that will need
>> > toast tables created in that later phase (those tables that didn't have
>> > a toast table in the old cluster, but need one in the new cluster).
>> > However, I can't see where to store that or how to pass that back into
>> > pg_upgrade.  I don't see a logical place in pg_class to put it.
>>
>> This seems overengineered. Why not just do
>> SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r';
>>
>> and in maintain_toast() just call AlterTableCreateToastTable()?
>
> I didn't think you could call into backend functions like that, and if I
> did, it might break something in the future.

It would be impossible to write meaningful code in extensions if that
were true.  Yeah, it could break in the future, but it's not
particularly likely, the regression tests should catch it if it
happens, and it's no worse a risk here than in any other extension
code which does this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:
> On Thu, Jul 10, 2014 at 06:17:14PM -0400, Bruce Momjian wrote:
> > Well, we are going to need to call internal C functions, often bypassing
> > their typical call sites and the assumption about locking, etc.  Perhaps
> > this could be done from a plpgsql function.  We could add and drop a
> > dummy column to force TOAST table creation --- we would then only need a
> > way to detect if a function _needs_ a TOAST table, which was skipped in
> > binary upgrade mode previously.
> >
> > That might be a minimalistic approach.
>
> I have thought some more on this.  I thought I would need to open
> pg_class in C and do complex backend stuff, but I now realize I can do
> it from libpq, and just call ALTER TABLE and I think that always
> auto-checks if a TOAST table is needed.  All I have to do is query
> pg_class from libpq, then construct ALTER TABLE commands for each item,
> and it will optionally create the TOAST table if needed.  I just have to
> use a no-op ALTER TABLE command, like SET STATISTICS.

Attached is a completed patch which handles oid conflicts in pg_class
and pg_type for TOAST tables that were not needed in the old cluster but
are needed in the new one.  I was able to recreate a failure case and
this fixed it.

The patch need to be backpatched because I am getting more-frequent bug
reports from users using pg_upgrade to leave now-end-of-life'ed 8.4.
There is not a good work-around for pg_upgrade failures without this
fix, but at least pg_upgrade throws an error.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Tue, Aug  5, 2014 at 07:31:21PM -0400, Bruce Momjian wrote:
> On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:
> > On Thu, Jul 10, 2014 at 06:17:14PM -0400, Bruce Momjian wrote:
> > > Well, we are going to need to call internal C functions, often bypassing
> > > their typical call sites and the assumption about locking, etc.  Perhaps
> > > this could be done from a plpgsql function.  We could add and drop a
> > > dummy column to force TOAST table creation --- we would then only need a
> > > way to detect if a function _needs_ a TOAST table, which was skipped in
> > > binary upgrade mode previously.
> > > 
> > > That might be a minimalistic approach.
> > 
> > I have thought some more on this.  I thought I would need to open
> > pg_class in C and do complex backend stuff, but I now realize I can do
> > it from libpq, and just call ALTER TABLE and I think that always
> > auto-checks if a TOAST table is needed.  All I have to do is query
> > pg_class from libpq, then construct ALTER TABLE commands for each item,
> > and it will optionally create the TOAST table if needed.  I just have to
> > use a no-op ALTER TABLE command, like SET STATISTICS.
> 
> Attached is a completed patch which handles oid conflicts in pg_class
> and pg_type for TOAST tables that were not needed in the old cluster but
> are needed in the new one.  I was able to recreate a failure case and
> this fixed it.
> 
> The patch need to be backpatched because I am getting more-frequent bug
> reports from users using pg_upgrade to leave now-end-of-life'ed 8.4. 
> There is not a good work-around for pg_upgrade failures without this
> fix, but at least pg_upgrade throws an error.

Patch applied through 9.3, with an additional Assert check. 9.2 code was
different enough that there was too high a risk for backpatching.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Based upon the dates the noted patch is not in 9.3.5; which was released a
couple of weeks previous to it being committed.

David J.


nyetter wrote
> I'm not sure it's fixed.  I am attempting a pg_upgrade from 9.2.8 to 9.3.5
> and it dies like so:
> 
> (...many relations restoring successfully snipped...)
> pg_restore: creating SEQUENCE address_address_id_seq
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 1410; 1259 17670 SEQUENCE
> address_address_id_seq javaprod
> pg_restore: [archiver (db)] could not execute query: ERROR:  could not
> create file "base/16414/17670": File exists
> 
> Inspecting a copy of the source cluster, OID 17670 does indeed correspond
> to address_address_id_seq, but inspecting the partially-upgraded cluster
> that OID is taken by pg_toast_202359_index.  Again conferring with a copy
> of the source (9.2.8) cluster, the relation corresponding to filenode
> 202359 does not have a toast table.
> 
> (I know pg-hackers isn't the right place to discuss admin issues, but this
> thread is the only evidence of this bug I can find.  If anyone can suggest
> a workaround I would be infinitely grateful.)
> 
> 
> On Thu, Aug 7, 2014 at 12:57 PM, Bruce Momjian <

> bruce@

> > wrote:
> 
>> On Tue, Aug  5, 2014 at 07:31:21PM -0400, Bruce Momjian wrote:
>> > On Thu, Jul 10, 2014 at 06:38:26PM -0400, Bruce Momjian wrote:
>> > > On Thu, Jul 10, 2014 at 06:17:14PM -0400, Bruce Momjian wrote:
>> > > > Well, we are going to need to call internal C functions, often
>> bypassing
>> > > > their typical call sites and the assumption about locking, etc.
>> Perhaps
>> > > > this could be done from a plpgsql function.  We could add and drop
>> a
>> > > > dummy column to force TOAST table creation --- we would then only
>> need a
>> > > > way to detect if a function _needs_ a TOAST table, which was
>> skipped
>> in
>> > > > binary upgrade mode previously.
>> > > >
>> > > > That might be a minimalistic approach.
>> > >
>> > > I have thought some more on this.  I thought I would need to open
>> > > pg_class in C and do complex backend stuff, but I now realize I can
>> do
>> > > it from libpq, and just call ALTER TABLE and I think that always
>> > > auto-checks if a TOAST table is needed.  All I have to do is query
>> > > pg_class from libpq, then construct ALTER TABLE commands for each
>> item,
>> > > and it will optionally create the TOAST table if needed.  I just have
>> to
>> > > use a no-op ALTER TABLE command, like SET STATISTICS.
>> >
>> > Attached is a completed patch which handles oid conflicts in pg_class
>> > and pg_type for TOAST tables that were not needed in the old cluster
>> but
>> > are needed in the new one.  I was able to recreate a failure case and
>> > this fixed it.
>> >
>> > The patch need to be backpatched because I am getting more-frequent bug
>> > reports from users using pg_upgrade to leave now-end-of-life'ed 8.4.
>> > There is not a good work-around for pg_upgrade failures without this
>> > fix, but at least pg_upgrade throws an error.
>>
>> Patch applied through 9.3, with an additional Assert check. 9.2 code was
>> different enough that there was too high a risk for backpatching.
>>
>> --
>>   Bruce Momjian  <

> bruce@

> >        http://momjian.us
>>   EnterpriseDB                             http://enterprisedb.com
>>
>>   + Everyone has their own god. +
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (

> pgsql-hackers@

> )
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>





--
View this message in context:
http://postgresql.1045698.n5.nabble.com/Pg-upgrade-and-toast-tables-bug-discovered-tp5810447p5817656.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Wed, Sep  3, 2014 at 05:12:30PM -0600, Noah Yetter wrote:
> I'm not sure it's fixed.  I am attempting a pg_upgrade from 9.2.8 to 9.3.5 and
> it dies like so:
> 
> (...many relations restoring successfully snipped...)
> pg_restore: creating SEQUENCE address_address_id_seq
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 1410; 1259 17670 SEQUENCE
> address_address_id_seq javaprod
> pg_restore: [archiver (db)] could not execute query: ERROR:  could not create
> file "base/16414/17670": File exists
> 
> Inspecting a copy of the source cluster, OID 17670 does indeed correspond to
> address_address_id_seq, but inspecting the partially-upgraded cluster that OID
> is taken by pg_toast_202359_index.  Again conferring with a copy of the source
> (9.2.8) cluster, the relation corresponding to filenode 202359 does not have a
> toast table.
> 
> (I know pg-hackers isn't the right place to discuss admin issues, but this
> thread is the only evidence of this bug I can find.  If anyone can suggest a
> workaround I would be infinitely grateful.)

Actually, there was a pg_upgrade fix _after_ the release of 9.3.5 which
explains this failure:
commit 4c6780fd17aa43ed6362aa682499cc2f9712cc8bAuthor: Bruce Momjian <bruce@momjian.us>Date:   Thu Aug 7 14:56:13 2014
-0400   pg_upgrade: prevent oid conflicts with new-cluster TOAST tables    Previously, TOAST tables only required in
thenew cluster could cause    oid conflicts if they were auto-numbered and a later conflicting oid had    to be
assigned.   Backpatch through 9.3
 

Any chance you can download the 9.3.X source tree and try that?  You
need an entire install, not just a new pg_upgrade binary.  I am
disapointed I could not fix this before 9.3.5 was released.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Sep  4, 2014 at 11:37:27AM -0600, Noah Yetter wrote:
> The 9.3.5 release notes contain...
> 
> 
>   • Fix pg_upgrade for cases where the new server creates a TOAST table but the
>     old version did not (Bruce Momjian)
> 
>     This rare situation would manifest as "relation OID mismatch" errors.
> 
> 
> ...which I thought was this bug, hence my confusion.  If anyone else is
> experiencing this bug, they may erroneously be led to believe that 9.3.5
> contains the fix.
> 
> 
> I will attempt to build 9.3 stable head and retry my upgrade.

Yes, please let us know.  The post-9.3.5 fix is for the reverse case,
where the new cluster needs a TOAST table that the old cluster didn't.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Sep  4, 2014 at 01:14:01PM -0600, Noah Yetter wrote:
> Isn't that exactly what the release note says? 
> "where the new server creates a TOAST table but the old version did not" 
> vs. 
> "where the new cluster needs a TOAST table that the old cluster didn't"

Sorry, yes, I got confused.  We have always handled cases where the old
cluster needed a TOAST table and the new cluster didn't.  The 9.3.5 fix
is to prevent a certain failure for a new-only TOAST table:
commit 3088cc37044a303fc50857d8d9e7e44b5c250642Author: Bruce Momjian <bruce@momjian.us>Date:   Mon Jul 7 13:24:08 2014
-0400   pg_upgrade: allow upgrades for new-only TOAST tables    Previously, when calculations on the need for toast
tableschanged,    pg_upgrade could not handle cases where the new cluster needed a TOAST    table and the old cluster
didnot.  (It already handled the opposite    case.)  This fixes the "OID mismatch" error typically generated in this
case.   Backpatch through 9.2
 

The post-9.3.5 fix is for OID conflict that _can_ happen from a new-only
TOAST tables:
commit 4c6780fd17aa43ed6362aa682499cc2f9712cc8bAuthor: Bruce Momjian <bruce@momjian.us>Date:   Thu Aug 7 14:56:13 2014
-0400   pg_upgrade: prevent oid conflicts with new-cluster TOAST tables    Previously, TOAST tables only required in
thenew cluster could cause    oid conflicts if they were auto-numbered and a later conflicting oid had    to be
assigned.   Backpatch through 9.3
 


> At any rate, I've additionally observed that the relation which is blowing up
> pg_upgrade is a VIEW in the source cluster but gets created as a TABLE in the
> upgraded cluster, which may better explain why it had no toast table before and
> now it does.  Is this some kind of expected behavior for views?

Uh, it certainly should not be creating a table instead of a view,
though it will get a pg_class entry.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Sep 4, 2014 at 3:35 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> At any rate, I've additionally observed that the relation which is blowing up
>> pg_upgrade is a VIEW in the source cluster but gets created as a TABLE in the
>> upgraded cluster, which may better explain why it had no toast table before and
>> now it does.  Is this some kind of expected behavior for views?
>
> Uh, it certainly should not be creating a table instead of a view,
> though it will get a pg_class entry.

Actually, there's a way this can happen.  If you create two (or more)
views with circular dependencies between them, then pg_dump will emit
commands to create one of them as a table first, then create the
others as views, then convert the first table to a view by adding a
_SELECT rule to it.

If pg_upgrade's logic can't cope with that, that's a bug in
pg_upgrade, because there's no other way to restore views with
circular dependency chains.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep  4, 2014 at 03:48:17PM -0400, Robert Haas wrote:
> On Thu, Sep 4, 2014 at 3:35 PM, Bruce Momjian <bruce@momjian.us> wrote:
> >> At any rate, I've additionally observed that the relation which is blowing up
> >> pg_upgrade is a VIEW in the source cluster but gets created as a TABLE in the
> >> upgraded cluster, which may better explain why it had no toast table before and
> >> now it does.  Is this some kind of expected behavior for views?
> >
> > Uh, it certainly should not be creating a table instead of a view,
> > though it will get a pg_class entry.
> 
> Actually, there's a way this can happen.  If you create two (or more)
> views with circular dependencies between them, then pg_dump will emit
> commands to create one of them as a table first, then create the
> others as views, then convert the first table to a view by adding a
> _SELECT rule to it.

Wow, that's super-interesting.

> If pg_upgrade's logic can't cope with that, that's a bug in
> pg_upgrade, because there's no other way to restore views with
> circular dependency chains.

I don't see why pg_upgrade would have any problem with it as it just
looks at the old schema and post-restore schema.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Sep  4, 2014 at 03:24:05PM -0600, Noah Yetter wrote:
> Doing the upgrade with an installation built from REL9_3_STABLE at
> commit 52eed3d4267faf671dae0450d99982cb9ba1ac52 was successful.
> 
> The view that I saw get re-created as a table doesn't have any circular
> references, or indeed any references to other views, nor do any other views
> reference it.  But since it does seem that there are valid cases where a view
> gets temporarily re-created as a table during an upgrade, I'm going to assume
> it's not a bug per se.  My upgraded cluster using built-from-source binaries
> has these views as views, so when the process is complete they end up in the
> correct state.
> 
> Is there an expected release date for 9.3.6?

I don't know, but your report makes it clear we will need one sooner
rather than later.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +