Re: Concurrent ALTER SEQUENCE RESTART Regression

On Wed, May 10, 2017 at 2:09 PM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)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