From 1682d6928c931e1c374328b03e96694fdc4c634f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 10 May 2017 00:18:05 -0400
Subject: [PATCH 2/3] Fix ALTER SEQUENCE locking

In 1753b1b027035029c2a2a1649065762fafbf63f3, the pg_sequence system
catalog was introduced.  This made sequence metadata changes
transactional, while the actual sequence values are still behaving
nontransactionally.  This requires some refinement in how ALTER
SEQUENCE, which operates on both, locks the sequence and the catalog.

The main problems were:

- Concurrent ALTER SEQUENCE causes "tuple concurrently updated" error,
  caused by updates to pg_sequence catalog.

- Sequence WAL writes and catalog updates are not protected by same
  lock, which could lead to inconsistent recovery order.

- nextval() disregarding uncommitted ALTER SEQUENCE changes.

To fix, nextval() and friends now lock the sequence using
RowExclusiveLock instead of AccessShareLock.  ALTER SEQUENCE locks the
sequence using ShareRowExclusiveLock.  This means that nextval() and
ALTER SEQUENCE block each other, and ALTER SEQUENCE on the same sequence
blocks itself.  Also, rearrange some code so that the entire
AlterSequence is protected by the lock on the sequence.

XXX credits
---
 src/backend/commands/sequence.c              | 22 ++++++++++++----------
 src/test/isolation/expected/sequence-ddl.out |  8 +++++---
 src/test/isolation/specs/sequence-ddl.spec   |  5 ++++-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 4a56926b53..44cfcef66b 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -427,7 +427,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 	HeapTuple	tuple;
 
 	/* Open and lock sequence. */
-	relid = RangeVarGetRelid(stmt->sequence, AccessShareLock, stmt->missing_ok);
+	relid = RangeVarGetRelid(stmt->sequence, ShareRowExclusiveLock, stmt->missing_ok);
 	if (relid == InvalidOid)
 	{
 		ereport(NOTICE,
@@ -443,12 +443,6 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
 					   stmt->sequence->relname);
 
-	/* lock page' buffer and read tuple into new sequence structure */
-	seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple);
-
-	/* Copy old sequence data into workspace */
-	memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data));
-
 	rel = heap_open(SequenceRelationId, RowExclusiveLock);
 	tuple = SearchSysCacheCopy1(SEQRELID,
 								ObjectIdGetDatum(relid));
@@ -458,6 +452,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 
 	seqform = (Form_pg_sequence) GETSTRUCT(tuple);
 
+	/* lock page' buffer and read tuple into new sequence structure */
+	seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple);
+
+	/* Copy old sequence data into workspace */
+	memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data));
+
 	/* Check and set new values */
 	init_params(pstate, stmt->options, stmt->for_identity, false, seqform, &changed_seqform, &newseqdata, &owned_by);
 
@@ -508,12 +508,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 
 	ObjectAddressSet(address, RelationRelationId, relid);
 
-	relation_close(seqrel, NoLock);
-
 	if (changed_seqform)
 		CatalogTupleUpdate(rel, &tuple->t_self, tuple);
 	heap_close(rel, RowExclusiveLock);
 
+	relation_close(seqrel, NoLock);
+
 	return address;
 }
 
@@ -1042,6 +1042,8 @@ setval3_oid(PG_FUNCTION_ARGS)
 
 
 /*
+ * TODO rename for lock level change
+ *
  * Open the sequence and acquire AccessShareLock if needed
  *
  * If we haven't touched the sequence already in this transaction,
@@ -1063,7 +1065,7 @@ open_share_lock(SeqTable seq)
 		PG_TRY();
 		{
 			CurrentResourceOwner = TopTransactionResourceOwner;
-			LockRelationOid(seq->relid, AccessShareLock);
+			LockRelationOid(seq->relid, RowExclusiveLock);
 		}
 		PG_CATCH();
 		{
diff --git a/src/test/isolation/expected/sequence-ddl.out b/src/test/isolation/expected/sequence-ddl.out
index ecd1f7204d..eb534c1006 100644
--- a/src/test/isolation/expected/sequence-ddl.out
+++ b/src/test/isolation/expected/sequence-ddl.out
@@ -8,9 +8,10 @@ ERROR:  nextval: reached maximum value of sequence "seq1" (10)
 
 starting permutation: s1alter s2nv s1commit
 step s1alter: ALTER SEQUENCE seq1 MAXVALUE 10;
-step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15);
-ERROR:  nextval: reached maximum value of sequence "seq1" (10)
+step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); <waiting ...>
 step s1commit: COMMIT;
+step s2nv: <... completed>
+error in steps s1commit s2nv: ERROR:  nextval: reached maximum value of sequence "seq1" (10)
 
 starting permutation: s2begin s2nv s1alter2 s2commit s1commit
 step s2begin: BEGIN;
@@ -32,6 +33,7 @@ nextval
 13             
 14             
 15             
-step s1alter2: ALTER SEQUENCE seq1 MAXVALUE 20;
+step s1alter2: ALTER SEQUENCE seq1 MAXVALUE 20; <waiting ...>
 step s2commit: COMMIT;
+step s1alter2: <... completed>
 step s1commit: COMMIT;
diff --git a/src/test/isolation/specs/sequence-ddl.spec b/src/test/isolation/specs/sequence-ddl.spec
index 9d1d438ead..a9969252f2 100644
--- a/src/test/isolation/specs/sequence-ddl.spec
+++ b/src/test/isolation/specs/sequence-ddl.spec
@@ -23,7 +23,10 @@ step "s2commit" { COMMIT; }
 
 permutation "s1alter" "s1commit" "s2nv"
 
-# s2nv sees the uncommitted s1alter change.
+# Prior to PG10, the s2nv would see the uncommitted s1alter change,
+# but now it waits.
 permutation "s1alter" "s2nv" "s1commit"
 
+# nextval doesn't release lock until transaction end, so s1alter2 has
+# to wait for s2commit.
 permutation "s2begin" "s2nv" "s1alter2" "s2commit" "s1commit"
-- 
2.12.2

