Re: Question about RI checks

Florian Pflug <fgp(at)phlo(dot)org> wrote:
> 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.

I go the REPEATABLE READ case wrong there, and that case is actually broken!.
I initially assumed that trying to lock a row whose latest version is invisible
a transaction in mode REPEATABLE READ or above will result in a serialization
error. But for the queries run by ri_triggers.c, this is not the case.

AFAICS, the only executor node which takes the crosscheck snapshot into account
is nodeModifyTable. So both a plain SELECT and a SELECT FOR [KEY] SHARE | UPDATE
will simply run with the snapshot passed to the SPI, which for the query in
question is always a *current* snapshot (we pass the original snapshot as the
crosscheck snapshot in REPEATABLE READ). Thus, if there's a child row that
is visible to the transaction deleting the parent row, but that child was meanwhile
removed, it seems that we currently *won't* complain. The same holds, of course
for mode SERIALIZABLE -- we don't distinguish the two in ri_triggers.c.

But that's wrong. The transaction's snapshot *would* see that row, so we
ought to raise an error. Note that this applies also to mode SERIALIZABLE, and
breaks true serializability in some cases, since we don't do conflict detection
for RI enforcement queries.

Here's a test case, involving two transaction A and B. I tried this on
REL9_4_STABLE.

Setup
-----
CREATE TABLE parent (id int NOT NULL PRIMARY KEY);
CREATE TABLE child (id int NOT NULL PRIMARY KEY,
parent_id int NOT NULL REFERENCES parent (id));
INSERT INTO parent (id) VALUES (1);
INSERT INTO child (id, parent_id) VALUES (1, 1);

Failure Case
------------
A:: set default_transaction_isolation='serializable';
A:: begin;
A:: select * from child;
-> id | parent_id
----+-----------
1 | 1
B:: set default_transaction_isolation='serializable';
B:: begin;
B:: delete from child;
-> DELETE 1
B:: commit;
A:: delete from parent;
-> DELETE 1
A:: commit;

A can neither come before B in any serializable history (since the DELETE
should fail then), nor after it (since it shouldn't see the row in the child
table then). Yet we don't complain, even though both transaction run in
SERIALIZABLE mode.

On Oct21, 2014, at 19:27 , Nick Barnes <nickbarnes01(at)gmail(dot)com> wrote:
>> On Wed, Oct 22, 2014 at 3:19 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>>
>> It doesn't seem like this analysis considers all of the available ON
>> DELETE and ON UPDATE behaviors available. Besides RESTRICT there is
>> CASCADE, SET NULL, SET DEFAULT, and NO ACTION. Some of those
>> require updating the referencing rows.
>
> I think the logic in question is specific to RESTRICT and NO ACTION.
> The other cases don't look like they need to explicitly lock anything;
> the UPDATE / DELETE itself should take care of that.

Exactly. We run the dubious SELECT FROM <fkrel> ... FOR KEY SHARE query *only*
for RESTRICT and NO ACTION.

I've traced through the revisions of ri_triggers.c a bit. Locking the
conflicting child rows in the parent table's DELETE and UPDATE triggers was
added in commit 0882951b0cdb4c6686e57121d56216cb2044f7eb which is dated
1999-12-08. (This was before we even has row-level SHARE locks, so the code
did a SELECT .. FOR UPDATE back then). This is also the commit which added
FOR UPDATE locking to RI_FKey_check, i.e. which ensured that parent rows are
locked when adding new child rows. It's commit message unfortunately doesn't
offer much in terms of explanations - it just says "Fixed concurrent visibility
bug."

When SHARE locks where implemented in commit bedb78d386a47fd66b6cda2040e0a5fb545ee371,
dating from 2005-04-28, it seems that "FOR UPDATE" was simply replaced by
"FOR SHARE" throughout ri_triggers.c. The same seems to have happened when
KEY SHARE locks where introduced on 2013-01-13 with commit
0ac5ad5134f2769ccbaefec73844f8504c4d6182.

best regards,
Florian Pflug