Date: Sun, 24 Oct 2021 08:10:44 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Ronan Dunklau <ronan.dunklau@aiven.io>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: pg_receivewal starting position
Message-ID: <YXSWdC0At7JBwoC+@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <5336539.LvFx2qVVIh@aivenronan>
 <YXEVCE7Td5WxTVpg@paquier.xyz>
 <4126495.44csPzL39Z@aivenronan>
 <YXO9QP5SJK1ExEN6@paquier.xyz>
 <CALj2ACV=U687py0cKWHomzAYsdVXK5K0M7gg2EWjLHq=gb6kTA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="0YoTBNT/JdY9aH9G"
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACV=U687py0cKWHomzAYsdVXK5K0M7gg2EWjLHq=gb6kTA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YXSWdC0At7JBwoC%2B%40paquier.xyz>
Precedence: bulk


--0YoTBNT/JdY9aH9G
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Oct 23, 2021 at 10:46:30PM +0530, Bharath Rupireddy wrote:
> 1) It's better to initialize nulls with false, we can avoid setting
> them to true. The instances where the columns are not nulls is going
> to be more than the columns with null values, so we could avoid some
> of nulls[i] = false; instructions.

I don't think that this is an improvement in this case as in the
default case we'd return a tuple full of NULL values if the slot does
not exist, so the existing code is simpler when we don't look at the
slot contents.

> 2) As I previously mentioned, we are not copying the slot contents
> while holding the spinlock, it's just we are taking the memory address
> and releasing the lock, so there is a chance that the memory we are
> looking at can become unavailable or stale while we access
> slot_contents. So, I suggest we do the memcpy of the *slot to
> slot_contents. I'm sure the memcpy ing the entire ReplicationSlot
> contents will be costlier, so let's just take the info that we need
> (data.database, data.restart_lsn) into local variables while we hold
> the spin lock

The style of the patch is consistent with what we do in other areas
(see pg_get_replication_slots as one example).

> + /* Copy slot contents while holding spinlock */
> + SpinLockAcquire(&slot->mutex);
> + slot_contents = *slot;

And what this does is to copy the contents of the slot into a local
area (note that we use a NameData pointing to an area with
NAMEDATALEN).  Aka if the contents of *slot are changed by whatever
reason (this cannot change as of the LWLock acquired), what we have
saved is unchanged as of this command's context.

> 3) The data that the new command returns to the client can actually
> become stale while it is captured and in transit to the client as we
> release the spinlock and other backends can drop or alter the info.
> So, it's better we talk about this in the documentation of the new
> command and also in the comments saying "clients will have to deal
> with it."

The same can be said with IDENTIFY_SYSTEM when the flushed location
becomes irrelevant.  I am not sure that this has any need to apply
here.  We could add that this is useful to get a streaming start
position though.

> 4) How about we be more descriptive about the error added? This will
> help identify for which replication slot the command has failed from
> tons of server logs which really help in debugging and analysis.
> I suggest we have this:
> errmsg("cannot use \"%s\" command with logical replication slot
> \"%s\"", "READ_REPLICATION_SLOT", cmd->slotname);
> instead of just a plain, non-informative, generic message:
> errmsg("cannot use \"%s\" with logical replication slots",

Yeah.  I don't mind adding the slot name in this string.
--
Michael

--0YoTBNT/JdY9aH9G
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmF0lnQACgkQnvQgOdby
QH1bjg/+PzrV40um2sX9U02C5sqvxJb8Vg3NgiOz+y7sNhqBN+YESaERSeOvcSMB
tox8oDh9SqE3wukWKJk/0T0mx9SABKfPyID/dL7+RmKoOXuPS3PCkx7vZIiFIj0Y
jgsn3lGPljrvWUFULKIiQ4RcvHhG1/DJVXmeLDLmP4ZkmXs8Y99KSYuF/zRqK2bj
gOLUa1Is2pbvq5L8X3pgSmZLBBht76A61KE9swyzahTQDZ5Zln/PoaCdihyjDZAP
06V2V0/kXT3Bukr/yOgx3tbpQCuW9pX+QhnNmf04rGUXkYyXzY3IBSSAFSV9gHoX
L4dq1074pZrSBtQMkjNk6gxuD8Fgxoe2CTvlgXSOgAPnn5QK81dMNwmg5wOKLa98
FXcPXHL0kli0yfzp43CukNz3lbIkG/g2iRDtQ4m2fFgCQnQwMqZbucue6jXFx2R8
7VEYprIDb6MMosc1eKlXzTEI3bFpkrQW/bwUAnD0aEWE4afkX7OM7fkQTU990Xq/
pW9HEuYVT/eMlrHs8TrokyDv6F6rrtABdt9ZmwPKwY18stoD6sKUre9sZAjDkheU
14kI8ewD48Z4t1NPqjAbenf3pzeqvV1F3JE59LC85+9UX5CXJJQ8LS9KQzU/t5lB
bAKw4fCN8XEssCJkLjAqHGUHLq195lCj/uhwuSj1Nt6ClkcLSVw=
=4inJ
-----END PGP SIGNATURE-----

--0YoTBNT/JdY9aH9G--