From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org,
 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: pg_receivewal starting position
Date: Mon, 25 Oct 2021 09:15:32 +0200
Message-ID: <5781690.lOV4Wx5bFT@aivenronan>
Organization: aiven
In-Reply-To: <YXZT64bdcSBQIJpj@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <4126495.44csPzL39Z@aivenronan> <YXZT64bdcSBQIJpj@paquier.xyz>
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/5781690.lOV4Wx5bFT%40aivenronan>
Precedence: bulk

Le lundi 25 octobre 2021, 08:51:23 CEST Michael Paquier a =E9crit :
> On Thu, Oct 21, 2021 at 10:36:42AM +0200, Ronan Dunklau wrote:
> > Done. I haven't touched the timeline switch test patch for now, but I
> > still
> > include it here for completeness.
>=20
> As 0001 and 0002 have been applied, I have put my hands on 0003, and
> found a couple of issues upon review.
>=20
> +       Assert(slot_name !=3D NULL);
> +       Assert(restart_lsn !=3D NULL);
> There is no need for those asserts, as we should support the case
> where the caller gives NULL for those variables.

Does it make sense though ? The NULL slot_name case handling is pretty=20
straight forward has it will be handled by string formatting, but in the ca=
se=20
of a null restart_lsn, we have no way of knowing if the command was issued =
at=20
all.=20

>=20
> +       if (PQserverVersion(conn) < 150000)
> +               return false;
> Returning false is incorrect for older server versions as we won't
> fallback to the old method when streaming from older server.  What
> this needs to do is return true and set restart_lsn to
> InvalidXLogRecPtr, so as pg_receivewal would just stream from the
> current flush location.  "false" should just be returned on error,
> with pg_log_error().

Thank you, this was an oversight when moving from the more complicated erro=
r=20
handling code.=20

>=20
> +$primary->psql('postgres',
> +       'INSERT INTO test_table VALUES (generate_series(1,100));');
> +$primary->psql('postgres', 'SELECT pg_switch_wal();');
> +$nextlsn =3D
> +  $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
> +chomp($nextlsn);
> There is no need to switch twice to a new WAL segment as we just need
> to be sure that the WAL segment of the restart_lsn is the one
> archived.  Note that RESERVE_WAL uses the last redo point, so it is
> better to use a checkpoint and reduce the number of logs we stream
> into the new location.
>=20
> Better to add some --no-sync to the new commands of pg_receivewal, to
> not stress the I/O more than necessary.  I have added some extra -n
> while on it to avoid loops on failure.
>=20
> Attached is the updated patch I am finishing with, which is rather
> clean now.  I have tweaked a couple of things while on it, and
> documented better the new GetSlotInformation() in streamutil.c.
> --
> Michael


=2D-=20
Ronan Dunklau