Re: ALTER TABLE ... REPLACE WITH

Hi Simon,

I'm reviewing this patch for CommitFest 2011-01.

On Sat, Jan 15, 2011 at 10:02:03PM +0000, Simon Riggs wrote:
> On Tue, 2010-12-14 at 19:48 +0000, Simon Riggs wrote:
> > REPLACE TABLE ying WITH yang
> Patch. Needs work.

First, I'd like to note that the thread for this patch had *four* "me-too"
responses to the use case. That's extremely unusual; the subject is definitely
compelling to people. It addresses the bad behavior of natural attempts to
atomically swap two tables in the namespace:

psql -c "CREATE TABLE t AS VALUES ('old'); CREATE TABLE new_t AS VALUES ('new')"
psql -c 'SELECT pg_sleep(2) FROM t' & # block the ALTER or DROP briefly
sleep 1 # give prev time to take AccessShareLock

# Do it this way, and the next SELECT gets data from the old table.
#psql -c 'ALTER TABLE t RENAME TO old_t; ALTER TABLE new_t RENAME TO t' &
# Do it this way, and get: ERROR: could not open relation with OID 41380
psql -c 'DROP TABLE t; ALTER TABLE new_t RENAME TO t' &

psql -c 'SELECT * FROM t' # I get 'old' or an error, never 'new'.
psql -c 'DROP TABLE IF EXISTS t, old_t, new_t'

by letting you do this instead:

psql -c "CREATE TABLE t AS VALUES ('old'); CREATE TABLE new_t AS VALUES ('new')"
psql -c 'SELECT pg_sleep(2) FROM t' & # block the ALTER or DROP briefly
sleep 1 # give prev time to take AccessShareLock

psql -c 'EXCHANGE TABLE new_t TO t &

psql -c 'SELECT * FROM t' # I get 'new', finally!
psql -c 'DROP TABLE IF EXISTS t, new_t'

I find Heikki's (4D07C6EC(dot)2030200(at)enterprisedb(dot)com) suggestion from the thread
interesting: can we just make the first example work? Even granting that the
second syntax may be a useful addition, the existing behavior of the first
example is surely worthless, even actively harmful. I tossed together a
proof-of-concept patch, attached, that makes the first example DTRT. Do you see
any value in going down that road?

As for your patch itself:

> + /*
> + * Exchange table contents
> + *
> + * Swap heaps, toast tables, toast indexes
> + * all forks
> + * all indexes

For indexes, would you basically swap yin<->yang in pg_index.indrelid, update
pg_class.relnamespace as needed, and check for naming conflicts (when yin and
yang differ in schema)? Is there more?

> + *
> + * Checks:
> + * * table definitions must match

Is there a particular reason to require this, or is it just a simplification to
avoid updating things to match?

> + * * constraints must match

Wouldn't CHECK constraints have no need to match?

> + * * indexes need not match
> + * * outbound FKs don't need to match
> + * * inbound FKs will be set to not validated

We would need to ensure that inbound FOREIGN KEY constraints still have indexes
suitable to implement them. The easiest thing there might be to internally drop
and recreate the constraint, so we get all that verification.

> + *
> + * No Trigger behaviour
> + *
> + * How is it WAL logged? By locks and underlying catalog updates
> + */

I see that the meat of the patch is yet to be written, so this ended up as more
of a design review based on your in-patch comments. Hopefully it's of some
value. I'll go ahead and mark the patch Returned with Feedback.

Thanks,
nm