Re: Reducing lock strength of adding foreign keys

On Wed, Oct 22, 2014 at 3:06 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> I have been thinking about why we need to grab an AccessExclusiveLock on the
> table with the PK when we add a foreign key. Adding new tables with foreign
> keys to old ones is common so it would be really nice if the lock strength
> could be reduced.
>
> A comment in the code claims that we need to lock so no rows are deleted
> under us and that adding a trigger will lock in AccessExclusive anyway. But
> with MVCC catalog access and the removal of SnapshotNow I do not see why
> adding a trigger would require an exclusive lock. Just locking for data
> changes should be enough.

The use of MVCC catalog access doesn't necessarily mean that adding a
trigger doesn't require an AccessExclusive lock.  Those changes - if I
dare to say so myself - solved a complex and important problem, but
only one of many problems in this area, and people seem prone to
thinking that they solved more problems than they in fact did.

I think instead of focusing on foreign keys, we should rewind a bit
and think about the locking level required to add a trigger.  If we
figure something out there, then we can consider how it affects
foreign keys.  I went looking for previous discussions of remaining
hazards and found these postings:

http://www.postgresql.org/message-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
http://www.postgresql.org/message-id/20893.1393892127@sss.pgh.pa.us
http://www.postgresql.org/message-id/20140306224340.GA3551655@tornado.leadboat.com

As far as triggers are concerned, the issue of skew between the
transaction snapshot and what the ruleutils.c snapshots do seems to be
the principal issue.  Commit e5550d5fec66aa74caad1f79b79826ec64898688
changed pg_get_constraintdef() to use an MVCC snapshot rather than a
current MVCC snapshot; if that change is safe, I am not aware of any
reason why we couldn't change pg_get_triggerdef() similarly.  Barring
further hazards I haven't thought of, I would expect that we could add
a trigger to a relation with only ShareRowExclusiveLock.  Anything
less than ShareRowExclusiveLock would open up strange timing races
around the firing of triggers by transactions writing the table: they
might or might not notice that a trigger had been added before
end-of-transaction, depending on the timing of cache flushes, which
certainly seems no good.  But even RowExclusiveLock seems like a large
improvement over AccessExclusiveLock.

When a constraint trigger - which is used to implement a foreign key -
is added, there are actually TWO tables involved: the table upon which
the trigger will actually fire, and some other table which is
mentioned in passing in the trigger definition.  It's possible that
the locking requirements for the secondary table are weaker since I
don't think the presence of the trigger actually affects runtime
behavior there.  However, there's probably little point in try to
weaken the lock to less than the level required for the main table
because a foreign key involves adding referential integrity triggers
to both tables.

So I tentatively propose (and with due regard for the possibility
others may see dangers that I've missed) that a reasonable goal would
be to lower the lock strength required for both CREATE TRIGGER and ADD
FOREIGN KEY from AccessExclusiveLock to ShareRowExclusiveLock,
allowing concurrent SELECT and SELECT FOR SHARE against the tables,
but not any actual write operations.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company