On Fri, Sep 03, 2021 at 11:58:27AM +0200, Ronan Dunklau wrote: > > 0002 for pg_receivewal tried to simplify the logic of information to return, > by using a dedicated struct for this. This accounts for Bahrath's demands to > return every possible field. > In particular, an is_logical field simplifies the detection of the type of > slot. > In my opinion it makes sense to simplify it like this on the client side > while > being more open-minded on the server side if we ever need to provide a new > type of slot. Also, GetSlotInformation now returns an enum to be able to > handle the different modes of failures, which differ between pg_receivewal > and > pg_basebackup.
+ if (PQserverVersion(conn) < 150000)
+ return READ_REPLICATION_SLOT_UNSUPPORTED;
[...]
+typedef enum {
+ READ_REPLICATION_SLOT_OK,
+ READ_REPLICATION_SLOT_UNSUPPORTED,
+ READ_REPLICATION_SLOT_ERROR,
+ READ_REPLICATION_SLOT_NONEXISTENT
+} ReadReplicationSlotStatus;
Do you really need this much complication? We could treat the
unsupported case and the non-existing case the same way: we don't know
so just assign InvalidXlogRecPtr or NULL to the fields of the
structure, and make GetSlotInformation() return false just on error,
with some pg_log_error() where adapted in its internals.
> There is still the concern raised by Bharath about being able to select the
> way to fetch the replication slot information for the user, and what should
> or
> should not be included in the documentation. I am in favor of documenting the
> process of selecting the wal start, and maybe include a --start-lsn option
> for
> the user to override it, but that's maybe for another patch.
The behavior of pg_receivewal that you are implementing should be
documented. We don't say either how the start location is selected
when working on an existing directory, so I would recommend to add a
paragraph in the description section to detail all that, as of:
- First a scan of the existing archives is done.
- If nothing is found, and if there is a slot, request the slot
information.
- If still nothing (aka slot undefined, or command not supported), use
the last flush location.
As a whole, I am not really convinced that we need a new option for
that as long as we rely on a slot with pg_receivewal as these are used
to make sure that we avoid holes in the WAL archives.
Regarding pg_basebackup, Daniel has proposed a couple of days ago a
different solution to trap errors earlier, which would cover the case
dealt with here:
https://www.postgresql.org/message-id/0f69e282-97f9-4db7-8d6d-f927aa634...@yesql.se
We should not mimic in the frontend errors that are safely trapped in
the backend with the proper locks, in any case.
While on it, READ_REPLICATION_SLOT returns a confirmed LSN when
grabbing the data of a logical slot. We are not going to use that
with pg_recvlogical as by default START_STREAMING 0/0 would just use
the confirmed LSN. Do we have use cases where this information would
help? There is the argument of consistency with physical slots and
that this can be helpful to do sanity checks for clients, but that's
rather thin.
--
Michael
signature.asc
Description: PGP signature
