Hi all, I have been chewing for the last couple of days on this email from Horiguchi-san: https://www.postgresql.org/message-id/20180622.163312.254556300.horiguchi.kyot...@lab.ntt.co.jp
As summarized, it is actually strange to be able to advance a slot which
has a non-reserved restart_lsn. For example, take that which can happen
on HEAD:
=# select pg_create_physical_replication_slot('toto');
pg_create_physical_replication_slot
-------------------------------------
(toto,)
(1 row)
=# select pg_replication_slot_advance('toto', '0/1');
pg_replication_slot_advance
-----------------------------
(toto,0/1)
(1 row)
=# select slot_name, restart_lsn from pg_replication_slots ;
slot_name | restart_lsn
-----------+-------------
toto | 0/1
(1 row)
KeepLogSeg() is careful enough to not go past LSN positions which do not
exist anymore in the backend, and a client trying to consume a LSN to a
position which does not exist would get back an error to the backend
mentioning that the wanted segment does not exist anymore. Hence it
seems to me that being to advance a slot for an unreserved slot should
result in an error. At least, that's easier to restrict things this
way as it could still be possible to advance a slot up to a position
where a WAL segment is still available on the backend. But I think that
allowing such a thing would be full of race conditions with WAL
recycling and I am not sure about the use case.
I think that documenting the fast that restart_lsn is NULL for a slot
which has not been reserved yet would be nice. Attached is an idea of
patch, ideas are welcome. An open item has been added as well.
Thanks,
--
Michael
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 21e9d56f73..3c1e034f83 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -131,3 +131,20 @@ SELECT pg_drop_replication_slot('regression_slot1');
ERROR: replication slot "regression_slot1" does not exist
SELECT pg_drop_replication_slot('regression_slot2');
ERROR: replication slot "regression_slot2" does not exist
+-- slot advance with physical slot, error with non-reserved slot
+SELECT slot_name FROM pg_create_physical_replication_slot('regression_slot3');
+ slot_name
+------------------
+ regression_slot3
+(1 row)
+
+SELECT pg_replication_slot_advance('regression_slot3', '0/0'); -- invalid LSN
+ERROR: invalid target wal lsn
+SELECT pg_replication_slot_advance('regression_slot3', '0/1'); -- error
+ERROR: cannot move slot with non-reserved restart_lsn
+SELECT pg_drop_replication_slot('regression_slot3');
+ pg_drop_replication_slot
+--------------------------
+
+(1 row)
+
diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql
index 706340c1d8..24cdf7155d 100644
--- a/contrib/test_decoding/sql/slot.sql
+++ b/contrib/test_decoding/sql/slot.sql
@@ -68,3 +68,9 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot1', 'test_
-- both should error as they should be dropped on error
SELECT pg_drop_replication_slot('regression_slot1');
SELECT pg_drop_replication_slot('regression_slot2');
+
+-- slot advance with physical slot, error with non-reserved slot
+SELECT slot_name FROM pg_create_physical_replication_slot('regression_slot3');
+SELECT pg_replication_slot_advance('regression_slot3', '0/0'); -- invalid LSN
+SELECT pg_replication_slot_advance('regression_slot3', '0/1'); -- error
+SELECT pg_drop_replication_slot('regression_slot3');
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3ed9021c2f..4851bc2e24 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9867,7 +9867,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<entry></entry>
<entry>The address (<literal>LSN</literal>) of oldest WAL which still
might be required by the consumer of this slot and thus won't be
- automatically removed during checkpoints.
+ automatically removed during checkpoints. <literal>NULL</literal>
+ if the <literal>LSN</literal> of this slot has never been reserved.
</entry>
</row>
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 2806e1076c..881dbcca33 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -483,6 +483,15 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
/* Acquire the slot so we "own" it */
ReplicationSlotAcquire(NameStr(*slotname), true);
+ /* A slot whose restart_lsn has never been reserved cannot be advanced */
+ if (XLogRecPtrIsInvalid(MyReplicationSlot->data.restart_lsn))
+ {
+ ReplicationSlotRelease();
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move slot with non-reserved restart_lsn")));
+ }
+
/*
* Check if the slot is not moving backwards. Physical slots rely simply
* on restart_lsn as a minimum point, while logical slots have confirmed
signature.asc
Description: PGP signature
