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
signature.asc
Description: PGP signature
