public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Smith <[email protected]>
To: Nisha Moond <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Bharath Rupireddy <[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: Mon, 30 Dec 2024 16:02:19 +1100
Message-ID: <CAHut+Ps2ecNTfG3vsGb91CYpEzWtffyvkOzk1jqwhqTCwH8HQA@mail.gmail.com> (raw)
In-Reply-To: <CABdArM4wO2gPfcjrFWWL=D18PyFeWJZJcJGY3uJMzqL+4LDGpw@mail.gmail.com>
References: <CALDaNm39dtWHoQej2wd74zYGphA0e54Rx6aTRoCEnMXZ5ysbow@mail.gmail.com>
	<CABdArM45=j=DAFTEZA8p=oqYo-XqRwhaPCdKpciQCJx+jNeTJg@mail.gmail.com>
	<TYAPR01MB5692B7687EE7981AA91BA5B9F5362@TYAPR01MB5692.jpnprd01.prod.outlook.com>
	<CABdArM4N60XShbmxXgz4z8ENQGPOMhXSKzvmskdv8V=mQsYOOA@mail.gmail.com>
	<CALDaNm0mTWwg0z4v-sorq08S2CdZmL2s+rh4nHpWeJaBQ2F+mg@mail.gmail.com>
	<CAHut+Psi-mso-qSyMkHRP8e+psDFFmMp4NZTiL_yrFWidHkGMw@mail.gmail.com>
	<CALDaNm3znxKvTv=MDmMBOSk6XurkKgYyh9cH49NLL7SmJ62Q_A@mail.gmail.com>
	<CABdArM7CPR0Ag6OgtufATUn+x=Q_h3cdBcc39r+xeKYDks5Mfw@mail.gmail.com>
	<CALDaNm1FQS04aG0C0gCRpvi-o-OTdq91y6Az34YKN-dVc9r5Ng@mail.gmail.com>
	<CABdArM7LJopsdDyBAhjbsW3K-Yq=f75bnjaU5HRSsTkGWXibaQ@mail.gmail.com>
	<CAHut+PvfSHzKgXJTd=PbJieaXFgR9+VhhR3+eb7jCFFj6kC5bA@mail.gmail.com>
	<CABdArM6qPsZE9v7qymNTwiRt68DgOJ+LsAMhweTxZx1xr1cVOA@mail.gmail.com>
	<CAA4eK1JO+3Q+wkyjhMK+h_5MveTLFY7q2KRbrg1ojtO6dLCaMw@mail.gmail.com>
	<CABdArM4wO2gPfcjrFWWL=D18PyFeWJZJcJGY3uJMzqL+4LDGpw@mail.gmail.com>

Hi Nisha,

Here are some review comments for the patch v57-0001.

======
src/backend/replication/slot.c

1.
+/*
+ * Invalidate replication slots that have remained idle longer than this
+ * duration; '0' disables it.
+ */
+int idle_replication_slot_timeout_min = HOURS_PER_DAY * MINS_PER_HOUR;

IMO it would be better to have the suffix "_mins" instead of "_min"
here to avoid any confusion with "minimum".

~~~

2.
+/*
+ * Can invalidate an idle replication slot?
+ *

Not an English sentence.

======
src/backend/utils/adt/timestamp.c

3.
+ /* Return if the difference meets or exceeds the threshold */
+ return (secs >= threshold_sec);

That comment may not be necessary; it is saying just the same as the code.

======
src/backend/utils/misc/guc_tables.c

4.
+ {
+ {"idle_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets the duration a replication slot can remain idle before "
+ "it is invalidated."),
+ NULL,
+ GUC_UNIT_MIN
+ },
+ &idle_replication_slot_timeout_min,
+ HOURS_PER_DAY * MINS_PER_HOUR, 0, INT_MAX / SECS_PER_MINUTE,
+ check_idle_replication_slot_timeout, NULL, NULL
+ },
+

Maybe it's better to include a comment that says "24 hours".

(e.g. like wal_summary_keep_time does)

======
src/backend/utils/misc/postgresql.conf.sample

5.
 #track_commit_timestamp = off # collect timestamp of transaction commit
  # (change requires restart)
+#idle_replication_slot_timeout = 1d # in minutes; 0 disables


I felt it might be better to say 24h here instead of 1d. And, that
would also be consistent with the docs, which said the default was 24
hours.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

view thread (3+ messages)

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], [email protected]
  Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
  In-Reply-To: <CAHut+Ps2ecNTfG3vsGb91CYpEzWtffyvkOzk1jqwhqTCwH8HQA@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