Re: Non-reserved replication slots and slot advancing

At Fri, 6 Jul 2018 14:26:42 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180706052642.GB776@paquier.xyz>
> On Fri, Jul 06, 2018 at 01:07:47PM +0900, Kyotaro HORIGUCHI wrote:
> > Form the reasons above, I'd like to vote +1 for just ERRORing
> > in the case.
> 
> Thanks for the lookup.  Do you have a good idea for the error message to
> provide to users in this case?  I would tend to think that "cannot move
> slot which has never been reserved" or "cannot move slot which has never
> been used", particularly the first one, are fine enough, but I am not
> stopped at one single idea in particular either.

Mmm. I feel so much that it's a kind of too much for me, but
FWIW...

I'm not so in favor of the word "reserve" in first place since it
doesn't seem intuitive for me, but "active" is already used for
the state where the connection with the peer is made. (The word
"reserve" may be misused since in the source code "reserve" is
used as "to reserve WAL", but used as "reserve a slot" in
documentation.)

"use", as you mentioned, is neutral but somewhat vague and what
is worse a immediately-reserved slot can be advanced before the
first use.

Most straightforward wording would be:

"ERROR: cannot advance/move slots with invalid restart_lsn"
"HINT: It will have a valid value after the first connection."

Another candidate from me is "initialize". This requires related
documentation fix. (first attached).
# I noticed that the documentation forgets that
# pg_replication_slots now stores two LSNs.

"ERROR: cannot advance/move uninitialized slots"

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index edc9be92a6..697146468c 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19186,7 +19186,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
         <indexterm>
          <primary>pg_create_physical_replication_slot</primary>
         </indexterm>
-        <literal><function>pg_create_physical_replication_slot(<parameter>slot_name</parameter> <type>name</type>
<optional>,<parameter>immediately_reserve</parameter> <type>boolean</type>, <parameter>temporary</parameter>
<type>boolean</type></optional>)</function></literal>
+        <literal><function>pg_create_physical_replication_slot(<parameter>slot_name</parameter> <type>name</type>
<optional>,<parameter>initialize</parameter> <type>boolean</type>, <parameter>temporary</parameter>
<type>boolean</type></optional>)</function></literal>
        </entry>
        <entry>
         (<parameter>slot_name</parameter> <type>name</type>, <parameter>lsn</parameter> <type>pg_lsn</type>)
@@ -19194,9 +19194,11 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
        <entry>
         Creates a new physical replication slot named
         <parameter>slot_name</parameter>. The optional second parameter,
-        when <literal>true</literal>, specifies that the <acronym>LSN</acronym> for this
-        replication slot be reserved immediately; otherwise
-        the <acronym>LSN</acronym> is reserved on first connection from a streaming
+        when <literal>true</literal>, specifies that the <literal>restart_lsn</literal>
+        for this replication slot be initialized immediately with REDO
+        <acronym>LSN</acronym> of the last checkpoint; otherwise it is
+        initialized on first connection from a streaming
+
         replication client. Streaming changes from a physical slot is only
         possible with the streaming-replication protocol —
         see <xref linkend="protocol-replication"/>. The optional third
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 2806e1076c..703b61768b 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -43,7 +43,7 @@ Datum
 pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
 {
     Name        name = PG_GETARG_NAME(0);
-    bool        immediately_reserve = PG_GETARG_BOOL(1);
+    bool        initialize = PG_GETARG_BOOL(1);
     bool        temporary = PG_GETARG_BOOL(2);
     Datum        values[2];
     bool        nulls[2];
@@ -67,7 +67,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
     values[0] = NameGetDatum(&MyReplicationSlot->data.name);
     nulls[0] = false;
 
-    if (immediately_reserve)
+    if (initialize)
     {
         /* Reserve WAL as the user asked for it */
         ReplicationSlotReserveWal();