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: Peter Smith <smithpb2250@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, shveta malik <shveta.malik@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Amit Kapila <amit.kapila16@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: 2024-11-27T10:54:49Z
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 Wed, Nov 27, 2024 at 8:39 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Nisha,
>
> Here are some review comments for the patch v50-0002.
>
> ======
> src/backend/replication/slot.c
>
> InvalidatePossiblyObsoleteSlot:
>
> 1.
> + if (now &&
> + TimestampDifferenceExceeds(s->inactive_since, now,
> +    replication_slot_inactive_timeout_sec * 1000))
>
> Previously this was using an additional call to SlotInactiveTimeoutCheckAllowed:
>
> + if (SlotInactiveTimeoutCheckAllowed(s) &&
> + TimestampDifferenceExceeds(s->inactive_since, now,
> +    replication_slot_inactive_timeout * 1000))
>
> Is it OK to skip that call? e.g. can the slot fields possibly change
> between assigning the 'now' and acquiring the mutex? If not, then the
> current code is fine. The only reason for asking is because it is
> slightly suspicious that it was not done this "easy" way in the first
> place.
>
Good catch! While the mutex was being acquired right after the now
assignment, there was a rare chance of another process modifying the
slot in the meantime. So, I reverted the change in v51. To optimize
the SlotInactiveTimeoutCheckAllowed() call, it's sufficient to check
it here instead of during the 'now' assignment.

Attached v51 patch-set addressing all comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CAHut%2BPtuiQj1hwm%3D73xJ8hWuw-9cXbN4dHJHpM6EXxubDJgmFA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPvi-g%2B9%2Bhjmjg44OzTN9L3YGQiCXBDAVaTVWvSn5SSwmw%40mail.gmail.com

--
Thanks,
Nisha