Re: pg_receivewal starting position

Le jeudi 2 septembre 2021, 09:28:29 CEST Michael Paquier a écrit :
> On Thu, Sep 02, 2021 at 02:45:54PM +0900, Kyotaro Horiguchi wrote:
> > At Wed, 1 Sep 2021 10:30:05 +0530, Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in If I read the patch
> > correctly the situation above is warned by the following message then
> > continue to the next step giving up to identify start position from slot
> > data.
>
> Better to fallback to the past behavior if attempting to use a
> pg_receivewal >= 15 with a PG instance older than 14.
>
> >> "server does not suport fetching the slot's position, resuming from the
> >> current server position instead">
> > (The message looks a bit too long, though..)
>
> Agreed. Falling back to a warning is not the best answer we can have
> here, as there could be various failure types, and for some of them a
> hard failure is adapted;
> - Failure in the backend while running READ_REPLICATION_SLOT. This
> should imply a hard failure, no?
> - Slot that does not exist. In this case, we could fall back to the
> current write position of the server.
>
> by default if the slot information cannot be retrieved.
> Something that's disturbing me in patch 0002 is that we would ignore
> the results of GetSlotInformation() if any error happens, even if
> there is a problem in the backend, like an OOM. We should be careful
> about the semantics here.

Ok !

>
> > However, if the operator doesn't know the server is old, pg_receivewal
> > starts streaming from unexpected position, which is a kind of
> > disaster. So I'm inclined to agree to Bharath, but rather I imagine of
> > an option to explicitly specify how to determine the start position.
> >
> > --start-source=[server,wal,slot] specify starting-LSN source, default is
> >
> > trying all of them in the order of wal, slot, server.
> >
> > I don't think the option doesn't need to accept multiple values at once.
>
> What is the difference between "wal" and "server"? "wal" stands for
> the start position of the set of files stored in the location
> directory, and "server" is the location that we'd receive from the
> server? I don't think that we need that because, when using a slot,
> we know that we can rely on the LSN that the slot retains for
> pg_receivewal as that should be the same point as what has been
> streamed last. Could there be an argument instead for changing the
> default and rely on the slot information rather than scanning the
> local WAL archive path for the start point when using --slot? When
> using pg_receivewal as a service, relying on a scan of the WAL archive
> directory if there is no slot and fallback to an invalid LSN if there
> is nothing is fine by me, but I think that just relying on the slot
> information is saner as the backend makes sure that nothing is
> missing. That's also more useful when streaming changes from a single
> slot from multiple locations (stream to location 1 with a slot, stop
> pg_receivewal, stream to location 2 that completes 1 with the same
> slot).

One benefit I see from first trying to get it from the local WAL stream is that
we may end up in a state where it has been flushed to disk but we couldn't
advance the replication slot. In that case it is better to resume from the
point on disk. Maybe taking the max(slot_lsn, local_file_lsn) would work best
for the use case you're describing.

>
> + pg_log_error("Slot \"%s\" is not a physical replication slot",
> + replication_slot);
> In 0003, the format of this error is not really project-like.
> Something like that perhaps would be more adapted:
> "cannot use the slot provided, physical slot expected."
>
> I am not really convinced about the need of getting the active state
> and the PID used in the backend when fetcing the slot data,
> particularly if that's just for some frontend-side checks. The
> backend has safeguards already for all that.

I could see the use for sending active_pid for use within pg_basebackup: at
least we could fail early if the slot is already in use. But at the same time,
maybe it won't be in use anymore once we need it.

>
> While looking at that, I have applied de1d4fe to add
> PostgresNode::command_fails_like(), coming from 0003, and put my hands
> on 0001 as per the attached, as the starting point. That basically
> comes down to all the points raised upthread, plus some tweaks for
> things I bumped into to get the semantics of the command to what looks
> like the right shape.

Thanks, I was about to send a new patchset with basically the same thing. It
would be nice to know we work on the same thing concurrently in the future to
avoid duplicate efforts. I'll rebase and send the updated version for patches
0002 and 0003 of my original proposal once we reach an agreement over the
behaviour / options of pg_receivewal.

Also considering the number of different fields to be filled by the
GetSlotInformation function, my local branch groups them into a dedicated
struct which is more convenient than having X possibly null arguments.

--
Ronan Dunklau