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: Tue, 31 Aug 2021 15:08:47 +0200
Message-ID: <9598692.SQ0H41ba6E@aivenronan>
Organization: aiven
In-Reply-To: 
 <CALj2ACXB9iGk=CdLYtaFVwaw316mOWdiF8zNEbnvFxRu2T8bPQ@mail.gmail.com>
References: <18708360.4lzOvYHigE@aivenronan> <3561829.oDkbz2t6ih@aivenronan>
 <CALj2ACXB9iGk=CdLYtaFVwaw316mOWdiF8zNEbnvFxRu2T8bPQ@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/9598692.SQ0H41ba6E%40aivenronan>
Precedence: bulk

Le mardi 31 ao=FBt 2021, 13:17:22 CEST Bharath Rupireddy a =E9crit :
> On Mon, Aug 30, 2021 at 3:26 PM Ronan Dunklau <ronan.dunklau@aiven.io>=20
wrote:
> > Thank you for this review ! Please see the other side of the thread whe=
re
> > I
> > answered Michael Paquier with a new patchset, which includes some of yo=
ur
> > proposed modifications.
>=20
> Thanks for the updated patches. Here are some comments on v3-0001
> patch. I will continue to review 0002 and 0003.

Thank you ! I will send a new version shortly, once I address your remarks=
=20
concerning patch 0002 (and hopefully 0003 :-) )=20
>=20
> 1) Missing full stops "." at the end.
> +               <literal>logical</literal>
> +               when following the current timeline history
> +               position, when following the current timeline history
>=20

Good catch, I will take care of it for the next version.

> 2) Can we have the "type" column as "slot_type" just to keep in sync
> with the pg_replication_slots view?

You're right, it makes more sense like this.

>=20
> 3) Can we mention the link to pg_replication_slots view in the columns
> - "type", "restart_lsn", "confirmed_flush_lsn"?
> Something like: the "slot_type"/"restart_lsn"/"confirmed_flush_lsn" is
> same as  <link
> linkend=3D"view-pg-replication-slots"><structname>pg_replication_slots</s=
truc
> tname></link> view.

Same as above, thanks.

>=20
> 4) Can we use "read_replication_slot" instead of
> "identify_replication_slot", just to be in sync with the actual
> command?

That must have been a leftover from an earlier version of the patch, I will=
 fix=20
it also.

>=20
> 5) Are you actually copying the slot contents into the slot_contents
> variable here? Isn't just taking the pointer to the shared memory?
> + /* Copy slot contents while holding spinlock */
> + SpinLockAcquire(&slot->mutex);
> + slot_contents =3D *slot;
> + SpinLockRelease(&slot->mutex);
> + LWLockRelease(ReplicationSlotControlLock);
>=20
> You could just do:
> + Oid dbid;
> + XLogRecPtr restart_lsn;
> + XLogRecPtr confirmed_flush;
>=20
> + /* Copy the required slot contents */
> + SpinLockAcquire(&slot->mutex);
> + dbid =3D slot.data.database;
> + restart_lsn =3D slot.data.restart_lsn;
> + confirmed_flush =3D slot.data.confirmed_flush;
> + SpinLockRelease(&slot->mutex);
> + LWLockRelease(ReplicationSlotControlLock);

It's probably simpler that way.

>=20
> 6) It looks like you are not sending anything as a response to the
> READ_REPLICATION_SLOT command, if the slot specified doesn't exist.
> You are just calling end_tup_output which just calls rShutdown (points
> to donothingCleanup of printsimpleDR)
> if (has_value)
> do_tup_output(tstate, values, nulls);
> end_tup_output(tstate);

>=20
> Can you test the use case where the pg_receivewal asks
> READ_REPLICATION_SLOT with a non-existent replication slot and see
> with your v3 patch how it behaves?

>=20
> Why don't you remove has_value flag and do this in ReadReplicationSlot:
> Datum values[5];
> bool nulls[5];
> MemSet(values, 0, sizeof(values));
> MemSet(nulls, 0, sizeof(nulls));
>=20
> + dest =3D CreateDestReceiver(DestRemoteSimple);
> + tstate =3D begin_tup_output_tupdesc(dest, tupdesc, &TTSOpsVirtual);
> + do_tup_output(tstate, values, nulls);
> + end_tup_output(tstate);

As for the API, I think it's cleaner to just send an empty result set inste=
ad=20
of a null tuple in that case but I won't fight over it if there is consensu=
s on=20
having an all-nulls tuple value instead.=20

There is indeed a bug, but not here, in the second patch: I still test the=
=20
slot type even when we didn't fetch anything. So I will add a test for that=
=20
too.=20
>=20
> 7) Why don't we use 2 separate variables "restart_tli",
> "confirmed_flush_tli" instead of "slots_position_timeline", just to be
> more informative?

You're right.

>=20
> 8) I still have the question like, how can a client (pg_receivewal for
> instance) know that it is the current owner/user of the slot it is
> requesting the info? As I said upthread, why don't we send "active"
> and "active_pid" fields of the pg_replication_slots view?
> Also, it would be good to send the "wal_status" field so that the
> client can check if the "wal_status" is not "lost"?

 As for pg_receivewal, it can only check that it's not active at that time,=
=20
since we only aquire the replication slot once we know the start_lsn. For t=
he=20
basebackup case it's the same thing as we only want to check if it exists.=
=20
As such, I didn't add them as I didn't see the need, but if it can be usefu=
l=20
why not ? I will do that in the next version.

>=20
> 9) There are 2 new lines at the end of ReadReplicationSlot. We give
> only one new line after each function definition.
> end_tup_output(tstate);
> }
> <<1stnewline>>
> <<2ndnewline>>
> /*
>  * Handle TIMELINE_HISTORY command.
>  */
>=20

Ok !


> 10) Why do we need to have two test cases for "non-existent" slots?
> Isn't the test case after "DROP REPLICATION" enough?
> +($ret, $stdout, $stderr) =3D $node_primary->psql(
> +  'postgres', 'READ_REPLICATION_SLOT non_existent_slot;',
> +  extra_params =3D> [ '-d', $connstr_rep ]);
> +ok( $ret =3D=3D 0,
> +  "READ_REPLICATION_SLOT does not produce an error with non existent
> slot"); +ok( $stdout eq '',
> +    "READ_REPLICATION_SLOT returns no tuple if a slot is non existent");
>=20
> You can just rename the test case name from:
> +    "READ_REPLICATION_SLOT returns no tuple if a slot has been dropped");
> to
> +    "READ_REPLICATION_SLOT returns no tuple if a slot is non existent");
>=20

I wanted to test both the case where no slot by this name exists, and the c=
ase=20
where it has been dropped hence still referenced but marked as not "in_use"=
=2E=20
Maybe it's not worth it and we can remove the first case as you suggest.

Best regards,

=2D-=20
Ronan Dunklau