Re: pg_receivewal starting position

On Mon, Aug 30, 2021 at 11:55:42AM +0200, Ronan Dunklau wrote:
> Le vendredi 27 août 2021, 05:44:32 CEST Michael Paquier a écrit :
>> +   if (slot == NULL || !slot->in_use)                                      
>>                                                                            
>>                                                                     [...] +
>>       ereport(ERROR,
>> +               (errcode(ERRCODE_UNDEFINED_OBJECT),
>> +                errmsg("replication slot \"%s\" does not exist",
>> +                       cmd->slotname)));
>> [...]
>> +       if (PQntuples(res) == 0)
>> +       {
>> +               pg_log_error("replication slot %s does not exist",
>> slot_name); +               PQclear(0);
>> +               return false;
>> So, the backend and ReadReplicationSlot() report an ERROR if a slot
>> does not exist but pg_basebackup's GetSlotInformation() does the same
>> if there are no tuples returned.  That's inconsistent.  Wouldn't it be
>> more instinctive to return a NULL tuple instead if the slot does not
>> exist to be able to check after real ERRORs in frontends using this
>> interface?  
> 
> The attached patch returns no tuple at all when the replication slot doesn't 
> exist. I'm not sure if that's what you meant by returning a NULL tuple ? 

Just return a tuple filled only with NULL values.  I would tend to
code things so as we set all the flags of nulls[] to true by default,
remove has_value and define the number of columns in a #define, as of:
#define READ_REPLICATION_SLOT_COLS 5
[...]
Datum       values[READ_REPLICATION_SLOT_COLS];
bool        nulls[READ_REPLICATION_SLOT_COLS];
[...]
MemSet(nulls, true, READ_REPLICATION_SLOT_COLS * sizeof(bool));
Assert(i == READ_REPLICATION_SLOT_COLS);    // when filling values.

This would make ReadReplicationSlot() cleaner by removing all the
else{} blocks coded now to handle the NULL values, and that would be
more in-line with the documentation where we state that one tuple is
returned.  Note that this is the same kind of behavior for similar
in-core functions where objects are queried if they don't exist.

I would also suggest a reword of some of the docs, say:
+    <listitem>
+     <para>
+      Read the information of a replication slot. Returns a tuple with
+      <literal>NULL</literal> values if the replication slot does not
+      exist.
+     </para>

> 
>> A slot in use exists, so the error is a bit confusing here
>> anyway, no?
> 
> From my understanding, a slot  *not* in use doesn't exist anymore, as such I 
> don't really understand this point. Could you clarify ?

Yeah, sorry about that.  I did not recall the exact meaning of
in_use.  Considering the slot as undefined if the flag is false is the
right thing to do.

> I was thinking that maybe instead of walking back the timeline history from 
> where we currently are on the server, we could allow an additional argument 
> for the client to specify which timeline it wants. But I guess a replication 
> slot can not be present for a past, divergent timeline ? I have removed that 
> suggestion.

The parent TLI history is linear, so I'd find that a bit strange in
concept, FWIW.

>> -               'slot0'
>> +               'slot0',                     '-p',
>> +               "$port"
>> Something we are missing here?
> 
> The thing we're missing here is a wrapper for command_fails_like. I've added 
> this to PostgresNode.pm.

It may be better to apply this bit separately, then.
--
Michael