From: Peter Eisentraut =0A= Date: Wed, 10 May 2017 00:18:05 -0400=0A= Subject: [PATCH 2/3] Fix ALTER SEQUENCE locking=0A= =0A= In 1753b1b027035029c2a2a1649065762fafbf63f3, the pg_sequence system=0A= catalog was introduced. This made sequence metadata changes=0A= transactional, while the actual sequence values are still behaving=0A= nontransactionally. This requires some refinement in how ALTER=0A= SEQUENCE, which operates on both, locks the sequence and the catalog.=0A= =0A= The main problems were:=0A= =0A= - Concurrent ALTER SEQUENCE causes "tuple concurrently updated" error,=0A= caused by updates to pg_sequence catalog.=0A= =0A= - Sequence WAL writes and catalog updates are not protected by same=0A= lock, which could lead to inconsistent recovery order.=0A= =0A= - nextval() disregarding uncommitted ALTER SEQUENCE changes.=0A= =0A= To fix, nextval() and friends now lock the sequence using=0A= RowExclusiveLock instead of AccessShareLock. ALTER SEQUENCE locks the=0A= sequence using ShareRowExclusiveLock. This means that nextval() and=0A= ALTER SEQUENCE block each other, and ALTER SEQUENCE on the same sequence=0A= blocks itself. Also, rearrange some code so that the entire=0A= AlterSequence is protected by the lock on the sequence.=0A= =0A= XXX credits=0A= ---=0A= src/backend/commands/sequence.c | 22 ++++++++++++----------= =0A= src/test/isolation/expected/sequence-ddl.out | 8 +++++---=0A= src/test/isolation/specs/sequence-ddl.spec | 5 ++++-=0A= 3 files changed, 21 insertions(+), 14 deletions(-)=0A= =0A= diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequenc= e.c=0A= index 4a56926b53..44cfcef66b 100644=0A= --- a/src/backend/commands/sequence.c=0A= +++ b/src/backend/commands/sequence.c=0A= @@ -427,7 +427,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)= =0A= HeapTuple tuple;=0A= =20=0A= /* Open and lock sequence. */=0A= - relid =3D RangeVarGetRelid(stmt->sequence, AccessShareLock, stmt->missing= _ok);=0A= + relid =3D RangeVarGetRelid(stmt->sequence, ShareRowExclusiveLock, stmt->m= issing_ok);=0A= if (relid =3D=3D InvalidOid)=0A= {=0A= ereport(NOTICE,=0A= @@ -443,12 +443,6 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)= =0A= aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,=0A= stmt->sequence->relname);=0A= =20=0A= - /* lock page' buffer and read tuple into new sequence structure */=0A= - seqdata =3D read_seq_tuple(seqrel, &buf, &seqdatatuple);=0A= -=0A= - /* Copy old sequence data into workspace */=0A= - memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data));=0A= -=0A= rel =3D heap_open(SequenceRelationId, RowExclusiveLock);=0A= tuple =3D SearchSysCacheCopy1(SEQRELID,=0A= ObjectIdGetDatum(relid));=0A= @@ -458,6 +452,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)= =0A= =20=0A= seqform =3D (Form_pg_sequence) GETSTRUCT(tuple);=0A= =20=0A= + /* lock page' buffer and read tuple into new sequence structure */=0A= + seqdata =3D read_seq_tuple(seqrel, &buf, &seqdatatuple);=0A= +=0A= + /* Copy old sequence data into workspace */=0A= + memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data));=0A= +=0A= /* Check and set new values */=0A= init_params(pstate, stmt->options, stmt->for_identity, false, seqform, &c= hanged_seqform, &newseqdata, &owned_by);=0A= =20=0A= @@ -508,12 +508,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)= =0A= =20=0A= ObjectAddressSet(address, RelationRelationId, relid);=0A= =20=0A= - relation_close(seqrel, NoLock);=0A= -=0A= if (changed_seqform)=0A= CatalogTupleUpdate(rel, &tuple->t_self, tuple);=0A= heap_close(rel, RowExclusiveLock);=0A= =20=0A= + relation_close(seqrel, NoLock);=0A= +=0A= return address;=0A= }=0A= =20=0A= @@ -1042,6 +1042,8 @@ setval3_oid(PG_FUNCTION_ARGS)=0A= =20=0A= =20=0A= /*=0A= + * TODO rename for lock level change=0A= + *=0A= * Open the sequence and acquire AccessShareLock if needed=0A= *=0A= * If we haven't touched the sequence already in this transaction,=0A= @@ -1063,7 +1065,7 @@ open_share_lock(SeqTable seq)=0A= PG_TRY();=0A= {=0A= CurrentResourceOwner =3D TopTransactionResourceOwner;=0A= - LockRelationOid(seq->relid, AccessShareLock);=0A= + LockRelationOid(seq->relid, RowExclusiveLock);=0A= }=0A= PG_CATCH();=0A= {=0A= diff --git a/src/test/isolation/expected/sequence-ddl.out b/src/test/isolat= ion/expected/sequence-ddl.out=0A= index ecd1f7204d..eb534c1006 100644=0A= --- a/src/test/isolation/expected/sequence-ddl.out=0A= +++ b/src/test/isolation/expected/sequence-ddl.out=0A= @@ -8,9 +8,10 @@ ERROR: nextval: reached maximum value of sequence "seq1" = (10)=0A= =20=0A= starting permutation: s1alter s2nv s1commit=0A= step s1alter: ALTER SEQUENCE seq1 MAXVALUE 10;=0A= -step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15);=0A= -ERROR: nextval: reached maximum value of sequence "seq1" (10)=0A= +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); =0A= step s1commit: COMMIT;=0A= +step s2nv: <... completed>=0A= +error in steps s1commit s2nv: ERROR: nextval: reached maximum value of se= quence "seq1" (10)=0A= =20=0A= starting permutation: s2begin s2nv s1alter2 s2commit s1commit=0A= step s2begin: BEGIN;=0A= @@ -32,6 +33,7 @@ nextval=0A= 13=20=20=20=20=20=20=20=20=20=20=20=20=20=0A= 14=20=20=20=20=20=20=20=20=20=20=20=20=20=0A= 15=20=20=20=20=20=20=20=20=20=20=20=20=20=0A= -step s1alter2: ALTER SEQUENCE seq1 MAXVALUE 20;=0A= +step s1alter2: ALTER SEQUENCE seq1 MAXVALUE 20; =0A= step s2commit: COMMIT;=0A= +step s1alter2: <... completed>=0A= step s1commit: COMMIT;=0A= diff --git a/src/test/isolation/specs/sequence-ddl.spec b/src/test/isolatio= n/specs/sequence-ddl.spec=0A= index 9d1d438ead..a9969252f2 100644=0A= --- a/src/test/isolation/specs/sequence-ddl.spec=0A= +++ b/src/test/isolation/specs/sequence-ddl.spec=0A= @@ -23,7 +23,10 @@ step "s2commit" { COMMIT; }=0A= =20=0A= permutation "s1alter" "s1commit" "s2nv"=0A= =20=0A= -# s2nv sees the uncommitted s1alter change.=0A= +# Prior to PG10, the s2nv would see the uncommitted s1alter change,=0A= +# but now it waits.=0A= permutation "s1alter" "s2nv" "s1commit"=0A= =20=0A= +# nextval doesn't release lock until transaction end, so s1alter2 has=0A= +# to wait for s2commit.=0A= permutation "s2begin" "s2nv" "s1alter2" "s2commit" "s1commit"=0A= --=20=0A= 2.12.2=0A= =0A= --------------DD308D3FEF781472E205F35F Content-Type: invalid/octet-stream; name="0003-Reduce-locking-for-ALTER-SEQUENCE-.-RESTART.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename*0="0003-Reduce-locking-for-ALTER-SEQUENCE-.-RESTART.patch"