From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: pg_receivewal starting position
Date: Mon, 30 Aug 2021 11:56:07 +0200
Message-ID: <3561829.oDkbz2t6ih@aivenronan>
Organization: aiven
In-Reply-To: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
References: <18708360.4lzOvYHigE@aivenronan> <2396633.4C23JXgCg5@aivenronan>
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/3561829.oDkbz2t6ih%40aivenronan>
Precedence: bulk

Le samedi 28 ao=FBt 2021, 14:10:25 CEST Bharath Rupireddy a =E9crit :
> On Thu, Aug 26, 2021 at 5:45 PM Ronan Dunklau <ronan.dunklau@aiven.io>=20
wrote:
> > order to fail early if the replication slot doesn't exist, so please fi=
nd
> > attached v2 for that.
>=20
> Thanks for the patches. Here are some comments:
>=20

Thank you for this review ! Please see the other side of the thread where I=
=20
answered Michael Paquier with a new patchset, which includes some of your=20
proposed modifications.

> 1) While the intent of these patches looks good, I have following
> concern with new replication command READ_REPLICATION_SLOT: what if
> the pg_receivewal exits (because user issued a SIGINT or for some
> reason) after flushing  the received WAL to disk, before it sends
> sendFeedback to postgres server's walsender so that it doesn't get a
> chance to update the restart_lsn in the replication slot via
> PhysicalConfirmReceivedLocation. If the pg_receivewal is started
> again, isn't it going to get the previous restart_lsn and receive the
> last chunk of flushed WAL again?

I've kept the existing directory as the source of truth if we have any WAL=
=20
there already. If we don't, we fallback to the restart_lsn on the replicati=
on=20
slot.
So in the event that we start it again from somewhere else where we don't h=
ave=20
access to those WALs anymore, we could be receiving it again, which in my=20
opinion is better than losing everything in between in that case.=20

>=20
> 2) What is the significance of READ_REPLICATION_SLOT for logical
> replication slots? I read above that somebody suggested to restrict
> the walsender to handle READ_REPLICATION_SLOT for physical replication
> slots so that the callers will see a command failure. But I tend to
> think that it is clean to have this command common for both physical
> and logical replication slots and the callers can have an Assert(type
> =3D=3D 'physical').

I've updated the patch to make it easy for the caller to check the slot's t=
ype=20
and added a verification for those cases.

In general, I tried to implement the meaning of the different fields exactl=
y as=20
it's done in the pg_replication_slots view.

>=20
> 3) Isn't it useful to send active, active_pid info of the replication
> slot via READ_REPLICATION_SLOT? pg_receivewal can use Assert(active =3D=3D
> true && active_pid =3D=3D getpid()) as an assertion to ensure that it is
> the sole owner of the replication slot? Also, is it good send
> wal_status info

Typically we read the slot before attaching to it, since what we want to do=
 is=20
check if it exists. It may be worthwile to check that it's not already used=
 by=20
another backend though.

>=20
> 4) I think below messages should start with lower case letter and also
> there are some typos:
> + pg_log_warning("Could not fetch the replication_slot \"%s\" information=
 "
> + pg_log_warning("Server does not suport fetching the slot's position, "
> something like:
> + pg_log_warning("could not fetch replication slot \"%s\" information, "
> +    "resuming from current server position instead", replication_slot);
> + pg_log_warning("server does not support fetching replication slot
> information, "
> +    "resuming from current server position instead");
>=20
I've rephrased it a bit in v3, let me know if that's what you had in mind.


> 5) How about emitting the above messages in case of "verbose"?

I think it is useful to warn the user even if not in the verbose case, but =
if=20
the consensus is to move it to verbose only output I can change it.

>=20
> 6) How about an assertion like below?
> + if (stream.startpos =3D=3D InvalidXLogRecPtr)
> + {
> + stream.startpos =3D serverpos;
> + stream.timeline =3D servertli;
> + }
> +
> +Assert(stream.startpos !=3D InvalidXLogRecPtr)>>

Good idea.

>=20
> 7) How about we let pg_receivewal use READ_REPLICATION_SLOT as an option?

=46rom my point of view, I already expected it to use something like that w=
hen=20
using a replication slot. Maybe an option to turn it off could be useful ?=
=20

>=20
> 8) Just an idea, how about we store pg_receivewal's lastFlushPosition
> in a file before pg_receivewal exits and compare it with the
> restart_lsn that it received from the replication slot, if
> lastFlushPosition =3D=3D received_restart_lsn well and good, if not, then
> something would have happened and we always start at the
> lastFlushPosition ?

The patch idea originally came from the fact that some utility use=20
pg_receivewal to fetch WALs, and move them elsewhere. In that sense I don't=
=20
really see what value this brings compared to the existing (and unmodified)=
 way=20
of computing the restart position from the already stored files ?

Best regards,

=2D-=20
Ronan Dunklau