pg_dump: address DROP COLUMN FIXME and add regression tests

After reviewing the use of ALTER TABLE DROP COLUMN in the pg_dump code,
we feel that the ordering of DDL statements in the dump do not raise any
concerns with respect to vanilla table inheritance hierarchies.

In the current implementation, we need to cascade the DROP COLUMN to
partitions, but we do not want to cascade to "normal" inherited child
tables. Because the child tables are not hooked into the inheritance
hierarchy until after the DROP COLUMN is performed, it looks like ALTER
TABLE and ALTER TABLE ONLY are equivalent for vanilla inheritance.

We've added some regression tests that will hopefully catch any changes
to the above assumptions. The added sql test file is run from not only
pg_regress, but also during the standalone pg_upgrade cluster test. It
is intended to contain corner cases specific to pg_upgrade where the
state that needs to be tested would otherwise be destroyed during a dump
and restore (such as dropped columns).
Co-authored-by: NJacob Champion <pchampion@pivotal.io>
Co-authored-by: NJim Doty <jdoty@pivotal.io>

concourse/scripts/test_upgrade.bash

@@ -98,6 +98,14 @@ load_old_db_data() {
         source '"${OLD_GPHOME}"'/greenplum_path.sh
         unxz < /tmp/dump.sql.xz | '"${psql_env}"' psql '"${psql_opts}"' -f - postgres
     '
+
+    # There are some states, important for upgrade, that can't be reached
+    # by restoring from a dump file only, so we explicitly setup these cases here.
+    scp "${DIRNAME}"/../../src/test/regress/sql/gp_upgrade_cornercases.sql ${MASTER_HOST}:/tmp/
+    ssh -n ${MASTER_HOST} '
+        source '"${OLD_GPHOME}"'/greenplum_path.sh
+        '"${psql_env}"' psql '"${psql_opts}"' -d postgres -f /tmp/gp_upgrade_cornercases.sql
+    '
 }
 
 dump_cluster() {

src/bin/pg_dump/pg_dump.c

@@ -14766,9 +14766,17 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 					}
 
 					/*
-					 * GPDB_94_MERGE_FIXME: Upstream uses ALTER TABLE ONLY
-					 * below. Not sure if gpdb needs to use ONLY or not. For
-					 * now using the gpdb version of query.
+					 * GPDB: Upstream uses ALTER TABLE ONLY below. Because we
+					 * need to cascade the DROP down into partitions as well,
+					 * we use ALTER TABLE instead.
+					 *
+					 * At the moment, we believe this does not cause problems
+					 * for vanilla inherited tables, because the tables aren't
+					 * plugged into the inheritance hierarchy until after this
+					 * code is run (see the ALTER TABLE ... INHERIT below), and
+					 * therefore ALTER TABLE and ALTER TABLE ONLY are
+					 * effectively the same. If that changes, this will need to
+					 * be revisited.
 					 */
 					if (tbinfo->relkind == RELKIND_RELATION)
 						appendPQExpBuffer(q, "ALTER TABLE %s ",

src/test/regress/expected/gp_upgrade_cornercases.out

+-- This file is run from not only pg_regress, but also during the standalone
+-- pg_upgrade cluster test. It contains corner cases specific to pg_upgrade
+-- where the state that needs to be tested would otherwise be destroyed during
+-- a dump and restore (such as dropped columns).
+DROP SCHEMA IF EXISTS upgrade_cornercases CASCADE;
+NOTICE:  schema "upgrade_cornercases" does not exist, skipping
+CREATE SCHEMA upgrade_cornercases;
+SET search_path TO upgrade_cornercases, public;
+-- dropping columns on tables with inheritance
+CREATE TABLE root1 (a int, b int, c int) DISTRIBUTED RANDOMLY;
+CREATE TABLE child1 (d int) INHERITS (root1);
+NOTICE:  Table has parent, setting distribution columns to match parent table
+INSERT INTO root1 VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+INSERT INTO child1
+VALUES (10, 100, 1000, 10000),
+       (11, 111, 1111, 11111),
+       (12, 123, 1234, 12345);
+ALTER TABLE root1 DROP COLUMN b;
+ALTER TABLE ONLY root1 DROP COLUMN c;
+-- adding child columns with name that clashes with dropped column
+CREATE TABLE root2 (a int, b int, c int) DISTRIBUTED RANDOMLY;
+INSERT INTO root2 VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+ALTER TABLE root2 DROP COLUMN a;
+CREATE TABLE child2 (a int, d int) INHERITS (root2);
+NOTICE:  Table has parent, setting distribution columns to match parent table
+INSERT INTO child2
+VALUES (10, 100, 1000, 10000),
+       (11, 111, 1111, 11111),
+       (12, 123, 1234, 12345);
+-- partitions with dropped columns
+CREATE TABLE part (a int, b int, c int)
+    DISTRIBUTED RANDOMLY
+    PARTITION BY RANGE (a) (
+        PARTITION part_1 START (0) INCLUSIVE END (3) EXCLUSIVE,
+        DEFAULT PARTITION part_extra
+);
+NOTICE:  CREATE TABLE will create partition "part_1_prt_part_extra" for table "part"
+NOTICE:  CREATE TABLE will create partition "part_1_prt_part_1" for table "part"
+INSERT INTO part VALUES (1, 2, 3), (4, 5, 6);
+ALTER TABLE part DROP COLUMN c;
+ALTER TABLE ONLY part DROP COLUMN b; -- note the ONLY here!
+-- DROP all the columns
+CREATE TABLE no_cols_left (a int, b int, c int) DISTRIBUTED RANDOMLY;
+INSERT INTO no_cols_left VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+ALTER TABLE no_cols_left DROP COLUMN a;
+ALTER TABLE no_cols_left DROP COLUMN b;
+ALTER TABLE no_cols_left DROP COLUMN c;

src/test/regress/greenplum_schedule

@@ -224,7 +224,7 @@ test: metadata_track
 test: workfile_mgr_test
 test: session_reset
 
-test: psql_gp_commands pg_resetxlog dropdb_check_shared_buffer_cache
+test: psql_gp_commands pg_resetxlog dropdb_check_shared_buffer_cache gp_upgrade_cornercases
 
 # Check for shmem leak for instrumentation slots
 test: instr_in_shmem_verify

src/test/regress/sql/gp_upgrade_cornercases.sql

+-- This file is run from not only pg_regress, but also during the standalone
+-- pg_upgrade cluster test. It contains corner cases specific to pg_upgrade
+-- where the state that needs to be tested would otherwise be destroyed during
+-- a dump and restore (such as dropped columns).
+
+DROP SCHEMA IF EXISTS upgrade_cornercases CASCADE;
+CREATE SCHEMA upgrade_cornercases;
+SET search_path TO upgrade_cornercases, public;
+
+-- dropping columns on tables with inheritance
+CREATE TABLE root1 (a int, b int, c int) DISTRIBUTED RANDOMLY;
+
+CREATE TABLE child1 (d int) INHERITS (root1);
+
+INSERT INTO root1 VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+INSERT INTO child1
+VALUES (10, 100, 1000, 10000),
+       (11, 111, 1111, 11111),
+       (12, 123, 1234, 12345);
+
+ALTER TABLE root1 DROP COLUMN b;
+ALTER TABLE ONLY root1 DROP COLUMN c;
+
+-- adding child columns with name that clashes with dropped column
+CREATE TABLE root2 (a int, b int, c int) DISTRIBUTED RANDOMLY;
+
+INSERT INTO root2 VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+
+ALTER TABLE root2 DROP COLUMN a;
+
+CREATE TABLE child2 (a int, d int) INHERITS (root2);
+
+INSERT INTO child2
+VALUES (10, 100, 1000, 10000),
+       (11, 111, 1111, 11111),
+       (12, 123, 1234, 12345);
+
+-- partitions with dropped columns
+CREATE TABLE part (a int, b int, c int)
+    DISTRIBUTED RANDOMLY
+    PARTITION BY RANGE (a) (
+        PARTITION part_1 START (0) INCLUSIVE END (3) EXCLUSIVE,
+        DEFAULT PARTITION part_extra
+);
+INSERT INTO part VALUES (1, 2, 3), (4, 5, 6);
+
+ALTER TABLE part DROP COLUMN c;
+ALTER TABLE ONLY part DROP COLUMN b; -- note the ONLY here!
+
+-- DROP all the columns
+CREATE TABLE no_cols_left (a int, b int, c int) DISTRIBUTED RANDOMLY;
+
+INSERT INTO no_cols_left VALUES (1, 2, 3), (4, 5, 6), (7, 8, 9);
+
+ALTER TABLE no_cols_left DROP COLUMN a;
+ALTER TABLE no_cols_left DROP COLUMN b;
+ALTER TABLE no_cols_left DROP COLUMN c;