Re: [HACKERS] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression - Mailing list pgsql-hackers

On Wed, May 10, 2017 at 2:09 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I think I have more clarity about the different, overlapping issues:
>
> 1. Concurrent ALTER SEQUENCE causes "tuple concurrently updated"
>    error, caused by updates to pg_sequence catalog.  This can be fixed
>    by taking a self-exclusive lock somewhere.
>
>    A typical answer for other catalogs is to use
>    ShareRowExclusiveLock.  But in another context it was argued that
>    is undesirable for other reasons, and it's better to stick with
>    RowExclusiveLock and deal with the occasional consequences.  But
>    this question is obsoleted by #2.

Yes.

> 2. There is some inconsistency or disagreement about what to lock
>    when modifying relation metadata.  When you alter a non-relation
>    object, you just have the respective catalog to lock, and you have
>    to make the trade-offs described in #1.  When you alter a relation
>    object, you can lock the catalog or the actual relation, or both.
>    I had gone with locking the catalog, but existing practice in
>    tablecmds.c is to lock the relation with the most appropriate lock
>    level, and use RowExclusiveLock for the catalog.  We can make
>    sequences do that, too.

Agreed, sequences are relations as well at the end. Consistency sounds
good to me.

> 3. Sequence WAL writes and catalog updates are not protected by same
>    lock.  We can fix that by holding a lock longer.  If per #2 we make
>    the lock on the sequence the "main" one, then we just hold that one
>    across the catalog update.
>
> 4. Some concerns about in AlterSequence() opening the pg_sequence
>    catalog while holding the sequence buffer lock.  Move some things
>    around to fix that.
>
> 5. nextval() disregarding uncommitted ALTER SEQUENCE changes.  In
>    <PG10, it would read the uncommitted metadata and observe it.
>    Currently, it goes ahead even if there is an uncommitted ALTER
>    SEQUENCE pending that would prohibit what it's about to do (e.g.,
>    MAXVALUE change).

Yes, and we want to block all nextval() calls until concurrent ALTER
SEQUENCE are committed.

>    I think the correct fix is to have nextval() and ALTER SEQUENCE use
>    sensible lock levels so that they block each other.  Since
>    nextval() currently uses AccessShareLock, the suggestion was for
>    ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
>    better idea would be for nextval() to use RowExclusiveLock
>    (analogous to UPDATE) and ALTER SEQUENCE to use
>    ShareRowExclusiveLock, which would also satisfy issue #1.

No objections to that.

> Attached patches:
>
> 0001 Adds isolation tests for sequence behavior, in particular regarding
> point #5.  The expected files in 0001 show how it behaves in 9.6, and if
> you apply it to 10 without the subsequent fixes, it will show some problems.
>
> 0002 Locking changes discussed above, with test changes.
>
> 0003 (optional) Reduce locking if only RESTART is specified (because
> that does not do a catalog update, so it can run concurrently with nextval).

Looking at 0001 and 0002... So you are correctly blocking nextval()
when ALTER SEQUENCE holds a lock on the sequence object. And
concurrent calls of nextval() don't conflict. As far as I can see this
matches the implementation of 3.

Here are some minor comments.

+   /* lock page' buffer and read tuple into new sequence structure */
Nit: there is a single quote in this comment.
/*
+ * TODO rename for lock level change
+ * * Open the sequence and acquire AccessShareLock if needed *
Some of the functions calling init_sequence() reference
AccessShareLock in comments. Those will need an update as well.

+++ b/src/test/isolation/expected/sequence-nextval.out
@@ -0,0 +1,35 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1a s1b s2a s2b s1c s1d s2c s2d
+step s1a: SELECT nextval('seq1');
+nextval
I am not sure that this brings much value...
-- 
Michael