Subject: sequences vs. synchronous replication

From: Tomas Vondra <[email-masked]>
Date: Sat, 18 Dec 2021 02:53:49 +0100

Hi,

while working on logical decoding of sequences, I ran into an issue with 
nextval() in a transaction that rolls back, described in [1]. But after 
thinking about it a bit more (and chatting with Petr Jelinek), I think 
this issue affects physical sync replication too.

Imagine you have a primary <-> sync_replica cluster, and you do this:

   CREATE SEQUENCE s;

   -- shutdown the sync replica

   BEGIN;
   SELECT nextval('s') FROM generate_series(1,50);
   ROLLBACK;

   BEGIN;
   SELECT nextval('s');
   COMMIT;

The natural expectation would be the COMMIT gets stuck, waiting for the 
sync replica (which is not running), right? But it does not.

The problem is exactly the same as in [1] - the aborted transaction 
generated WAL, but RecordTransactionAbort() ignores that and does not 
update LogwrtResult.Write, with the reasoning that aborted transactions 
do not matter. But sequences violate that, because we only write WAL 
once every 32 increments, so the following nextval() gets "committed" 
without waiting for the replica (because it did not produce WAL).

I'm not sure this is a clear data corruption bug, but it surely walks 
and quacks like one. My proposal is to fix this by tracking the lsn of 
the last LSN for a sequence increment, and then check that LSN in 
RecordTransactionCommit() before calling XLogFlush().

regards

[1] 
https://www.postgresql.org/message-id/ae3cab67-c31e-b527-dd73-08f196999ad4%40enterprisedb.com

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Fri, 17 Dec 2021 23:52:48 -0500

> without waiting for the replica (because it did not produce WAL).

Ugh.

> RecordTransactionCommit() before calling XLogFlush().

(1) Does that work if the aborted increment was in a different
session?  I think it is okay but I'm tired enough to not be sure.

(2) I'm starting to wonder if we should rethink the sequence logging
mechanism altogether.  It was cool when designed, but it seems
really problematic when you start thinking about replication
behaviors.  Perhaps if wal_level > minimal, we don't do things
the same way?

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 18 Dec 2021 07:00:45 +0100

> session?  I think it is okay but I'm tired enough to not be sure.

Good point - it doesn't :-( At least not by simply storing LSN in a 
global variable or something like that.

The second backend needs to know the LSN of the last WAL-logged sequence 
increment, but only the first backend knows that. So we'd need to share 
that between backends somehow. I doubt we want to track LSN for every 
individual sequence (because for clusters with many dbs / sequences that 
may be a lot).

Perhaps we could track just a fixed number o LSN values in shared memory 
(say, 1024), and update/read just the element determined by hash(oid). 
That is, the backend WAL-logging sequence with given oid would set the 
current LSN to array[hash(oid) % 1024], and backend doing nextval() 
would simply remember the LSN in that slot. Yes, if there are conflicts 
that'll flush more than needed.

Alternatively we could simply use the current insert LSN, but that's 
going to flush more stuff than needed all the time.

> behaviors.  Perhaps if wal_level > minimal, we don't do things

Maybe, but I have no idea how should the reworked WAL logging work. Any 
batching seems to have this issue, and loging individual increments is 
likely going to be slower.

Of course, reworking how sequences are WAL-logged may invalidate the 
"sequence decoding" patch I've been working on :-(

regards

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 18 Dec 2021 21:45:09 +0100

> that'll flush more than needed.

Here's a PoC demonstrating this idea. I'm not convinced it's the right 
way to deal with this - it surely seems more like a duct tape fix than a 
clean solution. But it does the trick.

I wonder if storing this in shmem is good enough - we lose the LSN info 
on restart, but the checkpoint should trigger FPI which makes it OK.

A bigger question is whether sequences are the only thing affected by 
this. If you look at RecordTransactionCommit() then we skip flush/wait 
in two cases:

1) !wrote_xlog - if the xact did not produce WAL

2) !markXidCommitted - if the xact does not have a valid XID

Both apply to sequences, and the PoC patch tweaks them. But maybe there 
are other places where we don't generate WAL and/or assign XID in some 
cases, to save time?

regards

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index e7b0bc804d..c6a0c07846 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -35,6 +35,7 @@
 #include "catalog/pg_enum.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
+#include "commands/sequence.h"
 #include "commands/tablecmds.h"
 #include "commands/trigger.h"
 #include "common/pg_prng.h"
@@ -1289,6 +1290,13 @@ RecordTransactionCommit(void)
 	bool		RelcacheInitFileInval = false;
 	bool		wrote_xlog;
 
+	/*
+	 * Force flush and synchronous replication even if no XID was assigned.
+	 * This is needed when depending on WAL produced by another transaction
+	 * (possibly in a different backend). Sequences need this, for example.
+	 */
+	bool		force_sync = false;
+
 	/*
 	 * Log pending invalidations for logical decoding of in-progress
 	 * transactions.  Normally for DDLs, we log this at each command end,
@@ -1299,6 +1307,24 @@ RecordTransactionCommit(void)
 	if (XLogLogicalInfoActive())
 		LogLogicalInvalidations();
 
+	/*
+	 * Check the LSN of increments for sequences we touched in this transaction.
+	 * If it's higher than XactLastRecEnd, we need to force flush/sync.
+	 */
+	{
+		/* Separate call, so that we can compare to XactLastRecEnd. */
+		XLogRecPtr tmpptr = SequenceGetLastLSN();
+
+		/*
+		 * If higher than XactLastRecEnd, make sure we flush even without
+		 * a XID assigned.
+		 */
+		force_sync = (tmpptr > XactLastRecEnd);
+
+		/* Override the XactLastRecEnd value. */
+		XactLastRecEnd = Max(XactLastRecEnd, tmpptr);
+	}
+
 	/* Get data needed for commit record */
 	nrels = smgrGetPendingDeletes(true, &rels);
 	nchildren = xactGetCommittedChildren(&children);
@@ -1446,7 +1472,7 @@ RecordTransactionCommit(void)
 	 * if all to-be-deleted tables are temporary though, since they are lost
 	 * anyway if we crash.)
 	 */
-	if ((wrote_xlog && markXidCommitted &&
+	if ((wrote_xlog && (markXidCommitted || force_sync) &&
 		 synchronous_commit > SYNCHRONOUS_COMMIT_OFF) ||
 		forceSyncCommit || nrels > 0)
 	{
@@ -1504,7 +1530,7 @@ RecordTransactionCommit(void)
 	 * Note that at this stage we have marked clog, but still show as running
 	 * in the procarray and continue to hold locks.
 	 */
-	if (wrote_xlog && markXidCommitted)
+	if (wrote_xlog && (markXidCommitted || force_sync))
 		SyncRepWaitForLSN(XactLastRecEnd, true);
 
 	/* remember end of last commit record */
@@ -1512,6 +1538,8 @@ RecordTransactionCommit(void)
 
 	/* Reset XactLastRecEnd until the next transaction writes something */
 	XactLastRecEnd = 0;
+	SequenceResetLastLSN();
+
 cleanup:
 	/* Clean up local data */
 	if (rels)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 72bfdc07a4..43fe19831e 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -109,6 +109,19 @@ static void init_params(ParseState *pstate, List *options, bool for_identity,
 static void do_setval(Oid relid, int64 next, bool iscalled);
 static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);
 
+/* handle flush/sync for sequences */
+#define SEQUENCE_LSN_COUNT 1024
+
+typedef struct SequenceLsnState {
+	slock_t		lock;
+	XLogRecPtr	lsn[SEQUENCE_LSN_COUNT];
+} SequenceLsnState;
+
+static SequenceLsnState *SequenceShmemInfo = NULL;
+static XLogRecPtr lastSequenceLSN = InvalidXLogRecPtr;
+
+static void SequenceSetLSN(Oid relid, XLogRecPtr recptr);
+static XLogRecPtr SequenceGetLSN(Oid relid);
 
 /*
  * DefineSequence
@@ -406,7 +419,12 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 		recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG);
 
 		PageSetLSN(page, recptr);
+
+		SequenceSetLSN(rel->rd_id, recptr);
+		lastSequenceLSN = recptr;
 	}
+	else
+		lastSequenceLSN = SequenceGetLSN(rel->rd_id);
 
 	END_CRIT_SECTION();
 
@@ -810,7 +828,12 @@ nextval_internal(Oid relid, bool check_permissions)
 		recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG);
 
 		PageSetLSN(page, recptr);
+
+		SequenceSetLSN(relid, recptr);
+		lastSequenceLSN = recptr;
 	}
+	else
+		lastSequenceLSN = SequenceGetLSN(relid);
 
 	/* Now update sequence tuple to the intended final state */
 	seq->last_value = last;		/* last fetched number */
@@ -1005,7 +1028,12 @@ do_setval(Oid relid, int64 next, bool iscalled)
 		recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG);
 
 		PageSetLSN(page, recptr);
+
+		SequenceSetLSN(relid, recptr);
+		lastSequenceLSN = recptr;
 	}
+	else
+		lastSequenceLSN = SequenceGetLSN(relid);
 
 	END_CRIT_SECTION();
 
@@ -1927,3 +1955,98 @@ seq_mask(char *page, BlockNumber blkno)
 
 	mask_unused_space(page);
 }
+
+
+/*
+ * Initialization of shared memory for sequence LSN positions.
+ *
+ * To track the last LSN for each sequence we use a fixed-length array, and use
+ * OID of the sequence to select the element. This allows us to handle unknown
+ * number of sequences in a limited amount of space, but it may force us to
+ * sync a bit too much if two sequences conflict (pick the same array element).
+ * But that seems like acceptable tradeoff.
+ */
+Size
+SequenceShmemSize(void)
+{
+	return sizeof(SequenceLsnState);
+}
+
+/*
+ * Initialize the LSN array (set it to InvalidXLogRecPtr).
+ */
+void
+SequenceShmemInit(void)
+{
+	bool		found;
+	SequenceShmemInfo
+		= (SequenceLsnState *) ShmemInitStruct("Sequences Shmem", SequenceShmemSize(), &found);
+
+	if (!found)
+	{
+		/* First time through, so initialize */
+		SpinLockInit(&(SequenceShmemInfo->lock));
+		MemSet(SequenceShmemInfo->lsn, 0, SEQUENCE_LSN_COUNT * sizeof(XLogRecPtr));
+	}
+}
+
+/*
+ * Update LSN tracked for a sequence with a given OID.
+ *
+ * XXX Maybe hash the OID.
+ */
+static void
+SequenceSetLSN(Oid relid, XLogRecPtr recptr)
+{
+	int	idx = (relid % SEQUENCE_LSN_COUNT);
+
+	Assert(SequenceShmemInfo);
+
+	SpinLockAcquire(&(SequenceShmemInfo->lock));
+	SequenceShmemInfo->lsn[idx] = Max(SequenceShmemInfo->lsn[idx], recptr);
+	SpinLockRelease(&(SequenceShmemInfo->lock));
+}
+
+/*
+ * Get LSN tracked for a sequence with a given OID.
+ *
+ * XXX What if the LSN is 0/0? Presumably that means we don't have any value
+ * (e.g. after a restart). Maybe we should use current insert LSN in that case?
+ * Although, shutdown forces checkpoint, and we write FPI or something?
+ */
+static XLogRecPtr
+SequenceGetLSN(Oid relid)
+{
+	int			idx = (relid % SEQUENCE_LSN_COUNT);
+	XLogRecPtr	lsn;
+
+	Assert(SequenceShmemInfo);
+
+	SpinLockAcquire(&(SequenceShmemInfo->lock));
+	lsn = SequenceShmemInfo->lsn[idx];
+	SpinLockRelease(&(SequenceShmemInfo->lock));
+
+	return lsn;
+}
+
+/*
+ * Return the higher LSN for any sequence we touched in this session (either
+ * the WAL we wrote, or WAL produced by other transaction we depend on).
+ */
+XLogRecPtr
+SequenceGetLastLSN(void)
+{
+	return lastSequenceLSN;
+}
+
+/*
+ * Reset the last LSN after we flush.
+ *
+ * XXX Not sure we actually need to do this. Maybe we don't need tp reset and
+ * keeping the value forever is fine.
+ */
+void
+SequenceResetLastLSN(void)
+{
+	lastSequenceLSN = InvalidXLogRecPtr;
+}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 9fa3e0631e..517cca450b 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -23,6 +23,7 @@
 #include "access/syncscan.h"
 #include "access/twophase.h"
 #include "commands/async.h"
+#include "commands/sequence.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/autovacuum.h"
@@ -143,6 +144,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, BTreeShmemSize());
 	size = add_size(size, SyncScanShmemSize());
 	size = add_size(size, AsyncShmemSize());
+	size = add_size(size, SequenceShmemSize());
 #ifdef EXEC_BACKEND
 	size = add_size(size, ShmemBackendArraySize());
 #endif
@@ -293,6 +295,7 @@ CreateSharedMemoryAndSemaphores(void)
 	BTreeShmemInit();
 	SyncScanShmemInit();
 	AsyncShmemInit();
+	SequenceShmemInit();
 
 #ifdef EXEC_BACKEND
 
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index 40544dd4c7..85fbc3789d 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -66,4 +66,10 @@ extern void seq_desc(StringInfo buf, XLogReaderState *rptr);
 extern const char *seq_identify(uint8 info);
 extern void seq_mask(char *pagedata, BlockNumber blkno);
 
+
+extern Size SequenceShmemSize(void);
+extern void SequenceShmemInit(void);
+extern XLogRecPtr SequenceGetLastLSN(void);
+extern void SequenceResetLastLSN(void);
+
 #endif							/* SEQUENCE_H */

---

From: Tom Lane <[email-masked]>
Date: Sat, 18 Dec 2021 16:27:12 -0500

> clean solution. But it does the trick.

I was imagining something a whole lot simpler, like "don't try to
cache unused sequence numbers when wal_level > minimal".  We've
accepted worse performance hits in that operating mode, and it'd
fix a number of user complaints we've seen about weird sequence
behavior on standbys.

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 18 Dec 2021 22:48:11 +0100

> fix a number of user complaints we've seen about weird sequence

What do you mean by "not caching unused sequence numbers"? Reducing 
SEQ_LOG_VALS to 1, i.e. WAL-logging every sequence increment?

That'd work, but I wonder how significant the impact will be. It'd bet 
it hurts the patch adding logical decoding of sequences quite a bit.

regards

---

From: Tom Lane <[email-masked]>
Date: Sat, 18 Dec 2021 16:51:07 -0500

> SEQ_LOG_VALS to 1, i.e. WAL-logging every sequence increment?

Right.

> That'd work, but I wonder how significant the impact will be.

As I said, we've accepted worse in order to have stable replication
behavior.

			regards, tom lane

---

From: Amit Kapila <[email-masked]>
Date: Sun, 19 Dec 2021 08:33:16 +0530

> sync replica (which is not running), right? But it does not.

How about if we always WAL log the first sequence change in a transaction?

-- 
With Regards,
Amit Kapila.

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 20 Dec 2021 15:31:11 +0100

> it hurts the patch adding logical decoding of sequences quite a bit.

It might be worth testing.  This behavior is ancient and has never 
really been scrutinized since it was added.

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 20 Dec 2021 17:40:14 +0100

> really been scrutinized since it was added.

OK, I'll do some testing to measure the overhead, and I'll see how much 
it affects the sequence decoding patch.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 21 Dec 2021 01:53:02 +0100

On 12/20/21 17:40, Tomas Vondra wrote:
>>> That'd work, but I wonder how significant the impact will be. It'd 
>>> bet it hurts the patch adding logical decoding of sequences quite a bit.
>>

OK, I did a quick test with two very simple benchmarks - simple select 
from a sequence, and 'pgbench -N' on scale 1. Benchmark was on current 
master, patched means SEQ_LOG_VALS was set to 1.

Average of 10 runs, each 30 seconds long, look like this:

1) select nextval('s');

      clients          1         4
     ------------------------------
      master       39497    123137
      patched       6813     18326
     ------------------------------
      diff          -83%      -86%

2) pgbench -N

      clients          1         4
     ------------------------------
      master        2935      9156
      patched       2937      9100
     ------------------------------
      diff            0%        0%

Clearly the extreme case (1) is hit pretty bad, while the much mure 
likely workload (2) is almost unaffected.

I'm not sure what conclusion to make from this, but assuming almost no 
one does just nextval calls, it should be acceptable.

regards

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Dec 2021 20:01:59 -0500

> master, patched means SEQ_LOG_VALS was set to 1.

But ... pgbench -N doesn't use sequences at all, does it?

Probably inserts into a table with a serial column would constitute a
plausible real-world case.

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 21 Dec 2021 03:49:33 +0100

> plausible real-world case.

D'oh! For some reason I thought pgbench has a sequence on the history 
table, but clearly I was mistaken. There's another thinko, because after 
inspecting pg_waldump output I realized "SEQ_LOG_VALS 1" actually logs 
only every 2nd increment. So it should be "SEQ_LOG_VALS 0".

So I repeated the test fixing SEQ_LOG_VALS, and doing the pgbench with a 
table like this:

   create table test (a serial, b int);

and a script doing

   insert into test (b) values (1);

The results look like this:

1) select nextval('s');

      clients          1         4
      master       39533    124998
      patched       3748      9114
      diff          -91%      -93%

2) insert into test (b) values (1);

      clients          1         4
      master        3718      9188
      patched       3698      9209

So the nextval() results are a bit worse, due to not caching 1/2 the 
nextval calls. The -90% is roughly expected, due to generating about 32x 
more WAL (and having to wait for commit).

But results for the more realistic insert workload are about the same as 
before (i.e. no measurable difference). Also kinda expected, because 
those transactions have to wait for WAL anyway.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 22 Dec 2021 02:57:02 +0100

> How about if we always WAL log the first sequence change in a transaction?

I've been thinking about doing something like this, but I think it would 
not have any significant advantages compared to using "SEQ_LOG_VALS 0". 
It would still have the same performance hit for plain nextval() calls, 
and there's no measurable impact on simple workloads that already write 
WAL in transactions even with SEQ_LOG_VALS 0.

regards

---

From: Fujii Masao <[email-masked]>
Date: Wed, 22 Dec 2021 13:56:23 +0900

On 2021/12/22 10:57, Tomas Vondra wrote:
> I've been thinking about doing something like this, but I think it would not have any significant advantages compared to using "SEQ_LOG_VALS 0". It would still have the same performance hit for plain nextval() calls, and there's no measurable impact on simple workloads that already write WAL in transactions even with SEQ_LOG_VALS 0.

Just idea; if wal_level > minimal, how about making nextval_internal() (1) check whether WAL is replicated to sync standbys, up to the page lsn of the sequence, and (2) forcibly emit a WAL record if not replicated yet? The similar check is performed at the beginning of SyncRepWaitForLSN(), so probably we can reuse that code.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 22 Dec 2021 13:11:54 +0100

On 12/22/21 05:56, Fujii Masao wrote:
>> I've been thinking about doing something like this, but I think it 
>> would not have any significant advantages compared to using 
>> "SEQ_LOG_VALS 0". It would still have the same performance hit for 
>> plain nextval() calls, and there's no measurable impact on simple 
>> workloads that already write WAL in transactions even with 
>> SEQ_LOG_VALS 0.
> 
> Just idea; if wal_level > minimal, how about making nextval_internal() 
> (1) check whether WAL is replicated to sync standbys, up to the page lsn 
> of the sequence, and (2) forcibly emit a WAL record if not replicated 
> yet? The similar check is performed at the beginning of 
> SyncRepWaitForLSN(), so probably we can reuse that code.
> 

Interesting idea, but I think it has a couple of issues :-(

1) We'd need to know the LSN of the last WAL record for any given 
sequence, and we'd need to communicate that between backends somehow. 
Which seems rather tricky to do without affecting performance.

2) SyncRepWaitForLSN() is used only in commit-like situations, and it's 
a simple wait, not a decision to write more WAL. Environments without 
sync replicas are affected by this too - yes, the data loss issue is not 
there, but the amount of WAL is still increased.

IIRC sync_standby_names can change while a transaction is running, even 
just right before commit, at which point we can't just go back in time 
and generate WAL for sequences accessed earlier. But we still need to 
ensure the sequence is properly replicated.

3) I don't think it'd actually reduce the amount of WAL records in 
environments with many sessions (incrementing the same sequence). In 
those cases the WAL (generated by in-progress xact from another session) 
is likely to not be flushed, so we'd generate the extra WAL record. (And 
if the other backends would need flush LSN of this new WAL record, which 
would make it more likely they have to generate WAL too.)

So I don't think this would actually help much.

regards

---

From: Fujii Masao <[email-masked]>
Date: Thu, 23 Dec 2021 02:50:49 +0900

> Interesting idea, but I think it has a couple of issues :-(

Thanks for the review!

> 1) We'd need to know the LSN of the last WAL record for any given sequence, and we'd need to communicate that between backends somehow. Which seems rather tricky to do without affecting performance.

How about using the page lsn for the sequence? nextval_internal() already uses that to check whether it's less than or equal to checkpoint redo location.

> 2) SyncRepWaitForLSN() is used only in commit-like situations, and it's a simple wait, not a decision to write more WAL. Environments without sync replicas are affected by this too - yes, the data loss issue is not there, but the amount of WAL is still increased.

How about reusing only a part of code in SyncRepWaitForLSN()? Attached is the PoC patch that implemented what I'm thinking.

> IIRC sync_standby_names can change while a transaction is running, even just right before commit, at which point we can't just go back in time and generate WAL for sequences accessed earlier. But we still need to ensure the sequence is properly replicated.

Yes. In the PoC patch, SyncRepNeedsWait() still checks sync_standbys_defined and uses SyncRepWaitMode. But they should not be checked nor used because their values can be changed on the fly, as you pointed out. Probably SyncRepNeedsWait() will need to be changed so that it doesn't use them.

> 3) I don't think it'd actually reduce the amount of WAL records in environments with many sessions (incrementing the same sequence). In those cases the WAL (generated by in-progress xact from another session) is likely to not be flushed, so we'd generate the extra WAL record. (And if the other backends would need flush LSN of this new WAL record, which would make it more likely they have to generate WAL too.)

With the PoC patch, only when previous transaction that executed nextval() and caused WAL record is aborted, subsequent nextval() generates additional WAL record. So this approach can reduce WAL volume than other approach?

In the PoC patch, to reduce WAL volume more, it might be better to make nextval_internal() update XactLastRecEnd and assign XID rather than emitting new WAL record, when SyncRepNeedsWait() returns true.

Regards,

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 72bfdc07a4..38cd55b81a 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -37,6 +37,7 @@
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "parser/parse_type.h"
+#include "replication/syncrep.h"
 #include "storage/lmgr.h"
 #include "storage/proc.h"
 #include "storage/smgr.h"
@@ -676,8 +677,9 @@ nextval_internal(Oid relid, bool check_permissions)
 	else
 	{
 		XLogRecPtr	redoptr = GetRedoRecPtr();
+		XLogRecPtr	pagelsn = PageGetLSN(page);
 
-		if (PageGetLSN(page) <= redoptr)
+		if (pagelsn <= redoptr || SyncRepNeedsWait(pagelsn))
 		{
 			/* last update of seq was before checkpoint */
 			fetch = log = fetch + SEQ_LOG_VALS;
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index bdbc9ef844..589364cb58 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -130,6 +130,34 @@ static bool SyncRepQueueIsOrderedByLSN(int mode);
  * ===========================================================
  */
 
+bool
+SyncRepNeedsWait(XLogRecPtr lsn)
+{
+	int			mode;
+
+	if (!SyncRepRequested() ||
+		!((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+		return false;
+
+	mode = Min(SyncRepWaitMode, SYNC_REP_WAIT_FLUSH);
+
+	Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
+	Assert(WalSndCtl != NULL);
+
+	LWLockAcquire(SyncRepLock, LW_SHARED);
+	Assert(MyProc->syncRepState == SYNC_REP_NOT_WAITING);
+
+	if (!WalSndCtl->sync_standbys_defined ||
+		lsn <= WalSndCtl->lsn[mode])
+	{
+		LWLockRelease(SyncRepLock);
+		return false;
+	}
+
+	LWLockRelease(SyncRepLock);
+	return true;
+}
+
 /*
  * Wait for synchronous replication, if requested by user.
  *
diff --git a/src/include/replication/syncrep.h b/src/include/replication/syncrep.h
index 4266afde8b..b08f3f32d1 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -82,6 +82,7 @@ extern char *syncrep_parse_error_msg;
 extern char *SyncRepStandbyNames;
 
 /* called by user backend */
+extern bool SyncRepNeedsWait(XLogRecPtr lsn);
 extern void SyncRepWaitForLSN(XLogRecPtr lsn, bool commit);
 
 /* called at backend exit */

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 22 Dec 2021 19:49:15 +0100

> The results look like this:

Attached is a patch tweaking WAL logging - in wal_level=minimal we do 
the same thing as now, in higher levels we log every sequence fetch.

After thinking about this a bit more, I think even the nextval workload 
is not such a big issue, because we can set cache for the sequences. 
Until now this had fairly limited impact, but it can significantly 
reduce the performance drop caused by WAL-logging every sequence fetch.

I've repeated the nextval test on a different machine (the one I used 
before is busy with something else), and the results look like this:

1) 1 client

     cache          1         32        128
     --------------------------------------
     master     13975      14425      19886
     patched      886       7900      18397
     --------------------------------------
     diff        -94%       -45%        -7%

4) 4 clients

     cache             1         32        128
     -----------------------------------------
     master         8338      12849      18248
     patched         331       8124      18983
     -----------------------------------------
     diff           -96%       -37%         4%

So I think this makes it acceptable / manageable. Of course, this means 
the values are much less monotonous (across backends), but I don't think 
we really promised that. And I doubt anyone is really using sequences 
like this (just nextval) in performance critical use cases.

regards

From eeaa7cb36c69af048f0321e4883864ebe2542429 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email-masked]>
Date: Wed, 22 Dec 2021 03:18:46 +0100
Subject: [PATCH 1/6] WAL-log individual sequence fetches

---
 src/backend/commands/sequence.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 72bfdc07a4..0f309d0a4e 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -52,6 +52,9 @@
  * We don't want to log each fetching of a value from a sequence,
  * so we pre-log a few fetches in advance. In the event of
  * crash we can lose (skip over) as many values as we pre-logged.
+ *
+ * We only pre-log fetches in wal_level=minimal. For higher levels we
+ * WAL-log every individual sequence increment, as if this was 0.
  */
 #define SEQ_LOG_VALS	32
 
@@ -666,11 +669,18 @@ nextval_internal(Oid relid, bool check_permissions)
 	 * WAL record to be written anyway, else replay starting from the
 	 * checkpoint would fail to advance the sequence past the logged values.
 	 * In this case we may as well fetch extra values.
+	 *
+	 * We only pre-log fetches in wal_level=minimal. For higher levels we
+	 * WAL-log every individual sequence increment.
 	 */
 	if (log < fetch || !seq->is_called)
 	{
 		/* forced log to satisfy local demand for values */
-		fetch = log = fetch + SEQ_LOG_VALS;
+		if (XLogIsNeeded())
+			fetch = log = fetch;
+		else
+			fetch = log = fetch + SEQ_LOG_VALS;
+
 		logit = true;
 	}
 	else
@@ -680,7 +690,11 @@ nextval_internal(Oid relid, bool check_permissions)
 		if (PageGetLSN(page) <= redoptr)
 		{
 			/* last update of seq was before checkpoint */
-			fetch = log = fetch + SEQ_LOG_VALS;
+			if (XLogIsNeeded())
+				fetch = log = fetch;
+			else
+				fetch = log = fetch + SEQ_LOG_VALS;
+
 			logit = true;
 		}
 	}
-- 
2.31.1

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 22 Dec 2021 20:00:09 +0100

On 12/22/21 18:50, Fujii Masao wrote:
> How about using the page lsn for the sequence? nextval_internal() 
> already uses that to check whether it's less than or equal to checkpoint 
> redo location.
> 

Hmm, maybe.

> 
>> 2) SyncRepWaitForLSN() is used only in commit-like situations, and 
>> it's a simple wait, not a decision to write more WAL. Environments 
>> without sync replicas are affected by this too - yes, the data loss 
>> issue is not there, but the amount of WAL is still increased.
> 
> How about reusing only a part of code in SyncRepWaitForLSN()? Attached 
> is the PoC patch that implemented what I'm thinking.
> 
> 
>> IIRC sync_standby_names can change while a transaction is running, 
>> even just right before commit, at which point we can't just go back in 
>> time and generate WAL for sequences accessed earlier. But we still 
>> need to ensure the sequence is properly replicated.
> 
> Yes. In the PoC patch, SyncRepNeedsWait() still checks 
> sync_standbys_defined and uses SyncRepWaitMode. But they should not be 
> checked nor used because their values can be changed on the fly, as you 
> pointed out. Probably SyncRepNeedsWait() will need to be changed so that 
> it doesn't use them.
> 

Right. I think the data loss with sync standby is merely a symptom, not 
the root cause. We'd need to deduce the LSN for which to wait at commit.

>> those cases the WAL (generated by in-progress xact from another 
>> session) is likely to not be flushed, so we'd generate the extra WAL 
>> record. (And if the other backends would need flush LSN of this new 
>> WAL record, which would make it more likely they have to generate WAL 
>> too.)
> 
> With the PoC patch, only when previous transaction that executed 
> nextval() and caused WAL record is aborted, subsequent nextval() 
> generates additional WAL record. So this approach can reduce WAL volume 
> than other approach?
>  > In the PoC patch, to reduce WAL volume more, it might be better to make
> nextval_internal() update XactLastRecEnd and assign XID rather than 
> emitting new WAL record, when SyncRepNeedsWait() returns true.
> 

Yes, but I think there are other cases. For example the WAL might have 
been generated by another backend, in a transaction that might be still 
running. In which case I don't see how updating XactLastRecEnd in 
nextval_internal would fix this, right?

I did some experiments with increasing CACHE for the sequence, and that 
mostly eliminates the overhead. See the message I sent a couple minutes 
ago. IMHO that's a reasonable solution for the tiny number of people 
using nextval() in a way that'd be affected by this (i.e. without 
writing anything else in the xact).

regards

---

From: Fujii Masao <[email-masked]>
Date: Thu, 23 Dec 2021 23:42:05 +0900

On 2021/12/23 3:49, Tomas Vondra wrote:
> Attached is a patch tweaking WAL logging - in wal_level=minimal we do the same thing as now, in higher levels we log every sequence fetch.

Thanks for the patch!

With the patch, I found that the regression test for sequences failed.

+			fetch = log = fetch;

This should be "log = fetch"?

On second thought, originally a sequence doesn't guarantee that the value already returned by nextval() will never be returned by subsequent nextval() after the server crash recovery. That is, nextval() may return the same value across crash recovery. Is this understanding right? For example, this case can happen if the server crashes after nextval() returned the value but before WAL for the sequence was flushed to the permanent storage. So it's not a bug that sync standby may return the same value as already returned in the primary because the corresponding WAL has not been replicated yet, isn't it?

BTW, if the returned value is stored in database, the same value is guaranteed not to be returned again after the server crash or by sync standby. Because in that case the WAL of the transaction storing that value is flushed and replicated.

> So I think this makes it acceptable / manageable. Of course, this means the values are much less monotonous (across backends), but I don't think we really promised that. And I doubt anyone is really using sequences like this (just nextval) in performance critical use cases.

I think that this approach is not acceptable to some users. So, if we actually adopt WAL-logging every sequence fetch, also how about exposing SEQ_LOG_VALS as reloption for a sequence? If so, those who want to log every sequence fetch can set this SEQ_LOG_VALS reloption to 0. OTOH, those who prefer the current behavior in spite of the risk we're discussing at this thread can set the reloption to 32 like it is for now, for example.

Regards,

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 23 Dec 2021 19:50:22 +0100

On 12/23/21 15:42, Fujii Masao wrote:
> On second thought, originally a sequence doesn't guarantee that the 
> value already returned by nextval() will never be returned by subsequent 
> nextval() after the server crash recovery. That is, nextval() may return 
> the same value across crash recovery. Is this understanding right? For 
> example, this case can happen if the server crashes after nextval() 
> returned the value but before WAL for the sequence was flushed to the 
> permanent storage.

I think the important step is commit. We don't guarantee anything for 
changes in uncommitted transactions. If you do nextval in a transaction 
and the server crashes before the WAL gets flushed before COMMIT, then 
yes, nextval may generate the same nextval again. But after commit that 
is not OK - it must not happen.

> So it's not a bug that sync standby may return the same value as
> already returned in the primary because the corresponding WAL has not
> been replicated yet, isn't it?
> 

No, I don't think so. Once the COMMIT happens (and gets confirmed by the 
sync standby), it should be possible to failover to the sync replica 
without losing any data in committed transaction. Generating duplicate 
values is a clear violation of that.

IMHO the fact that we allow a transaction to commit (even just locally) 
without flushing all the WAL it depends on is clearly a data loss bug.

> BTW, if the returned value is stored in database, the same value is 
> guaranteed not to be returned again after the server crash or by sync 
> standby. Because in that case the WAL of the transaction storing that 
> value is flushed and replicated.
> 

True, assuming the table is WAL-logged etc. I agree the issue may be 
affecting a fairly small fraction of workloads, because most people use 
sequences to generate data for inserts etc.

>> So I think this makes it acceptable / manageable. Of course, this 
>> means the values are much less monotonous (across backends), but I 
>> don't think we really promised that. And I doubt anyone is really 
>> using sequences like this (just nextval) in performance critical use 
>> cases.
> 
> I think that this approach is not acceptable to some users. So, if we 
> actually adopt WAL-logging every sequence fetch, also how about exposing 
> SEQ_LOG_VALS as reloption for a sequence? If so, those who want to log 
> every sequence fetch can set this SEQ_LOG_VALS reloption to 0. OTOH, 
> those who prefer the current behavior in spite of the risk we're 
> discussing at this thread can set the reloption to 32 like it is for 
> now, for example.
> 

I think it'd be worth explaining why you think it's not acceptable?

I've demonstrated the impact on regular workloads (with other changes 
that write stuff to WAL) is not measurable, and enabling sequence 
caching eliminates most of the overhead for the rare corner case 
workloads if needed. It does generate a bit more WAL, but the sequence 
WAL records are pretty tiny.

I'm opposed to adding relooptions that affect correctness - it just 
seems like a bad idea to me. Moreover setting the CACHE for a sequence 
does almost the same thing - if you set CACHE 32, we only generate WAL 
once every 32 increments. The only difference is that this cache is not 
shared between backends, so one backend will generate 1,2,3,... and 
another backend will generate 33,34,35,... etc. I don't think that's a 
problem, because if you want strictly monotonous / gap-less sequences 
you can't use our sequences anyway. Yes, with short-lived backends this 
may consume the sequences faster, but well - short-lived backends are 
expensive anyway and overflowing bigserial is still unlikely.

regards

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 24 Dec 2021 14:37:25 +0900 (JST)

At Thu, 23 Dec 2021 19:50:22 +0100, Tomas Vondra <[email-masked]> wrote in 
> > value already returned by nextval() will never be returned by
> > subsequent nextval() after the server crash recovery. That is,
> > nextval() may return the same value across crash recovery. Is this
> > understanding right? For example, this case can happen if the server
> > crashes after nextval() returned the value but before WAL for the
> > sequence was flushed to the permanent storage.
> 
> changes in uncommitted transactions. If you do nextval in a
> transaction and the server crashes before the WAL gets flushed before
> COMMIT, then yes, nextval may generate the same nextval again. But
> after commit that is not OK - it must not happen.

I don't mean to stand on Fujii-san's side particularly, but it seems
to me sequences of RDBSs are not rolled back generally.  Some googling
told me that at least Oracle (documented), MySQL, DB2 and MS-SQL
server doesn't rewind sequences at rollback, that is, sequences are
incremented independtly from transaction control.  It seems common to
think that two nextval() calls for the same sequence must not return
the same value in any context.

> No, I don't think so. Once the COMMIT happens (and gets confirmed by
> the sync standby), it should be possible to failover to the sync
> replica without losing any data in committed transaction. Generating
> duplicate values is a clear violation of that.

So, strictly speaking, that is a violation of the constraint I
mentioned regardless whether the transaction is committed or
not. However we have technical limitations as below.

> IMHO the fact that we allow a transaction to commit (even just
> locally) without flushing all the WAL it depends on is clearly a data
> loss bug.
> 
> affecting a fairly small fraction of workloads, because most people
> use sequences to generate data for inserts etc.

It seems to me, from the fact that sequences are designed explicitly
untransactional and that behavior is widely adopted, the discussion
might be missing some significant use-cases.  But there's a
possibility that the spec of sequence came from some technical
limitation in the past, but I'm not sure..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 24 Dec 2021 08:23:13 +0100

> the same value in any context.

Yes, sequences are not rolled back on abort generally. That would 
require much stricter locking, and that'd go against using sequences in 
concurrent sessions.

But we're not talking about sequence rollback - we're talking about data 
loss, caused by failure to flush WAL for a sequence. But that affects 
the *current* code too, and to much greater extent.

Consider this:

BEGIN;
SELECT nextval('s') FROM generate_series(1,1000) s(i);
ROLLBACK; -- or crash of a different backend

BEGIN;
SELECT nextval('s');
COMMIT;

With the current code, this may easily lose the WAL, and we'll generate 
duplicate values from the sequence. We pretty much ignore the COMMIT.

With the proposed change to WAL logging, that is not possible. The 
COMMIT flushes enough WAL to prevent this issue.

So this actually makes this issue less severe.

Maybe I'm missing some important detail, though. Can you show an example 
where the proposed changes make the issue worse?

> not. However we have technical limitations as below.

I don't follow. What violates what?

If the transaction commits (and gets a confirmation from sync replica), 
the modified WAL logging prevents duplicate values. It does nothing for 
uncommitted transactions. Seems like an improvement to me.

> limitation in the past, but I'm not sure..

No idea. IMHO from the correctness / behavior point of view, the 
modified logging is an improvement. The only issue is the additional 
overhead, and I think the cache addresses that quite well.

regards

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 24 Dec 2021 17:04:51 +0900 (JST)

At Fri, 24 Dec 2021 08:23:13 +0100, Tomas Vondra <[email-masked]> wrote in 
> require much stricter locking, and that'd go against using sequences
> in concurrent sessions.

I thinks so.

> But we're not talking about sequence rollback - we're talking about
> data loss, caused by failure to flush WAL for a sequence. But that
> affects the *current* code too, and to much greater extent.

Ah, yes, I don't object to that aspect.

> With the current code, this may easily lose the WAL, and we'll
> generate duplicate values from the sequence. We pretty much ignore the
> COMMIT.
>
> Maybe I'm missing some important detail, though. Can you show an
> example where the proposed changes make the issue worse?

No. It seems to me improvoment at least from the current state, for
the reason you mentioned.

> If the transaction commits (and gets a confirmation from sync
> replica), the modified WAL logging prevents duplicate values. It does
> nothing for uncommitted transactions. Seems like an improvement to me.

Sorry for the noise. I misunderstand that ROLLBACK is being changed to
rollback sequences.

> overhead, and I think the cache addresses that quite well.

Now I understand the story here.

I agree that the patch is improvment from the current behavior.
I agree that the overhead is eventually-nothing for WAL-emitting workloads.

Still, as Fujii-san concerns, I'm afraid that some people may suffer
the degradation the patch causes.  I wonder it is acceptable to get
back the previous behavior by exposing SEQ_LOG_VALS itself or a
boolean to do that, as a 'not-recommended-to-use' variable.

regards.

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 24 Dec 2021 11:40:20 +0100

On 12/24/21 09:04, Kyotaro Horiguchi wrote:
>>> ...

No problem, this part of the code is certainly rather confusing due to 
several layers of caching and these WAL-logging optimizations.

> I agree that the overhead is eventually-nothing for WAL-emitting workloads.

OK, thanks.

> boolean to do that, as a 'not-recommended-to-use' variable.

Maybe, but what would such workload look like? Based on the tests I did, 
such workload probably can't generate any WAL. The amount of WAL added 
by the change is tiny, the regression is caused by having to flush WAL.

The only plausible workload I can think of is just calling nextval, and 
the cache pretty much fixes that.

FWIW I plan to explore the idea of looking at sequence page LSN, and 
flushing up to that position.

regards

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 27 Dec 2021 21:24:43 +0100

> boolean to do that, as a 'not-recommended-to-use' variable.

There is also the possibility of unlogged sequences if you want to avoid 
the WAL logging and get higher performance.

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 28 Dec 2021 02:39:51 +0100

> the WAL logging and get higher performance.

But unlogged sequences are not supported:

   test=# create unlogged sequence s;
   ERROR:  unlogged sequences are not supported

And even if we did, what would be the behavior after crash? For tables 
we discard the contents, so for sequences we'd probably discard it too 
and start from scratch? That doesn't seem particularly useful.

We could also write / fsync the sequence buffer, but that has other 
downsides. But that's not implemented either, and it's certainly out of 
scope for this patch.

regards

---

From: Fujii Masao <[email-masked]>
Date: Tue, 28 Dec 2021 15:56:22 +0900

On 2021/12/24 19:40, Tomas Vondra wrote:
> Maybe, but what would such workload look like? Based on the tests I did, such workload probably can't generate any WAL. The amount of WAL added by the change is tiny, the regression is caused by having to flush WAL.
> 
> The only plausible workload I can think of is just calling nextval, and the cache pretty much fixes that.

Some users don't want to increase cache setting, do they? Because

- They may expect that setval() affects all subsequent nextval(). But if cache is set to greater than one, the value set by setval() doesn't affect other backends until they consumed all the cached sequence values.
- They may expect that the value returned from nextval() is basically increased monotonically. If cache is set to greater than one, subsequent nextval() can easily return smaller value than one returned by previous nextval().
- They may want to avoid "hole" of a sequence as much as possible, e.g., as far as the server is running normally. If cache is set to greater than one, such "hole" can happen even thought the server doesn't crash yet.

> FWIW I plan to explore the idea of looking at sequence page LSN, and flushing up to that position.

Sounds great, thanks!

Regards,

---

From: Sascha Kuhl <[email-masked]>
Date: Tue, 28 Dec 2021 09:28:30 +0100

Sequence validation by step, in total is great. If the sequence is Familie
or professional, does it make sense to a have a total validation by an
expert. I can only say true by chi square Networks, but would a medical
opinion be an improvement?

Fujii Masao <[email-masked]> schrieb am Di., 28. Dez. 2021,
07:56:

> such workload probably can't generate any WAL. The amount of WAL added by
> the change is tiny, the regression is caused by having to flush WAL.
> >
> - They may expect that setval() affects all subsequent nextval(). But if
> cache is set to greater than one, the value set by setval() doesn't affect
> other backends until they consumed all the cached sequence values.
> - They may expect that the value returned from nextval() is basically
> increased monotonically. If cache is set to greater than one, subsequent
> nextval() can easily return smaller value than one returned by previous
> nextval().
> - They may want to avoid "hole" of a sequence as much as possible, e.g.,
> as far as the server is running normally. If cache is set to greater than
> one, such "hole" can happen even thought the server doesn't crash yet.
>
>

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 28 Dec 2021 09:50:39 +0100

Hi

> opinion be an improvement?

Is it generated by boot or by a human?

>> > Maybe, but what would such workload look like? Based on the tests I
>> did, such workload probably can't generate any WAL. The amount of WAL added

---

From: Sascha Kuhl <[email-masked]>
Date: Tue, 28 Dec 2021 09:53:41 +0100

Pavel Stehule <[email-masked]> schrieb am Di., 28. Dez. 2021,
09:51:

>> Sequence validation by step, in total is great. If the sequence is
>> Familie or professional, does it make sense to a have a total validation by
>> an expert. I can only say true by chi square Networks, but would a medical

I validation my family and Société, only when them Show me not their
Sekret, part of their truth. Works fine by a Boot level, as far as I can
detektei, without the Boot showing up 😉

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 28 Dec 2021 09:57:28 +0100

> detektei, without the Boot showing up 😉

don't spam this mailing list, please

Thank you

Pavel

>>
>>
>>> Fujii Masao <[email-masked]> schrieb am Di., 28. Dez.
>>> 2021, 07:56:
>>>
>>>>
>>>>
>>>> - They may expect that setval() affects all subsequent nextval(). But
>>>> if cache is set to greater than one, the value set by setval() doesn't
>>>> affect other backends until they consumed all the cached sequence values.
>>>> - They may want to avoid "hole" of a sequence as much as possible,
>>>> e.g., as far as the server is running normally. If cache is set to greater
>>>> than one, such "hole" can happen even thought the server doesn't crash yet.
>>>>
>>>>