public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nisha Moond <[email protected]>
To: vignesh C <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Peter Smith <[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:50:51 +0530
Message-ID: <CABdArM5wR1ea8NpNU+tG0bB1m6G=Lep3bd1uthtSGj64JyqJsw@mail.gmail.com> (raw)
In-Reply-To: <CALDaNm2VQW_gpOJ-QWkEA_h18DN31ELEz2_7QmwWCAg9=Zew4A@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>
<CALDaNm2VQW_gpOJ-QWkEA_h18DN31ELEz2_7QmwWCAg9=Zew4A@mail.gmail.com>
On Thu, Nov 14, 2024 at 9:14 AM vignesh C <[email protected]> wrote:
>
> On Wed, 13 Nov 2024 at 15:00, Nisha Moond <[email protected]> wrote:
> >
> > Please find the v48 patch attached.
> >
> > On Thu, Sep 19, 2024 at 9:40 AM shveta malik <[email protected]> wrote:
> > >
> > > When we promote hot standby with synced logical slots to become new
> > > primary, the logical slots are never invalidated with
> > > 'inactive_timeout' on new primary. It seems the check in
> > > SlotInactiveTimeoutCheckAllowed() is wrong. We should allow
> > > invalidation of slots on primary even if they are marked as 'synced'.
> >
> > fixed.
> >
> > > I have raised 4 issues so far on v46, the first 3 are in [1],[2],[3].
> > > Once all these are addressed, I can continue reviewing further.
> > >
> >
> > Fixed issues reported in [1], [2].
>
> Few comments:
Thanks for the review.
>
> 2) Currently it allows a minimum value of less than 1 second like in
> milliseconds, I feel we can have some minimum value at least something
> like checkpoint_timeout:
> diff --git a/src/backend/utils/misc/guc_tables.c
> b/src/backend/utils/misc/guc_tables.c
> index 8a67f01200..367f510118 100644
> --- a/src/backend/utils/misc/guc_tables.c
> +++ b/src/backend/utils/misc/guc_tables.c
> @@ -3028,6 +3028,18 @@ struct config_int ConfigureNamesInt[] =
> NULL, NULL, NULL
> },
>
> + {
> + {"replication_slot_inactive_timeout", PGC_SIGHUP,
> REPLICATION_SENDING,
> + gettext_noop("Sets the amount of time a
> replication slot can remain inactive before "
> + "it will be invalidated."),
> + NULL,
> + GUC_UNIT_S
> + },
> + &replication_slot_inactive_timeout,
> + 0, 0, INT_MAX,
> + NULL, NULL, NULL
> + },
>
Currently, the feature is disabled by default when
replication_slot_inactive_timeout = 0. However, if we set a minimum
value, the default_val cannot be less than min_val, making it
impossible to use 0 to disable the feature.
Thoughts or any suggestions?
>
> 4) I'm not sure if this change required by this patch or is it a
> general optimization, if it is required for this patch we can detail
> the comments:
> @@ -2208,6 +2328,7 @@ RestoreSlotFromDisk(const char *name)
> bool restored = false;
> int readBytes;
> pg_crc32c checksum;
> + TimestampTz now;
>
> /* no need to lock here, no concurrent access allowed yet */
>
> @@ -2368,6 +2489,9 @@ RestoreSlotFromDisk(const char *name)
> NameStr(cp.slotdata.name)),
> errhint("Change \"wal_level\" to be
> \"replica\" or higher.")));
>
> + /* Use same inactive_since time for all slots */
> + now = GetCurrentTimestamp();
> +
> /* nothing can be active yet, don't lock anything */
> for (i = 0; i < max_replication_slots; i++)
> {
> @@ -2400,7 +2524,7 @@ RestoreSlotFromDisk(const char *name)
> * slot from the disk into memory. Whoever acquires
> the slot i.e.
> * makes the slot active will reset it.
> */
> - slot->inactive_since = GetCurrentTimestamp();
> + slot->inactive_since = now;
>
After removing the "ReplicationSlotSetInactiveSince" from here, it
became irrelevant to this patch. Now, it is a general optimization to
set the same timestamp for all slots while restoring from disk. I have
added a few comments as per Peter's suggestion.
> 5) Why should the slot invalidation be updated during shutdown,
> shouldn't the inactive_since value be intact during shutdown?
> - <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
>
The "inactive_since" data of a slot is not stored on disk, so the
older value cannot be restored after a restart.
--
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], [email protected]
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
In-Reply-To: <CABdArM5wR1ea8NpNU+tG0bB1m6G=Lep3bd1uthtSGj64JyqJsw@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