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

Attachment: 0001-Add-isolation-tests-for-sequences.patch
Description: invalid/octet-stream

Attachment: 0002-Fix-ALTER-SEQUENCE-locking.patch
Description: invalid/octet-stream

Attachment: 0003-Reduce-locking-for-ALTER-SEQUENCE-.-RESTART.patch
Description: invalid/octet-stream

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to