Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)

On Fri, Dec 18, 2015 at 12:53 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:

> Hmm. So one thing against this patch is that some results of existing
> isolation tests change. For instance, this one (lock-update-delete):
>
> starting permutation: s2b s1l s2u s2_blocker3 s2c s2_unlock

[I had trouble reading your summary -- redone, hopefully better]

session 2
---------
SELECT pg_advisory_lock(0);
BEGIN;

session 1
---------
SELECT * FROM foo
WHERE pg_advisory_xact_lock(0) IS NOT NULL
AND key = 1 FOR KEY SHARE;
<session 1 blocks>

UPDATE foo SET value = 2 WHERE key = 1;
UPDATE foo SET value = 2 WHERE key = 1;
SELECT pg_advisory_unlock(0);

<session 1 unblocks>

COMMIT;

> The initial SELECT is blocked and returns the updated tuple; if I patch
> as proposed above, the returned tuple is the *original* tuple (i.e. it
> returns value=1 instead). I think there's room for arguing that that is
> the correct result; since the tuple lock acquired by FOR KEY SHARE does
> not conflict with the UPDATE, what would be the reason to reject the
> original tuple?

I completely agree -- if SELECT ... FOR KEY SHARE is not blocked,
it should return the original value. I consider it a bug to do
otherwise. If it *is* blocked, then it should follow the chain and
show the latest version of the row (or not find the row visible if
it has been deleted).

> In fact, in the permutation that does the advisory_unlock before
> the commit, that's exactly what happens (value=1 is returned).

Right; why should it be different if the commit comes first?

> Also, in the repeatable read isolation level, the expected file has this
> permutation as failing with "could not serialize" -- but with the patch,
> it no longer fails and instead it returns the value=1 tuple instead.
> Kevin, what's your opinion on that change? Is this case supposed to
> raise an error or not?

At the REPEATABLE READ isolation level, you are only supposed to
get a serialization failure on a write conflict. I don't see why a
SELECT ... FOR KEY SHARE against the row should count as a write
for that purpose, unless there is a conflicting row lock (including
those acquired by DELETE or UPDATE statements). At the
SERIALIZABLE transaction isolation level there is no escaping the
fact that there can be some false positives (where a serialization
failure may occur even where it is not strictly necessary), and the
documentation mentions this; but at the REPEATABLE READ isolation
level it is arguably a bug to generate false positives. If it does
not throw an error, the only valid value to return would be 1.

I don't see a reason that a SERIALIZABLE transaction cannot behave
the same as a REPEATABLE READ transaction for this case, but at
least it would not necessarily be a bug to get a false positive
(unnecessary serialization failure) in that case.

> I have a hard time convincing myself that it's acceptable to back-patch
> such a change, in any case. I observed no other regression failure, but
> what did change does make me a bit uncomfortable.

I think it is fair to consider it a bug fix, but not necessarily
for a serious enough bug to back-patch, especially if it is
reasonable to think that people might be counting on the current
behavior. I'm not sure that I buy that anyone will be, though,
especially since a tiny shift in timing that would not generally be
controlled by the user will generate the post-patch value anyway.

It would be interesting to hear whether anyone else thinks that the
old behavior is something someone might be counting on.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company