From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: pgsql-hackers@lists.postgresql.org
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 Michael Paquier <michael@paquier.xyz>
Subject: Re: pg_receivewal starting position
Date: Thu, 21 Oct 2021 08:29:54 +0200
Message-ID: <5336539.LvFx2qVVIh@aivenronan>
Organization: aiven
In-Reply-To: <YXD8DHlf/cB1db5D@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <4755573.0VBMTVartN@aivenronan> <YXD8DHlf/cB1db5D@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/5336539.LvFx2qVVIh%40aivenronan>
Precedence: bulk

Le jeudi 21 octobre 2021, 07:35:08 CEST Michael Paquier a =E9crit :
> On Wed, Oct 20, 2021 at 02:58:26PM +0200, Ronan Dunklau wrote:
> > After sending the previous patch suite, I figured it would be worthwhile
> > to
> > also have tests covering timeline switches, which was not covered befor=
e.
>=20
> That seems independent to me.  I'll take a look.
>=20
> > So please find attached a new version with an additional patch for those
> > tests, covering both  "resume from last know archive" and "resume from
> > the replication slots position" cases.
>=20
> So, taking things in order, I have looked at 0003 and 0001, and
> attached are refined versions for both of them.
>=20
> 0003 is an existing hole in the docs, which I think we had better
> address first and backpatch, taking into account that the starting
> point calculation considers compressed segments when looking for
> completed segments.

Ok, do you want me to propose a different patch for previous versions ?

>=20
> Regarding 0001, I have found the last test to check for NULL values
> returned by READ_REPLICATION_SLOT after dropping the slot overlaps
> with the first test, so I have removed that.  I have expanded a bit
> the use of like(), and there were some confusion with
> PostgresNode::psql and some extra arguments (see DROP_REPLICATION_SLOT
> and CREATE_REPLICATION_SLOT, and no need for return values in the
> CREATE case either).  Some comments, docs and code have been slightly
> tweaked.

Thank you for this.=20


>=20
> Here are some comments about 0002.
>=20
> +       /* The commpand should always return precisely one tuple */
> s/commpand/command/
>=20
> +       pg_log_error("could not fetch replication slot: got %d rows and %d
> fields, expected %d rows and %d or more fields", +                  =20
> PQntuples(res), PQnfields(res), 1, 3);
> Should this be "could not read" instead?
>=20
> +       if (sscanf(PQgetvalue(res, 0, 1), "%X/%X", &hi, &lo) !=3D 2)
> +       {
> +           pg_log_error("could not parse slot's restart_lsn \"%s\"",
> +                        PQgetvalue(res, 0, 1));
> +           PQclear(res);
> +           return false;
> +       }
> Wouldn't it be saner to initialize *restart_lsn and *restart_tli to
> some default values at the top of GetSlotInformation() instead, if
> they are specified by the caller? =20

Ok.

> And I think that we should still
> complain even if restart_lsn is NULL.

Do you mean restart_lsn as the pointer argument to the function, or=20
restart_lsn as the field returned by the command ? If it's the first, I'll=
=20
change it but if it's the latter it is expected that we sometime run this o=
n a=20
slot where WAL has never been reserved yet.

>=20
> On a quick read of 0004, I find the split of the logic with
> change_timeline() a bit hard to understand.  It looks like we should
> be able to make a cleaner split, but I am not sure how that would
> look, though.

Thanks, at least if the proposal to test this seems sensible I can move=20
forward. I wanted to avoid having a lot of code duplication since the test=
=20
setup is a bit more complicated.=20
My first approach was to split it into two functions, setup_standby and=20
change_timeline, but then realized that what would happen between the two=20
invocations would basically be the same for the two test cases, so I ended =
up=20
with that patch. I'll try to see if I can see a better way of organizing th=
at=20
code.

=2D-=20
Ronan Dunklau