On Mon, Jul 09, 2018 at 02:48:28PM -0400, Alvaro Herrera wrote:
> On 2018-Jul-09, Andres Freund wrote:
> > On 2018-07-09 15:48:33 +0900, Michael Paquier wrote:
> > > + if (XLogRecPtrIsInvalid(MyReplicationSlot->data.restart_lsn))
> > > + {
> > > + ReplicationSlotRelease();
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > + errmsg("cannot move slot not reserving WAL")));
> > > + }
> > > +
> >
> > I don't like this error message. It's unclear what refers to exactly
> > what. Nor is "move" a terminology used otherwise. How about:
> > "cannot advance replication slot that has not previously reserved WAL"
> > or something similar?"move" is used in another error message ten lines below. > > > > Also, ERRCODE_FEATURE_NOT_SUPPORTED doesn't quite seem right. How about > > ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE? > > +1 to both of Andres' suggestions. Those indeed sound better. What do you think of the attached? -- Michael
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 21e9d56f73..2737a8a301 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 advance replication slot that has not previously reserved WAL
+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..afdcd5749e 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_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot advance replication slot that has not previously reserved WAL")));
+ }
+
/*
* Check if the slot is not moving backwards. Physical slots rely simply
* on restart_lsn as a minimum point, while logical slots have confirmed
@@ -499,7 +508,7 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
ReplicationSlotRelease();
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move slot to %X/%X, minimum is %X/%X",
+ errmsg("cannot advance replication slot to %X/%X, minimum is %X/%X",
(uint32) (moveto >> 32), (uint32) moveto,
(uint32) (minlsn >> 32), (uint32) minlsn)));
}
signature.asc
Description: PGP signature
