Horton

Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly

Vitaly Davydov <v.davydov@postgrespro.ru>

View thread
From: "Vitaly Davydov" <v.davydov@postgrespro.ru>
To: "Amit Kapila" <amit.kapila16@gmail.com>
Cc: "Alexander Korotkov" <aekorotkov@gmail.com>, "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Masahiko Sawada" <sawada.mshk@gmail.com>, tomas@vondra.me
Date: 2025-05-26T15:34:07Z
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. Update comment for ReplicationSlot.last_saved_restart_lsn

  2. Fix CheckPointReplicationSlots() with max_replication_slots == 0

  3. Remove excess assert from InvalidatePossiblyObsoleteSlot()

  4. Improve runtime and output of tests for replication slots checkpointing.

  5. Keep WAL segments by slot's last saved restart LSN

  6. Fix an assert in CheckPointReplicationSlots().

Dear Amit,

> OTOH, if we don't want to adjust physical
> slot machinery, it seems saving the logical slots to disk immediately
> when its restart_lsn is updated is a waste of effort after your patch,
> no? If so, why are we okay with that?

I agree, that saving logical slots at advance is a possible waste of effort. But
I don't understand original ideas behind it. I haven't touched it to make
the minimal patch which should not break the existing functionality.

We trim WAL in checkpoint (or restart point) operation only. The slots'
restart_lsn is used to keep the wal from truncation. I believe, we need to
compute the slots' oldest lsn as the minimal value of restart_lsn values only
when executing checkpoint (or restart point). I guess, it doesn't depend on
slot's type (logical or physical). We have 0003 patch to fix it.

I haven't deeply investigated yet slot's xmin values but I guess the xmin values
are a different story than restart_lsn. It is used to avoid tuple deletions by
vacuum and it is updated by a different way. I can't say that
LogicalConfirmReceivedLocation is the right place to update saved on disk xmin
values. I would propose to update these values in SaveSlotToPath under some lock
to avoid concurrent reads of unsaved values or do in a checkpoint like as for
restart_lsn. We may investigate and improve it in an another patch.

With best regards,
Vitaly