Fix an assert in CheckPointReplicationSlots().

Amit Kapila · Amit Kapila · commit d1ffcc7fa3c5 · 2024-06-11T10:51:34.000+05:30
Commit e0b2eed assumed that the confirmed_flush LSN can't go backward. However, it is possible that confirmed_flush LSN can go backward temporarily when the client acknowledges a prior value of flush location. This can happen when the client (subscriber in this case) acknowledges an LSN it doesn't have to do anything for (say for DDLs) and thus didn't store persistently. After restart, the client sends the prior value of flush LSN which it had stored persistently and the server updates the confirmed_flush LSN with that value. The fix is to remove the assumption and not allow the prior value of confirmed_flush LSN to be flushed to the disk. Author: Vignesh C Reviewed-by: Amit Kapila, Shlok Kyal Discussion: https://postgr.es/m/CALDaNm3hgow2+oEov5jBk4iYP5eQrUCF1yZtW7+dV3J__p4KLQ@mail.gmail.com
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
@@ -1874,10 +1874,8 @@ CheckPointReplicationSlots(bool is_shutdown)
 		{
 			SpinLockAcquire(&s->mutex);
 
-			Assert(s->data.confirmed_flush >= s->last_saved_confirmed_flush);
-
 			if (s->data.invalidated == RS_INVAL_NONE &&
-				s->data.confirmed_flush != s->last_saved_confirmed_flush)
+				s->data.confirmed_flush > s->last_saved_confirmed_flush)
 			{
 				s->just_dirtied = true;
 				s->dirty = true;

Original file line number	Diff line number	Diff line change
`@@ -1874,10 +1874,8 @@ CheckPointReplicationSlots(bool is_shutdown)`
`1874`	`1874`	`{`
`1875`	`1875`	`SpinLockAcquire(&s->mutex);`
`1876`	`1876`
`1877`		`- Assert(s->data.confirmed_flush >= s->last_saved_confirmed_flush);`
`1878`		`-`
`1879`	`1877`	`if (s->data.invalidated == RS_INVAL_NONE &&`
`1880`		`- s->data.confirmed_flush != s->last_saved_confirmed_flush)`
	`1878`	`+ s->data.confirmed_flush > s->last_saved_confirmed_flush)`
`1881`	`1879`	`{`
`1882`	`1880`	`s->just_dirtied = true;`
`1883`	`1881`	`s->dirty = true;`