Re: ALTER TABLE lock downgrades have broken pg_upgrade - Mailing list pgsql-hackers

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

diff --git a/src/test/regress/expected/indirect_toast.out b/src/test/regress/expected/indirect_toast.out
index 4f4bf41..3ed0189 100644
*** a/src/test/regress/expected/indirect_toast.out
--- b/src/test/regress/expected/indirect_toast.out
*************** SELECT substring(toasttest::text, 1, 200
*** 149,151 ****
--- 149,177 ----

  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+ SELECT relname, reltoastrelid <> 0 AS has_toast_table
+    FROM pg_class
+    WHERE oid::regclass IN ('i_have_useless_toast_table', 'i_need_a_toast_table')
+    ORDER BY 1;
+           relname           | has_toast_table
+ ----------------------------+-----------------
+  i_have_useless_toast_table | t
+  i_need_a_toast_table       | f
+ (2 rows)
+
diff --git a/src/test/regress/sql/indirect_toast.sql b/src/test/regress/sql/indirect_toast.sql
index d502480..15640fc 100644
*** a/src/test/regress/sql/indirect_toast.sql
--- b/src/test/regress/sql/indirect_toast.sql
*************** SELECT substring(toasttest::text, 1, 200
*** 59,61 ****
--- 59,85 ----

  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+
+ SELECT relname, reltoastrelid <> 0 AS has_toast_table
+    FROM pg_class
+    WHERE oid::regclass IN ('i_have_useless_toast_table', 'i_need_a_toast_table')
+    ORDER BY 1;