ALTER TABLE lock downgrades have broken pg_upgrade

Started by Tom Lanealmost 10 years ago27 messageshackers
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

There is logic in pg_upgrade plus the backend, mostly added by commit
4c6780fd1, to cope with the corner cases that sometimes arise where the
old and new versions have different ideas about whether a given table
needs a TOAST table. The more common case is where there's a TOAST table
in the old DB, but (perhaps as a result of having dropped all the wide
columns) the new cluster doesn't think the table definition requires a
TOAST table. The reverse is also possible, although according to the
existing code comments it can only happen when upgrading from pre-9.1.
The way pg_upgrade handles that case is that after running all the
table creation operations it issues this command:

PQclear(executeQueryOrDie(conn, "ALTER TABLE %s.%s RESET (binary_upgrade_dummy_option);",
quote_identifier(PQgetvalue(res, rowno, i_nspname)),
quote_identifier(PQgetvalue(res, rowno, i_relname))));

which doesn't actually do anything (no such reloption being set) but
nonetheless triggers a call of AlterTableCreateToastTable, which
will cause a toast table to be created if the new server thinks the
table definition requires one.

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock. Now you get a failure.
I haven't tried to construct a pre-9.1 database that would trigger
this, but you can make it happen by applying the attached patch
to create a toast-table-less table in the regression tests,
and then doing "make check" in src/bin/pg_upgrade. You get this:

...
Restoring database schemas in the new cluster
ok
Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_once_had_a_toast_table" RESET (binary_upgrade_dummy_option);
ERROR: AccessExclusiveLock required to add toast table.

Failure, exiting
+ rm -rf /tmp/pg_upgrade_check-o0CUMm
make: *** [check] Error 1

I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

regards, tom lane

Attachments:

fake-toast-less-table.patchtext/x-diff; charset=us-ascii; name=fake-toast-less-table.patchDownload+15-0
#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

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

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

#3Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#2)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

Thanks!

Stephen

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#3)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

If we can put together a script that runs test.sh for various versions
and then verifies the runs, we could use it in both buildfarm and
coverage.

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

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

#5Andrew Dunstan
andrew@dunslane.net
In reply to: Stephen Frost (#3)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

I have an module that does it, although it's not really stable enough.
But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm&gt;

cheers

andrew

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

#6Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#4)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

If we can put together a script that runs test.sh for various versions
and then verifies the runs, we could use it in both buildfarm and
coverage.

One other point is that pg_dump goes quite a bit farther back than just
what we currently support (or at least, it tries to). I think that,
generally, that's a good thing, but it does mean we have a lot of cases
that don't get tested nearly as much...

I was able to get back to 7.4 up and running without too much trouble,
but even that doesn't cover all the cases we have in pg_dump. I'm not
sure if we want to define a "we will support pg_dump back to X" cut-off
or if we want to try and get older versions to run on modern systems,
but it's definitely worth pointing out that we're trying to support much
farther back than what is currently supported in pg_dump today.

Thanks!

Stephen

#7Andrew Dunstan
andrew@dunslane.net
In reply to: Andrew Dunstan (#5)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 05/03/2016 01:28 PM, Andrew Dunstan wrote:

On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the
latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

I have an module that does it, although it's not really stable enough.
But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm&gt;

Incidentally, just as a warning for anyone trying, this uses up a quite
a lot of disk space.

You would need several GB available.

cheers

andrew

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

#8Andrew Dunstan
andrew@dunslane.net
In reply to: Andrew Dunstan (#7)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 05/03/2016 01:33 PM, Andrew Dunstan wrote:

On 05/03/2016 01:28 PM, Andrew Dunstan wrote:

On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Tom Lane wrote:

More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the
latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

I have an module that does it, although it's not really stable
enough. But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm&gt;

Incidentally, just as a warning for anyone trying, this uses up a
quite a lot of disk space.

You would need several GB available.

And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

cheers

andrew

Attachments:

upgrade_dump_diff.tgzapplication/x-tar; name=upgrade_dump_diff.tgzDownload
#9Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#6)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Stephen Frost wrote:

One other point is that pg_dump goes quite a bit farther back than just
what we currently support (or at least, it tries to). I think that,
generally, that's a good thing, but it does mean we have a lot of cases
that don't get tested nearly as much...

I was able to get back to 7.4 up and running without too much trouble,
but even that doesn't cover all the cases we have in pg_dump. I'm not
sure if we want to define a "we will support pg_dump back to X" cut-off
or if we want to try and get older versions to run on modern systems,
but it's definitely worth pointing out that we're trying to support much
farther back than what is currently supported in pg_dump today.

Yeah. Trying to compile old stuff with current tools (Debian jessie):

7.0's configure does not recognize my system:

checking host system type... Invalid configuration `x86_64-unknown-linux-gnu': machine `x86_64-unknown' not recognized

7.1's configure fails for accept() detection:

checking types of arguments for accept()... configure: error: could not determine argument types

7.2's configure works, but apparently it failed to find flex:

make[3]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/backend/bootstrap'
***
ERROR: `flex' is missing on your system. It is needed to create the
file `bootscanner.c'. You can either get flex from a GNU mirror site
or download an official distribution of PostgreSQL, which contains
pre-packaged flex output.
***
Makefile:60: recipe for target 'bootscanner.c' failed
make[3]: *** [bootscanner.c] Error 1

7.3 fails in ecpg preprocessor:

make[4]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/interfaces/ecpg/preproc'
make -C ../../../../src/port all
make[5]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/port'
make[5]: Nothing to be done for 'all'.
make[5]: Leaving directory '/home/alvherre/Code/pgsql/source/throwaway/src/port'
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes -Wmissing-declarations -Wno-error -I./../include -I. -I../../../../src/include -DMAJOR_VERSION=2 -DMINOR_VERSION=10 -DPATCHLEVEL=0 -DINCLUDE_PATH=\"/usr/local/pgsql/include\" -c -o preproc.o preproc.c
In file included from preproc.y:5571:0:
pgc.c:170:18: error: conflicting types for ‘yyleng’
extern yy_size_t yyleng;
^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:304:11: error: conflicting types for ‘yyleng’
yy_size_t yyleng;
^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:2723:16: warning: ‘input’ defined but not used [-Wunused-function]
static int input (void)
^
<builtin>: recipe for target 'preproc.o' failed
make[4]: *** [preproc.o] Error 1

7.4 seems to work fine. I suppose it should be fine to remove pg_dump's
support for pre-7.3; people wanting to upgrade from before 7.3 (if any)
could use 9.6's pg_dump as an intermediate jump.

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

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

#10Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Hi,

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

- Andres

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

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Stephen Frost (#6)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Stephen Frost <sfrost@snowman.net> writes:

One other point is that pg_dump goes quite a bit farther back than just
what we currently support (or at least, it tries to). I think that,
generally, that's a good thing, but it does mean we have a lot of cases
that don't get tested nearly as much...

Yeah. I do periodically fire up servers back to 7.0 and see if pg_dump
can dump from them, but I don't pretend that that's a very thorough test.

I was able to get back to 7.4 up and running without too much trouble,
but even that doesn't cover all the cases we have in pg_dump. I'm not
sure if we want to define a "we will support pg_dump back to X" cut-off
or if we want to try and get older versions to run on modern systems,
but it's definitely worth pointing out that we're trying to support much
farther back than what is currently supported in pg_dump today.

I've been thinking of proposing that it's time (not now, at this point,
but for 9.7) to rip out libpq's support for V2 protocol as well as
pg_dump's support for pre-7.4 backends. That's a quite significant
amount of what at this point is very poorly tested code. And I doubt
that it would be productive to try to improve the test coverage rather
than just removing it.

There might be an argument for moving pg_dump's cutoff further than that,
but going to 7.3 or later is significant because it would allow removal of
the kluges for schema-less and dependency-less servers. I suggested 7.4
because it'd comport with removal of V2 wire protocol support, and because
7.4 is also our cutoff for describe support in psql.

I'm hesitant to move the cutoff really far, because we do still hear from
people running really old versions, and sooner or later those people will
want to upgrade. It'd be good if they could use a modern pg_dump for the
purpose.

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

#12Stephen Frost
sfrost@snowman.net
In reply to: Andres Freund (#10)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* Andres Freund (andres@anarazel.de) wrote:

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

+1

Thanks!

Stephen

#13Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#11)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 2016-05-03 13:47:14 -0400, Tom Lane wrote:

I've been thinking of proposing that it's time (not now, at this point,
but for 9.7) to rip out libpq's support for V2 protocol as well as
pg_dump's support for pre-7.4 backends.

+1

There might be an argument for moving pg_dump's cutoff further than that,
but going to 7.3 or later is significant because it would allow removal of
the kluges for schema-less and dependency-less servers. I suggested 7.4
because it'd comport with removal of V2 wire protocol support, and because
7.4 is also our cutoff for describe support in psql.

I think we can be a lot more aggressive moving the cuttoff for psql than
for pg_dump; but that's more an argument ripping out some old psql code.

I'm hesitant to move the cutoff really far, because we do still hear from
people running really old versions, and sooner or later those people will
want to upgrade. It'd be good if they could use a modern pg_dump for the
purpose.

I think we should consider making the cutoff point for pg_dump somewhat
predicatable. E.g. saying that we support 5 more versions than the
actually maintained ones. The likelihood of breakages seems to
increase a good bit for older versions.

Andres

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

#14Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Andrew Dunstan (#8)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Andrew Dunstan wrote:

And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

Interesting, thanks. I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear. (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world". Could we add a buildfarm
script to run, standalone?

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

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

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#10)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Andres Freund <andres@anarazel.de> writes:

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

We used to have an explicit "ALTER TABLE CREATE TOAST TABLE", IIRC,
but we got rid of it. In any case, forcible creation is not what
we're after here, we just want to create it if needs_toast_table()
thinks we need one. I'm fine with it being a hack, as long as we
have test coverage so we notice when somebody breaks the hack.

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

#16Andrew Dunstan
andrew@dunslane.net
In reply to: Alvaro Herrera (#14)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 05/03/2016 01:58 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:

And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

Interesting, thanks. I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear. (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world". Could we add a buildfarm
script to run, standalone?

Well, to run it you just run a buildfarm animal, possibly not even
registered, with the module enabled. The module is actually in every
buildfarm release, and has been for some time, but it's not enabled.
Right now even if it runs it doesn't report anything to the server, it
just outputs success/failure lines to stdout.

cheers

andrew

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

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

I wrote:

I haven't tried to construct a pre-9.1 database that would trigger
this, but you can make it happen by applying the attached patch
to create a toast-table-less table in the regression tests,
and then doing "make check" in src/bin/pg_upgrade. You get this:

...
Restoring database schemas in the new cluster
ok
Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_once_had_a_toast_table" RESET (binary_upgrade_dummy_option);
ERROR: AccessExclusiveLock required to add toast table.
Failure, exiting

I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

I tried fixing it like that. The alternate RESET target had behaved as
expected when I'd tested by hand, but in pg_upgrade it still fails,
only now with

Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_need_a_toast_table" RESET (user_catalog_table);
ERROR: pg_type OID value not set when in binary upgrade mode

This implies that there was some totally other patch, probably quite
pg_upgrade-specific, that broke this case independently of the
lock-downgrade change.

My conclusion is that we probably do need a specific pg_upgrade support
function to handle the case, rather than trying to sneak it through via
ALTER TABLE, which means that we won't be able to back-patch a fix.

I have no more time to work on this, but I think it needs to be fixed, and
I definitely think we had better put in test coverage when we do fix it.
Attached is a proposed patch that adds regression test coverage for this
and a related case (and triggers the failures I've been complaining of).

regards, tom lane

Attachments:

pg_upgrade-toast-tests.patchtext/x-diff; charset=us-ascii; name=pg_upgrade-toast-tests.patchDownload+50-0
#18Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#17)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

I wrote:

I have no more time to work on this, but I think it needs to be fixed, and
I definitely think we had better put in test coverage when we do fix it.

Actually, there is a really easy fix we could put in, which is to decide
that optionally_create_toast_tables() is useless and get rid of it.

Point 1: if a table did not have a TOAST table in the old database,
any decision that it needs one in the new database must be a very
close-to-the-edge situation; certainly the 9.2 change we had in this area
was a matter of rounding things off differently. needs_toast_table()'s
threshold for max tuple length is only a quarter page, so there's a huge
amount of daylight between where we'd choose to create a toast table and
where users would actually see failures from not having one. It's pretty
hard to believe that cross-version differences in the do-we-need-a-toast-
table heuristic would exceed that.

Point 2: the current code is broken, and will cause a pg_upgrade failure
if the situation of concern occurs. That's certainly much worse than
not adding a marginally-useful toast table.

Point 3: in view of point 2, the lack of field complaints says that this
situation doesn't actually happen in the field.

Barring complaints, I'll fix this bug by removing that code altogether.

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

#19Simon Riggs
simon@2ndQuadrant.com
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 3 May 2016 at 18:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.

On reflection, this still seems like a good idea.

Now you get a failure.

Failure condition as an exception to that.

I haven't tried to construct a pre-9.1 database that would trigger
this, but you can make it happen by applying the attached patch
to create a toast-table-less table in the regression tests,
and then doing "make check" in src/bin/pg_upgrade. You get this:

...
Restoring database schemas in the new cluster
ok
Creating newly-required TOAST tables SQL command
failed
ALTER TABLE "public"."i_once_had_a_toast_table" RESET
(binary_upgrade_dummy_option);
ERROR: AccessExclusiveLock required to add toast table.

Failure, exiting

It appears that pg_upgrade is depending upon an undocumented side-effect of
ALTER TABLE RESET.

I would say this side-effect should not exist, which IIUC is the same
conclusion on your latest post.

If pg_upgrade needs this, we should implement a specific function that does
what pg_upgrade needs. That way we can isolate the requirement for an
AccessExclusiveLock to the place that needs it: pg_upgrade. That will also
make it less fragile in the future. I don't think that needs a specific
command, just a function.

I accept that it is my bug and should fix it.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Simon Riggs (#19)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Simon Riggs <simon@2ndquadrant.com> writes:

On 3 May 2016 at 18:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.

On reflection, this still seems like a good idea.

Yes, what pg_upgrade was doing was clearly a hack, and not a very nice one.

I accept that it is my bug and should fix it.

It's moot at this point, see 1a2c17f8e.

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

#21Simon Riggs
simon@2ndQuadrant.com
In reply to: Tom Lane (#20)
#22Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#9)
#23Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#1)
#24Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#17)
#25Peter Eisentraut
peter_e@gmx.net
In reply to: Alvaro Herrera (#4)
#26Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#25)
#27Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Eisentraut (#25)