Обсуждение: Question about InvalidatePossiblyObsoleteSlot()
Hi, all,
I have a question about a behavioral difference in InvalidatePossiblyObsoleteSlot() between PG15 (and earlier) and PG16 (and later):
Looking forward to your insights.
In PG15 and earlier: while attempting to acquire a slot, if the slot's restart_lsn advanced to be greater than oldestLSN during the process, the slot would not be marked invalid.
In PG16 and later: the invalidation decision is made solely based on the initial_restart_lsn captured at the start of the check, even if the slot's restart_lsn advances above oldestLSN during the process, the slot may still be marked invalid.
I wonder why not decide whether to mark the slot as invalid based on the slot's current restart_lsn? If a slot's restart_lsn has already advanced sufficiently during the invalidation process, indicating it's actively being used, shouldn't we refrain from invalidating it? What is the rationale behind this design change?
Best Regards,
suyu.cmj
Hi, On Tue, Sep 23, 2025 at 10:38:14PM +0800, suyu.cmj wrote: > Hi, all, > I have a question about a behavioral difference in InvalidatePossiblyObsoleteSlot() between PG15 (and earlier) and PG16(and later): > In PG15 and earlier: while attempting to acquire a slot, if the slot's restart_lsn advanced to be greater than oldestLSNduring the process, the slot would not be marked invalid. > In PG16 and later: the invalidation decision is made solely based on the initial_restart_lsn captured at the start of thecheck, even if the slot's restart_lsn advances above oldestLSN during the process, the slot may still be marked invalid. > I wonder why not decide whether to mark the slot as invalid based on the slot's current restart_lsn? If a slot's restart_lsnhas already advanced sufficiently during the invalidation process, indicating it's actively being used, shouldn'twe refrain from invalidating it? What is the rationale behind this design change? > Looking forward to your insights. That comes from 818fefd8fd4. Does the wording in the commit message ([1]) and the linked thread ([2]) answer your question? [1]: postgr.es/c/818fefd8fd4 [2]: postgr.es/m/ZaTjW2Xh+TQUCOH0@ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi,
Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of introducing initial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop.
However, I still have a few concerns about this design change:
1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances significantly during the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it?
2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the invalidation process? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point should not be marked invalid.
3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this behavior? The behavioral difference across versions may lead to different operational outcomes in otherwise similar situations.
I would appreciate your insights on these points.
Best regards,
suyu.cmjHi, On Thu, Oct 09, 2025 at 10:49:39AM +0800, suyu.cmj wrote: > Hi, > Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of introducinginitial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop. > However, I still have a few concerns about this design change: > 1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances significantlyduring the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it? > 2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the invalidationprocess? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point shouldnot be marked invalid. > 3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this behavior?The behavioral difference across versions may lead to different operational outcomes in otherwise similar situations. > I would appreciate your insights on these points. I agree that before 818fefd8fd4 the invalidation cause could move from RS_INVAL_WAL_REMOVED to RS_INVAL_NONE if the slot restart lsn has been able to advance enough between the time we release the mutex and do the next check. With 818fefd8fd4 that's not the case anymore and we keep WAL_REMOVED as the invalidation cause (even if the slot restart lsn has been able to advance enough). That looks safe to use the pre 818fefd8fd4 behavior for the slot restart lsn case because the WAL files have not yet been removed by the checkpointer/startup process when it's busy in InvalidatePossiblyObsoleteSlot(). I think that we could get rid of the initial_restart_lsn and just use s->data.restart_lsn here (while keeping initial xmin ones to preserve the intent of 818fefd8fd4 for those). Something like the attached? Your concern is only about the restart_lsn, right? (asking because I don't think it's safe not to rely on the initial_* for the xmin ones, see [1]). [1]: https://www.postgresql.org/message-id/ZaACEPtVovKlRVUf%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
Hi,
Thanks for your detailed reply.
> I think that we could get rid of the initial_restart_lsn and just use
> s->data.restart_lsn here (while keeping initial xmin ones to preserve the
> intent of 818fefd8fd4 for those).
I agree with your proposed change.
> Your concern is only about the restart_lsn, right? (asking because I don't think
> it's safe not to rely on the initial_* for the xmin ones, see [1]).
My primary concern is indeed the restart_lsn issue, and thanks again for the clear explanation.
Best regards,
Best regards,
suyu.cmj
On Sun, Oct 12, 2025 at 11:27 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Thu, Oct 09, 2025 at 10:49:39AM +0800, suyu.cmj wrote: > > Hi, > > Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of introducinginitial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop. > > However, I still have a few concerns about this design change: > > 1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances significantlyduring the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it? > > 2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the invalidationprocess? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point shouldnot be marked invalid. > > 3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this behavior?The behavioral difference across versions may lead to different operational outcomes in otherwise similar situations. > > I would appreciate your insights on these points. > > I agree that before 818fefd8fd4 the invalidation cause could move from > RS_INVAL_WAL_REMOVED to RS_INVAL_NONE if the slot restart lsn has been able to > advance enough between the time we release the mutex and do the next check. > > With 818fefd8fd4 that's not the case anymore and we keep WAL_REMOVED as the > invalidation cause (even if the slot restart lsn has been able to advance > enough). > > That looks safe to use the pre 818fefd8fd4 behavior for the slot restart lsn > case because the WAL files have not yet been removed by the checkpointer/startup > process when it's busy in InvalidatePossiblyObsoleteSlot(). > > I think that we could get rid of the initial_restart_lsn and just use > s->data.restart_lsn here (while keeping initial xmin ones to preserve the > intent of 818fefd8fd4 for those). IIUC with the proposed patch, it's possible that we report the slot invalidation once but don't actually invalidate the slot if slot's restart_lsn gets advanced and becomes greater than the oldestLSN after the report, is that right? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Hi, On Tue, Oct 14, 2025 at 04:06:44PM -0700, Masahiko Sawada wrote: > On Sun, Oct 12, 2025 at 11:27 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Hi, > > > > On Thu, Oct 09, 2025 at 10:49:39AM +0800, suyu.cmj wrote: > > > Hi, > > > Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of introducinginitial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop. > > > However, I still have a few concerns about this design change: > > > 1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances significantlyduring the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it? > > > 2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the invalidationprocess? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point shouldnot be marked invalid. > > > 3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this behavior?The behavioral difference across versions may lead to different operational outcomes in otherwise similar situations. > > > I would appreciate your insights on these points. > > > > I agree that before 818fefd8fd4 the invalidation cause could move from > > RS_INVAL_WAL_REMOVED to RS_INVAL_NONE if the slot restart lsn has been able to > > advance enough between the time we release the mutex and do the next check. > > > > With 818fefd8fd4 that's not the case anymore and we keep WAL_REMOVED as the > > invalidation cause (even if the slot restart lsn has been able to advance > > enough). > > > > That looks safe to use the pre 818fefd8fd4 behavior for the slot restart lsn > > case because the WAL files have not yet been removed by the checkpointer/startup > > process when it's busy in InvalidatePossiblyObsoleteSlot(). > > > > I think that we could get rid of the initial_restart_lsn and just use > > s->data.restart_lsn here (while keeping initial xmin ones to preserve the > > intent of 818fefd8fd4 for those). > > IIUC Thanks for looking at it! > with the proposed patch, it's possible that we report the slot > invalidation once but don't actually invalidate the slot if slot's > restart_lsn gets advanced and becomes greater than the oldestLSN after > the report, is that right? We don't really report an "invalidation", what we report is: LOG: terminating process 3998707 to release replication slot "logical_slot" DETAIL: The slot's restart_lsn 0/00842480 exceeds the limit by 2874240 bytes. HINT: You might need to increase "max_slot_wal_keep_size". and we terminate the process: FATAL: terminating connection due to administrator command We are not reporting: DETAIL: This replication slot has been invalidated due to "wal_removed". and the slot is still valid. That's the pre 818fefd8fd4 behavior. Ideally, I think that we should not report anything and not terminate the process. I did not look at it, maybe we could look at it as a second step (first step being to restore the pre 818fefd8fd4 behavior)? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Oct 14, 2025 at 9:51 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Oct 14, 2025 at 04:06:44PM -0700, Masahiko Sawada wrote: > > On Sun, Oct 12, 2025 at 11:27 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > Hi, > > > > > > On Thu, Oct 09, 2025 at 10:49:39AM +0800, suyu.cmj wrote: > > > > Hi, > > > > Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of introducinginitial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop. > > > > However, I still have a few concerns about this design change: > > > > 1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances significantlyduring the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it? > > > > 2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the invalidationprocess? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point shouldnot be marked invalid. > > > > 3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this behavior?The behavioral difference across versions may lead to different operational outcomes in otherwise similar situations. > > > > I would appreciate your insights on these points. > > > > > > I agree that before 818fefd8fd4 the invalidation cause could move from > > > RS_INVAL_WAL_REMOVED to RS_INVAL_NONE if the slot restart lsn has been able to > > > advance enough between the time we release the mutex and do the next check. > > > > > > With 818fefd8fd4 that's not the case anymore and we keep WAL_REMOVED as the > > > invalidation cause (even if the slot restart lsn has been able to advance > > > enough). > > > > > > That looks safe to use the pre 818fefd8fd4 behavior for the slot restart lsn > > > case because the WAL files have not yet been removed by the checkpointer/startup > > > process when it's busy in InvalidatePossiblyObsoleteSlot(). > > > > > > I think that we could get rid of the initial_restart_lsn and just use > > > s->data.restart_lsn here (while keeping initial xmin ones to preserve the > > > intent of 818fefd8fd4 for those). > > > > IIUC > > Thanks for looking at it! > > > with the proposed patch, it's possible that we report the slot > > invalidation once but don't actually invalidate the slot if slot's > > restart_lsn gets advanced and becomes greater than the oldestLSN after > > the report, is that right? > > We don't really report an "invalidation", what we report is: > > LOG: terminating process 3998707 to release replication slot "logical_slot" > DETAIL: The slot's restart_lsn 0/00842480 exceeds the limit by 2874240 bytes. > HINT: You might need to increase "max_slot_wal_keep_size". > > and we terminate the process: > > FATAL: terminating connection due to administrator command > > We are not reporting: > > DETAIL: This replication slot has been invalidated due to "wal_removed". > > and the slot is still valid. > > That's the pre 818fefd8fd4 behavior. Thank you for the clarification! Understood. > Ideally, I think that we should not report anything and not terminate the > process. I did not look at it, maybe we could look at it as a second step (first > step being to restore the pre 818fefd8fd4 behavior)? I find that reporting of terminating a process having an possibly obsolete slot is fine, but reading some related threads[1][2] it seems to me that a problem we want to avoid is that we report "terminated" without leading to an "obsolete" message. Does it make sense to report explicitly that the slot's restart_lsn gets recovered and we therefore skipped to invalidate it? Regards, [1] https://www.postgresql.org/message-id/flat/ZaTjW2Xh%2BTQUCOH0%40ip-10-97-1-34.eu-west-3.compute.internal [2] https://www.postgresql.org/message-id/7c025095-5763-17a6-8c80-899b35bd0459@gmail.com -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Hi, On Wed, Oct 15, 2025 at 04:24:03PM -0700, Masahiko Sawada wrote: > On Tue, Oct 14, 2025 at 9:51 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > We don't really report an "invalidation", what we report is: > > > > LOG: terminating process 3998707 to release replication slot "logical_slot" > > DETAIL: The slot's restart_lsn 0/00842480 exceeds the limit by 2874240 bytes. > > HINT: You might need to increase "max_slot_wal_keep_size". > > > > and we terminate the process: > > > > FATAL: terminating connection due to administrator command > > > > We are not reporting: > > > > DETAIL: This replication slot has been invalidated due to "wal_removed". > > > > and the slot is still valid. > > > > That's the pre 818fefd8fd4 behavior. > > Thank you for the clarification! Understood. > > > Ideally, I think that we should not report anything and not terminate the > > process. I did not look at it, maybe we could look at it as a second step (first > > step being to restore the pre 818fefd8fd4 behavior)? > > I find that reporting of terminating a process having an possibly > obsolete slot is fine, but reading some related threads[1][2] it seems > to me that a problem we want to avoid is that we report "terminated" > without leading to an "obsolete" message. Right, the focus at that time was around the invalidations related to the xmin conflicts (mainly because it felt unsafe to "ignore" the invalidation for them). Then later the restart_lsn was added to the discussion [1]. But after more thought, I do think it's safe for the restart_lsn case (for the reasons mentioned above). > Does it make sense to report > explicitly that the slot's restart_lsn gets recovered and we therefore > skipped to invalidate it? I'm not sure. The existing logging shows when we invalidate a slot, so the absence of that message indicates we skipped it. Users can also verify the slot status in the view if needed. Regards, [1]: https://www.postgresql.org/message-id/CALj2ACUo_rDTonZCkqdSnsc3tT5_cFcJQHSQrsyAYyO5MLO52A%40mail.gmail.com -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Oct 16, 2025 at 1:23 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Oct 15, 2025 at 04:24:03PM -0700, Masahiko Sawada wrote:
> > On Tue, Oct 14, 2025 at 9:51 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > We don't really report an "invalidation", what we report is:
> > >
> > > LOG: terminating process 3998707 to release replication slot "logical_slot"
> > > DETAIL: The slot's restart_lsn 0/00842480 exceeds the limit by 2874240 bytes.
> > > HINT: You might need to increase "max_slot_wal_keep_size".
> > >
> > > and we terminate the process:
> > >
> > > FATAL: terminating connection due to administrator command
> > >
> > > We are not reporting:
> > >
> > > DETAIL: This replication slot has been invalidated due to "wal_removed".
> > >
> > > and the slot is still valid.
> > >
> > > That's the pre 818fefd8fd4 behavior.
> >
> > Thank you for the clarification! Understood.
> >
> > > Ideally, I think that we should not report anything and not terminate the
> > > process. I did not look at it, maybe we could look at it as a second step (first
> > > step being to restore the pre 818fefd8fd4 behavior)?
> >
> > I find that reporting of terminating a process having an possibly
> > obsolete slot is fine, but reading some related threads[1][2] it seems
> > to me that a problem we want to avoid is that we report "terminated"
> > without leading to an "obsolete" message.
>
> Right, the focus at that time was around the invalidations related to the
> xmin conflicts (mainly because it felt unsafe to "ignore" the invalidation
> for them). Then later the restart_lsn was added to the discussion [1].
>
> But after more thought, I do think it's safe for the restart_lsn case
> (for the reasons mentioned above).
>
> > Does it make sense to report
> > explicitly that the slot's restart_lsn gets recovered and we therefore
> > skipped to invalidate it?
>
> I'm not sure. The existing logging shows when we invalidate a slot, so the
> absence of that message indicates we skipped it. Users can also verify the
> slot status in the view if needed.
>
I'm somewhat confused. As the commit message of 818fefd8fd4 says, the
invalidation of an active slot is done in two steps:
- Termination of the backend holding it, if any.
- Report that the slot is obsolete, with a conflict cause depending on
the slot's data.
Before commit 818fefd8fd4, it was possible that if slot's
effective_xmin, catalog_effective_xmin, or restart_lsn gets advanced
between two steps, we ended up skipping the actual invalidation for
the slot. It was not possible that the invalidation cause used when
terminating the process changes (like RS_INVAL_HORIZON to
RS_INVAL_WAL_REMOVED) between two steps since the possible_cause
doesn't change within InvalidatePossiblyObsoleteSlot(). The commit
818fefd8fd4 introduced initial_xxx variables to ensure that if we
report the "terminated" for a reason we invalidate the slot for the
same reason.
What the patch proposed on this thread does is that we allow such pre-
818fefd8fd4 behavior only for restart_lsn. That is, we skip the actual
slot invalidation if the slot's restart_lsn gets advanced and exceeds
the oldestLSN between two steps.
I thought the reason why we moved away from pre-818fefd8fd4 was that
we wanted to avoid reporting only "terminated" and to report both
things consistently. But if we can accept pre-818fefd8fd4 for
restart_lsn, why can we not do the same for effective_xmin and
catalog_effective_xmin? I guess that the same is true also for
effective_xmin and catalog_effective_xmin that we can skip the actual
slot invalidation if those values get advanced and exceed
snapshotConflictHorizon.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Hi, On Thu, Oct 16, 2025 at 11:32:03AM -0700, Masahiko Sawada wrote: > On Thu, Oct 16, 2025 at 1:23 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > I'm not sure. The existing logging shows when we invalidate a slot, so the > > absence of that message indicates we skipped it. Users can also verify the > > slot status in the view if needed. > > > > I'm somewhat confused. As the commit message of 818fefd8fd4 says, the > invalidation of an active slot is done in two steps: > > - Termination of the backend holding it, if any. > - Report that the slot is obsolete, with a conflict cause depending on > the slot's data. > > Before commit 818fefd8fd4, it was possible that if slot's > effective_xmin, catalog_effective_xmin, or restart_lsn gets advanced > between two steps, we ended up skipping the actual invalidation for > the slot. If advancing far enough to reach RS_INVAL_NONE, right. > What the patch proposed on this thread does is that we allow such pre- > 818fefd8fd4 behavior only for restart_lsn. That is, we skip the actual > slot invalidation if the slot's restart_lsn gets advanced and exceeds > the oldestLSN between two steps. Exactly. > I thought the reason why we moved away from pre-818fefd8fd4 was that > we wanted to avoid reporting only "terminated" and to report both > things consistently. But if we can accept pre-818fefd8fd4 for > restart_lsn, why can we not do the same for effective_xmin and > catalog_effective_xmin? > I guess that the same is true also for > effective_xmin and catalog_effective_xmin that we can skip the actual > slot invalidation if those values get advanced and exceed > snapshotConflictHorizon. I think this is safe to do for the restart_lsn because that's the same process (checkpointer/startup) that is executing InvalidatePossiblyObsoleteSlot() and will remove the WALs. It can not remove the WALs as long as it is busy in InvalidatePossiblyObsoleteSlot() so that I think this is safe for the restart_lsn case. I'm not that confident for the xmin cases, do you think that's also safe to not invalidate the slots for effective_xmin and catalog_effective_xmin if they advance far enough? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Oct 17, 2025 at 12:18 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Thu, Oct 16, 2025 at 11:32:03AM -0700, Masahiko Sawada wrote: > > On Thu, Oct 16, 2025 at 1:23 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > I'm not sure. The existing logging shows when we invalidate a slot, so the > > > absence of that message indicates we skipped it. Users can also verify the > > > slot status in the view if needed. > > > > > > > I'm somewhat confused. As the commit message of 818fefd8fd4 says, the > > invalidation of an active slot is done in two steps: > > > > - Termination of the backend holding it, if any. > > - Report that the slot is obsolete, with a conflict cause depending on > > the slot's data. > > > > Before commit 818fefd8fd4, it was possible that if slot's > > effective_xmin, catalog_effective_xmin, or restart_lsn gets advanced > > between two steps, we ended up skipping the actual invalidation for > > the slot. > > If advancing far enough to reach RS_INVAL_NONE, right. > > > What the patch proposed on this thread does is that we allow such pre- > > 818fefd8fd4 behavior only for restart_lsn. That is, we skip the actual > > slot invalidation if the slot's restart_lsn gets advanced and exceeds > > the oldestLSN between two steps. > > Exactly. > > > I thought the reason why we moved away from pre-818fefd8fd4 was that > > we wanted to avoid reporting only "terminated" and to report both > > things consistently. But if we can accept pre-818fefd8fd4 for > > restart_lsn, why can we not do the same for effective_xmin and > > catalog_effective_xmin? > > I guess that the same is true also for > > effective_xmin and catalog_effective_xmin that we can skip the actual > > slot invalidation if those values get advanced and exceed > > snapshotConflictHorizon. > > I think this is safe to do for the restart_lsn because that's the same process > (checkpointer/startup) that is executing InvalidatePossiblyObsoleteSlot() > and will remove the WALs. It can not remove the WALs as long as it is busy in > InvalidatePossiblyObsoleteSlot() so that I think this is safe for the restart_lsn > case. > > I'm not that confident for the xmin cases, do you think that's also safe to not > invalidate the slots for effective_xmin and catalog_effective_xmin if they > advance far enough? I find the same in xmin cases. ResolveRecoveryConflictWithSnapshot() is called only during the recovery by the startup process, and it also tries to invalidate possibly obsolete slots. Catalog tuples are not removed as long as the startup calls ResolveRecoveryConflictWithSnapshot() before actually removing the tuples and it's busy in InvalidatePossiblyObsoleteSlot(). I might be missing something but this is the reason why I'm confused with the 818fefd8fd4 fix and the proposed change. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Hi, On Fri, Oct 17, 2025 at 03:08:07PM -0700, Masahiko Sawada wrote: > On Fri, Oct 17, 2025 at 12:18 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > do you think that's also safe to not > > invalidate the slots for effective_xmin and catalog_effective_xmin if they > > advance far enough? > > I find the same in xmin cases. ResolveRecoveryConflictWithSnapshot() > is called only during the recovery by the startup process, and it also > tries to invalidate possibly obsolete slots. Catalog tuples are not > removed as long as the startup calls > ResolveRecoveryConflictWithSnapshot() before actually removing the > tuples and it's busy in InvalidatePossiblyObsoleteSlot(). I looked more closely at the xmin related cases and I agree with the above. > I might be > missing something but this is the reason why I'm confused with the > 818fefd8fd4 fix and the proposed change. Yeah so 818fefd8fd4 is well suited for tests consistency but in some rare cases it invalidates a slot while it would be safe not to do so. OTOH it looks to me that the initial pre-818fefd8fd4 intend was to invalidate the slot as per this comment: " /* * Re-acquire lock and start over; we expect to invalidate the * slot next time (unless another process acquires the slot in the * meantime). */ " The fact that it could move forward far enough before we terminate the process holding the slot is a race condition due to the fact that we released the mutex. If the above looks right to you then 818fefd8fd4 is doing what was "initially" expected, do you agree? If so, then maybe it's fine to keep 818fefd8fd4 as is? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Mon, Oct 20, 2025 at 11:41 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > On Fri, Oct 17, 2025 at 03:08:07PM -0700, Masahiko Sawada wrote: > > On Fri, Oct 17, 2025 at 12:18 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > do you think that's also safe to not > > > invalidate the slots for effective_xmin and catalog_effective_xmin if they > > > advance far enough? > > > > I find the same in xmin cases. ResolveRecoveryConflictWithSnapshot() > > is called only during the recovery by the startup process, and it also > > tries to invalidate possibly obsolete slots. Catalog tuples are not > > removed as long as the startup calls > > ResolveRecoveryConflictWithSnapshot() before actually removing the > > tuples and it's busy in InvalidatePossiblyObsoleteSlot(). > > I looked more closely at the xmin related cases and I agree with the above. > > > I might be > > missing something but this is the reason why I'm confused with the > > 818fefd8fd4 fix and the proposed change. > > Yeah so 818fefd8fd4 is well suited for tests consistency but in some rare cases > it invalidates a slot while it would be safe not to do so. > > OTOH it looks to me that the initial pre-818fefd8fd4 intend was to invalidate > the slot as per this comment: > > " > /* > * Re-acquire lock and start over; we expect to invalidate the > * slot next time (unless another process acquires the slot in the > * meantime). > */ > " > The comment doesn't indicate the intent that we will invalidate the slot after re-acquiring the lock even when the new conditions don't warrant the slot to be invalidated. The comment could be improved though. > The fact that it could move forward far enough before we terminate the > process holding the slot is a race condition due to the fact that we released > the mutex. > > If the above looks right to you then 818fefd8fd4 is doing what was "initially" > expected, do you agree? > Based on the discussion and points presented in this thread, I don't agree. I feel we should restore behavior prior to 818fefd8fd4 and fix the test case which relies on different messages. > If so, then maybe it's fine to keep 818fefd8fd4 as is? > I don't think so. -- With Regards, Amit Kapila.
Hi, On Wed, Oct 22, 2025 at 02:18:33PM +0530, Amit Kapila wrote: > On Mon, Oct 20, 2025 at 11:41 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Yeah so 818fefd8fd4 is well suited for tests consistency but in some rare cases > > it invalidates a slot while it would be safe not to do so. > > > > OTOH it looks to me that the initial pre-818fefd8fd4 intend was to invalidate > > the slot as per this comment: > > > > " > > /* > > * Re-acquire lock and start over; we expect to invalidate the > > * slot next time (unless another process acquires the slot in the > > * meantime). > > */ > > " > > > > The comment doesn't indicate the intent that we will invalidate the > slot after re-acquiring the lock even when the new conditions don't > warrant the slot to be invalidated. The comment could be improved > though. Thanks for looking at it and clarifying this point. In the attached I try to improve the comment. > > The fact that it could move forward far enough before we terminate the > > process holding the slot is a race condition due to the fact that we released > > the mutex. > > > > If the above looks right to you then 818fefd8fd4 is doing what was "initially" > > expected, do you agree? > > > > Based on the discussion and points presented in this thread, I don't > agree. I feel we should restore behavior prior to 818fefd8fd4 Done in the attached. > and fix > the test case which relies on different messages. I think that 105b2cb3361 fixed it already or do you have something else in mind? [1]: https://www.postgresql.org/message-id/5d0e5bec-67f9-9164-36cb-c4ff5f95d1ed%40gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com