Horton

Re: Introduce XID age and inactive timeout based replication slot invalidation

Nisha Moond <nisha.moond412@gmail.com>

View thread
From: Nisha Moond <nisha.moond412@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, vignesh C <vignesh21@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Ajin Cherian <itsajin@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nathan Bossart <nathandbossart@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-01-31T12:20:26Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add a test for commit ac0e33136a using the injection point.

  2. Invalidate inactive replication slots.

  3. Fix incorrect slot type in BuildTupleHashTableExt

  4. Allow synced slots to have their inactive_since.

  5. Change last_inactive_time to inactive_since in pg_replication_slots.

  6. Track last_inactive_time in pg_replication_slots.

  7. Track invalidation_reason in pg_replication_slots.

  8. Add option force_initdb to PostgreSQL::Test::Cluster:init()

  9. Add a failover option to subscriptions.

  10. Allow setting failover property in the replication command.

  11. Allow to enable failover property for replication slots via SQL API.

  12. Track conflict_reason in pg_replication_slots.

  13. Log messages for replication slot acquisition and release.

  14. Remove vacuum_defer_cleanup_age

  15. Fix corruption due to vacuum_defer_cleanup_age underflowing 64bit xids

  16. meson: Add initial version of meson based build system

Attachments

On Fri, Jan 31, 2025 at 2:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jan 31, 2025 at 10:40 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > ======
> > src/backend/replication/slot.c
> >
> > ReportSlotInvalidation:
> >
> > 1.
> > +
> > + case RS_INVAL_IDLE_TIMEOUT:
> > + Assert(inactive_since > 0);
> > + /* translator: second %s is a GUC variable name */
> > + appendStringInfo(&err_detail,
> > + _("The slot has remained idle since %s, which is longer than the
> > configured \"%s\" duration."),
> > + timestamptz_to_str(inactive_since),
> > + "idle_replication_slot_timeout");
> > + break;
> > +
> >
> > errdetail:
> >
> > I guess it is no fault of this patch because I see you've only copied
> > nearby code, but AFAICT this function is still having an each-way bet
> > by using a mixture of _() macro which is for strings intended be
> > translated, but then only using them in errdetail_internal() which is
> > for strings that are NOT intended to be translated. Isn't it
> > contradictory? Why don't we use errdetail() here?
> >
>
> Your question is valid and I don't have an answer. I encourage you to
> start a new thread to clarify this.
>
> > errhint:
> >
> > Also, the way the 'hint' is implemented can only be meaningful for
> > RS_INVAL_WAL_REMOVED. This is also existing code that IMO it was
> > always strange, but now that this patch has added another kind of
> > switch (cause) this hint implementation now looks increasingly hacky
> > to me; it is also inflexible -- e.g. if you ever wanted to add
> > different hints. A neater implementation would be to make the code
> > more like how the err_detail is handled, so then the errhint string
> > would only be assigned within the "case RS_INVAL_WAL_REMOVED:"
> >
>
> This makes sense to me.
>
> +
> + case RS_INVAL_IDLE_TIMEOUT:
> + Assert(inactive_since > 0);
> + /* translator: second %s is a GUC variable name */
> + appendStringInfo(&err_detail,
> + _("The slot has remained idle since %s, which is longer than the
> configured \"%s\" duration."),
> + timestamptz_to_str(inactive_since),
> + "idle_replication_slot_timeout");
>
> I think the above message should be constructed on a model similar to
> the following nearby message:"The slot's restart_lsn %X/%X exceeds the
> limit by %llu bytes.". So, how about the following: "The slot's idle
> time %s exceeds the configured \"%s\" duration"?
>
> Also, similar to max_slot_wal_keep_size, we should give a hint in this
> case to increase idle_replication_slot_timeout.
>
> It is not clear why the injection point test is doing
> pg_sync_replication_slots() etc. in the patch. The test should be
> simple such that after creating a new physical or logical slot, enable
> the injection point, then run the manual checkpoint command, and check
> the invalidation status of the slot.
>

Thanks for the review! I have incorporated the above comments. The
test in patch-002 has been optimized as suggested and now completes in
less than a second.
Please find the attached v66 patch set. The base patch(v65-001) is
committed now, so I have rebased the patches.

Thank you, Kuroda-san, for working on patch-002.

--
Thanks,
Nisha