Re: ALTER TABLE ... REPLACE WITH

On Thu, Jan 20, 2011 at 10:07:23AM +0000, Simon Riggs wrote:
> On Wed, 2011-01-19 at 17:46 -0500, Noah Misch wrote:
>
> > 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 I said previously on the thread you quote, having this happen
> implicitly is not a good thing, and IMHO, definitely not "the right
> thing".

When DDL has taken AccessExclusiveLock and a query waits for it, it's the Right
Thing for that query to wake up and proceed based on the complete, final state
of that committed DDL. Aside from the waiting itself, the query should behave
as though it started after the DDL completed.

In my example, the SELECT silently reads data from a table named "old_t". What
if that were an INSERT? The data falls in the wrong table.

> Heikki's suggestion, and your patch, contain no checking to see whether
> the old and new tables are similar. If they are not similar then we have
> all the same problems raised by my patch. SQL will suddenly fail because
> columns have ceased to exist, FKs suddenly disappear etc..

Indeed, Heikki's suggestion and my patch would not do such verification. I
can't see detecting and blocking some patterns of ALTER TABLE RENAME or DROP
...; CREATE ...; than we allow today. Those need to stay open-ended, with the
user responsible for choosing well. So, what's the right concurrent behavior
around use of those statements? I answer that above.

That said, I see utility in a feature that compares two tables, swaps them if
similar, and fixes up foreign keys. Having such a feature does not justify
wrong concurrent behavior around ALTER TABLE RENAME. Having right concurrent
behavior around ALTER TABLE RENAME does not remove the utility of this feature.
We should do both.

> I don't see how having a patch helps at all. I didn't think it was the
> right way before you wrote it and I still disagree now you've written
> it.

Perhaps it helped me more than anyone else, and I should have kept it to myself.
Heikki's suggestion seemed straightforward, so much so that I couldn't figure
why nobody had done it. That would usually mean I'm missing something. So, I
implemented it in a effort to discover what I had missed, failing to do so.
Then, I sent it with the review in case you might spot what I had missed.
Failure to add some kind of table similarity check was intentional, per above.

nm