From: Michael Paquier <michael@paquier.xyz>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Ronan Dunklau <ronan.dunklau@aiven.io>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: pg_receivewal starting position
Date: Sun, 24 Oct 2021 08:10:44 +0900
Message-ID: <YXSWdC0At7JBwoC+@paquier.xyz> (raw)
In-Reply-To: <CALj2ACV=U687py0cKWHomzAYsdVXK5K0M7gg2EWjLHq=gb6kTA@mail.gmail.com>
References: <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
<5336539.LvFx2qVVIh@aivenronan>
<YXEVCE7Td5WxTVpg@paquier.xyz>
<4126495.44csPzL39Z@aivenronan>
<YXO9QP5SJK1ExEN6@paquier.xyz>
<CALj2ACV=U687py0cKWHomzAYsdVXK5K0M7gg2EWjLHq=gb6kTA@mail.gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, bharath.rupireddyforpostgres@gmail.com, ronan.dunklau@aiven.io, pgsql-hackers@lists.postgresql.org, horikyota.ntt@gmail.com, sawada.mshk@gmail.com
Subject: Re: pg_receivewal starting position
In-Reply-To: <YXSWdC0At7JBwoC+@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox