(CCing Alvaro, since he implemented KEY SHARE locks) On Oct16, 2014, at 15:51 , Nick Barnes <nickbarne...@gmail.com> wrote: > One of the queries in ri_triggers.c has be a little baffled. > > For (relatively) obvious reasons, a FK insert triggers a SELECT 1 FROM pk_rel > ... FOR KEY SHARE. > For not-so-obvious reasons, a PK delete triggers a SELECT 1 FROM fk_rel ... > FOR KEY SHARE. > > I can't see what the lock on fk_rel achieves. Both operations are already > contending for the lock on the PK row, which seems like enough to cover > every eventuality.
I remember wondering about this too, quite a while ago. That was before we had KEY locks, i.e. it simply read "FOR SHARE" back then. I don't think I ever figured out if and why that lock is necessary - so here's an attempt at unraveling this. The lock certainly isn't required to ensure that we see all the child rows in the fk_rel -- since we're doing this in an AFTER trigger, we're already holding the equivalent of an UPDATE lock on the parent row, so no new fk_rel rows can appear concurrently. Note that "seeing" here refers to an up-to-date snapshot -- in REPEATABLE READ mode and above, our transaction's snapshot doesn't necessarily see those rows, but ri_PerformCheck ensure that we error out if our transaction's snapshot prevents us from seeing any newly added child rows (c.f. detectNewRows). However, DELETing from fk_rel isn't restricted by any of the constraint triggers, so child rows may still be deleted concurrently. So without the lock, it might happen that we report a constraint violation, yet the offending row is already gone by the time we report the error. Note that ri_PerformCheck's detectNewRows flag doesn't enter here -- AFAIK, it only raises an error if the transaction's snapshot sees *fewer* rows than a current snapshot. So AFAICS, the current behaviour (sans the KEY SHARE / SHARE confusion, see below) is this: In READ COMMITED mode, we'll ignore rows that are deleted or reparented concurrently (after waiting for the concurrent transaction to commit, of course) In REPEATABLE READ mode and above, we'll report a serialization error if a child row is deleted or reparented concurrently (if the concurrent transaction committed, of course) Without the lock, we'd report a constraint violation in both cases. So in conclusion, the lock avoids raising constraint violation errors in a few cases in READ COMMITTED mode. In REPEATABLE READ mode, it converts some constraint violation errors into serialization failures. Or at least that's how it looks to me. > And even if the lock serves a purpose, KEY SHARE is an odd choice, since > the referencing field is, in general, not a "key" in this sense. Hm, yeah, that's certainly weird. AFAICS, what this will do is prevent any concurrent update of any columns mentions in any unique index on the *fk_rel* - but as you say, that set doesn't necessarily the set of columns in the FK constraint at all. So currently, it seems that the lock only prevent concurrent DELETES, but not necessarily concurrent UPDATEs, even if such an UPDATE changes the parent that a child row refers to. Independent from whether the lock is actually desirable or not, that inconsistency certainly looks like a bug to me... best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
