Re: PATCH: Reducing lock strength of trigger and foreign key DDL

On Fri, Jan 16, 2015 at 04:59:56PM +0100, Andres Freund wrote:
> On 2015-01-16 15:16:20 +0100, Andreas Karlsson wrote:
> > For this reason I opted to only lower the lock levels of ADD and ALTER
> > TRIGGER, and not DROP TRIGGER. Neither of those require MVCC of then
> > WHEN clause.
>
> I'm unconvinced that this is true. Using a snapshot for part of getting
> a definition certainly opens the door for getting strange
> results.
>
> Acquiring a lock that prevents schema changes on the table and then
> getting the definition using the syscaches guarantees that that
> definition is at least self consistent because no further schema changes
> are possible and the catalog caches will be up2date.
>
> What you're doing though is doing part of the scan using the
> transaction's snapshot (as used by pg_dump that will usually be a
> repeatable read snapshot and possibly quite old) and the other using a
> fresh catalog snapshot. This can result in rather odd things.
>
> Just consider:
> S1: CREATE TABLE flubber(id serial primary key, data text);
> S1: CREATE FUNCTION blarg() RETURNS TRIGGER LANGUAGE plpgsql AS $$BEGIN RETURN NEW; END;$$;
> S1: CREATE TRIGGER flubber_blarg BEFORE INSERT ON flubber FOR EACH ROW WHEN (NEW.data IS NOT NULL) EXECUTE PROCEDURE blarg();
> S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> S2: SELECT 'dosomethingelse';
> S1: ALTER TABLE flubber RENAME TO wasflubber;
> S1: ALTER TABLE wasflubber RENAME COLUMN data TO wasdata;
> S1: ALTER TRIGGER flubber_blarg ON wasflubber RENAME TO wasflubber_blarg;
> S1: ALTER FUNCTION blarg() RENAME TO wasblarg;
> S2: SELECT pg_get_triggerdef(oid) FROM pg_trigger;
>
> This will give you: The old trigger name. The new table name. The new
> column name. The new function name.
>
> I don't think using a snapshot for tiny parts of these functions
> actually buys anything. Now, this isn't a pattern you introduced. But I
> think we should think about this for a second before expanding it
> further.

Fair enough. It did reinforce pg_get_constraintdef() as a subroutine of
pg_dump rather than an independent, rigorous interface. It perhaps made the
function worse for non-pg_dump callers. In that vein, each one of these hacks
has a cost. One could make a reasonable argument that ALTER TRIGGER RENAME
locking is not important enough to justify spreading the hack from
pg_get_constraintdef() to pg_get_triggerdef(). Lowering the CREATE TRIGGER
lock level does not require any ruleutils.c change for the benefit of pg_dump,
because pg_dump won't see the pg_trigger row of a too-recent trigger.

> Before you argue that this isn't relevant for pg_dump: It is. Precisely
> the above can happen - just replace the 'dosomethingelse' with several
> LOCK TABLEs as pg_dump does. The first blocks after a snapshot has been
> acquired. While waiting, all the ALTERs happen.

We wish pg_dump would take a snapshot of the database; that is, we wish its
output always matched some serial execution of transactions. pg_dump has,
since ancient times, failed to achieve that if non-table DDL commits during
the dump or if table DDL commits between acquiring the dump transaction
snapshot and acquiring the last table lock. My reviews have defended the
standard that table DDL issued after pg_dump has acquired locks does not
change the dump. That's what we bought with pg_get_constraintdef()'s use of
the transaction snapshot and would buy with the same in pg_get_triggerdef().
My reviews have deliberately ignored effects on scenarios where pg_dump
already fails to guarantee snapshot-like output.

> Arguably the benefit here is that the window for this issue is becoming
> smaller as pg_dump (and hopefully other possible callers) acquire
> exclusive locks on the table. I.e. that the lowering of the lock level
> doesn't introduce new races. But on the other side of the coin, this
> makes the result of pg_get_triggerdef() even *more* inaccurate in many
> cases.

What is this about pg_dump acquiring exclusive locks?

To summarize, the problem you raise has been out of scope, because it affects
pg_dump only at times when pg_dump is already wrong.