Re: Concurrent ALTER SEQUENCE RESTART Regression

On 5/7/17 19:43, Andres Freund wrote:
> 3. Keep the catalog, make ALTER properly transactional, blocking
> concurrent nextval()s. This resolves the issue that nextval() can't
> see the changed definition of the sequence.

This was the intended choice.

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.

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.

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).

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.

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).

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services