public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nisha Moond <[email protected]>
To: Peter Smith <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
Date: Tue, 19 Nov 2024 12:47:37 +0530
Message-ID: <CABdArM4w-rBmSMqPvsWLdj+OwLZzOZ-MX_596qhHmc+XvP9dVg@mail.gmail.com> (raw)
In-Reply-To: <CAHut+Pt6s-qNPdxH5=-fr2QKLEv0h16sQ8EvLiGJ-SdQNS6pbw@mail.gmail.com>
References: <CAD21AoAd0BX0L17YxSCbSNbj=3V7dvLVGY0WhwLHAt724rNCZA@mail.gmail.com>
<[email protected]>
<CAJpy0uBthb-RG=Xg+mM-G6Xy01JLcBsizJuz+xbti08qP_W7AQ@mail.gmail.com>
<CALj2ACVfDBcbybgyga7ia_KiQPAfEi_3=VDOGf2VU8RkJZSQpw@mail.gmail.com>
<[email protected]>
<CALj2ACUmp-F+irHYfNhmo_9yaCo7aGi7ZB20KQcDR3S9eT56hg@mail.gmail.com>
<Zg1WqRS/[email protected]>
<CALj2ACUBiRLHF7VYD-0nYVHSvVBSCQ_5OMbP4tE8Ys_rpvQ--A@mail.gmail.com>
<[email protected]>
<CALj2ACXbUzp-9tixeUK_dws5k+ARJaXxj8cj9W3adA0XNUS4Hg@mail.gmail.com>
<[email protected]>
<CALj2ACW8SYiRL8vGAhez=FdrzxO0W5pVFvN8GF7yKCO8_c=QdQ@mail.gmail.com>
<CAA4eK1+-ipXB+LuUiDNQ5L+sWJFpzK89k-Oc7xOG37AefrptoQ@mail.gmail.com>
<CALj2ACWhTY9mRc7sOpuao=GGidRjCVXc0SNtJ_DNman4nh5CcQ@mail.gmail.com>
<CALj2ACVY+Fd5vC0VjW=5VDK9mmt-Y+PDZxnBp8ngGAZc24Vv9g@mail.gmail.com>
<CALj2ACX0FVrx9f0c2g19Rb-WAJhm3wdyttde3YQtkotL6bUveA@mail.gmail.com>
<CALj2ACXQNGXokgx8APwdxrG4MHMF=cOz6XQtUL7EHua9oUfkgA@mail.gmail.com>
<CAFPTHDZPOc7pY4E0zJ2om0MZLMt+UuiX-VLsJyRO_aSyR8DsnA@mail.gmail.com>
<CALj2ACVu0EJ+QQ7W0oHbL_+hCtk+p-00MNhE4hfjFcXaMepMfg@mail.gmail.com>
<CAA4eK1KVbicrQQbMLEQiHEi9=VJpxrErrK7CiMamwaMPfff3WA@mail.gmail.com>
<CALj2ACXe8+xSNdMXTMaSRWUwX7v61Ad4iddUwnn=djSwx3GLLg@mail.gmail.com>
<CAHut+PuFzCHPCiZbpoQX59kgZbebuWT0gR0O7rOe4t_sdYu=OA@mail.gmail.com>
<CALj2ACUJg2faYd-_Wr3NWoj_GtbW5Bje3G9+o595GyEhcBkbrg@mail.gmail.com>
<CAHut+PuC7nwuT3+36zsLxtBAcUHrSaysrHb0_dxTXqMnFBKZBA@mail.gmail.com>
<CALj2ACWXQT3_HY40ceqKf1DadjLQP6b1r=0sZRh-xhAOd-b0pA@mail.gmail.com>
<CAHut+PtJxxaVhjDCFPqE_V63VaAN_bL+PHRMak=dkghoGrZc0Q@mail.gmail.com>
<CALj2ACXjkM4nEE5dXeV=NA2NmbE3AmonKEgudYBMqU3qO9-9fw@mail.gmail.com>
<CAJpy0uAwxc49Dz6t=-y_-z-MU+A4RWX4BR3Zri_jj2qgGMq_8g@mail.gmail.com>
<CAJpy0uC6nN3SLbEuCvz7-CpaPdNdXxH=feW5MhYQch-JWV0tLg@mail.gmail.com>
<CAJpy0uBXXJC6f04+FU1axKaU+p78wN0SEhUNE9XoqbjXj=hhgw@mail.gmail.com>
<CAJpy0uB48aCJX8gpwwv-kGoOK3WYXWatB=1pgr__UW-OjwKkDg@mail.gmail.com>
<CABdArM44oW0dQa6Vv5c4xcY54rvhaKsBOpbDVaYzf0F3hUZFPw@mail.gmail.com>
<CAHut+Pt6s-qNPdxH5=-fr2QKLEv0h16sQ8EvLiGJ-SdQNS6pbw@mail.gmail.com>
On Thu, Nov 14, 2024 at 5:29 AM Peter Smith <[email protected]> wrote:
>
> Hi Nisha.
>
> Thanks for the recent patch updates. Here are my review comments for
> the latest patch v48-0001.
>
Thank you for the review. Comments are addressed in v49 version.
Below is my response to comments that may require further discussion.
> ======
> doc/src/sgml/system-views.sgml
>
> 2.
> <para>
> The time since the slot has become inactive.
> - <literal>NULL</literal> if the slot is currently being used.
> - Note that for slots on the standby that are being synced from a
> + <literal>NULL</literal> if the slot is currently being used. Once the
> + slot is invalidated, this value will remain unchanged until we shutdown
> + the server. Note that for slots on the standby that are being
> synced from a
> primary server (whose <structfield>synced</structfield> field is
> <literal>true</literal>), the
>
> Is this change related to the new inactivity timeout feature or are
> you just clarifying the existing behaviour of the 'active_since'
> field.
>
Yes, this patch introduces inactive_timeout invalidation and prevents
updates to inactive_since for invalid slots. Only a node restart can
modify it, so, I believe we should retain these lines in this patch.
> Note there is already another thread [1] created to patch/clarify this
> same field. So if you are just clarifying existing behavior then IMO
> it would be better if you can to try and get your desired changes
> included there quickly before that other patch gets pushed.
>
Thanks for the reference, I have posted my suggestion on the thread.
>
> ReplicationSlotAcquire:
>
> 5.
> + *
> + * An error is raised if error_if_invalid is true and the slot has been
> + * invalidated previously.
> */
> void
> -ReplicationSlotAcquire(const char *name, bool nowait)
> +ReplicationSlotAcquire(const char *name, bool nowait, bool error_if_invalid)
>
> This function comment makes it seem like "invalidated previously"
> might mean *any* kind of invalidation, but later in the body of the
> function we find the logic is really only used for inactive timeout.
>
> + /*
> + * An error is raised if error_if_invalid is true and the slot has been
> + * previously invalidated due to inactive timeout.
> + */
>
> So, I think a better name for that parameter might be
> 'error_if_inactive_timeout'
>
> OTOH, if it really is supposed to erro for *any* kind of invalidation
> then there needs to be more ereports.
>
+1 to the idea.
I have created a separate patch v49-0001 adding more ereports for all
kinds of invalidations.
> ~~~
> SlotInactiveTimeoutCheckAllowed:
>
> 8.
> +/*
> + * Is this replication slot allowed for inactive timeout invalidation check?
> + *
> + * Inactive timeout invalidation is allowed only when:
> + *
> + * 1. Inactive timeout is set
> + * 2. Slot is inactive
> + * 3. Server is in recovery and slot is not being synced from the primary
> + *
> + * Note that the inactive timeout invalidation mechanism is not
> + * applicable for slots on the standby server that are being synced
> + * from the primary server (i.e., standby slots having 'synced' field 'true').
> + * Synced slots are always considered to be inactive because they don't
> + * perform logical decoding to produce changes.
> + */
>
> 8a.
> Somehow that first sentence seems strange. Would it be better to write it like:
>
> SUGGESTION
> Can this replication slot timeout due to inactivity?
>
I feel the suggestion is not very clear on the purpose of the
function, This function doesn't check inactivity or decide slot
timeout invalidation. It only pre-checks if the slot qualifies for an
inactivity check, which the caller will perform.
As I have changed function name too as per commnet#9, I used the following -
"Is inactive timeout invalidation possible for this replication slot?"
Thoughts?
> ~
> 8c.
> Similarly, I think something about that "Note that the inactive
> timeout invalidation mechanism is not applicable..." paragraph needs
> tweaking because IMO that should also now be saying something about
> 'RecoveryInProgress'.
>
'RecoveryInProgress' check indicates that the server is a standby, and
the mentioned paragraph uses the term "standby" to describe the
condition. It seems unnecessary to mention RecoveryInProgress
separately.
> ~~~
>
> InvalidatePossiblyObsoleteSlot:
>
> 10.
> break;
> + case RS_INVAL_INACTIVE_TIMEOUT:
> +
> + /*
> + * Check if the slot needs to be invalidated due to
> + * replication_slot_inactive_timeout GUC.
> + */
>
> Since there are no other blank lines anywhere in this switch, the
> introduction of this one in v48 looks out of place to me.
pgindent automatically added this blank line after 'case
RS_INVAL_INACTIVE_TIMEOUT'.
> IMO it would
> be more readable if a blank line followed each/every of the breaks,
> but then that is not a necessary change for this patch so...
>
Since it's not directly related to the patch, I feel it might be best
to leave it as is for now.
> ~~~
>
> 11.
> + /*
> + * Invalidation due to inactive timeout implies that
> + * no one is using the slot.
> + */
> + Assert(s->active_pid == 0);
>
> Given this assertion, does it mean that "(s->active_pid == 0)" should
> have been another condition done up-front in the function
> 'SlotInactiveTimeoutCheckAllowed'?
>
I don't think it's a good idea to check (s->active_pid == 0) upfront,
before the timeout-invalidation check. AFAIU, this assertion is meant
to ensure active_pid = 0 only if the slot is going to be invalidated,
i.e., when the following condition is true:
TimestampDifferenceExceeds(s->inactive_since, now,
replication_slot_inactive_timeout_sec * 1000)
Thoughts? Open to others' opinions too.
> ~~~
>
> 12.
> /*
> - * If the slot can be acquired, do so and mark it invalidated
> - * immediately. Otherwise we'll signal the owning process, below, and
> - * retry.
> + * If the slot can be acquired, do so and mark it as invalidated. If
> + * the slot is already ours, mark it as invalidated. Otherwise, we'll
> + * signal the owning process below and retry.
> */
> - if (active_pid == 0)
> + if (active_pid == 0 ||
> + (MyReplicationSlot == s &&
> + active_pid == MyProcPid))
>
> I wasn't sure how this change belongs to this patch, because the logic
> of the previous review comment said for the case of invalidation due
> to inactivity that active_id must be 0. e.g. Assert(s->active_pid ==
> 0);
>
I don't fully understand the purpose of this change yet. I'll look
into it further and get back.
> ~~~
>
> RestoreSlotFromDisk:
>
> 13.
> - slot->inactive_since = GetCurrentTimestamp();
> + slot->inactive_since = now;
>
> In v47 this assignment used to call the function
> 'ReplicationSlotSetInactiveSince'. I recognise there is a very subtle
> difference between direct assignment and the function, because the
> function will skip assignment if the slot is already invalidated.
> Anyway, if you are *deliberately* not wanting to call
> ReplicationSlotSetInactiveSince here then I think this assignment
> should be commented to explain the reason why not, otherwise someone
> in the future might be tempted to think it was just an oversight and
> add the call back in that you don't want.
>
Added comment saying avoid using ReplicationSlotSetInactiveSince()
here as it will skip the invalid slots.
~~~~
--
Thanks,
Nisha
view thread (98+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
In-Reply-To: <CABdArM4w-rBmSMqPvsWLdj+OwLZzOZ-MX_596qhHmc+XvP9dVg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox