public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Nisha Moond <[email protected]>
Cc: vignesh C <[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: Thu, 28 Nov 2024 10:50:05 +1100
Message-ID: <CAHut+Ps=H6EBO1ssGfykrJfUQQGh76L0eKuU5XkR9GMs96ZT3g@mail.gmail.com> (raw)
In-Reply-To: <CABdArM4QR7LPoJ_ES+_1miQZ9ziwsOUun4FGSDhPdrxsugH_0A@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>
<CABdArM5141FHGvfNYD=pCybLDpmEWbRXFJW3HnEoieMScZ62-g@mail.gmail.com>
<CALDaNm0g86wD2=bQdFOy0smsP0MZWyz0CUqXej=Qi-hCEeqkag@mail.gmail.com>
<CABdArM6A3xsedesUWaQYnOM+mAGGj--N2ohoLrk2tO-W8UfR5g@mail.gmail.com>
<CAHut+Pvi-g+9+hjmjg44OzTN9L3YGQiCXBDAVaTVWvSn5SSwmw@mail.gmail.com>
<CABdArM4QR7LPoJ_ES+_1miQZ9ziwsOUun4FGSDhPdrxsugH_0A@mail.gmail.com>
Hi Nisha. Here are some review comments for patch v51-0002.
======
doc/src/sgml/system-views.sgml
1.
The time when the slot became inactive. <literal>NULL</literal> if the
- slot is currently being streamed.
+ slot is currently being streamed. Once the slot is invalidated, this
+ value will remain unchanged until we shutdown the server.
.
I think "Once the ..." kind of makes it sound like invalidation is
inevitable. Also maybe it's better to remove the "we".
SUGGESTION:
If the slot becomes invalidated, this value will remain unchanged
until server shutdown.
======
src/backend/replication/slot.c
ReplicationSlotAcquire:
2.
GENERAL.
This just is a question/idea. It may not be feasible to change. It
seems like there is a lot of overlap between the error messages in
'ReplicationSlotAcquire' which are saying "This slot has been
invalidated because...", and with the other function
'ReportSlotInvalidation' which is kind of the same but called in
different circumstances and with slightly different message text. I
wondered if there is a way to use common code to unify these messages
instead of having a nearly duplicate set of messages for all the
invalidation causes?
~~~
3.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+ appendStringInfo(&err_detail, _("inactivity exceeded the time limit
set by \"%s\"."),
+ "replication_slot_inactive_timeout");
+ break;
Should this err_detail also say "This slot has been invalidated
because ..." like all the others?
~~~
InvalidatePossiblyObsoleteSlot:
4.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+
+ /*
+ * Check if the slot needs to be invalidated due to
+ * replication_slot_inactive_timeout GUC.
+ */
+ if (IsSlotInactiveTimeoutPossible(s) &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+ replication_slot_inactive_timeout_sec * 1000))
+ {
Maybe this code should have Assert(now > 0); before the condition just
as a way to 'document' that it is assumed 'now' was already set this
outside the mutex.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
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: <CAHut+Ps=H6EBO1ssGfykrJfUQQGh76L0eKuU5XkR9GMs96ZT3g@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