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
0001-Add-isolation-tests-for-sequences.patch
Description: invalid/octet-stream
0002-Fix-ALTER-SEQUENCE-locking.patch
Description: invalid/octet-stream
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: http://www.postgresql.org/mailpref/pgsql-hackers
