Re: FIX: BUG #19687: ALTER SEQUENCE missing lock

Hi,

I applied v1 on top of 3c5d9d914f and built it with cassert. regress
and isolation pass, and Alexander's script no longer fails for me
(200 iterations, with and without the forced parallel plan; on HEAD
it fails within 50).

However, the lock upgrade in v1 is exactly what Ayush described
upthread, not a lock inside RelationSetNewRelfilenumber():

+ LockRelationOid(relid, AccessExclusiveLock);

At this point we already hold ShareRowExclusiveLock, so a transaction
that has read the sequence and then calls nextval() deadlocks with us:

s2: BEGIN; SELECT last_value FROM seq1;
s1: BEGIN; ALTER SEQUENCE seq1 AS int; -- waits
s2: SELECT nextval('seq1');
ERROR: deadlock detected

The same deadlock occurs if s2 runs ALTER SEQUENCE instead of
nextval(). On HEAD, s2 waits for s1 in both cases.

Taking AccessExclusiveLock at the initial RangeVarGetRelidExtended()
call avoids these deadlocks: s1 waits for s2, and s2 can finish.
This also fixes the reported failures. ResetSequence() and
SequenceChangePersistence() already use this lock mode.

This change was also discussed in the bug thread:
https://postgr.es/m/19687-dd094472b0c48afb@postgresql.org

The downside is that ALTER SEQUENCE ... OWNED BY alone would also
block readers, even though it doesn't rewrite the sequence. We could
choose the lock mode from the option list up front to preserve the
current behavior for that case, but I'm not sure the extra complexity
is worth it.

AccessExclusiveLock also covers hot standby, since it is WAL-logged.
On HEAD, a standby query that reads the sequence across the replay of
the ALTER's commit fails with

ERROR: could not open file "base/5/16384": No such file or directory

while with the stronger lock it gets the usual recovery conflict.

Also I think the patch needs a test. An isolation permutation in
sequence-ddl.spec with a cursor open on the sequence works
deterministically: on HEAD the ALTER doesn't wait and the following
FETCH silently returns zero rows, with the fix the ALTER waits.
A SELECT-then-nextval() permutation would catch the deadlock above.

The new comment isn't quite accurate: RelationSetNewRelfilenumber()
doesn't destroy the old file, it is dropped at commit while other
backends may still be reading it.

Best regards,
Andrew Krylosov