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

On 2015-01-16 15:16:20 +0100, Andreas Karlsson wrote:
> Indeed. As Noah and I discussed previously in this thread we would need to
> do quite a bit of refactoring of ruleutils.c to make it fully MVCC.

Right.

> 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.

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.

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.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services