On Wed, May 10, 2017 at 2:09 PM, Peter Eisentraut <peter.eisentr...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers