Horton

Thread

Commits

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

  1. Rename column slotsync_skip_at to slotsync_last_skip.

  2. Add slotsync_skip_reason column to pg_replication_slots view.

  3. Fix test failure caused by commit 76b78721ca.

  4. Fix a BF failure caused by commit 76b78721ca.

  5. Add slotsync skip statistics.

  1. How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-08-28T05:36:42Z

    Hi,

We have seen cases where slot synchronization gets delayed, for example
when the slot is behind the failover standby or vice versa, and the slot
sync worker has to wait for one to catch up with the other. During this
waiting period, users querying pg_replication_slots can only see whether
the slot has been synchronized or not. If it has already synchronized,
that’s fine, but if synchronization is taking longer, users would naturally
want to understand the reason for the delay.

Is there a way for end users to know the cause of slot synchronization
delays, so they can take appropriate actions to speed it up?

I understand that server logs are emitted in such cases, but logs are not
something end users would want to check regularly. Moreover, since logging
is configuration-based, relevant messages may sometimes be skipped or
suppressed.

Thanks & Regards,
Ashutosh Sharma.

  2. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-08-28T09:56:25Z

    On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
>
> Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
>
> I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
>

Currently, the way to see the reason for sync skip is LOGs but I think
it is better to add a new column like sync_skip_reason in
pg_replication_slots. This can show the reasons like
standby_LSN_ahead_remote_LSN. I think ideally users can compare
standby's slot LSN/XMIN with remote_slot being synced. Do you have any
better ideas?

-- 
With Regards,
Amit Kapila.

  3. Re: How can end users know the cause of LR slot sync delays?

    Kirill Reshke <reshkekirill@gmail.com> — 2025-08-28T09:59:40Z

    On Thu, 28 Aug 2025 at 14:56, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
> >
> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
> >
> > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
> >
>
> Currently, the way to see the reason for sync skip is LOGs but I think
> it is better to add a new column like sync_skip_reason in
> pg_replication_slots. This can show the reasons like
> standby_LSN_ahead_remote_LSN. I think ideally users can compare
> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> better ideas?
>

How about something like pg_stat_progress_replication_slot with remote
LSN/standby LSN/catalog XID etc?
Wouldn't this be in sync with all other debug pg_stat_progress* views
and thus more Postgres-y?


-- 
Best regards,
Kirill Reshke

  4. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-08-29T06:48:50Z

    On Thu, Aug 28, 2025 at 3:29 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> On Thu, 28 Aug 2025 at 14:56, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
> > >
> > > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
> > >
> > > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
> > >
> >
> > Currently, the way to see the reason for sync skip is LOGs but I think
> > it is better to add a new column like sync_skip_reason in
> > pg_replication_slots. This can show the reasons like
> > standby_LSN_ahead_remote_LSN. I think ideally users can compare
> > standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> > better ideas?
> >
>
> How about something like pg_stat_progress_replication_slot with remote
> LSN/standby LSN/catalog XID etc?
> Wouldn't this be in sync with all other debug pg_stat_progress* views
> and thus more Postgres-y?
>

Yes, that is another option. I am a little worried that it is not
always the sync lags behind, so having a separate view just for sync
progress may be too much. Yet another option is existing view
pg_stat_replication_slots but it seems sync progress doesn't directly
match there. For example, we can add a counter sync_skipped, time of
last sync_skip, and last_sync_skip_reason that could be sufficient to
dig the problem further.

-- 
With Regards,
Amit Kapila.

  5. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-03T12:22:21Z

    Hi Amit,

On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com>
> wrote:
> >
> > We have seen cases where slot synchronization gets delayed, for example
> when the slot is behind the failover standby or vice versa, and the slot
> sync worker has to wait for one to catch up with the other. During this
> waiting period, users querying pg_replication_slots can only see whether
> the slot has been synchronized or not. If it has already synchronized,
> that’s fine, but if synchronization is taking longer, users would naturally
> want to understand the reason for the delay.
> >
> > Is there a way for end users to know the cause of slot synchronization
> delays, so they can take appropriate actions to speed it up?
> >
> > I understand that server logs are emitted in such cases, but logs are
> not something end users would want to check regularly. Moreover, since
> logging is configuration-based, relevant messages may sometimes be skipped
> or suppressed.
> >
>
> Currently, the way to see the reason for sync skip is LOGs but I think
> it is better to add a new column like sync_skip_reason in
> pg_replication_slots. This can show the reasons like
> standby_LSN_ahead_remote_LSN. I think ideally users can compare
> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> better ideas?
>
>
I have similar thoughts, but for clarity, I’d like to outline some of the
key steps I plan to take:

Step 1: Define an enum for all possible reasons a slot persistence was
skipped.

/*
 * Reasons why a replication slot sync was skipped.
 */
typedef enum ReplicationSlotSyncSkipReason
{
    RS_SYNC_SKIP_NONE = 0,                 /* No skip */

    RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind local
reserved LSN */

    RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of remote,
risk of data loss */

    RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a
consistent snapshot */
} ReplicationSlotSyncSkipReason;

--

Step 2: Introduce new column to pg_replication_slots to store the skip
reason

/* Inside pg_replication_slots table */
ReplicationSlotSyncSkipReason slot_sync_skip_reason;

--

Step 3: Function to convert enum to human-readable string that can be
stored in pg_replication_slots.

/*
 * Convert ReplicationSlotSyncSkipReason bitmask to human-readable string.
 *
 * Returns a palloc'd string; caller is responsible for freeing it.
 */
static char *
replication_slot_sync_skip_reason_str(ReplicationSlotSyncSkipReason reason)
{
    StringInfoData buf;
    initStringInfo(&buf);

    if (reason == RS_SYNC_SKIP_NONE)
    {
        appendStringInfoString(&buf, "none");
        return buf.data;
    }

    if (reason & RS_SYNC_SKIP_REMOTE_BEHIND)
        appendStringInfoString(&buf, "remote_behind|");
    if (reason & RS_SYNC_SKIP_DATA_LOSS)
        appendStringInfoString(&buf, "data_loss|");
    if (reason & RS_SYNC_SKIP_NO_SNAPSHOT)
        appendStringInfoString(&buf, "no_snapshot|");

    /* Remove trailing '|' */
    if (buf.len > 0 && buf.data[buf.len - 1] == '|')
        buf.data[buf.len - 1] = '\0';

    return buf.data;
}

--

Step 4: Capture slot_sync_skip_reason whenever the relevant LOG messages
are generated, primarily inside update_local_synced_slot or
update_and_persist_local_synced_slot. This value will can later be
persisted in the pg_replication_slots catalog.

--

Please let me know if you have any objections. I’ll share the wip patch in
a few days.

--
With Regards,
Ashutosh Sharma.

  6. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-04T14:56:01Z

    Hi Ashutosh,

I am also interested in this thread. And was working on a patch for it.

On Wed, 3 Sept 2025 at 17:52, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Amit,
>
> On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> >
>> > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
>> >
>> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
>> >
>> > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
>> >
>>
>> Currently, the way to see the reason for sync skip is LOGs but I think
>> it is better to add a new column like sync_skip_reason in
>> pg_replication_slots. This can show the reasons like
>> standby_LSN_ahead_remote_LSN. I think ideally users can compare
>> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
>> better ideas?
>>
>
> I have similar thoughts, but for clarity, I’d like to outline some of the key steps I plan to take:
>
> Step 1: Define an enum for all possible reasons a slot persistence was skipped.
>
> /*
>  * Reasons why a replication slot sync was skipped.
>  */
> typedef enum ReplicationSlotSyncSkipReason
> {
>     RS_SYNC_SKIP_NONE = 0,                 /* No skip */
>
>     RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind local reserved LSN */
>
>     RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of remote, risk of data loss */
>
>     RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a consistent snapshot */
> } ReplicationSlotSyncSkipReason;
>
> --
>
I think we should also add the case when "remote_slot->confirmed_lsn >
latestFlushPtr" (WAL corresponding to the confirmed lsn on remote slot
is still not flushed on the Standby). In this case as well we are
skipping the slot sync.

> Step 2: Introduce new column to pg_replication_slots to store the skip reason
>
> /* Inside pg_replication_slots table */
> ReplicationSlotSyncSkipReason slot_sync_skip_reason;
>
> --
>
As per the discussion [1], I think it is more of stat related data and
we should add it in the pg_stat_replication_slots view. Also we can
add columns for 'slot sync skip count' and 'last slot sync skip'.
Thoughts?

> Step 3: Function to convert enum to human-readable string that can be stored in pg_replication_slots.
>
> /*
>  * Convert ReplicationSlotSyncSkipReason bitmask to human-readable string.
>  *
>  * Returns a palloc'd string; caller is responsible for freeing it.
>  */
> static char *
> replication_slot_sync_skip_reason_str(ReplicationSlotSyncSkipReason reason)
> {
>     StringInfoData buf;
>     initStringInfo(&buf);
>
>     if (reason == RS_SYNC_SKIP_NONE)
>     {
>         appendStringInfoString(&buf, "none");
>         return buf.data;
>     }
>
>     if (reason & RS_SYNC_SKIP_REMOTE_BEHIND)
>         appendStringInfoString(&buf, "remote_behind|");
>     if (reason & RS_SYNC_SKIP_DATA_LOSS)
>         appendStringInfoString(&buf, "data_loss|");
>     if (reason & RS_SYNC_SKIP_NO_SNAPSHOT)
>         appendStringInfoString(&buf, "no_snapshot|");
>
>     /* Remove trailing '|' */
>     if (buf.len > 0 && buf.data[buf.len - 1] == '|')
>         buf.data[buf.len - 1] = '\0';
>
>     return buf.data;
> }
>
> --
>
Why are we showing the cause of the slot sync delay as an aggregate of
all causes occuring? I thought we should show the reason for the last
slot sync delay?

> Step 4: Capture slot_sync_skip_reason whenever the relevant LOG messages are generated, primarily inside update_local_synced_slot or update_and_persist_local_synced_slot. This value will can later be persisted in the pg_replication_slots catalog.
>
> --
>
> Please let me know if you have any objections. I’ll share the wip patch in a few days.
>
> --
I have attached a patch which I have worked on.

Thanks,
Shlok Kyal

  7. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-05T07:19:55Z

    Hi Shlok,

Good to hear that you’re also interested in working on this task.

On Thu, Sep 4, 2025 at 8:26 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

> Hi Ashutosh,
>
> I am also interested in this thread. And was working on a patch for it.
>
> On Wed, 3 Sept 2025 at 17:52, Ashutosh Sharma <ashu.coek88@gmail.com>
> wrote:
> >
> > Hi Amit,
> >
> > On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >>
> >> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com>
> wrote:
> >> >
> >> > We have seen cases where slot synchronization gets delayed, for
> example when the slot is behind the failover standby or vice versa, and the
> slot sync worker has to wait for one to catch up with the other. During
> this waiting period, users querying pg_replication_slots can only see
> whether the slot has been synchronized or not. If it has already
> synchronized, that’s fine, but if synchronization is taking longer, users
> would naturally want to understand the reason for the delay.
> >> >
> >> > Is there a way for end users to know the cause of slot
> synchronization delays, so they can take appropriate actions to speed it up?
> >> >
> >> > I understand that server logs are emitted in such cases, but logs are
> not something end users would want to check regularly. Moreover, since
> logging is configuration-based, relevant messages may sometimes be skipped
> or suppressed.
> >> >
> >>
> >> Currently, the way to see the reason for sync skip is LOGs but I think
> >> it is better to add a new column like sync_skip_reason in
> >> pg_replication_slots. This can show the reasons like
> >> standby_LSN_ahead_remote_LSN. I think ideally users can compare
> >> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> >> better ideas?
> >>
> >
> > I have similar thoughts, but for clarity, I’d like to outline some of
> the key steps I plan to take:
> >
> > Step 1: Define an enum for all possible reasons a slot persistence was
> skipped.
> >
> > /*
> >  * Reasons why a replication slot sync was skipped.
> >  */
> > typedef enum ReplicationSlotSyncSkipReason
> > {
> >     RS_SYNC_SKIP_NONE = 0,                 /* No skip */
> >
> >     RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind
> local reserved LSN */
> >
> >     RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of
> remote, risk of data loss */
> >
> >     RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a
> consistent snapshot */
> > } ReplicationSlotSyncSkipReason;
> >
> > --
> >
> I think we should also add the case when "remote_slot->confirmed_lsn >
> latestFlushPtr" (WAL corresponding to the confirmed lsn on remote slot
> is still not flushed on the Standby). In this case as well we are
> skipping the slot sync.
>

Yes, we can include this case as well.


>
> > Step 2: Introduce new column to pg_replication_slots to store the skip
> reason
> >
> > /* Inside pg_replication_slots table */
> > ReplicationSlotSyncSkipReason slot_sync_skip_reason;
> >
> > --
> >
> As per the discussion [1], I think it is more of stat related data and
> we should add it in the pg_stat_replication_slots view. Also we can
> add columns for 'slot sync skip count' and 'last slot sync skip'.
> Thoughts?
>

It’s not a bad choice, but what makes it a bit confusing for me is that
some of the slot sync information is stored in pg_replication_slots, while
some is in pg_stat_replication_slots.

Is there a possibility that when an end user queries pg_replication_slots,
it shows a particular slot as synced, but querying
pg_stat_replication_slots instead reveals a sync skip reason, or the other
way around?

Moreover, these views are primary data sources for end users, and the
information is critical for their operations. Splitting related information
across multiple views could increase the complexity of their queries.


>
> > Step 3: Function to convert enum to human-readable string that can be
> stored in pg_replication_slots.
> >
> > /*
> >  * Convert ReplicationSlotSyncSkipReason bitmask to human-readable
> string.
> >  *
> >  * Returns a palloc'd string; caller is responsible for freeing it.
> >  */
> > static char *
> > replication_slot_sync_skip_reason_str(ReplicationSlotSyncSkipReason
> reason)
> > {
> >     StringInfoData buf;
> >     initStringInfo(&buf);
> >
> >     if (reason == RS_SYNC_SKIP_NONE)
> >     {
> >         appendStringInfoString(&buf, "none");
> >         return buf.data;
> >     }
> >
> >     if (reason & RS_SYNC_SKIP_REMOTE_BEHIND)
> >         appendStringInfoString(&buf, "remote_behind|");
> >     if (reason & RS_SYNC_SKIP_DATA_LOSS)
> >         appendStringInfoString(&buf, "data_loss|");
> >     if (reason & RS_SYNC_SKIP_NO_SNAPSHOT)
> >         appendStringInfoString(&buf, "no_snapshot|");
> >
> >     /* Remove trailing '|' */
> >     if (buf.len > 0 && buf.data[buf.len - 1] == '|')
> >         buf.data[buf.len - 1] = '\0';
> >
> >     return buf.data;
> > }
> >
> > --
> >
> Why are we showing the cause of the slot sync delay as an aggregate of
> all causes occuring? I thought we should show the reason for the last
> slot sync delay?
>

Yes we should just be showing the reason for the last sync skip, no
aggregation is needed here.


>
> > Step 4: Capture slot_sync_skip_reason whenever the relevant LOG messages
> are generated, primarily inside update_local_synced_slot or
> update_and_persist_local_synced_slot. This value will can later be
> persisted in the pg_replication_slots catalog.
> >
> > --
> >
> > Please let me know if you have any objections. I’ll share the wip patch
> in a few days.
> >
> > --
> I have attached a patch which I have worked on.
>

Thanks, I will look into it, in fact I have already looked into it, but
before I make any comments, I think maybe we should try to finalize the
approach first.

--
With Regards,
Ashutosh Sharma.

  8. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-06T05:16:05Z

    On Fri, Sep 5, 2025 at 12:50 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Good to hear that you’re also interested in working on this task.
>
> On Thu, Sep 4, 2025 at 8:26 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>>
>> Hi Ashutosh,
>>
>> I am also interested in this thread. And was working on a patch for it.
>>
>> On Wed, 3 Sept 2025 at 17:52, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> >
>> > Hi Amit,
>> >
>> > On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >>
>> >> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> >> >
>> >> > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
>> >> >
>> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
>> >> >
>> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
>> >> >
>> >>
>> >> Currently, the way to see the reason for sync skip is LOGs but I think
>> >> it is better to add a new column like sync_skip_reason in
>> >> pg_replication_slots. This can show the reasons like
>> >> standby_LSN_ahead_remote_LSN. I think ideally users can compare
>> >> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
>> >> better ideas?
>> >>
>> >
>> > I have similar thoughts, but for clarity, I’d like to outline some of the key steps I plan to take:
>> >
>> > Step 1: Define an enum for all possible reasons a slot persistence was skipped.
>> >
>> > /*
>> >  * Reasons why a replication slot sync was skipped.
>> >  */
>> > typedef enum ReplicationSlotSyncSkipReason
>> > {
>> >     RS_SYNC_SKIP_NONE = 0,                 /* No skip */
>> >
>> >     RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind local reserved LSN */
>> >
>> >     RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of remote, risk of data loss */
>> >
>> >     RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a consistent snapshot */
>> > } ReplicationSlotSyncSkipReason;
>> >
>> > --
>> >
>> I think we should also add the case when "remote_slot->confirmed_lsn >
>> latestFlushPtr" (WAL corresponding to the confirmed lsn on remote slot
>> is still not flushed on the Standby). In this case as well we are
>> skipping the slot sync.
>
>
> Yes, we can include this case as well.
>
>>
>>
>> > Step 2: Introduce new column to pg_replication_slots to store the skip reason
>> >
>> > /* Inside pg_replication_slots table */
>> > ReplicationSlotSyncSkipReason slot_sync_skip_reason;
>> >
>> > --
>> >
>> As per the discussion [1], I think it is more of stat related data and
>> we should add it in the pg_stat_replication_slots view. Also we can
>> add columns for 'slot sync skip count' and 'last slot sync skip'.
>> Thoughts?
>
>
> It’s not a bad choice, but what makes it a bit confusing for me is that some of the slot sync information is stored in pg_replication_slots, while some is in pg_stat_replication_slots.
>

How about keeping sync_skip_reason in pg_replication_slots and
sync_skip_count in pg_stat_replication_slots?

-- 
With Regards,
Amit Kapila.

  9. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-08T04:22:25Z

    Hi Amit,

On Sat, Sep 6, 2025 at 10:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Sep 5, 2025 at 12:50 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Good to hear that you’re also interested in working on this task.
> >
> > On Thu, Sep 4, 2025 at 8:26 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >>
> >> Hi Ashutosh,
> >>
> >> I am also interested in this thread. And was working on a patch for it.
> >>
> >> On Wed, 3 Sept 2025 at 17:52, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >> >
> >> > Hi Amit,
> >> >
> >> > On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> >>
> >> >> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >> >> >
> >> >> > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
> >> >> >
> >> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
> >> >> >
> >> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
> >> >> >
> >> >>
> >> >> Currently, the way to see the reason for sync skip is LOGs but I think
> >> >> it is better to add a new column like sync_skip_reason in
> >> >> pg_replication_slots. This can show the reasons like
> >> >> standby_LSN_ahead_remote_LSN. I think ideally users can compare
> >> >> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> >> >> better ideas?
> >> >>
> >> >
> >> > I have similar thoughts, but for clarity, I’d like to outline some of the key steps I plan to take:
> >> >
> >> > Step 1: Define an enum for all possible reasons a slot persistence was skipped.
> >> >
> >> > /*
> >> >  * Reasons why a replication slot sync was skipped.
> >> >  */
> >> > typedef enum ReplicationSlotSyncSkipReason
> >> > {
> >> >     RS_SYNC_SKIP_NONE = 0,                 /* No skip */
> >> >
> >> >     RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind local reserved LSN */
> >> >
> >> >     RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of remote, risk of data loss */
> >> >
> >> >     RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a consistent snapshot */
> >> > } ReplicationSlotSyncSkipReason;
> >> >
> >> > --
> >> >
> >> I think we should also add the case when "remote_slot->confirmed_lsn >
> >> latestFlushPtr" (WAL corresponding to the confirmed lsn on remote slot
> >> is still not flushed on the Standby). In this case as well we are
> >> skipping the slot sync.
> >
> >
> > Yes, we can include this case as well.
> >
> >>
> >>
> >> > Step 2: Introduce new column to pg_replication_slots to store the skip reason
> >> >
> >> > /* Inside pg_replication_slots table */
> >> > ReplicationSlotSyncSkipReason slot_sync_skip_reason;
> >> >
> >> > --
> >> >
> >> As per the discussion [1], I think it is more of stat related data and
> >> we should add it in the pg_stat_replication_slots view. Also we can
> >> add columns for 'slot sync skip count' and 'last slot sync skip'.
> >> Thoughts?
> >
> >
> > It’s not a bad choice, but what makes it a bit confusing for me is that some of the slot sync information is stored in pg_replication_slots, while some is in pg_stat_replication_slots.
> >
>
> How about keeping sync_skip_reason in pg_replication_slots and
> sync_skip_count in pg_stat_replication_slots?
>

I think we can do that, since sync_skip_reason appears to be a
descriptive metadata rather than statistical data like
slot_sync_skip_count and last_slot_sync_skip. However, it's also true
that all three pieces of data are transient by nature - they will just
be present in the runtime.

--
With Regards,
Ashutosh Sharma.

  10. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-12T07:37:46Z

    Hi Amit,

On Mon, Sep 8, 2025 at 9:52 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Amit,
>
> On Sat, Sep 6, 2025 at 10:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Sep 5, 2025 at 12:50 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > Good to hear that you’re also interested in working on this task.
> > >
> > > On Thu, Sep 4, 2025 at 8:26 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >>
> > >> Hi Ashutosh,
> > >>
> > >> I am also interested in this thread. And was working on a patch for it.
> > >>
> > >> On Wed, 3 Sept 2025 at 17:52, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >> >
> > >> > Hi Amit,
> > >> >
> > >> > On Thu, Aug 28, 2025 at 3:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >> >>
> > >> >> On Thu, Aug 28, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >> >> >
> > >> >> > We have seen cases where slot synchronization gets delayed, for example when the slot is behind the failover standby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it has already synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the reason for the delay.
> > >> >> >
> > >> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions to speed it up?
> > >> >> >
> > >> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to check regularly. Moreover, since logging is configuration-based, relevant messages may sometimes be skipped or suppressed.
> > >> >> >
> > >> >>
> > >> >> Currently, the way to see the reason for sync skip is LOGs but I think
> > >> >> it is better to add a new column like sync_skip_reason in
> > >> >> pg_replication_slots. This can show the reasons like
> > >> >> standby_LSN_ahead_remote_LSN. I think ideally users can compare
> > >> >> standby's slot LSN/XMIN with remote_slot being synced. Do you have any
> > >> >> better ideas?
> > >> >>
> > >> >
> > >> > I have similar thoughts, but for clarity, I’d like to outline some of the key steps I plan to take:
> > >> >
> > >> > Step 1: Define an enum for all possible reasons a slot persistence was skipped.
> > >> >
> > >> > /*
> > >> >  * Reasons why a replication slot sync was skipped.
> > >> >  */
> > >> > typedef enum ReplicationSlotSyncSkipReason
> > >> > {
> > >> >     RS_SYNC_SKIP_NONE = 0,                 /* No skip */
> > >> >
> > >> >     RS_SYNC_SKIP_REMOTE_BEHIND = (1 << 0), /* Remote slot is behind local reserved LSN */
> > >> >
> > >> >     RS_SYNC_SKIP_DATA_LOSS = (1 << 1),     /* Local slot ahead of remote, risk of data loss */
> > >> >
> > >> >     RS_SYNC_SKIP_NO_SNAPSHOT = (1 << 2)    /* Standby could not build a consistent snapshot */
> > >> > } ReplicationSlotSyncSkipReason;
> > >> >
> > >> > --
> > >> >
> > >> I think we should also add the case when "remote_slot->confirmed_lsn >
> > >> latestFlushPtr" (WAL corresponding to the confirmed lsn on remote slot
> > >> is still not flushed on the Standby). In this case as well we are
> > >> skipping the slot sync.
> > >
> > >
> > > Yes, we can include this case as well.
> > >
> > >>
> > >>
> > >> > Step 2: Introduce new column to pg_replication_slots to store the skip reason
> > >> >
> > >> > /* Inside pg_replication_slots table */
> > >> > ReplicationSlotSyncSkipReason slot_sync_skip_reason;
> > >> >
> > >> > --
> > >> >
> > >> As per the discussion [1], I think it is more of stat related data and
> > >> we should add it in the pg_stat_replication_slots view. Also we can
> > >> add columns for 'slot sync skip count' and 'last slot sync skip'.
> > >> Thoughts?
> > >
> > >
> > > It’s not a bad choice, but what makes it a bit confusing for me is that some of the slot sync information is stored in pg_replication_slots, while some is in pg_stat_replication_slots.
> > >
> >
> > How about keeping sync_skip_reason in pg_replication_slots and
> > sync_skip_count in pg_stat_replication_slots?
> >
>
> I think we can do that, since sync_skip_reason appears to be a
> descriptive metadata rather than statistical data like
> slot_sync_skip_count and last_slot_sync_skip. However, it's also true
> that all three pieces of data are transient by nature - they will just
> be present in the runtime.
>

After spending some more time on this, I found that maintaining
sync_skip_reason in pg_replication_slots would make the code changes a
bit messy and harder to maintain. I think storing all 3 pieces of
information - sync_skip_reason, sync_skip_count, and last_sync_skip in
pg_stat_replication_slots would be a cleaner solution. This way, all
the sync-related info stays together and the code remains
straightforward.

@Sholk, do let me know if you agree with this?

--
With Regards,
Ashutosh Sharma.

  11. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-12T10:54:44Z

    On Fri, Sep 12, 2025 at 1:07 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Mon, Sep 8, 2025 at 9:52 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > I think we can do that, since sync_skip_reason appears to be a
> > descriptive metadata rather than statistical data like
> > slot_sync_skip_count and last_slot_sync_skip. However, it's also true
> > that all three pieces of data are transient by nature - they will just
> > be present in the runtime.
> >
>
> After spending some more time on this, I found that maintaining
> sync_skip_reason in pg_replication_slots would make the code changes a
> bit messy and harder to maintain.
>

What exactly is your worry? It seems more logical to have
sync_skip_reason in pg_replication_slots and other two in
pg_stat_replication_slots as the latter is purely a stats view and the
sync_skip_count/last_sync_skip suits there better.

 I think storing all 3 pieces of
> information - sync_skip_reason, sync_skip_count, and last_sync_skip in
> pg_stat_replication_slots would be a cleaner solution. This way, all
> the sync-related info stays together and the code remains
> straightforward.
>

Having all the sync information together has merit but don't you think
in this case the sync_skip_reason doesn't seem to be matching with the
existing columns in pg_stat_replication_slots?

-- 
With Regards,
Amit Kapila.

  12. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-12T11:57:59Z

    Hi Amit,

On Fri, Sep 12, 2025 at 4:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Sep 12, 2025 at 1:07 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Mon, Sep 8, 2025 at 9:52 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > I think we can do that, since sync_skip_reason appears to be a
> > > descriptive metadata rather than statistical data like
> > > slot_sync_skip_count and last_slot_sync_skip. However, it's also true
> > > that all three pieces of data are transient by nature - they will just
> > > be present in the runtime.
> > >
> >
> > After spending some more time on this, I found that maintaining
> > sync_skip_reason in pg_replication_slots would make the code changes a
> > bit messy and harder to maintain.
> >
>
> What exactly is your worry? It seems more logical to have
> sync_skip_reason in pg_replication_slots and other two in
> pg_stat_replication_slots as the latter is purely a stats view and the
> sync_skip_count/last_sync_skip suits there better.
>

The code changes for adding the skip reason to pg_replication_slots
feel a bit hacky compared to the approach for incorporating all three
pieces of information in pg_stat_replication_slots. I thought many
might prefer simplicity over hackiness, which is why having everything
in pg_stat_replication_slots could be more acceptable. That said, we
could maybe prepare a POC patch with this approach as well, compare
the two, and then decide which path to take.

--
With Regards,
Ashutosh Sharma.

  13. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-13T07:15:10Z

    Hi Amit, Ashutosh,

On Fri, 12 Sept 2025 at 17:28, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Amit,
>
> On Fri, Sep 12, 2025 at 4:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Sep 12, 2025 at 1:07 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > On Mon, Sep 8, 2025 at 9:52 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > > >
> > > > I think we can do that, since sync_skip_reason appears to be a
> > > > descriptive metadata rather than statistical data like
> > > > slot_sync_skip_count and last_slot_sync_skip. However, it's also true
> > > > that all three pieces of data are transient by nature - they will just
> > > > be present in the runtime.
> > > >
> > >
> > > After spending some more time on this, I found that maintaining
> > > sync_skip_reason in pg_replication_slots would make the code changes a
> > > bit messy and harder to maintain.
> > >
> >
> > What exactly is your worry? It seems more logical to have
> > sync_skip_reason in pg_replication_slots and other two in
> > pg_stat_replication_slots as the latter is purely a stats view and the
> > sync_skip_count/last_sync_skip suits there better.
> >
>
> The code changes for adding the skip reason to pg_replication_slots
> feel a bit hacky compared to the approach for incorporating all three
> pieces of information in pg_stat_replication_slots. I thought many
> might prefer simplicity over hackiness, which is why having everything
> in pg_stat_replication_slots could be more acceptable. That said, we
> could maybe prepare a POC patch with this approach as well, compare
> the two, and then decide which path to take.
>

Here are my thought on this:

I believe that if we decide to keep skip_reason in
pg_stat_replication_slot, it should mean "reason of last slot sync
skip" as I think it would make more sense in the sense of statistics.
And if we decide to keep skip_reason in pg_replication_slot, it would
be more appropriate to keep the latest slot data ( It should display
skip_reason only if the current slot sync cycle is skipped).
This is my observation based on the behaviour of  current columns in
these views. Thoughts?

I have also attached POC patches for both approaches as per discussion above.
v2_approach1 : It adds all columns 'slot_sync_skip_reason',
'slot_sync_skip_count' and 'last_slot_sync_skip' to
pg_stat_replication_slots
v2_approach2 : It adds column 'slot_sync_skip_reason' to
pg_replication_slots and columns  'slot_sync_skip_count' and
'last_slot_sync_skip' to pg_stat_replication_slots

Thanks,
Shlok Kyal

  14. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-09-17T10:54:39Z

    Dear Shlok,

Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
indicate the current status of synchronization, it just shows the history.
I feel approach2 has more information than approach1.

Below are my comments for your patch.

01.
```
+        Number of times the slot sync is skipped.
```

"slot sync" should be "slot synchronization". Same thing can be saied for other
attributes.

02.
```
+        Reason of the last slot sync skip.
```

Possible values must be clarified.

03.
```
+            s.slot_sync_skip_count,
+            s.last_slot_sync_skip,
+                       s.slot_sync_skip_reason,
```

Last line has tab-blank but others have space-blank.

04.
```
+typedef enum SlotSyncSkipReason
+{
+       SLOT_SYNC_SKIP_NONE,            /* No skip */
+       SLOT_SYNC_SKIP_STANDBY_BEHIND,  /* Standby is behind the remote slot */
+       SLOT_SYNC_SKIP_REMOTE_BEHIND,   /* Remote slot is behind the local slot */
+       SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT   /* Standby could not reach a
+                                                                                        * consistent snapshot */
+}                      SlotSyncSkipReason
```

a.
Can we add comment atop the enum?

b.
SLOT_SYNC_SKIP_STANDBY_BEHIND is misleading; it indicates the standby server has
not received WALs required by slots, but enum does not clarify it.
How about SLOT_SYNC_SKIP_MISSING_WAL_RECORDS or something? Better naming are very
welcome.

c
s/reach/build/.

05.
```
@@ -646,11 +652,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
                                           remote_slot->name,
                                           LSN_FORMAT_ARGS(latestFlushPtr)));
 
-               return false;
+               /* If the slot is not present on the local */
+               if (!(slot = SearchNamedReplicationSlot(remote_slot->name, true)))
+                       return false;
        }
```

Looks like if users try to sync slots via SQL interfaces, the statistics cannot
be updated.

06. update_slot_sync_skip_stats
```
+       /* Update the slot sync reason */
+       SpinLockAcquire(&slot->mutex);
+       slot->slot_sync_skip_reason = skip_reason;
+       SpinLockRelease(&slot->mutex);
```

I feel no need to update the reason if the slot->slot_sync_skip_reason
and skip_reason are the same.

07. synchronize_one_slot
```
+               /*
+                * If standby is behind remote slot and the synced slot is present on
+                * local.
+                */
+               if (remote_slot->confirmed_lsn > latestFlushPtr)
+               {
+                       if (synced)
+                               update_slot_sync_skip_stats(slot, skip_reason);
+                       return false;
+               }
```

This condition exist in the same function; can we combine?

08. GetSlotSyncSkipReason()

Do we have to do pstrdup() here? I found a similar function get_snapbuild_state_desc(),
and it does not use.

09.

Can you consider a test for added codes?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  15. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-17T11:44:26Z

    On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> indicate the current status of synchronization, it just shows the history.
> I feel approach2 has more information than approach1.
>

I also think so but Ashutosh thought that it would be hacky. Ashutosh,
did you have an opinion on this matter after seeing the patches?

-- 
With Regards,
Amit Kapila.

  16. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-17T14:49:05Z

    Hi Amit,

On Wed, Sep 17, 2025 at 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shlok,
> >
> > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > indicate the current status of synchronization, it just shows the history.
> > I feel approach2 has more information than approach1.
> >
>
> I also think so but Ashutosh thought that it would be hacky. Ashutosh,
> did you have an opinion on this matter after seeing the patches?
>

Yes, I’ve looked into both the patches. Approach 1 seems quite
straightforward. In approach 2, we need to pass some additional
arguments to update_local_sync_slot and
update_and_persist_local_synced_slot, which makes it feel a little
less clean compared to approach 1, where we simply add a new function
and call it directly. That said, this is just my view on code
cleanliness, I’m fine with proceeding with approach 2 if that’s
considered the better option.

--
With Regards,
Ashutosh Sharma,

  17. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-18T06:01:12Z

    On Wed, Sep 17, 2025 at 8:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Shlok,
> > >
> > > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > > indicate the current status of synchronization, it just shows the history.
> > > I feel approach2 has more information than approach1.
> > >
> >
> > I also think so but Ashutosh thought that it would be hacky. Ashutosh,
> > did you have an opinion on this matter after seeing the patches?
> >
>
> Yes, I’ve looked into both the patches. Approach 1 seems quite
> straightforward. In approach 2, we need to pass some additional
> arguments to update_local_sync_slot and
> update_and_persist_local_synced_slot, which makes it feel a little
> less clean compared to approach 1, where we simply add a new function
> and call it directly.
>

This is because the approach-1 doesn't show the latest value of
sync_status. I mean in the latest cycle if the sync is successful, it
won't update the stats which I am not sure is correct because users
may want to know the recent status of sync cycle. Otherwise, the patch
should be almost the same. I think we can even try to write a patch
for approach-2 without an additional out parameter in some of the
functions.

-- 
With Regards,
Amit Kapila.

  18. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-18T07:46:47Z

    Hi Amit,

On Thu, Sep 18, 2025 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 8:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
> > > <kuroda.hayato@fujitsu.com> wrote:
> > > >
> > > > Dear Shlok,
> > > >
> > > > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > > > indicate the current status of synchronization, it just shows the history.
> > > > I feel approach2 has more information than approach1.
> > > >
> > >
> > > I also think so but Ashutosh thought that it would be hacky. Ashutosh,
> > > did you have an opinion on this matter after seeing the patches?
> > >
> >
> > Yes, I’ve looked into both the patches. Approach 1 seems quite
> > straightforward. In approach 2, we need to pass some additional
> > arguments to update_local_sync_slot and
> > update_and_persist_local_synced_slot, which makes it feel a little
> > less clean compared to approach 1, where we simply add a new function
> > and call it directly.
> >
>
> This is because the approach-1 doesn't show the latest value of
> sync_status. I mean in the latest cycle if the sync is successful, it
> won't update the stats which I am not sure is correct because users
> may want to know the recent status of sync cycle. Otherwise, the patch
> should be almost the same.

This should be manageable, no? If we add an additional call to the
stats report function immediately after ReplicationSlotPersist(),
wouldn’t that address the issue? Please correct me if I’m overlooking
something.

@@ -600,6 +600,8 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid)

        ReplicationSlotPersist();

+       pgstat_report_replslot_sync_skip(slot, SLOT_SYNC_SKIP_NONE);
+
        ereport(LOG,
                        errmsg("newly created replication slot \"%s\"
is sync-ready now",
                                   remote_slot->name));

In addition to this, should anyone really need to query the skip
reason if pg_replication_slots already shows that the slot is synced
and not temporary? Ideally, users should check the slot status in
pg_replication_slots, and if it indicates the slot is persisted, there
seems little value in enquiring pg_stat_replication_slots for the skip
reason. That said, it’s important to ensure the information in both
views remains consistent.

> I think we can even try to write a patch
> for approach-2 without an additional out parameter in some of the
> functions.

We can aim for this, if possible.

--
With Regards,
Ashutosh Sharma.

  19. Re: How can end users know the cause of LR slot sync delays?

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-22T05:28:54Z

    Hi,

@@ -646,7 +670,11 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
     remote_slot->name,
     LSN_FORMAT_ARGS(latestFlushPtr)));

- return false;
+ /* If slot is present on the local, update the slot sync skip stats */
+ if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
+ skip_reason = SLOT_SYNC_SKIP_STANDBY_BEHIND;
+ else
+ return false;

With this change, you’re likely enforcing sync slot creation, whereas
earlier that might not have been the case. This introduces a
behavioral change, which may not be well received.

--

I think we can avoid passing skip_reason as a new argument to
update_local_synced_slot(). It only needs to be passed to
update_and_persist_local_synced_slot(). When
update_local_synced_slot() is invoked from within
update_and_persist_local_synced_slot(), we can simply rely on the two
flags, remote_slot_precedes and found_consistent_snapshot and set the
skip_reason accordingly, thoughts?

If update_local_synced_slot is being called from any other place that
means the slot is already persisted.

--

+typedef enum SlotSyncSkipReason
+{
+ SLOT_SYNC_SKIP_NONE, /* No skip */
+ SLOT_SYNC_SKIP_STANDBY_BEHIND, /* Standby is behind the remote slot */
+ SLOT_SYNC_SKIP_REMOTE_BEHIND, /* Remote slot is behind the local slot */
+ SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT /* Standby could not reach a
+ * consistent snapshot */
+} SlotSyncSkipReason;
+

I would suggest shortening the enum names like maybe SS_SKIP_NONE
instead of SLOT_SYNC_SKIP_NONE.

--
With Regards,
Ashutosh Sharma.

  20. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-22T10:11:33Z

    On Thu, 18 Sept 2025 at 13:17, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Amit,
>
> On Thu, Sep 18, 2025 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 8:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > On Wed, Sep 17, 2025 at 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
> > > > <kuroda.hayato@fujitsu.com> wrote:
> > > > >
> > > > > Dear Shlok,
> > > > >
> > > > > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > > > > indicate the current status of synchronization, it just shows the history.
> > > > > I feel approach2 has more information than approach1.
> > > > >
> > > >
> > > > I also think so but Ashutosh thought that it would be hacky. Ashutosh,
> > > > did you have an opinion on this matter after seeing the patches?
> > > >
> > >
> > > Yes, I’ve looked into both the patches. Approach 1 seems quite
> > > straightforward. In approach 2, we need to pass some additional
> > > arguments to update_local_sync_slot and
> > > update_and_persist_local_synced_slot, which makes it feel a little
> > > less clean compared to approach 1, where we simply add a new function
> > > and call it directly.
> > >
> >
> > This is because the approach-1 doesn't show the latest value of
> > sync_status. I mean in the latest cycle if the sync is successful, it
> > won't update the stats which I am not sure is correct because users
> > may want to know the recent status of sync cycle. Otherwise, the patch
> > should be almost the same.
>
> This should be manageable, no? If we add an additional call to the
> stats report function immediately after ReplicationSlotPersist(),
> wouldn’t that address the issue? Please correct me if I’m overlooking
> something.
>
> @@ -600,6 +600,8 @@ update_and_persist_local_synced_slot(RemoteSlot
> *remote_slot, Oid remote_dbid)
>
>         ReplicationSlotPersist();
>
> +       pgstat_report_replslot_sync_skip(slot, SLOT_SYNC_SKIP_NONE);
> +
>         ereport(LOG,
>                         errmsg("newly created replication slot \"%s\"
> is sync-ready now",
>                                    remote_slot->name));
>
Currently, in this code change, the skip_reason is updated to 'none'
only when the slot state changes from temporary to persistent.
However, this logic does not handle cases where the slot is already a
persistent sync slot.
I believe we should also sync skip can happen for persistent slots.
That means, on a successful slot sync, we should update the
skip_reason to 'none' even for slots that are already persistent.

> In addition to this, should anyone really need to query the skip
> reason if pg_replication_slots already shows that the slot is synced
> and not temporary? Ideally, users should check the slot status in
> pg_replication_slots, and if it indicates the slot is persisted, there
> seems little value in enquiring pg_stat_replication_slots for the skip
> reason. That said, it’s important to ensure the information in both
> views remains consistent.
>
I have a doubt. Why don't we want to report the sync skip reason once
the slots are persisted?
for the case:

latestFlushPtr = GetStandbyFlushRecPtr(NULL);
    if (remote_slot->confirmed_lsn > latestFlushPtr)
    {
        /*
         * Can get here only if GUC 'synchronized_standby_slots' on the
         * primary server was not configured correctly.
         */
        ereport(AmLogicalSlotSyncWorkerProcess() ? LOG : ERROR,
                errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                errmsg("skipping slot synchronization because the
received slot sync"
                       " LSN %X/%08X for slot \"%s\" is ahead of the
standby position %X/%08X",
                       LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
                       remote_slot->name,
                       LSN_FORMAT_ARGS(latestFlushPtr)));

        return false;
    }

Slot sync skip can happen even for persistent slots. So why should we
avoid displaying the skip reason in such cases?
I checked that if the synchronized_standby_slots GUC is not set
properly, we can hit this condition even for persistent slots.
I think we should still display the skip reason if the user does not
configure this GUC  as expected.

> > I think we can even try to write a patch
> > for approach-2 without an additional out parameter in some of the
> > functions.
>
> We can aim for this, if possible.

Thanks,
Shlok Kyal

  21. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-22T11:38:13Z

    On Mon, Sep 22, 2025 at 3:41 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 18 Sept 2025 at 13:17, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
>
> > In addition to this, should anyone really need to query the skip
> > reason if pg_replication_slots already shows that the slot is synced
> > and not temporary? Ideally, users should check the slot status in
> > pg_replication_slots, and if it indicates the slot is persisted, there
> > seems little value in enquiring pg_stat_replication_slots for the skip
> > reason. That said, it’s important to ensure the information in both
> > views remains consistent.
> >
> I have a doubt. Why don't we want to report the sync skip reason once
> the slots are persisted?
> for the case:
>
> latestFlushPtr = GetStandbyFlushRecPtr(NULL);
>     if (remote_slot->confirmed_lsn > latestFlushPtr)
>     {
>         /*
>          * Can get here only if GUC 'synchronized_standby_slots' on the
>          * primary server was not configured correctly.
>          */
>         ereport(AmLogicalSlotSyncWorkerProcess() ? LOG : ERROR,
>                 errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>                 errmsg("skipping slot synchronization because the
> received slot sync"
>                        " LSN %X/%08X for slot \"%s\" is ahead of the
> standby position %X/%08X",
>                        LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
>                        remote_slot->name,
>                        LSN_FORMAT_ARGS(latestFlushPtr)));
>
>         return false;
>     }
>
> Slot sync skip can happen even for persistent slots. So why should we
> avoid displaying the skip reason in such cases?
>

We should display the skip reason even for persistent slots and clear
the same after a successful sync.

-- 
With Regards,
Amit Kapila.

  22. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-24T11:09:33Z

    On Wed, 17 Sept 2025 at 16:24, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> indicate the current status of synchronization, it just shows the history.
> I feel approach2 has more information than approach1.
>
I agree with your point. I think the preferred behaviour of
slot_sync_skip_reason is to indicate the current status of
synchronization. And taking account the current behaviour of columns
of views pg_replication_slots and pg_stat_replication_slots, I think
slot_sync_skip_reason is suited for pg_replication_slots view and
I am proceeding ahead with approach2

> Below are my comments for your patch.
>
> 01.
> ```
> +        Number of times the slot sync is skipped.
> ```
>
> "slot sync" should be "slot synchronization". Same thing can be saied for other
> attributes.
>
> 02.
> ```
> +        Reason of the last slot sync skip.
> ```
>
> Possible values must be clarified.
>
> 03.
> ```
> +            s.slot_sync_skip_count,
> +            s.last_slot_sync_skip,
> +                       s.slot_sync_skip_reason,
> ```
>
> Last line has tab-blank but others have space-blank.
>
> 04.
> ```
> +typedef enum SlotSyncSkipReason
> +{
> +       SLOT_SYNC_SKIP_NONE,            /* No skip */
> +       SLOT_SYNC_SKIP_STANDBY_BEHIND,  /* Standby is behind the remote slot */
> +       SLOT_SYNC_SKIP_REMOTE_BEHIND,   /* Remote slot is behind the local slot */
> +       SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT   /* Standby could not reach a
> +                                                                                        * consistent snapshot */
> +}                      SlotSyncSkipReason
> ```
>
> a.
> Can we add comment atop the enum?
>
> b.
> SLOT_SYNC_SKIP_STANDBY_BEHIND is misleading; it indicates the standby server has
> not received WALs required by slots, but enum does not clarify it.
> How about SLOT_SYNC_SKIP_MISSING_WAL_RECORDS or something? Better naming are very
> welcome.
>
> c
> s/reach/build/.
>
> 05.
> ```
> @@ -646,11 +652,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
>                                            remote_slot->name,
>                                            LSN_FORMAT_ARGS(latestFlushPtr)));
>
> -               return false;
> +               /* If the slot is not present on the local */
> +               if (!(slot = SearchNamedReplicationSlot(remote_slot->name, true)))
> +                       return false;
>         }
> ```
>
> Looks like if users try to sync slots via SQL interfaces, the statistics cannot
> be updated.
>
> 06. update_slot_sync_skip_stats
> ```
> +       /* Update the slot sync reason */
> +       SpinLockAcquire(&slot->mutex);
> +       slot->slot_sync_skip_reason = skip_reason;
> +       SpinLockRelease(&slot->mutex);
> ```
>
> I feel no need to update the reason if the slot->slot_sync_skip_reason
> and skip_reason are the same.
>
> 07. synchronize_one_slot
> ```
> +               /*
> +                * If standby is behind remote slot and the synced slot is present on
> +                * local.
> +                */
> +               if (remote_slot->confirmed_lsn > latestFlushPtr)
> +               {
> +                       if (synced)
> +                               update_slot_sync_skip_stats(slot, skip_reason);
> +                       return false;
> +               }
> ```
>
> This condition exist in the same function; can we combine?
>
> 08. GetSlotSyncSkipReason()
>
> Do we have to do pstrdup() here? I found a similar function get_snapbuild_state_desc(),
> and it does not use.
>
I check similar functions and I think we can remove pstrup. Removed in
the latest patch.

> 09.
>
> Can you consider a test for added codes?
Added test in 0002 patch

I have also addressed remaining comments and attached the latest
version of patch.

Thanks,
Shlok Kyal

  23. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-24T11:10:40Z

    On Thu, 18 Sept 2025 at 11:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 8:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Sep 17, 2025 at 4:24 PM Hayato Kuroda (Fujitsu)
> > > <kuroda.hayato@fujitsu.com> wrote:
> > > >
> > > > Dear Shlok,
> > > >
> > > > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > > > indicate the current status of synchronization, it just shows the history.
> > > > I feel approach2 has more information than approach1.
> > > >
> > >
> > > I also think so but Ashutosh thought that it would be hacky. Ashutosh,
> > > did you have an opinion on this matter after seeing the patches?
> > >
> >
> > Yes, I’ve looked into both the patches. Approach 1 seems quite
> > straightforward. In approach 2, we need to pass some additional
> > arguments to update_local_sync_slot and
> > update_and_persist_local_synced_slot, which makes it feel a little
> > less clean compared to approach 1, where we simply add a new function
> > and call it directly.
> >
>
> This is because the approach-1 doesn't show the latest value of
> sync_status. I mean in the latest cycle if the sync is successful, it
> won't update the stats which I am not sure is correct because users
> may want to know the recent status of sync cycle. Otherwise, the patch
> should be almost the same. I think we can even try to write a patch
> for approach-2 without an additional out parameter in some of the
> functions.
>
Hi Amit,

I have written a patch which removes passing this extra parameter to
the functions.
I have attached the latest patch in [1].

[1]: https://www.postgresql.org/message-id/CANhcyEVFZN2Mkjs0QHshKm2_3AkQ0eufjkD12eL2MeuVkPyGbw%40mail.gmail.com

Thanks,
Shlok Kyal

  24. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-24T11:12:13Z

    Hi Ashutosh,

Thanks for reviewing the patch.

On Mon, 22 Sept 2025 at 10:59, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
> @@ -646,7 +670,11 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
> remote_dbid)
>      remote_slot->name,
>      LSN_FORMAT_ARGS(latestFlushPtr)));
>
> - return false;
> + /* If slot is present on the local, update the slot sync skip stats */
> + if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
> + skip_reason = SLOT_SYNC_SKIP_STANDBY_BEHIND;
> + else
> + return false;
>
> With this change, you’re likely enforcing sync slot creation, whereas
> earlier that might not have been the case. This introduces a
> behavioral change, which may not be well received.
>
> --
I have fixed it in latest version of patch

>
> I think we can avoid passing skip_reason as a new argument to
> update_local_synced_slot(). It only needs to be passed to
> update_and_persist_local_synced_slot(). When
> update_local_synced_slot() is invoked from within
> update_and_persist_local_synced_slot(), we can simply rely on the two
> flags, remote_slot_precedes and found_consistent_snapshot and set the
> skip_reason accordingly, thoughts?
>
> If update_local_synced_slot is being called from any other place that
> means the slot is already persisted.
>
I came up with a solution on similar lines as above in the attached patch.
And also as per Amit's comment in [1], I have kept the behaviour to
show skip reason even for persistent slots.

> --
>
> +typedef enum SlotSyncSkipReason
> +{
> + SLOT_SYNC_SKIP_NONE, /* No skip */
> + SLOT_SYNC_SKIP_STANDBY_BEHIND, /* Standby is behind the remote slot */
> + SLOT_SYNC_SKIP_REMOTE_BEHIND, /* Remote slot is behind the local slot */
> + SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT /* Standby could not reach a
> + * consistent snapshot */
> +} SlotSyncSkipReason;
> +
>
> I would suggest shortening the enum names like maybe SS_SKIP_NONE
> instead of SLOT_SYNC_SKIP_NONE.
>
Fixed it.

I have attached the latest patch [2].

[1]: https://www.postgresql.org/message-id/CAA4eK1%2B6UsZiN0GnRNue_Vs8007jdcDFetNq%2BapubHcrqzjwpQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CANhcyEVFZN2Mkjs0QHshKm2_3AkQ0eufjkD12eL2MeuVkPyGbw%40mail.gmail.com

Thanks,
Shlok Kyal

  25. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-26T11:35:54Z

    On Wed, 24 Sept 2025 at 16:39, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Wed, 17 Sept 2025 at 16:24, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shlok,
> >
> > Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
> > indicate the current status of synchronization, it just shows the history.
> > I feel approach2 has more information than approach1.
> >
> I agree with your point. I think the preferred behaviour of
> slot_sync_skip_reason is to indicate the current status of
> synchronization. And taking account the current behaviour of columns
> of views pg_replication_slots and pg_stat_replication_slots, I think
> slot_sync_skip_reason is suited for pg_replication_slots view and
> I am proceeding ahead with approach2
>
> > Below are my comments for your patch.
> >
> > 01.
> > ```
> > +        Number of times the slot sync is skipped.
> > ```
> >
> > "slot sync" should be "slot synchronization". Same thing can be saied for other
> > attributes.
> >
> > 02.
> > ```
> > +        Reason of the last slot sync skip.
> > ```
> >
> > Possible values must be clarified.
> >
> > 03.
> > ```
> > +            s.slot_sync_skip_count,
> > +            s.last_slot_sync_skip,
> > +                       s.slot_sync_skip_reason,
> > ```
> >
> > Last line has tab-blank but others have space-blank.
> >
> > 04.
> > ```
> > +typedef enum SlotSyncSkipReason
> > +{
> > +       SLOT_SYNC_SKIP_NONE,            /* No skip */
> > +       SLOT_SYNC_SKIP_STANDBY_BEHIND,  /* Standby is behind the remote slot */
> > +       SLOT_SYNC_SKIP_REMOTE_BEHIND,   /* Remote slot is behind the local slot */
> > +       SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT   /* Standby could not reach a
> > +                                                                                        * consistent snapshot */
> > +}                      SlotSyncSkipReason
> > ```
> >
> > a.
> > Can we add comment atop the enum?
> >
> > b.
> > SLOT_SYNC_SKIP_STANDBY_BEHIND is misleading; it indicates the standby server has
> > not received WALs required by slots, but enum does not clarify it.
> > How about SLOT_SYNC_SKIP_MISSING_WAL_RECORDS or something? Better naming are very
> > welcome.
> >
> > c
> > s/reach/build/.
> >
> > 05.
> > ```
> > @@ -646,11 +652,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
> >                                            remote_slot->name,
> >                                            LSN_FORMAT_ARGS(latestFlushPtr)));
> >
> > -               return false;
> > +               /* If the slot is not present on the local */
> > +               if (!(slot = SearchNamedReplicationSlot(remote_slot->name, true)))
> > +                       return false;
> >         }
> > ```
> >
> > Looks like if users try to sync slots via SQL interfaces, the statistics cannot
> > be updated.
> >
> > 06. update_slot_sync_skip_stats
> > ```
> > +       /* Update the slot sync reason */
> > +       SpinLockAcquire(&slot->mutex);
> > +       slot->slot_sync_skip_reason = skip_reason;
> > +       SpinLockRelease(&slot->mutex);
> > ```
> >
> > I feel no need to update the reason if the slot->slot_sync_skip_reason
> > and skip_reason are the same.
> >
> > 07. synchronize_one_slot
> > ```
> > +               /*
> > +                * If standby is behind remote slot and the synced slot is present on
> > +                * local.
> > +                */
> > +               if (remote_slot->confirmed_lsn > latestFlushPtr)
> > +               {
> > +                       if (synced)
> > +                               update_slot_sync_skip_stats(slot, skip_reason);
> > +                       return false;
> > +               }
> > ```
> >
> > This condition exist in the same function; can we combine?
> >
> > 08. GetSlotSyncSkipReason()
> >
> > Do we have to do pstrdup() here? I found a similar function get_snapbuild_state_desc(),
> > and it does not use.
> >
> I check similar functions and I think we can remove pstrup. Removed in
> the latest patch.
>
> > 09.
> >
> > Can you consider a test for added codes?
> Added test in 0002 patch
>
> I have also addressed remaining comments and attached the latest
> version of patch.
>
The CF Bot was failing as meson.build was not formatted appropriately.
I have fixed it and made some cosmetic changes in the test file.
I ran the CF tests on my local repository and it is passing all tests.

I have attached the updated version.

Thanks,
Shlok Kyal

  26. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-09-30T12:51:58Z

    Dear Shlok,

Thanks for updating the patch. Here are my comments.

01.
```
+       /* Update the slot sync reason */
+       SpinLockAcquire(&slot->mutex);
+       if (slot->slot_sync_skip_reason != skip_reason)
+               slot->slot_sync_skip_reason = skip_reason;
+       SpinLockRelease(&slot->mutex);
```

Per my understanding, spinlock is acquired when the attribute on the shared memory
is updating. Can you check other parts and follow the rukle?

02.
```
+                       SpinLockAcquire(&slot->mutex);
+                       synced = slot->data.synced;
+                       SpinLockRelease(&slot->mutex);
```

Same as 1.

03.
```
```


04.
```
+#ifdef USE_INJECTION_POINTS
+       INJECTION_POINT("slot-sync-skip", NULL);
+#endif
```

No need do do #ifdef here. INJECTION_POINT() itself checks internally.

05.
```
+# Initialize the primary cluster
+my $primary = PostgreSQL::Test::Cluster->new('publisher');
+$primary->init(
+       allows_streaming => 'logical',
+       auth_extra => [ '--create-role' => 'repl_role' ]);
```

Do we have to create repl_role? I'm not sure where it's used.

06.
```
+$primary->append_conf(
+       'postgresql.conf', qq{
+autovacuum = off
+max_prepared_transactions = 1
+});
```

Do we have to set max_prepared_transactions? PREPARE command is not used.

07.
```
+# Load the injection_points extension
+$primary->safe_psql('postgres', q(CREATE EXTENSION injection_points))
```

We must check whether the injection_points is available or not. See 047_checkpoint_physical_slot.pl.

08.
```
+# Initialize standby from primary backup
+my $standby1 = PostgreSQL::Test::Cluster->new('standby1');
+$standby1->init_from_backup(
+       $primary, $backup_name,
+       has_streaming => 1,
+       has_restoring => 1);
```

To clarify, is there a reason why we set has_restoring? Can we remove it?

09.
```
+my $connstr_1 = $primary->connstr;
```

Since this is an only connection string in the test, suffix _1 is not needed.

10.
```
+# Simulate standby connection failure by modifying pg_hba.conf
+unlink($primary->data_dir . '/pg_hba.conf');
+$primary->append_conf('pg_hba.conf',
+       qq{local   all             all                                     trust}
+);
```

What if the system does not have Unix domain socket? I'm afraid all connections
could be brocked in this case.

11.
```
+# Attempt to sync replication slots while standby is behind
+$standby1->psql('postgres', "SELECT pg_sync_replication_slots();");
```

When the command can be failed, can you set a error message from the command to
a variable? Otherwise an ERROR is output to the result file, which is suprising.

```
psql:<stdin>:1: ERROR:  skipping slot synchronization because the received slot sync LSN 0/03019E58 for slot "slot_sync" is ahead of the standby position 0/030000D8
[17:23:21.495](0.384s) ok 2 - slot sync skip when standby is behind
```

12.
```
+# Restore connectivity between primary and standby
+unlink($primary->data_dir . '/pg_hba.conf');
+$primary->append_conf(
+       'pg_hba.conf',
+       qq{
+local   all             all                                     trust
+local   replication     all                                     trust
+});
```

Same as 10. Also, no need to do unlink() here.

13.
```
+# Create a new logical slot for testing injection point
+$primary->safe_psql('postgres',
+       "SELECT pg_create_logical_replication_slot('slot_sync', 'test_decoding', false, false, true)"
+);
```

Before here can you add a description what you would test?

14.
```
# Create a physical replication slot on primary for standby
$primary->psql('postgres',
	q{SELECT pg_create_physical_replication_slot('sb1_slot');});
```
Use safe_psql instead of psql().

15.
```
-       if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
+       if (slot || (slot = SearchNamedReplicationSlot(remote_slot->name, true)))
```

Is there a possibility that slot is not NULL? It is used when confirmed_flush exceeds
the latestFlushPtr, but in this case the function cannot reach here.


Best regards,
Hayato Kuroda
FUJITSU LIMITED

  27. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-01T02:56:30Z

    Dear Shlok,

> Thanks for updating the patch. Here are my comments.

I found one more comment.

```
+       /*
+        * If found_consistent_snapshot is not NULL and a consistent snapshot is
+        * found set the slot sync skip reason to none. Else, if consistent
+        * snapshot is not found the stats will be updated in the function
+        * update_and_persist_local_synced_slot
+        */
+       if (!found_consistent_snapshot || *found_consistent_snapshot)
+               update_slot_sync_skip_stats(slot, SS_SKIP_NONE);
```

I think the condition is confusing; in code level there is a path that
found_consistent_snapshot is NULL but synchronization happened. (Not sure it is
possible though).

I think it is better to put update_slot_sync_skip_stats() near the sync part.
If the snapshot exists from the beginning, it can be done unconditionally,
otherwise we can check again. Attached .diffs file implements it.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  28. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-15T10:41:33Z

    On Tue, 30 Sept 2025 at 18:22, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for updating the patch. Here are my comments.
>
> 01.
> ```
> +       /* Update the slot sync reason */
> +       SpinLockAcquire(&slot->mutex);
> +       if (slot->slot_sync_skip_reason != skip_reason)
> +               slot->slot_sync_skip_reason = skip_reason;
> +       SpinLockRelease(&slot->mutex);
> ```
>
> Per my understanding, spinlock is acquired when the attribute on the shared memory
> is updating. Can you check other parts and follow the rukle?
>
> 02.
> ```
> +                       SpinLockAcquire(&slot->mutex);
> +                       synced = slot->data.synced;
> +                       SpinLockRelease(&slot->mutex);
> ```
>
> Same as 1.
>
I checked and found the following comment:
 * - Individual fields are protected by mutex where only the backend owning
 * the slot is authorized to update the fields from its own slot.  The
 * backend owning the slot does not need to take this lock when reading its
 * own fields, while concurrent backends not owning this slot should take the
 * lock when reading this slot's data.

So for the above two cases we are updating the
'slot->slot_sync_skip_reason' and reading 'slot->data.synced' and this
can happen before the slot sync worker acquires the slot or owns the
slot.
Also in the same code at a later stage we are again checking the
synced flag and we do that while holding a spin lock. Based on these
observations I think we should take Spinlock in both cases.

> 03.
> ```
> ```
>
>
> 04.
> ```
> +#ifdef USE_INJECTION_POINTS
> +       INJECTION_POINT("slot-sync-skip", NULL);
> +#endif
> ```
>
> No need do do #ifdef here. INJECTION_POINT() itself checks internally.
>
Fixed

> 05.
> ```
> +# Initialize the primary cluster
> +my $primary = PostgreSQL::Test::Cluster->new('publisher');
> +$primary->init(
> +       allows_streaming => 'logical',
> +       auth_extra => [ '--create-role' => 'repl_role' ]);
> ```
>
> Do we have to create repl_role? I'm not sure where it's used.
>
It is not needed, I have removed it.

> 06.
> ```
> +$primary->append_conf(
> +       'postgresql.conf', qq{
> +autovacuum = off
> +max_prepared_transactions = 1
> +});
> ```
>
> Do we have to set max_prepared_transactions? PREPARE command is not used.
>
It is not needed. Removed it.

> 07.
> ```
> +# Load the injection_points extension
> +$primary->safe_psql('postgres', q(CREATE EXTENSION injection_points))
> ```
>
> We must check whether the injection_points is available or not. See 047_checkpoint_physical_slot.pl.
>
Added the check

> 08.
> ```
> +# Initialize standby from primary backup
> +my $standby1 = PostgreSQL::Test::Cluster->new('standby1');
> +$standby1->init_from_backup(
> +       $primary, $backup_name,
> +       has_streaming => 1,
> +       has_restoring => 1);
> ```
>
> To clarify, is there a reason why we set has_restoring? Can we remove it?
>
It is not needed. Removed it.

> 09.
> ```
> +my $connstr_1 = $primary->connstr;
> ```
>
> Since this is an only connection string in the test, suffix _1 is not needed.
>
Fixed

> 10.
> ```
> +# Simulate standby connection failure by modifying pg_hba.conf
> +unlink($primary->data_dir . '/pg_hba.conf');
> +$primary->append_conf('pg_hba.conf',
> +       qq{local   all             all                                     trust}
> +);
> ```
>
> What if the system does not have Unix domain socket? I'm afraid all connections
> could be brocked in this case.
>
I have used an injection point to simulate this scenario instead of
changing the contents of pg_hba.conf files.

> 11.
> ```
> +# Attempt to sync replication slots while standby is behind
> +$standby1->psql('postgres', "SELECT pg_sync_replication_slots();");
> ```
>
> When the command can be failed, can you set a error message from the command to
> a variable? Otherwise an ERROR is output to the result file, which is suprising.
>
> ```
> psql:<stdin>:1: ERROR:  skipping slot synchronization because the received slot sync LSN 0/03019E58 for slot "slot_sync" is ahead of the standby position 0/030000D8
> [17:23:21.495](0.384s) ok 2 - slot sync skip when standby is behind
> ```
>

In the existing tests I find similar cases where ERROR is output to
the result file.
For example in recovery/001_stream file:
[10:33:55.443](0.099s) ok 5 - pg_sequence_last_value() on unlogged
sequence on standby 1
psql:<stdin>:1: ERROR:  cannot execute INSERT in a read-only transaction
[10:33:55.468](0.025s) ok 6 - read-only queries on standby 1
psql:<stdin>:1: ERROR:  cannot execute INSERT in a read-only transaction

In 006_logical_decoding
[10:34:19.233](0.050s) ok 8 - pg_recvlogical acknowledged changes
psql:<stdin>:1: ERROR:  replication slot "test_slot" was not created
in this database
[10:34:19.432](0.199s) ok 9 - replaying logical slot from another database fails
psql:<stdin>:1: ERROR:  database "otherdb" is used by an active
logical replication slot
DETAIL:  There is 1 active slot.

But I think it is a good idea to set the error message to variable to
avoid ERROR messages in the regress log file.
Added the change for the same.

> 12.
> ```
> +# Restore connectivity between primary and standby
> +unlink($primary->data_dir . '/pg_hba.conf');
> +$primary->append_conf(
> +       'pg_hba.conf',
> +       qq{
> +local   all             all                                     trust
> +local   replication     all                                     trust
> +});
> ```
>
> Same as 10. Also, no need to do unlink() here.
>
I have used an injection point to simulate this scenario instead of
changing the contents of pg_hba.conf files.

> 13.
> ```
> +# Create a new logical slot for testing injection point
> +$primary->safe_psql('postgres',
> +       "SELECT pg_create_logical_replication_slot('slot_sync', 'test_decoding', false, false, true)"
> +);
> ```
>
> Before here can you add a description what you would test?
>
Added

> 14.
> ```
> # Create a physical replication slot on primary for standby
> $primary->psql('postgres',
>         q{SELECT pg_create_physical_replication_slot('sb1_slot');});
> ```
> Use safe_psql instead of psql().
>
Fixed

> 15.
> ```
> -       if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
> +       if (slot || (slot = SearchNamedReplicationSlot(remote_slot->name, true)))
> ```
>
> Is there a possibility that slot is not NULL? It is used when confirmed_flush exceeds
> the latestFlushPtr, but in this case the function cannot reach here.
>
This change is not required. I have reverted this change.

Thanks,
Shlok Kyal

  29. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-15T10:45:01Z

    On Wed, 1 Oct 2025 at 08:26, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> > Thanks for updating the patch. Here are my comments.
>
> I found one more comment.
>
> ```
> +       /*
> +        * If found_consistent_snapshot is not NULL and a consistent snapshot is
> +        * found set the slot sync skip reason to none. Else, if consistent
> +        * snapshot is not found the stats will be updated in the function
> +        * update_and_persist_local_synced_slot
> +        */
> +       if (!found_consistent_snapshot || *found_consistent_snapshot)
> +               update_slot_sync_skip_stats(slot, SS_SKIP_NONE);
> ```
>
> I think the condition is confusing; in code level there is a path that
> found_consistent_snapshot is NULL but synchronization happened. (Not sure it is
> possible though).
>
> I think it is better to put update_slot_sync_skip_stats() near the sync part.
> If the snapshot exists from the beginning, it can be done unconditionally,
> otherwise we can check again. Attached .diffs file implements it.
>

I agree that the condition can be confusing. I checked your patch and
have a concern.
For the change:
```
  LogicalSlotAdvanceAndCheckSnapState(remote_slot->confirmed_lsn,
  found_consistent_snapshot);

+ /* Update the slot sync skip reason if snapshot could be created */
+ if (SnapBuildSnapshotExists(remote_slot->restart_lsn))
+ {
+ Assert(!found_consistent_snapshot ||
+    *found_consistent_snapshot);
+ update_slot_sync_skip_stats(slot, SS_SKIP_NONE);
+ }
+
```

I debugged and noticed that, on a successful sync slot run, in some
cases we do not enter the if condition here which can lead to
misleading slot_sync_skip_reason.
I noticed that even if the synced slot is advanced and
'found_consistent_snapshot' set as true,
SnapBuildSnapshotExists(remote_slot->restart_lsn) can return false.

As far as I understand, LogicalSlotAdvanceAndCheckSnapState will
advance the slot but will not serialize the snapshot and hence
SnapBuildSnapshotExists can return false.
I have also added a test case in 049_slot_skip_stats named
"slot_sync_skip_reason is reset after successful sync"  which can
reproduce it.

I think we can update the stats when "slot->data.confirmed_flush ==
remote_slot->confirmed_lsn" instead of checking
'SnapBuildSnapshotExists'. Thoughts?

Thanks,
Shlok Kyal

  30. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-20T08:57:32Z

    Dear Shlok,

> > 01.
> > ```
> > +       /* Update the slot sync reason */
> > +       SpinLockAcquire(&slot->mutex);
> > +       if (slot->slot_sync_skip_reason != skip_reason)
> > +               slot->slot_sync_skip_reason = skip_reason;
> > +       SpinLockRelease(&slot->mutex);
> > ```
> >
> > Per my understanding, spinlock is acquired when the attribute on the shared
> memory
> > is updating. Can you check other parts and follow the rukle?
> >
> > 02.
> > ```
> > +                       SpinLockAcquire(&slot->mutex);
> > +                       synced = slot->data.synced;
> > +                       SpinLockRelease(&slot->mutex);
> > ```
> >
> > Same as 1.
> >
> I checked and found the following comment:
>  * - Individual fields are protected by mutex where only the backend owning
>  * the slot is authorized to update the fields from its own slot.  The
>  * backend owning the slot does not need to take this lock when reading its
>  * own fields, while concurrent backends not owning this slot should take the
>  * lock when reading this slot's data.
> 
> So for the above two cases we are updating the
> 'slot->slot_sync_skip_reason' and reading 'slot->data.synced' and this
> can happen before the slot sync worker acquires the slot or owns the
> slot.
> Also in the same code at a later stage we are again checking the
> synced flag and we do that while holding a spin lock. Based on these
> observations I think we should take Spinlock in both cases.

Hmm, regarding the update_slot_sync_skip_stats(), the replication slot has already been
acquired except synchronize_one_slot() case.
Can we avoid acquiring the spinlock as much as possible by adding an argument?
Or it just introduces additional complexity?

> > 09.
> > ```
> > +my $connstr_1 = $primary->connstr;
> > ```
> >
> > Since this is an only connection string in the test, suffix _1 is not needed.
> >
> Fixed

Same as the comment, can you replace "standby1" to "stanby"?

> 
> > 10.
> > ```
> > +# Simulate standby connection failure by modifying pg_hba.conf
> > +unlink($primary->data_dir . '/pg_hba.conf');
> > +$primary->append_conf('pg_hba.conf',
> > +       qq{local   all             all
> trust}
> > +);
> > ```
> >
> > What if the system does not have Unix domain socket? I'm afraid all connections
> > could be brocked in this case.
> >
> I have used an injection point to simulate this scenario instead of
> changing the contents of pg_hba.conf files.

Can you clarify the reason why you used the injection point? 
I'm not sure the injection point is beneficial here. I feel the point can be added
when we handle the timing issue, race condition etc, but walreceiver may not have
strong reasons to stop exact at that point.

Regarding the content of pg_hba.conf, I felt below lines might be enough:
```
local   all             all                                     trust 
host    all             all             127.0.0.1/32            trust 
```

Also, here are comments for v5.

```
+      <para>
+       Reason of the last slot synchronization skip.
+      </para></entry>
```

Possible values must be clarified. This was posted in [1] but seemed to be missed.

```
+       /* Update the slot sync reason */
```

It is better to clarify updating the *skip* reason


```
-       ReplicationSlot *slot;
+       ReplicationSlot *slot = NULL;
```

No need to initialize as NULL.

```
+#include "utils/injection_point.h"
...
+                               INJECTION_POINT("walreceiver", NULL);
```

As I told above, I have a concern to add the injection point. I want to hear
other's opinion as well.

```
+                       else
+                       {
+                               /* Update the slot sync stats */
+                               Assert(!found_consistent_snapshot ||
+                                          *found_consistent_snapshot);
+                               update_slot_sync_skip_stats(slot, SS_SKIP_NONE);
+                       }
```

Your patch may have another issue; if both confirmed_flush_lsn are the same
but we do not have the consistent snapshot yet, we would get the assertion failure.
(Again, not sure it can really happen)
Can we use the condition as another if part? At that time we must clarify why
it is OK to pass in case of found_consistent_snapshot == NULL.

```
+# Attach injection point to simulate wait
+$standby_psql->query_safe(
+       q(select injection_points_attach('slot-sync-skip','wait')));
```

I have been considering whether we can remove the injection point here or not.
I think the point is used because the being synchronized slot is still temporary
one; they would be cleaned up by ReplicationSlotCleanup().
Can we reproduce the skip event for the permanent slot? I cannot come up with,
but if possible no need to introduce the injection point.

[1]: https://www.postgresql.org/message-id/OSCPR01MB14966A618A8C61EC3DEE486A4F517A%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  31. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-30T12:16:02Z

    On Mon, 20 Oct 2025 at 14:27, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> > > 01.
> > > ```
> > > +       /* Update the slot sync reason */
> > > +       SpinLockAcquire(&slot->mutex);
> > > +       if (slot->slot_sync_skip_reason != skip_reason)
> > > +               slot->slot_sync_skip_reason = skip_reason;
> > > +       SpinLockRelease(&slot->mutex);
> > > ```
> > >
> > > Per my understanding, spinlock is acquired when the attribute on the shared
> > memory
> > > is updating. Can you check other parts and follow the rukle?
> > >
> > > 02.
> > > ```
> > > +                       SpinLockAcquire(&slot->mutex);
> > > +                       synced = slot->data.synced;
> > > +                       SpinLockRelease(&slot->mutex);
> > > ```
> > >
> > > Same as 1.
> > >
> > I checked and found the following comment:
> >  * - Individual fields are protected by mutex where only the backend owning
> >  * the slot is authorized to update the fields from its own slot.  The
> >  * backend owning the slot does not need to take this lock when reading its
> >  * own fields, while concurrent backends not owning this slot should take the
> >  * lock when reading this slot's data.
> >
I realised that the patch does not entirely follow the above rule. As
per my understanding rule says:
1. If we want to update a field of a slot we need to own the slot (or
we can say acquire the slot)
2. In the above case we also need to take a Spinlock on the slot to
update any field.
3. If we want to read a field and if we own the slot we do not need a
spinlock on the slot.
4. If we want to read a field and if we do not own the slot we need a
spinlock on the slot.

For the current patch (v5), in the function "synchronize_one_slot" we
are calling "update_slot_sync_skip_stats" even if we do not own the
slot.
So, as per the rule I have updated  "update_slot_sync_skip_stats" to
own the slot before updating.

> > So for the above two cases we are updating the
> > 'slot->slot_sync_skip_reason' and reading 'slot->data.synced' and this
> > can happen before the slot sync worker acquires the slot or owns the
> > slot.
> > Also in the same code at a later stage we are again checking the
> > synced flag and we do that while holding a spin lock. Based on these
> > observations I think we should take Spinlock in both cases.
>
> Hmm, regarding the update_slot_sync_skip_stats(), the replication slot has already been
> acquired except synchronize_one_slot() case.
> Can we avoid acquiring the spinlock as much as possible by adding an argument?
> Or it just introduces additional complexity?
>
After updating the code as per rule, I think we always have to take a
Spinlock on the slot when we are updating any field.

> > > 09.
> > > ```
> > > +my $connstr_1 = $primary->connstr;
> > > ```
> > >
> > > Since this is an only connection string in the test, suffix _1 is not needed.
> > >
> > Fixed
>
> Same as the comment, can you replace "standby1" to "stanby"?
>
Fixed

> >
> > > 10.
> > > ```
> > > +# Simulate standby connection failure by modifying pg_hba.conf
> > > +unlink($primary->data_dir . '/pg_hba.conf');
> > > +$primary->append_conf('pg_hba.conf',
> > > +       qq{local   all             all
> > trust}
> > > +);
> > > ```
> > >
> > > What if the system does not have Unix domain socket? I'm afraid all connections
> > > could be brocked in this case.
> > >
> > I have used an injection point to simulate this scenario instead of
> > changing the contents of pg_hba.conf files.
>
> Can you clarify the reason why you used the injection point?
> I'm not sure the injection point is beneficial here. I feel the point can be added
> when we handle the timing issue, race condition etc, but walreceiver may not have
> strong reasons to stop exact at that point.
>
> Regarding the content of pg_hba.conf, I felt below lines might be enough:
> ```
> local   all             all                                     trust
> host    all             all             127.0.0.1/32            trust
> ```
>
I checked this.
By default pg_hba.conf has contents as:
```
# "local" is for Unix domain socket connections only
local   all             all                                     trust
# IPv4 local connections:
host    all             all             127.0.0.1/32            trust
# IPv6 local connections:
host    all             all             ::1/128                 trust
# Allow replication connections from localhost, by a user with the
# replication privilege.
local   replication     all                                     trust
host    replication     all             127.0.0.1/32            trust
host    replication     all             ::1/128                 trust
```
Now for our test to prevent the streaming replication we can set pg_hba.conf to
```
local   all             all                                     trust
host    all             all             127.0.0.1/32            trust
host    all             all             ::1/128                 trust
```
And then to restore streaming replication we can add following to pg_hba.conf:
```
local   replication     all                                     trust
host    replication     all             127.0.0.1/32            trust
host    replication     all             ::1/128                 trust
```
I think this would be sufficient for our testing. Thoughts?

> Also, here are comments for v5.
>
> ```
> +      <para>
> +       Reason of the last slot synchronization skip.
> +      </para></entry>
> ```
>
> Possible values must be clarified. This was posted in [1] but seemed to be missed.
Sorry, I missed it. I have updated it in the latest patch.

>
> ```
> +       /* Update the slot sync reason */
> ```
>
> It is better to clarify updating the *skip* reason
Fixed

>
> ```
> -       ReplicationSlot *slot;
> +       ReplicationSlot *slot = NULL;
> ```
>
> No need to initialize as NULL.
>
Fixed

> ```
> +#include "utils/injection_point.h"
> ...
> +                               INJECTION_POINT("walreceiver", NULL);
> ```
>
> As I told above, I have a concern to add the injection point. I want to hear
> other's opinion as well.
>
Removed it for now as per my analysis we can modify pg_hba.conf to
simulate the scenario.

> ```
> +                       else
> +                       {
> +                               /* Update the slot sync stats */
> +                               Assert(!found_consistent_snapshot ||
> +                                          *found_consistent_snapshot);
> +                               update_slot_sync_skip_stats(slot, SS_SKIP_NONE);
> +                       }
> ```
>
> Your patch may have another issue; if both confirmed_flush_lsn are the same
> but we do not have the consistent snapshot yet, we would get the assertion failure.
> (Again, not sure it can really happen)
> Can we use the condition as another if part? At that time we must clarify why
> it is OK to pass in case of found_consistent_snapshot == NULL.
>
Fixed

> ```
> +# Attach injection point to simulate wait
> +$standby_psql->query_safe(
> +       q(select injection_points_attach('slot-sync-skip','wait')));
> ```
>
> I have been considering whether we can remove the injection point here or not.
> I think the point is used because the being synchronized slot is still temporary
> one; they would be cleaned up by ReplicationSlotCleanup().
> Can we reproduce the skip event for the permanent slot? I cannot come up with,
> but if possible no need to introduce the injection point.
I tried reproducing it but was not able to come up with a test without
injection point. Will further try to reproduce it without injection
point.

>
> [1]: https://www.postgresql.org/message-id/OSCPR01MB14966A618A8C61EC3DEE486A4F517A%40OSCPR01MB14966.jpnprd01.prod.outlook.com
>

I have attached the latest patch.

Thanks,
Shlok Kyal

  32. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-31T05:59:59Z

    Dear Shlok,

Thanks for updating the patch. Few comments:

```
       The reason for the last slot synchronization skip. This field is set only
       for logical slots that are being synced from a primary server (that is,
       those whose <structfield>synced</structfield> field is
       <literal>true</literal>).
```

What happens if the slot has a skip reason and the standby is promoted?
Will the attribute be retained? If so, do we have to add some notes like "sync"?

```
+/* Update slot sync skip stats */
+static void
+update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason skip_reason,
+                                                       bool acquire_slot)
```

Let's follow existing codes; ReplicationSlotSetInactiveSince(), third argument
can be `acquire_lock`.

```
+       /*
+        * Update the slot sync related stats in pg_stat_replication_slot when a
+        * slot sync is skipped
+        */
+       if (skip_reason != SS_SKIP_NONE)
+               pgstat_report_replslot_sync_skip(slot);
```

Is it OK to call pgstat_report_replslot_sync_skip() without any locks?

```
		ReplicationSlotAcquire(NameStr(slot->data.name), true, true);
```

Can you clarify the reason error_if_invalid=true? Other codes in the file use
error_if_invalid=false.

```
+       /* Update the slot sync skip reason */
+       SpinLockAcquire(&slot->mutex);
+       if (slot->slot_sync_skip_reason != skip_reason)
+               slot->slot_sync_skip_reason = skip_reason;
+       SpinLockRelease(&slot->mutex);
```

Now the replication slot can be always acquired. Do we still have to acquire the
spinlock even for reading the value? In other words, can we move SpinLockAcquire()
and SpinLockRelease() into inside the if block?

```
# Copyright (c) 2024-2025, PostgreSQL Global Development Group
```

I think 2024 can be removed.

```
my $primary = PostgreSQL::Test::Cluster->new('publisher');
```

s/publisher/primary/.

```
# Pause steaming replication connection so that standby can lag behind
unlink($primary->data_dir . '/pg_hba.conf');
$primary->append_conf(
	'pg_hba.conf', qq{
local   all             all                                     trust
host    all             all             127.0.0.1/32            trust
host    all             all             ::1/128                 trust
});
$primary->restart;
```

Not sure it can be called like "Pause". how about like:
```
Update pg_hba.conf and restart primar to reject streaming replication connections.
WAL records won't be replicated to the standby until .conf is restored.
```

```
# Attempt to sync replication slots while standby is behind
($result, $stdout, $stderr) =
  $standby->psql('postgres', "SELECT pg_sync_replication_slots();");
```

Can you verify the $stderr that synchornization was failed? I cannot find other
tests which checks the message. It is enough to do once.

```
$result = $standby->safe_psql(
	'postgres',
	"SELECT slot_sync_skip_reason FROM pg_replication_slots
     WHERE slot_name = 'slot_sync' AND synced AND NOT temporary"
);
is($result, 'missing_wal_record', "slot sync skip when standby is behind");
```

I found the test does twice; can we remove second one?

```
# Cleanup: drop the logical slot and ensure standby catches up
$primary->safe_psql('postgres',
	"SELECT pg_drop_replication_slot('slot_sync')");
$primary->wait_for_replay_catchup($standby);

$standby->safe_psql('postgres', "SELECT pg_sync_replication_slots();");

# Test for case when slot sync is skipped when the remote slot is
# behind the local slot.
$primary->safe_psql('postgres',
	"SELECT pg_create_logical_replication_slot('slot_sync', 'test_decoding', false, false, true)"
);
```

Can we use reset function instead of dropping it?


Best regards,
Hayato Kuroda
FUJITSU LIMITED

  33. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-03T09:44:10Z

    >
> I have attached the latest patch.
>

Thanks. I have started going through it.

I’m not sure if this has already been discussed; I couldn’t find any
mention of it in the thread. Why don’t we persist
'slot_sync_skip_reason' (it is outside of
ReplicationSlotPersistentData)? If a slot wasn’t synced during the
last cycle and the server restarts, it would be helpful to know the
reason it wasn’t synced prior to the node restart.

thanks
Shveta

  34. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-04T09:47:18Z

    On Mon, Nov 3, 2025 at 3:14 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> >
> > I have attached the latest patch.
> >
>
> Thanks. I have started going through it.
>
> I’m not sure if this has already been discussed; I couldn’t find any
> mention of it in the thread. Why don’t we persist
> 'slot_sync_skip_reason' (it is outside of
> ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> last cycle and the server restarts, it would be helpful to know the
> reason it wasn’t synced prior to the node restart.
>

Please find a few more comments:

1)
last_slot_sync_skip

Will 'last_slotsync_skip_at' be a better name?

Since we refer worker as slotsync in docs, I feel slotsync seems a
more natural choice than slot_sync. Also '_at' gives clarity that it
is about time rather than a boolean (which currently it seems like).

Same goes for slot_sync_skip_count and slot_sync_skip_reason. Shall
these be slotsync_skip_count and slotsync_skip_reason.

2)
+update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason
skip_reason,
+ bool acquire_slot)

It looks like there is only one caller that passes acquire_slot as
true, while all others pass false. Instead of keeping the acquire_slot
parameter, will it be better if we remove it and add an
Assert(MyReplication) to ensure the slot is already acquired? We can
add a comment stating that this function expects the slot to be
acquired by the caller. The one caller that currently passes
acquire_slot = true can acquire the slot explicitly before invoking
this function. Thoughts?

3)
In update_and_persist_local_synced_slot(), we get both
'found_consistent_snapshot' and 'remote_slot_precedes' from
update_local_synced_slot(). But skipsync-reason for
'remote_slot_precedes' is updated inside update_local_synced_slot()
while skipsync-reason for '!found_consistent_snapshot' is updated in
caller update_and_persist_local_synced_slot.  Is there a reason for
that?

4)
What about the case where the slot is invalidated and sync is skipped?
I do not see any stats for that. See 'Skip the sync of an invalidated
slot' in synchronize_one_slot(). If it is already discussed and
concluded, please add a comment.

thanks
Shveta

  35. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-05T06:19:28Z

    On Tue, Nov 4, 2025 at 3:17 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Nov 3, 2025 at 3:14 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > >
> > > I have attached the latest patch.
> > >
> >
> > Thanks. I have started going through it.
> >
> > I’m not sure if this has already been discussed; I couldn’t find any
> > mention of it in the thread. Why don’t we persist
> > 'slot_sync_skip_reason' (it is outside of
> > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > last cycle and the server restarts, it would be helpful to know the
> > reason it wasn’t synced prior to the node restart.
> >
>
> Please find a few more comments:
>
> 1)
> last_slot_sync_skip
>
> Will 'last_slotsync_skip_at' be a better name?
>
> Since we refer worker as slotsync in docs, I feel slotsync seems a
> more natural choice than slot_sync. Also '_at' gives clarity that it
> is about time rather than a boolean (which currently it seems like).
>
> Same goes for slot_sync_skip_count and slot_sync_skip_reason. Shall
> these be slotsync_skip_count and slotsync_skip_reason.
>
> 2)
> +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason
> skip_reason,
> + bool acquire_slot)
>
> It looks like there is only one caller that passes acquire_slot as
> true, while all others pass false. Instead of keeping the acquire_slot
> parameter, will it be better if we remove it and add an
> Assert(MyReplication) to ensure the slot is already acquired? We can
> add a comment stating that this function expects the slot to be
> acquired by the caller. The one caller that currently passes
> acquire_slot = true can acquire the slot explicitly before invoking
> this function. Thoughts?
>
> 3)
> In update_and_persist_local_synced_slot(), we get both
> 'found_consistent_snapshot' and 'remote_slot_precedes' from
> update_local_synced_slot(). But skipsync-reason for
> 'remote_slot_precedes' is updated inside update_local_synced_slot()
> while skipsync-reason for '!found_consistent_snapshot' is updated in
> caller update_and_persist_local_synced_slot.  Is there a reason for
> that?
>
> 4)
> What about the case where the slot is invalidated and sync is skipped?
> I do not see any stats for that. See 'Skip the sync of an invalidated
> slot' in synchronize_one_slot(). If it is already discussed and
> concluded, please add a comment.
>

Few more on 001:

5)
The name SS_SKIP_MISSING_WAL_RECORD doesn’t seem appropriate. It
sounds more like some WAL issue, rather than indicating that the WAL
hasn’t been flushed. A better name could be SS_SKIP_WAL_NOT_FLUSHED.

6)
Instead of calling 'update_slot_sync_skip_stats' at multiple places,
how about we just update the skip_reason everywhere and make a call to
'update_slot_sync_skip_stats' only in synchronize_one_slot(). IMO,
that will look cleaner. Thoughts?

thanks
Shveta

  36. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-12T10:55:32Z

    On Wed, 5 Nov 2025 at 11:49, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Nov 4, 2025 at 3:17 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Nov 3, 2025 at 3:14 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > >
> > > > I have attached the latest patch.
> > > >
> > >
> > > Thanks. I have started going through it.
> > >
> > > I’m not sure if this has already been discussed; I couldn’t find any
> > > mention of it in the thread. Why don’t we persist
> > > 'slot_sync_skip_reason' (it is outside of
> > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > last cycle and the server restarts, it would be helpful to know the
> > > reason it wasn’t synced prior to the node restart.
> > >
Actually I did not think in this direction. I think it will be useful
to persist 'slot_sync_skip_reason'. I have made the change for the
same in the latest patch.

> >
> > Please find a few more comments:
> >
> > 1)
> > last_slot_sync_skip
> >
> > Will 'last_slotsync_skip_at' be a better name?
> >
> > Since we refer worker as slotsync in docs, I feel slotsync seems a
> > more natural choice than slot_sync. Also '_at' gives clarity that it
> > is about time rather than a boolean (which currently it seems like).
> >
> > Same goes for slot_sync_skip_count and slot_sync_skip_reason. Shall
> > these be slotsync_skip_count and slotsync_skip_reason.
> >
Fixed it.

> > 2)
> > +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason
> > skip_reason,
> > + bool acquire_slot)
> >
> > It looks like there is only one caller that passes acquire_slot as
> > true, while all others pass false. Instead of keeping the acquire_slot
> > parameter, will it be better if we remove it and add an
> > Assert(MyReplication) to ensure the slot is already acquired? We can
> > add a comment stating that this function expects the slot to be
> > acquired by the caller. The one caller that currently passes
> > acquire_slot = true can acquire the slot explicitly before invoking
> > this function. Thoughts?
This idea looks good to me. I have updated the patch accordingly.

> >
> > 3)
> > In update_and_persist_local_synced_slot(), we get both
> > 'found_consistent_snapshot' and 'remote_slot_precedes' from
> > update_local_synced_slot(). But skipsync-reason for
> > 'remote_slot_precedes' is updated inside update_local_synced_slot()
> > while skipsync-reason for '!found_consistent_snapshot' is updated in
> > caller update_and_persist_local_synced_slot.  Is there a reason for
> > that?
> >
update_and_persist_local_synced_slot  is called when the synced slot
is in temporary state and we are calling function
'update_local_synced_slot'  directly for permanent slots.Slot sync
skip when "remote_slot_precedes" is true can happen for both permanent
and temporary slot. So I think we need to update the stats in
"update_local_synced_slot"
Whereas we are skipping slot sync only for temporary slots when a
consistent snapshot is not found. So I added this in the function
"update_and_persist_local_synced_slot".

> > 4)
> > What about the case where the slot is invalidated and sync is skipped?
> > I do not see any stats for that. See 'Skip the sync of an invalidated
> > slot' in synchronize_one_slot(). If it is already discussed and
> > concluded, please add a comment.
> >
It was not discussed earlier.
The pg_replication_slots already have a column name
'invalidation_reason'. And when the remote slot is invalidated the
local slot's is also invalidated. So, should we be required to
maintain this in 'slotsync_skip_reason' as well? I think it would be
kind of redundant? Thoughts?

>
> Few more on 001:
>
> 5)
> The name SS_SKIP_MISSING_WAL_RECORD doesn’t seem appropriate. It
> sounds more like some WAL issue, rather than indicating that the WAL
> hasn’t been flushed. A better name could be SS_SKIP_WAL_NOT_FLUSHED.
>
I think that the suggested name is better. I have updated the patch accordingly.

> 6)
> Instead of calling 'update_slot_sync_skip_stats' at multiple places,
> how about we just update the skip_reason everywhere and make a call to
> 'update_slot_sync_skip_stats' only in synchronize_one_slot(). IMO,
> that will look cleaner. Thoughts?
>
I tried this approach in [1] (see v2_approach2). This approach would
require passing extra parameters to the functions.
Here Amit suggested that we should try an approach where this can be
avoided. So, I came up with the current approach. See [2].

I have addressed the comments and attached the updated v7 patch.

[1]: https://www.postgresql.org/message-id/CANhcyEXHcdoRRo0N0uib-t7mfkbotv%3DaYjAWAekDAbHCRe%2BBng%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1KZLPxv7VBZf%3DBp9%3D-pzKNfvNmFDqFYYzwkowE4FpRs1A%40mail.gmail.com

Thanks,
Shlok Kyal

  37. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-12T10:57:56Z

    On Fri, 31 Oct 2025 at 11:30, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for updating the patch. Few comments:
>
> ```
>        The reason for the last slot synchronization skip. This field is set only
>        for logical slots that are being synced from a primary server (that is,
>        those whose <structfield>synced</structfield> field is
>        <literal>true</literal>).
> ```
>
> What happens if the slot has a skip reason and the standby is promoted?
> Will the attribute be retained? If so, do we have to add some notes like "sync"?
>
I tested it and I agree that slot sync skip stats will be retained. I
have updated the docs accordingly

> ```
> +/* Update slot sync skip stats */
> +static void
> +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason skip_reason,
> +                                                       bool acquire_slot)
> ```
>
> Let's follow existing codes; ReplicationSlotSetInactiveSince(), third argument
> can be `acquire_lock`.
>
I have updated it according to latest comment by Shveta in [1]

> ```
> +       /*
> +        * Update the slot sync related stats in pg_stat_replication_slot when a
> +        * slot sync is skipped
> +        */
> +       if (skip_reason != SS_SKIP_NONE)
> +               pgstat_report_replslot_sync_skip(slot);
> ```
>
> Is it OK to call pgstat_report_replslot_sync_skip() without any locks?
>
I think we need to acquire the slot before the call to
pgstat_report_replslot_sync_skip. This ensures
'pgstat_acquire_replslot' is called and the stats entry for the slot
is already present when we are updating the slot. Also in a similar
function 'pgstat_report_replslot', the comments says we should ensure
that pgstat_create_replslot() or pgstat_acquire_replslot() is called
before updating the stats.

> ```
>                 ReplicationSlotAcquire(NameStr(slot->data.name), true, true);
> ```
>
> Can you clarify the reason error_if_invalid=true? Other codes in the file use
> error_if_invalid=false.
>
I agree we should keep error_if_invalid=false because if
error_if_invalid=true and we try to acquire an invalidated slot an
error is thrown and it will exit the slotsync worker.

> ```
> +       /* Update the slot sync skip reason */
> +       SpinLockAcquire(&slot->mutex);
> +       if (slot->slot_sync_skip_reason != skip_reason)
> +               slot->slot_sync_skip_reason = skip_reason;
> +       SpinLockRelease(&slot->mutex);
> ```
>
> Now the replication slot can be always acquired. Do we still have to acquire the
> spinlock even for reading the value? In other words, can we move SpinLockAcquire()
> and SpinLockRelease() into inside the if block?
>
I agree that we use SpinLockAcquire() and SpinLockRelease() inside the
if block. I have fixed it.

> ```
> # Copyright (c) 2024-2025, PostgreSQL Global Development Group
> ```
>
> I think 2024 can be removed.
>
Fixed

> ```
> my $primary = PostgreSQL::Test::Cluster->new('publisher');
> ```
>
> s/publisher/primary/.
>
Fixed

> ```
> # Pause steaming replication connection so that standby can lag behind
> unlink($primary->data_dir . '/pg_hba.conf');
> $primary->append_conf(
>         'pg_hba.conf', qq{
> local   all             all                                     trust
> host    all             all             127.0.0.1/32            trust
> host    all             all             ::1/128                 trust
> });
> $primary->restart;
> ```
>
> Not sure it can be called like "Pause". how about like:
> ```
> Update pg_hba.conf and restart primar to reject streaming replication connections.
> WAL records won't be replicated to the standby until .conf is restored.
> ```
Fixed

> ```
> # Attempt to sync replication slots while standby is behind
> ($result, $stdout, $stderr) =
>   $standby->psql('postgres', "SELECT pg_sync_replication_slots();");
> ```
>
> Can you verify the $stderr that synchornization was failed? I cannot find other
> tests which checks the message. It is enough to do once.
Added

> ```
> $result = $standby->safe_psql(
>         'postgres',
>         "SELECT slot_sync_skip_reason FROM pg_replication_slots
>      WHERE slot_name = 'slot_sync' AND synced AND NOT temporary"
> );
> is($result, 'missing_wal_record', "slot sync skip when standby is behind");
> ```
>
> I found the test does twice; can we remove second one?
>
Fixed

> ```
> # Cleanup: drop the logical slot and ensure standby catches up
> $primary->safe_psql('postgres',
>         "SELECT pg_drop_replication_slot('slot_sync')");
> $primary->wait_for_replay_catchup($standby);
>
> $standby->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
>
> # Test for case when slot sync is skipped when the remote slot is
> # behind the local slot.
> $primary->safe_psql('postgres',
>         "SELECT pg_create_logical_replication_slot('slot_sync', 'test_decoding', false, false, true)"
> );
> ```
>
> Can we use reset function instead of dropping it?
>
Here the test checks 'remote_behind' case for first slot sync cycle
(when slot is in temporary slot). To achieve that we need to recreate
the slot.

I have addressed the comment and attached the updated patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEXEyUt8TycOqkgdWT%2BNeJASM%3D1acU1dK64UNsJeKMQFQA%40mail.gmail.com

Thanks,
Shlok Kyal

  38. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-11-14T08:43:04Z

    Dear Shlok,

Thanks for updating the patch. Few more comments.

> > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > mention of it in the thread. Why don’t we persist
> > > > 'slot_sync_skip_reason' (it is outside of
> > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > last cycle and the server restarts, it would be helpful to know the
> > > > reason it wasn’t synced prior to the node restart.
> > > >
> Actually I did not think in this direction. I think it will be useful
> to persist 'slot_sync_skip_reason'. I have made the change for the
> same in the latest patch.

Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
are essential to decode or replicate changes correctly, but sync status is not
used for the purpose. Personally considered, slot sync would re-start soon after
the reboot so that it is OK to start with empty. How about others?

If we want to serialize the info, we should do further tasks:
 - update SLOT_VERSION
 - make the slot dirty then SaveSlotToPath() when the status is updated.

```
+static void
+update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason skip_reason)
+{
+       Assert(MyReplicationSlot);
```

I think no need to require *slot as an argument. We can use the variable to shorten
like update_local_synced_slot().

```
# Verify pg_sync_replication_slots is failing
ok( $stderr =~ /skipping slot synchronization because the received slot sync/,
	'pg_sync_replication_slots failed as expected');
```

This may be matter of taste, but can you check whole of log message? Latter part
indicates the actual reason.

```
# Detach injection point
$standby->safe_psql(
	'postgres', q{
	SELECT injection_points_detach('slot-sync-skip');
	SELECT injection_points_wakeup('slot-sync-skip');
});
```

Not mandatory, but you can quit the background session if you release the
injection point.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  39. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-18T10:36:53Z

    On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for updating the patch. Few more comments.
>
> > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > mention of it in the thread. Why don’t we persist
> > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > reason it wasn’t synced prior to the node restart.
> > > > >
> > Actually I did not think in this direction. I think it will be useful
> > to persist 'slot_sync_skip_reason'. I have made the change for the
> > same in the latest patch.
>
> Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> are essential to decode or replicate changes correctly, but sync status is not
> used for the purpose. Personally considered, slot sync would re-start soon after
> the reboot so that it is OK to start with empty. How about others?
>
> If we want to serialize the info, we should do further tasks:
>  - update SLOT_VERSION
>  - make the slot dirty then SaveSlotToPath() when the status is updated.
>
I agree with your point. Slot synchronization will restart shortly
after a reboot, so it seems reasonable to begin with an empty state
rather than persisting slot_sync_skip_reason.
For now, I’ve updated the patch so that slot_sync_skip_reason is no
longer persisted; its initialization is kept outside of
ReplicationSlotPersistentData. I’d also like to hear what others
think.

> ```
> +static void
> +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason skip_reason)
> +{
> +       Assert(MyReplicationSlot);
> ```
>
> I think no need to require *slot as an argument. We can use the variable to shorten
> like update_local_synced_slot().
>
Fixed

> ```
> # Verify pg_sync_replication_slots is failing
> ok( $stderr =~ /skipping slot synchronization because the received slot sync/,
>         'pg_sync_replication_slots failed as expected');
> ```
>
> This may be matter of taste, but can you check whole of log message? Latter part
> indicates the actual reason.
>
The latter part of the message contains LSN values, which are not
stable across runs. To avoid hard-coding specific LSNs, I matched the
fixed, non-variable parts of the message while still covering the
reason for the failure.

> ```
> # Detach injection point
> $standby->safe_psql(
>         'postgres', q{
>         SELECT injection_points_detach('slot-sync-skip');
>         SELECT injection_points_wakeup('slot-sync-skip');
> });
> ```
>
> Not mandatory, but you can quit the background session if you release the
> injection point.
Fixed

Apart from the above changes. I have renamed the functions to
consistently use the term 'slotsync' instead of 'slot_sync'
update_slot_sync_skip_stats -> update_slotsync_skip_stats
pgstat_report_replslot_sync_skip -> pgstat_report_replslotsync_skip

I have attached the updated v8 patch with the latest changes.

Thanks,
Shlok Kyal

  40. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-21T03:22:20Z

    On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shlok,
> >
> > Thanks for updating the patch. Few more comments.
> >
> > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > mention of it in the thread. Why don’t we persist
> > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > reason it wasn’t synced prior to the node restart.
> > > > > >
> > > Actually I did not think in this direction. I think it will be useful
> > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > same in the latest patch.
> >
> > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > are essential to decode or replicate changes correctly, but sync status is not
> > used for the purpose. Personally considered, slot sync would re-start soon after
> > the reboot so that it is OK to start with empty. How about others?
> >
> > If we want to serialize the info, we should do further tasks:
> >  - update SLOT_VERSION
> >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> >
> I agree with your point. Slot synchronization will restart shortly
> after a reboot, so it seems reasonable to begin with an empty state
> rather than persisting slot_sync_skip_reason.
> For now, I’ve updated the patch so that slot_sync_skip_reason is no
> longer persisted; its initialization is kept outside of
> ReplicationSlotPersistentData. I’d also like to hear what others
> think.
>

Users may even use an API to synchronize the slots rather than
slotsync worker. In that case synchronization won't start immediately
after server-restart. Having said that, overall the statistical data
getting lost on restart doesn't seem right to me. But I would like to
know what others think.


> > ```
> > +static void
> > +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason skip_reason)
> > +{
> > +       Assert(MyReplicationSlot);
> > ```
> >
> > I think no need to require *slot as an argument. We can use the variable to shorten
> > like update_local_synced_slot().
> >
> Fixed
>
> > ```
> > # Verify pg_sync_replication_slots is failing
> > ok( $stderr =~ /skipping slot synchronization because the received slot sync/,
> >         'pg_sync_replication_slots failed as expected');
> > ```
> >
> > This may be matter of taste, but can you check whole of log message? Latter part
> > indicates the actual reason.
> >
> The latter part of the message contains LSN values, which are not
> stable across runs. To avoid hard-coding specific LSNs, I matched the
> fixed, non-variable parts of the message while still covering the
> reason for the failure.
>
> > ```
> > # Detach injection point
> > $standby->safe_psql(
> >         'postgres', q{
> >         SELECT injection_points_detach('slot-sync-skip');
> >         SELECT injection_points_wakeup('slot-sync-skip');
> > });
> > ```
> >
> > Not mandatory, but you can quit the background session if you release the
> > injection point.
> Fixed
>
> Apart from the above changes. I have renamed the functions to
> consistently use the term 'slotsync' instead of 'slot_sync'
> update_slot_sync_skip_stats -> update_slotsync_skip_stats
> pgstat_report_replslot_sync_skip -> pgstat_report_replslotsync_skip
>
> I have attached the updated v8 patch with the latest changes.
>
> Thanks,
> Shlok Kyal

  41. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-21T04:28:37Z

    On Fri, Nov 21, 2025 at 8:52 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Shlok,
> > >
> > > Thanks for updating the patch. Few more comments.
> > >
> > > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > > mention of it in the thread. Why don’t we persist
> > > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > > reason it wasn’t synced prior to the node restart.
> > > > > > >
> > > > Actually I did not think in this direction. I think it will be useful
> > > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > > same in the latest patch.
> > >
> > > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > > are essential to decode or replicate changes correctly, but sync status is not
> > > used for the purpose. Personally considered, slot sync would re-start soon after
> > > the reboot so that it is OK to start with empty. How about others?
> > >
> > > If we want to serialize the info, we should do further tasks:
> > >  - update SLOT_VERSION
> > >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> > >
> > I agree with your point. Slot synchronization will restart shortly
> > after a reboot, so it seems reasonable to begin with an empty state
> > rather than persisting slot_sync_skip_reason.
> > For now, I’ve updated the patch so that slot_sync_skip_reason is no
> > longer persisted; its initialization is kept outside of
> > ReplicationSlotPersistentData. I’d also like to hear what others
> > think.
> >
>
> Users may even use an API to synchronize the slots rather than
> slotsync worker. In that case synchronization won't start immediately
> after server-restart.
>

But I think after restart in most cases, the slot will be created
fresh as we persist the slot for the first time only when sync is
successful. Now, when the standby has not flushed the WAL
corresponding to remote_lsn (SS_SKIP_WAL_NOT_FLUSHED), the slotsync
can be skipped even for persisted slots but that should be rare and we
anyway won't be able to persist the slot_skip reason in other cases as
slot itself won't be persisted by that time. So, I feel keeping the
slot_sync_skip_reason in memory is sufficient.

-- 
With Regards,
Amit Kapila.

  42. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-21T05:30:36Z

    On Fri, Nov 21, 2025 at 9:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 8:52 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> > > <kuroda.hayato@fujitsu.com> wrote:
> > > >
> > > > Dear Shlok,
> > > >
> > > > Thanks for updating the patch. Few more comments.
> > > >
> > > > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > > > mention of it in the thread. Why don’t we persist
> > > > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > > > reason it wasn’t synced prior to the node restart.
> > > > > > > >
> > > > > Actually I did not think in this direction. I think it will be useful
> > > > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > > > same in the latest patch.
> > > >
> > > > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > > > are essential to decode or replicate changes correctly, but sync status is not
> > > > used for the purpose. Personally considered, slot sync would re-start soon after
> > > > the reboot so that it is OK to start with empty. How about others?
> > > >
> > > > If we want to serialize the info, we should do further tasks:
> > > >  - update SLOT_VERSION
> > > >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> > > >
> > > I agree with your point. Slot synchronization will restart shortly
> > > after a reboot, so it seems reasonable to begin with an empty state
> > > rather than persisting slot_sync_skip_reason.
> > > For now, I’ve updated the patch so that slot_sync_skip_reason is no
> > > longer persisted; its initialization is kept outside of
> > > ReplicationSlotPersistentData. I’d also like to hear what others
> > > think.
> > >
> >
> > Users may even use an API to synchronize the slots rather than
> > slotsync worker. In that case synchronization won't start immediately
> > after server-restart.
> >
>
> But I think after restart in most cases, the slot will be created
> fresh as we persist the slot for the first time only when sync is
> successful. Now, when the standby has not flushed the WAL
> corresponding to remote_lsn (SS_SKIP_WAL_NOT_FLUSHED), the slotsync
> can be skipped even for persisted slots but that should be rare and we
> anyway won't be able to persist the slot_skip reason in other cases as
> slot itself won't be persisted by that time. So, I feel keeping the
> slot_sync_skip_reason in memory is sufficient.
>

Okay, makes sense.

A few comments on 001:

1)
+        slots, but may (if leftover from a promotedstandby) contain a
timestamp.
promotedstandby --> promoted standby

2)
+            s.slotsync_skip_count,
+            s.last_slotsync_skip_at,

Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
way all slotsync related stats columns will have same prefix.

3)
+#include "utils/injection_point.h"

+ INJECTION_POINT("slot-sync-skip", NULL);

I think we can move both to patch 002 as these are needed for test alone.

4)
+
+ /*
+ * If found_consistent_snapshot is not NULL, a true value means
+ * the slot synchronization was successful, while a false value
+ * means it was skipped (see
+ * update_and_persist_local_synced_slot()). If
+ * found_consistent_snapshot is NULL, no such check exists, so the
+ * stats can be updated directly.
+ */
+ if (!found_consistent_snapshot || *found_consistent_snapshot)
+ update_slotsync_skip_stats(SS_SKIP_NONE);

I see that when 'found_consistent_snapshot' is true we update stats
here but when it is false, we update stats in the caller. Also for
'remote_slot_precedes' case, we update stats to SS_SKIP_REMOTE_BEHIND
here itself. I think  for 'SS_SKIP_NO_CONSISTENT_SNAPSHOT' as well, we
should update stats here instead of caller.

We can do this:
update_local_synced_slot()
{
skip_reason = none;

if (remote is behind)
  skip_reason = SS_SKIP_REMOTE_BEHIND;

if (found_consistent_snapshot && (*found_consistent_snapshot == false))
  skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;

--Later in this function, when syncing is done:
update_slotsync_skip_stats(skip_reason)
}

5)
+ if (synced)
+ {
+ ReplicationSlotAcquire(NameStr(slot->data.name), true, false);
+
+ if (slot->data.invalidated == RS_INVAL_NONE)
+ update_slotsync_skip_stats(SS_SKIP_WAL_NOT_FLUSHED);
+
+ ReplicationSlotRelease();
+ }

Shall we check 'slot->data.invalidated' along  with 'synced'
condition. That way, no need to acquire or release the slot if it is
invalidated. We can fetch 'invalidated' under the same SpinLock
itself.

6)
+ SS_SKIP_WAL_NOT_FLUSHED, /* Standby did not flush the wal coresponding
+ * to confirmed flush on remote slot */

on --> of
coresponding --> corresponding

thanks
Shveta

  43. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-21T06:00:25Z

    On Fri, Nov 21, 2025 at 11:00 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> A few comments on 001:
>
> 1)
> +        slots, but may (if leftover from a promotedstandby) contain a
> timestamp.
> promotedstandby --> promoted standby
>
> 2)
> +            s.slotsync_skip_count,
> +            s.last_slotsync_skip_at,
>
> Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> way all slotsync related stats columns will have same prefix.
>

Sounds reasonable especially when the doc explains that this is the
time at which last slot synchronization was skipped.

BTW, can we split the patch into two? First for slot sync skip stats,
and the second one for SlotSyncSkipReason? It would be easier to
review and commit that way.

-- 
With Regards,
Amit Kapila.

  44. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-21T10:11:44Z

    On Fri, 21 Nov 2025 at 11:00, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 9:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Nov 21, 2025 at 8:52 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > >
> > > > On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> > > > <kuroda.hayato@fujitsu.com> wrote:
> > > > >
> > > > > Dear Shlok,
> > > > >
> > > > > Thanks for updating the patch. Few more comments.
> > > > >
> > > > > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > > > > mention of it in the thread. Why don’t we persist
> > > > > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > > > > reason it wasn’t synced prior to the node restart.
> > > > > > > > >
> > > > > > Actually I did not think in this direction. I think it will be useful
> > > > > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > > > > same in the latest patch.
> > > > >
> > > > > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > > > > are essential to decode or replicate changes correctly, but sync status is not
> > > > > used for the purpose. Personally considered, slot sync would re-start soon after
> > > > > the reboot so that it is OK to start with empty. How about others?
> > > > >
> > > > > If we want to serialize the info, we should do further tasks:
> > > > >  - update SLOT_VERSION
> > > > >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> > > > >
> > > > I agree with your point. Slot synchronization will restart shortly
> > > > after a reboot, so it seems reasonable to begin with an empty state
> > > > rather than persisting slot_sync_skip_reason.
> > > > For now, I’ve updated the patch so that slot_sync_skip_reason is no
> > > > longer persisted; its initialization is kept outside of
> > > > ReplicationSlotPersistentData. I’d also like to hear what others
> > > > think.
> > > >
> > >
> > > Users may even use an API to synchronize the slots rather than
> > > slotsync worker. In that case synchronization won't start immediately
> > > after server-restart.
> > >
> >
> > But I think after restart in most cases, the slot will be created
> > fresh as we persist the slot for the first time only when sync is
> > successful. Now, when the standby has not flushed the WAL
> > corresponding to remote_lsn (SS_SKIP_WAL_NOT_FLUSHED), the slotsync
> > can be skipped even for persisted slots but that should be rare and we
> > anyway won't be able to persist the slot_skip reason in other cases as
> > slot itself won't be persisted by that time. So, I feel keeping the
> > slot_sync_skip_reason in memory is sufficient.
> >
>
> Okay, makes sense.
>
> A few comments on 001:
>
> 1)
> +        slots, but may (if leftover from a promotedstandby) contain a
> timestamp.
> promotedstandby --> promoted standby
>
> 2)
> +            s.slotsync_skip_count,
> +            s.last_slotsync_skip_at,
>
> Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> way all slotsync related stats columns will have same prefix.
>
> 3)
> +#include "utils/injection_point.h"
>
> + INJECTION_POINT("slot-sync-skip", NULL);
>
> I think we can move both to patch 002 as these are needed for test alone.
>
I have merged the test patch in the main patch and split the patch as
per Amit's suggestion in [1].
So this change will not be required.

> 4)
> +
> + /*
> + * If found_consistent_snapshot is not NULL, a true value means
> + * the slot synchronization was successful, while a false value
> + * means it was skipped (see
> + * update_and_persist_local_synced_slot()). If
> + * found_consistent_snapshot is NULL, no such check exists, so the
> + * stats can be updated directly.
> + */
> + if (!found_consistent_snapshot || *found_consistent_snapshot)
> + update_slotsync_skip_stats(SS_SKIP_NONE);
>
> I see that when 'found_consistent_snapshot' is true we update stats
> here but when it is false, we update stats in the caller. Also for
> 'remote_slot_precedes' case, we update stats to SS_SKIP_REMOTE_BEHIND
> here itself. I think  for 'SS_SKIP_NO_CONSISTENT_SNAPSHOT' as well, we
> should update stats here instead of caller.
>
> We can do this:
> update_local_synced_slot()
> {
> skip_reason = none;
>
> if (remote is behind)
>   skip_reason = SS_SKIP_REMOTE_BEHIND;
>
> if (found_consistent_snapshot && (*found_consistent_snapshot == false))
>   skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
>
> --Later in this function, when syncing is done:
> update_slotsync_skip_stats(skip_reason)
> }
>
I agree with your suggestion that we should update the stats for case
of 'SS_SKIP_NO_CONSISTENT_SNAPSHOT' in update_local_synced_slot
instead of update_and_persist_local_synced_slot. I have made the
change for the same.

For your second suggestion of using the 'skip_reason' variable instead.
For the 'if (remote is behind)' we are returning inside this if
condition itself. So for this case we need to call function
update_slotsync_skip_stats directly. But for case of
'found_consistent_snapshot' we can optimise it.

> 5)
> + if (synced)
> + {
> + ReplicationSlotAcquire(NameStr(slot->data.name), true, false);
> +
> + if (slot->data.invalidated == RS_INVAL_NONE)
> + update_slotsync_skip_stats(SS_SKIP_WAL_NOT_FLUSHED);
> +
> + ReplicationSlotRelease();
> + }
>
> Shall we check 'slot->data.invalidated' along  with 'synced'
> condition. That way, no need to acquire or release the slot if it is
> invalidated. We can fetch 'invalidated' under the same SpinLock
> itself.
>
Also I think to check 'slot->data.invalidated' we need to acquire the
slot due a similar race condition can occur as described in comments
below:
         * The slot has been synchronized before.
         *
         * It is important to acquire the slot here before checking
         * invalidation. If we don't acquire the slot first, there could be a
         * race condition that the local slot could be invalidated just after
         * checking the 'invalidated' flag here and we could end up
         * overwriting 'invalidated' flag to remote_slot's value. See
         * InvalidatePossiblyObsoleteSlot() where it invalidates slot directly
         * if the slot is not acquired by other processes.
         *

I thought about it and I agree with your suggestion in [2] to add a
new reason in slotsync_skip_reason to indicate the slot skip is
happening due to an invalidated slot.
I think it is cleaner and would avoid confusion for users. I made the
changes for the same in the latest patch.


> 6)
> + SS_SKIP_WAL_NOT_FLUSHED, /* Standby did not flush the wal coresponding
> + * to confirmed flush on remote slot */
>
> on --> of
> coresponding --> corresponding
>
I have also addressed the remaining comments. I have attached the
updated v9 patches.
[1]: https://www.postgresql.org/message-id/CAA4eK1JM%2BroQMyXekvwJprMMaK_-HL%2Bn5twinZQ8fufnDEU28g%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAJpy0uC9WsJhc-qeFmz_JTPjW1vH3Zm%2BzS6jX0PKTY6vtEp38w%40mail.gmail.com

Thanks,
Shlok Kyal

  45. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-21T10:12:48Z

    On Fri, 21 Nov 2025 at 09:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 8:52 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> > > <kuroda.hayato@fujitsu.com> wrote:
> > > >
> > > > Dear Shlok,
> > > >
> > > > Thanks for updating the patch. Few more comments.
> > > >
> > > > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > > > mention of it in the thread. Why don’t we persist
> > > > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > > > reason it wasn’t synced prior to the node restart.
> > > > > > > >
> > > > > Actually I did not think in this direction. I think it will be useful
> > > > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > > > same in the latest patch.
> > > >
> > > > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > > > are essential to decode or replicate changes correctly, but sync status is not
> > > > used for the purpose. Personally considered, slot sync would re-start soon after
> > > > the reboot so that it is OK to start with empty. How about others?
> > > >
> > > > If we want to serialize the info, we should do further tasks:
> > > >  - update SLOT_VERSION
> > > >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> > > >
> > > I agree with your point. Slot synchronization will restart shortly
> > > after a reboot, so it seems reasonable to begin with an empty state
> > > rather than persisting slot_sync_skip_reason.
> > > For now, I’ve updated the patch so that slot_sync_skip_reason is no
> > > longer persisted; its initialization is kept outside of
> > > ReplicationSlotPersistentData. I’d also like to hear what others
> > > think.
> > >
> >
> > Users may even use an API to synchronize the slots rather than
> > slotsync worker. In that case synchronization won't start immediately
> > after server-restart.
> >
>
> But I think after restart in most cases, the slot will be created
> fresh as we persist the slot for the first time only when sync is
> successful. Now, when the standby has not flushed the WAL
> corresponding to remote_lsn (SS_SKIP_WAL_NOT_FLUSHED), the slotsync
> can be skipped even for persisted slots but that should be rare and we
> anyway won't be able to persist the slot_skip reason in other cases as
> slot itself won't be persisted by that time. So, I feel keeping the
> slot_sync_skip_reason in memory is sufficient.
>
I agree.
I have added a comment why we decided to keep slotsync_skip_reason in_memory

Attached the latest patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEUiY4ENuoi7kZSsLJFLn6yA_-oPCKrek%3DBaMfFfY3%3DP1w%40mail.gmail.com

Thanks
Shlok Kyal

  46. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-21T10:13:00Z

    On Fri, 21 Nov 2025 at 11:30, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 11:00 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > A few comments on 001:
> >
> > 1)
> > +        slots, but may (if leftover from a promotedstandby) contain a
> > timestamp.
> > promotedstandby --> promoted standby
> >
> > 2)
> > +            s.slotsync_skip_count,
> > +            s.last_slotsync_skip_at,
> >
> > Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> > way all slotsync related stats columns will have same prefix.
> >
>
> Sounds reasonable especially when the doc explains that this is the
> time at which last slot synchronization was skipped.
>
Made the change in the latest patch.

> BTW, can we split the patch into two? First for slot sync skip stats,
> and the second one for SlotSyncSkipReason? It would be easier to
> review and commit that way.
>
I have split the patch into two:
0001 - Adds columns slotsync_skip_count and slotsync_last_skip_at in
pg_stats_replication_slots view
0002 - Adds column slotsync_skip_reason in pg_replication_slots view

Please find the latest patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEUiY4ENuoi7kZSsLJFLn6yA_-oPCKrek%3DBaMfFfY3%3DP1w%40mail.gmail.com

Thanks,
Shlok Kyal

  47. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-21T12:51:19Z

    On Fri, 21 Nov 2025 at 15:41, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Fri, 21 Nov 2025 at 11:00, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Nov 21, 2025 at 9:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Nov 21, 2025 at 8:52 AM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Tue, Nov 18, 2025 at 4:07 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > > >
> > > > > On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu)
> > > > > <kuroda.hayato@fujitsu.com> wrote:
> > > > > >
> > > > > > Dear Shlok,
> > > > > >
> > > > > > Thanks for updating the patch. Few more comments.
> > > > > >
> > > > > > > > > > I’m not sure if this has already been discussed; I couldn’t find any
> > > > > > > > > > mention of it in the thread. Why don’t we persist
> > > > > > > > > > 'slot_sync_skip_reason' (it is outside of
> > > > > > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the
> > > > > > > > > > last cycle and the server restarts, it would be helpful to know the
> > > > > > > > > > reason it wasn’t synced prior to the node restart.
> > > > > > > > > >
> > > > > > > Actually I did not think in this direction. I think it will be useful
> > > > > > > to persist 'slot_sync_skip_reason'. I have made the change for the
> > > > > > > same in the latest patch.
> > > > > >
> > > > > > Hmm, I'm wondering it should be written on the disk. Other attributes on the disk
> > > > > > are essential to decode or replicate changes correctly, but sync status is not
> > > > > > used for the purpose. Personally considered, slot sync would re-start soon after
> > > > > > the reboot so that it is OK to start with empty. How about others?
> > > > > >
> > > > > > If we want to serialize the info, we should do further tasks:
> > > > > >  - update SLOT_VERSION
> > > > > >  - make the slot dirty then SaveSlotToPath() when the status is updated.
> > > > > >
> > > > > I agree with your point. Slot synchronization will restart shortly
> > > > > after a reboot, so it seems reasonable to begin with an empty state
> > > > > rather than persisting slot_sync_skip_reason.
> > > > > For now, I’ve updated the patch so that slot_sync_skip_reason is no
> > > > > longer persisted; its initialization is kept outside of
> > > > > ReplicationSlotPersistentData. I’d also like to hear what others
> > > > > think.
> > > > >
> > > >
> > > > Users may even use an API to synchronize the slots rather than
> > > > slotsync worker. In that case synchronization won't start immediately
> > > > after server-restart.
> > > >
> > >
> > > But I think after restart in most cases, the slot will be created
> > > fresh as we persist the slot for the first time only when sync is
> > > successful. Now, when the standby has not flushed the WAL
> > > corresponding to remote_lsn (SS_SKIP_WAL_NOT_FLUSHED), the slotsync
> > > can be skipped even for persisted slots but that should be rare and we
> > > anyway won't be able to persist the slot_skip reason in other cases as
> > > slot itself won't be persisted by that time. So, I feel keeping the
> > > slot_sync_skip_reason in memory is sufficient.
> > >
> >
> > Okay, makes sense.
> >
> > A few comments on 001:
> >
> > 1)
> > +        slots, but may (if leftover from a promotedstandby) contain a
> > timestamp.
> > promotedstandby --> promoted standby
> >
> > 2)
> > +            s.slotsync_skip_count,
> > +            s.last_slotsync_skip_at,
> >
> > Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> > way all slotsync related stats columns will have same prefix.
> >
> > 3)
> > +#include "utils/injection_point.h"
> >
> > + INJECTION_POINT("slot-sync-skip", NULL);
> >
> > I think we can move both to patch 002 as these are needed for test alone.
> >
> I have merged the test patch in the main patch and split the patch as
> per Amit's suggestion in [1].
> So this change will not be required.
>
> > 4)
> > +
> > + /*
> > + * If found_consistent_snapshot is not NULL, a true value means
> > + * the slot synchronization was successful, while a false value
> > + * means it was skipped (see
> > + * update_and_persist_local_synced_slot()). If
> > + * found_consistent_snapshot is NULL, no such check exists, so the
> > + * stats can be updated directly.
> > + */
> > + if (!found_consistent_snapshot || *found_consistent_snapshot)
> > + update_slotsync_skip_stats(SS_SKIP_NONE);
> >
> > I see that when 'found_consistent_snapshot' is true we update stats
> > here but when it is false, we update stats in the caller. Also for
> > 'remote_slot_precedes' case, we update stats to SS_SKIP_REMOTE_BEHIND
> > here itself. I think  for 'SS_SKIP_NO_CONSISTENT_SNAPSHOT' as well, we
> > should update stats here instead of caller.
> >
> > We can do this:
> > update_local_synced_slot()
> > {
> > skip_reason = none;
> >
> > if (remote is behind)
> >   skip_reason = SS_SKIP_REMOTE_BEHIND;
> >
> > if (found_consistent_snapshot && (*found_consistent_snapshot == false))
> >   skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
> >
> > --Later in this function, when syncing is done:
> > update_slotsync_skip_stats(skip_reason)
> > }
> >
> I agree with your suggestion that we should update the stats for case
> of 'SS_SKIP_NO_CONSISTENT_SNAPSHOT' in update_local_synced_slot
> instead of update_and_persist_local_synced_slot. I have made the
> change for the same.
>
> For your second suggestion of using the 'skip_reason' variable instead.
> For the 'if (remote is behind)' we are returning inside this if
> condition itself. So for this case we need to call function
> update_slotsync_skip_stats directly. But for case of
> 'found_consistent_snapshot' we can optimise it.
>
> > 5)
> > + if (synced)
> > + {
> > + ReplicationSlotAcquire(NameStr(slot->data.name), true, false);
> > +
> > + if (slot->data.invalidated == RS_INVAL_NONE)
> > + update_slotsync_skip_stats(SS_SKIP_WAL_NOT_FLUSHED);
> > +
> > + ReplicationSlotRelease();
> > + }
> >
> > Shall we check 'slot->data.invalidated' along  with 'synced'
> > condition. That way, no need to acquire or release the slot if it is
> > invalidated. We can fetch 'invalidated' under the same SpinLock
> > itself.
> >
> Also I think to check 'slot->data.invalidated' we need to acquire the
> slot due a similar race condition can occur as described in comments
> below:
>          * The slot has been synchronized before.
>          *
>          * It is important to acquire the slot here before checking
>          * invalidation. If we don't acquire the slot first, there could be a
>          * race condition that the local slot could be invalidated just after
>          * checking the 'invalidated' flag here and we could end up
>          * overwriting 'invalidated' flag to remote_slot's value. See
>          * InvalidatePossiblyObsoleteSlot() where it invalidates slot directly
>          * if the slot is not acquired by other processes.
>          *
>
> I thought about it and I agree with your suggestion in [2] to add a
> new reason in slotsync_skip_reason to indicate the slot skip is
> happening due to an invalidated slot.
> I think it is cleaner and would avoid confusion for users. I made the
> changes for the same in the latest patch.
>
>
> > 6)
> > + SS_SKIP_WAL_NOT_FLUSHED, /* Standby did not flush the wal coresponding
> > + * to confirmed flush on remote slot */
> >
> > on --> of
> > coresponding --> corresponding
> >
> I have also addressed the remaining comments. I have attached the
> updated v9 patches.
> [1]: https://www.postgresql.org/message-id/CAA4eK1JM%2BroQMyXekvwJprMMaK_-HL%2Bn5twinZQ8fufnDEU28g%40mail.gmail.com
> [2]: https://www.postgresql.org/message-id/CAJpy0uC9WsJhc-qeFmz_JTPjW1vH3Zm%2BzS6jX0PKTY6vtEp38w%40mail.gmail.com
>
The Cbot complained that it was not able to build the docs. I have
fixed it and attached the latest patch.

Thanks
Shlok Kyal

  48. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-24T05:49:23Z

    On Fri, Nov 21, 2025 at 6:21 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> The Cbot complained that it was not able to build the docs. I have
> fixed it and attached the latest patch.
>

Few comments on 0001:
1.
+      <entry role="catalog_table_entry"><para role="column_definition">
+        <structfield>slotsync_last_skip_at</structfield><type>timestamp
with time zone</type>
+       </para>
+       <para>
+        Time at which last slot synchronization was skipped.

How important it is to use last in the above field? Isn't
slotsync_skip_at sufficient, as the description says all?

2.
+ /* If slot is present on the local, update the slot sync skip stats */
+ if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
+ {
+ bool synced;
+
+ SpinLockAcquire(&slot->mutex);
+ synced = slot->data.synced;
+ SpinLockRelease(&slot->mutex);
+
+ if (synced)
+ {
+ ReplicationSlotAcquire(NameStr(slot->data.name), true, false);
+
+ pgstat_report_replslotsync_skip(slot);
+
+ ReplicationSlotRelease();
+ }
+ }

I think acquiring the slot just for reporting the stats sounds too
much. How about instead moving the check if
(remote_slot->confirmed_lsn > latestFlushPtr) in the later if/else
blocks where we have the slot acquire acquired?

In if block, we can move it just before below check
if (slot->data.persistency == RS_TEMPORARY)
{
slot_updated = update_and_persist_local_synced_slot(remote_slot,

In the else block, just before following
update_and_persist_local_synced_slot(remote_slot, remote_dbid);

3.
We can only get here if pgstat_create_replslot() or
+ * pgstat_acquire_replslot() have already been called.
+ */
+void
+pgstat_report_replslotsync_skip(ReplicationSlot *slot)

3A. Instead of having such a comment, it is better to have elog(ERROR,
.. or Assertion if the stats entry doesn't exist by this time.
3B. Also, let's name the function as pgstat_report_replslotsync().

4. I think the current test used in the patch is very complex as it
requires multiple server restarts. Instead, we can use a test similar
to what is being used in the patch proposed in the thread [1].

[1] - https://www.postgresql.org/message-id/CAFPTHDYgcvZ60eHWUav3VQSeVibivx7A31rp_pFAkMQrW%3Dj%3D5A%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  49. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-24T05:53:56Z

    On Fri, Nov 21, 2025 at 6:21 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> The Cbot complained that it was not able to build the docs. I have
> fixed it and attached the latest patch.
>

Few comments on 001:

1)
In pgstat_report_replslotsync_skip(), shall we have a sanity check to
ensure that slot is logical and function is called on a
standby(RecoveryInProgress)?

2)
In update_and_persist_local_synced_slot(), we have comments at 3
places to indicate that stats are updated in some other
function.Instead, shall we have a generic comment in the header of
this function?

3)
Shall we have the test moved to the existing file
040_standby_failover_slots_sync?

4)
We should be able to make test work without injection point, please
try for that. Also it should be enough to test stats for one flow
instead of multiple flows.

thanks
Shveta

  50. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-25T08:24:56Z

    On Mon, Nov 24, 2025 at 11:24 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 6:21 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > The Cbot complained that it was not able to build the docs. I have
> > fixed it and attached the latest patch.
> >
>
> Few comments on 001:
>

Fixed these and other open comments on 0001 and pushed. But I see a BF
failure [1]. I think it is due to  the reason that the patch forgot to
release the slot in one of the code paths. I'll investigate it a bit
more and push the fix.

[1] - https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=scorpion&dt=2025-11-25%2007%3A38%3A19&stg=recovery-check

-- 
With Regards,
Amit Kapila.

  51. RE: How can end users know the cause of LR slot sync delays?

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-11-25T08:33:10Z

    On Tuesday, November 25, 2025 4:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Nov 24, 2025 at 11:24 AM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > On Fri, Nov 21, 2025 at 6:21 PM Shlok Kyal <shlok.kyal.oss@gmail.com>
> wrote:
> > >
> > > The Cbot complained that it was not able to build the docs. I have
> > > fixed it and attached the latest patch.
> > >
> >
> > Few comments on 001:
> >
> 
> Fixed these and other open comments on 0001 and pushed. But I see a BF
> failure [1]. I think it is due to  the reason that the patch forgot to release the
> slot in one of the code paths.

Right, I agree. Here is the patch to release the slot at necessary places.

Best Regards,
Hou zj

  52. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-11-25T10:30:11Z

    Dear Hou, Amit,

> Right, I agree. Here is the patch to release the slot at necessary places.

Thanks for working on it. However, BF machines have not satisfied the fix yet.
There are still two failures after 3df4df53b06 [1] [2].

The reported issue was that standby server failed to synchronize the slot after
the slot is re-created on the primary. According to [1], slots on standby has
newer catalog xmin than primary. Like:

```
LOG:  could not synchronize replication slot "lsub1_slot"
DETAIL:  Synchronization could lead to data loss, because the remote slot needs WAL at LSN 0/030163A8 and catalog xmin 758, but the standby has LSN 0/030163A8 and catalog xmin 759.
```

Per analysis, the newly created logical slot on primary has the initial catalog_xmin
as 758 due to the physical slot holding catalog_xmin:758. The standby does not
have slots, so the new slot will have the latest xid (759) as catalog_xmin.

Anyway, I think this is a test issue.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=scorpion&dt=2025-11-25%2009%3A03%3A17
[2]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=grassquit&dt=2025-11-25%2009%3A01%3A08

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  53. RE: How can end users know the cause of LR slot sync delays?

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-11-25T11:48:48Z

    On Tuesday, November 25, 2025 6:30 PM Kuroda, Hayato <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Hou, Amit,
> 
> > Right, I agree. Here is the patch to release the slot at necessary places.
> 
> Thanks for working on it. However, BF machines have not satisfied the fix yet.
> There are still two failures after 3df4df53b06 [1] [2].
> 
> The reported issue was that standby server failed to synchronize the slot after
> the slot is re-created on the primary. According to [1], slots on standby has
> newer catalog xmin than primary. Like:
> 
> ```
> LOG:  could not synchronize replication slot "lsub1_slot"
> DETAIL:  Synchronization could lead to data loss, because the remote slot
> needs WAL at LSN 0/030163A8 and catalog xmin 758, but the standby has
> LSN 0/030163A8 and catalog xmin 759.
> ```
> 
> Per analysis, the newly created logical slot on primary has the initial
> catalog_xmin as 758 due to the physical slot holding catalog_xmin:758. The
> standby does not have slots, so the new slot will have the latest xid (759) as
> catalog_xmin.
> 
> Anyway, I think this is a test issue.

The issue is that the physical slot on the primary retains a catalog_xmin of
758, causing newly created slots to inherit the same catalog_xmin. In contrast,
the standby, lacking slots, assigns an initial catalog_xmin of 759 to newly
synced slots. The problem arises because the logical slot on the primary isn't
being consumed, preventing the catalog_xmin from advancing, which leads to the
test timing out.

Previously, we avoided this issue by intentionally preventing xid assignment
during slotsync tests, ensuring xmin/catalog_xmin remained static in most cases.
However, the new test involves some DDLs in between tests causing this issue.
Rather than adding additional wait events for control, we discussed to relocate
the test to the end—after promoting the standby—where syncing the slot
successfully isn't necessary. Since the test's goal is solely to verify slotsync
skip statistics, this approach should suffice.

Here is the patch to modify the test.

> 
> [1]: https://buildfarm.postgresql.org/cgi-
> bin/show_log.pl?nm=scorpion&dt=2025-11-25%2009%3A03%3A17
> [2]: https://buildfarm.postgresql.org/cgi-
> bin/show_log.pl?nm=grassquit&dt=2025-11-25%2009%3A01%3A08

Best Regards,
Hou zj

  54. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-26T02:59:35Z

    On Tue, Nov 25, 2025 at 5:18 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the patch to modify the test.
>

+##################################################
+# Verify that slotsync skip statistics are correctly updated when the
+# slotsync operation is skipped.
+##################################################
....
+$standby2->reload;
+
+# Commit the pending prepared transaction
+$primary->safe_psql('postgres', "COMMIT PREPARED 'test_twophase_slotsync';");
+$primary->wait_for_replay_catchup($standby2);
+
+# Remove all logical replication slots on the primary server to ensure the
+# corresponding synced slots are also removed. This guarantees that the safest
+# catalog_xmin on the standby is not preserved by existing slots,
allowing newly
+# created slots to have a fresher initial catalog_xmin.
+$primary->psql('postgres', qq(
+ SELECT pg_drop_replication_slot('lsub1_slot');
+ SELECT pg_drop_replication_slot('snap_test_slot');
+));
+
+$subscriber2->safe_psql(
+ 'postgres', 'DROP SUBSCRIPTION regress_mysub2;');
+
+$standby2->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
+
+is( $standby2->safe_psql(
+ 'postgres',
+ q{SELECT count(*) = 0 FROM pg_replication_slots WHERE slot_name IN
('lsub1_slot', 'lsub2_slot', 'snap_test_slot');}
+ ),
+ "t",
+ 'synchronized slot has been dropped');

This is too much dependency of previous tests on the new one. We
should do cleanup of previous tests separately, if we want to use some
existing set up. Also, do we need to ensure that standby2's slots are
dropped? Did we ever sync slots on standby2? If so, cleaning up here
looks odd. This is to make new tests rely less on the outcome of
previous tests.

-- 
With Regards,
Amit Kapila.

  55. RE: How can end users know the cause of LR slot sync delays?

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-11-26T03:24:12Z

    On Wednesday, November 26, 2025 11:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Tue, Nov 25, 2025 at 5:18 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > Here is the patch to modify the test.
> >
> 
> +##################################################
> +# Verify that slotsync skip statistics are correctly updated when the #
> +slotsync operation is skipped.
> +##################################################
> ....
> +$standby2->reload;
> +
> +# Commit the pending prepared transaction
> +$primary->safe_psql('postgres', "COMMIT PREPARED
> +'test_twophase_slotsync';");
> +$primary->wait_for_replay_catchup($standby2);
> +
> +# Remove all logical replication slots on the primary server to ensure
> +the # corresponding synced slots are also removed. This guarantees that
> +the safest # catalog_xmin on the standby is not preserved by existing
> +slots,
> allowing newly
> +# created slots to have a fresher initial catalog_xmin.
> +$primary->psql('postgres', qq(
> + SELECT pg_drop_replication_slot('lsub1_slot');
> + SELECT pg_drop_replication_slot('snap_test_slot');
> +));
> +
> +$subscriber2->safe_psql(
> + 'postgres', 'DROP SUBSCRIPTION regress_mysub2;');
> +
> +$standby2->safe_psql('postgres', "SELECT
> +pg_sync_replication_slots();");
> +
> +is( $standby2->safe_psql(
> + 'postgres',
> + q{SELECT count(*) = 0 FROM pg_replication_slots WHERE slot_name IN
> ('lsub1_slot', 'lsub2_slot', 'snap_test_slot');}
> + ),
> + "t",
> + 'synchronized slot has been dropped');
> 
> This is too much dependency of previous tests on the new one. We should do
> cleanup of previous tests separately, if we want to use some existing set up.
> Also, do we need to ensure that standby2's slots are dropped? Did we ever
> sync slots on standby2? If so, cleaning up here looks odd. This is to make new
> tests rely less on the outcome of previous tests.

I think we did not sync slots to standby2, so I removed the checks for that.

I also adjusted the test in a way that it cleans up existing slots before starting
new tests.

Here is the updated version.

Best Regards,
Hou zj

  56. RE: How can end users know the cause of LR slot sync delays?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-11-26T03:30:02Z

    Dear Hou,

> I think we did not sync slots to standby2, so I removed the checks for that.
> 
> I also adjusted the test in a way that it cleans up existing slots before starting
> new tests.

Thanks for updating the patch. I confirmed on my env that your patch could be
applied cleanly and tests were passed. Pgperltidy say nothing for your patch.
Also, I preferred the current style.

I think it is worth checking on BF to see how they say.

  57. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-26T04:00:15Z

    On Wed, 26 Nov 2025 at 09:00, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Hou,
>
> > I think we did not sync slots to standby2, so I removed the checks for that.
> >
> > I also adjusted the test in a way that it cleans up existing slots before starting
> > new tests.
>
> Thanks for updating the patch. I confirmed on my env that your patch could be
> applied cleanly and tests were passed. Pgperltidy say nothing for your patch.
> Also, I preferred the current style.
>
> I think it is worth checking on BF to see how they say.
>

Thanks Amit for pushing the 0001 patch.
Thanks Hou-san and Kuroda-san on fixing the test.

I have rebased the 0002 patch on the current HEAD.

Thanks,
Shlok Kyal

  58. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-26T04:54:59Z

    On Wed, Nov 26, 2025 at 9:30 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Wed, 26 Nov 2025 at 09:00, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Hou,
> >
> > > I think we did not sync slots to standby2, so I removed the checks for that.
> > >
> > > I also adjusted the test in a way that it cleans up existing slots before starting
> > > new tests.
> >
> > Thanks for updating the patch. I confirmed on my env that your patch could be
> > applied cleanly and tests were passed. Pgperltidy say nothing for your patch.
> > Also, I preferred the current style.
> >
> > I think it is worth checking on BF to see how they say.
> >
>
> Thanks Amit for pushing the 0001 patch.
> Thanks Hou-san and Kuroda-san on fixing the test.
>
> I have rebased the 0002 patch on the current HEAD.
>

Thanks. Please find a few comments:

1)

+       The reason for the last slot synchronization skip. This field
is set only
+       for logical slots that are being synchronized from a primary
server (that
+       is, those whose <structfield>synced</structfield> field is
+       <literal>true</literal>). The value of this column has no meaning on the
+       primary server; it defaults to <literal>none</literal> for all
slots, but
+       may (if leftover from a promoted standby) also have a value other than
+       <literal>none</literal>. Possible values are:

We can make this similar to existing fields (slotsync_skip_count and
slotsync_skip_at):

Slot synchronization occurs only on standby servers and thus this column has
no meaning on the primary server.

2)
Doc on possible values of slotsync_skip_reason can be improved. Example

+          <literal>remote_behind</literal> means that the last slot
+          synchronization was skipped because the slot is ahead of the
+          corresponding failover slot on the primary.

We can get rid of 'last slot synchronization was skipped' from all the
reasons. (See 'invalidation_reason' possible values for reference, it
does not mention 'was invalidated' in any).

3)
+          <literal>wal_not_flushed</literal> means that the last slot
+          synchronization was skipped because the standby had not flushed the
+          WAL corresponding to the confirmed flush position on the remote slot.

I am not sure if we need to mention 'confirmed flush position'. Shall we say:

'.....because the standby had not flushed the WAL corresponding to the
position reserved on the remote slot'.
Thoughts?

4)
+          <literal>none</literal> means that the last slot synchronization
+          completed successfully.

Do we even need to mention 'none' in doc? 'invalidation_reason' does
not mention it.

5)
postgres=# select slot_name, invalidation_reason, slotsync_skip_reason
from pg_replication_slots;
   slot_name    | invalidation_reason | slotsync_skip_reason
----------------+---------------------+----------------------
 failover_slot2 |                     | none

Shall we keep 'slotsync_skip_reason' as NULL instead of 'none' similar
to invalidation_reason. Thoughts?

6)
If we plan to keep slotsync_skip_reason as NULL instead of 'none' for
non-skipped cases (above comment), then below code can be optimised as
we do not need to update 'none' as stats.
'skip_reason' and last update() call can then be removed and we can
simply call update_slotsync_skip_stats() instead of 'skip_reason =
SS_SKIP_NO_CONSISTENT_SNAPSHOT'.

update_local_synced_slot():

+ SlotSyncSkipReason skip_reason = SS_SKIP_NONE;
+ update_slotsync_skip_stats(SS_SKIP_REMOTE_BEHIND);
+ skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
+ /* Update slot sync skip stats */
+ update_slotsync_skip_stats(skip_reason);

7)

+static char *
+GetSlotSyncSkipReason(SlotSyncSkipReason reason)

We are passing 'SlotSyncSkipReason' and function name says
'GetSlotSyncSkipReason', looks confusing.
Shall we rename function name to GetSlotSyncSkipString or
GetSlotSyncSkipReasonName (similar to GetSlotInvalidationCauseName)

8)

+ * A slotsync skip typically occurs only for temporary slots. For
+ * persistent slots it is extremely rare (e.g., cases like
+ * SS_SKIP_WAL_NOT_FLUSHED or SS_SKIP_REMOTE_BEHIND). Also, temporary
+ * slots are dropped after server restart, so there is no value in
+ * persisting the slotsync_skip_reason.
+ */
+ SlotSyncSkipReason slotsync_skip_reason;

I feel, 'Also-->Since' will make more sense here.


9)
In doc for slotsync_skip_count and slotsync_skip_at

+ Slot
+        synchronization occur only on standby servers and thus this column has
+        no meaning on the primary server.

occur --> occurs

thanks
Shveta

  59. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-26T06:28:41Z

    On Wed, 26 Nov 2025 at 10:25, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Nov 26, 2025 at 9:30 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Wed, 26 Nov 2025 at 09:00, Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Hou,
> > >
> > > > I think we did not sync slots to standby2, so I removed the checks for that.
> > > >
> > > > I also adjusted the test in a way that it cleans up existing slots before starting
> > > > new tests.
> > >
> > > Thanks for updating the patch. I confirmed on my env that your patch could be
> > > applied cleanly and tests were passed. Pgperltidy say nothing for your patch.
> > > Also, I preferred the current style.
> > >
> > > I think it is worth checking on BF to see how they say.
> > >
> >
> > Thanks Amit for pushing the 0001 patch.
> > Thanks Hou-san and Kuroda-san on fixing the test.
> >
> > I have rebased the 0002 patch on the current HEAD.
> >
>
> Thanks. Please find a few comments:
>
> 1)
>
> +       The reason for the last slot synchronization skip. This field
> is set only
> +       for logical slots that are being synchronized from a primary
> server (that
> +       is, those whose <structfield>synced</structfield> field is
> +       <literal>true</literal>). The value of this column has no meaning on the
> +       primary server; it defaults to <literal>none</literal> for all
> slots, but
> +       may (if leftover from a promoted standby) also have a value other than
> +       <literal>none</literal>. Possible values are:
>
> We can make this similar to existing fields (slotsync_skip_count and
> slotsync_skip_at):
>
> Slot synchronization occurs only on standby servers and thus this column has
> no meaning on the primary server.
>
> 2)
> Doc on possible values of slotsync_skip_reason can be improved. Example
>
> +          <literal>remote_behind</literal> means that the last slot
> +          synchronization was skipped because the slot is ahead of the
> +          corresponding failover slot on the primary.
>
> We can get rid of 'last slot synchronization was skipped' from all the
> reasons. (See 'invalidation_reason' possible values for reference, it
> does not mention 'was invalidated' in any).
>
> 3)
> +          <literal>wal_not_flushed</literal> means that the last slot
> +          synchronization was skipped because the standby had not flushed the
> +          WAL corresponding to the confirmed flush position on the remote slot.
>
> I am not sure if we need to mention 'confirmed flush position'. Shall we say:
>
> '.....because the standby had not flushed the WAL corresponding to the
> position reserved on the remote slot'.
> Thoughts?
>
I think the suggested wording would be more clear to understand for
the user. Added the change.

> 4)
> +          <literal>none</literal> means that the last slot synchronization
> +          completed successfully.
>
> Do we even need to mention 'none' in doc? 'invalidation_reason' does
> not mention it.
>
> 5)
> postgres=# select slot_name, invalidation_reason, slotsync_skip_reason
> from pg_replication_slots;
>    slot_name    | invalidation_reason | slotsync_skip_reason
> ----------------+---------------------+----------------------
>  failover_slot2 |                     | none
>
> Shall we keep 'slotsync_skip_reason' as NULL instead of 'none' similar
> to invalidation_reason. Thoughts?
>
I agree with your suggestion. I have removed the 'none' value and used
NULL instead.

> 6)
> If we plan to keep slotsync_skip_reason as NULL instead of 'none' for
> non-skipped cases (above comment), then below code can be optimised as
> we do not need to update 'none' as stats.
> 'skip_reason' and last update() call can then be removed and we can
> simply call update_slotsync_skip_stats() instead of 'skip_reason =
> SS_SKIP_NO_CONSISTENT_SNAPSHOT'.
>
> update_local_synced_slot():
>
> + SlotSyncSkipReason skip_reason = SS_SKIP_NONE;
> + update_slotsync_skip_stats(SS_SKIP_REMOTE_BEHIND);
> + skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
> + /* Update slot sync skip stats */
> + update_slotsync_skip_stats(skip_reason);
>
I think we need this change even if we use NULL instead of 'none'.
This change ensures that the slot sync reason is set to NULL if slot
sync is successful.

> 7)
>
> +static char *
> +GetSlotSyncSkipReason(SlotSyncSkipReason reason)
>
> We are passing 'SlotSyncSkipReason' and function name says
> 'GetSlotSyncSkipReason', looks confusing.
> Shall we rename function name to GetSlotSyncSkipString or
> GetSlotSyncSkipReasonName (similar to GetSlotInvalidationCauseName)
>
> 8)
>
> + * A slotsync skip typically occurs only for temporary slots. For
> + * persistent slots it is extremely rare (e.g., cases like
> + * SS_SKIP_WAL_NOT_FLUSHED or SS_SKIP_REMOTE_BEHIND). Also, temporary
> + * slots are dropped after server restart, so there is no value in
> + * persisting the slotsync_skip_reason.
> + */
> + SlotSyncSkipReason slotsync_skip_reason;
>
> I feel, 'Also-->Since' will make more sense here.
>
>
> 9)
> In doc for slotsync_skip_count and slotsync_skip_at
>
> + Slot
> +        synchronization occur only on standby servers and thus this column has
> +        no meaning on the primary server.
>
> occur --> occurs

I have also addressed the remaining comments and attached the updated patch.

Thanks,
Shlok Kyal

  60. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-26T07:13:26Z

    On Wed, Nov 26, 2025 at 11:58 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
>
> I have also addressed the remaining comments and attached the updated patch.
>

Thanks. Please find a few comment:

1)
+       <structfield>slotsync_skip_reason</structfield><type>text</type>
+      </para>
+      <para>
+       The reason for the last slot synchronization skip. Slot synchronization
+       occurs only on standby servers and for synced slots (that is,
those whose
+       <structfield>synced</structfield> field is <literal>true</literal>).
+       Thus, this column has no meaning on the primary server and
slot which are
+       not synced. <literal>NULL</literal> if slot synchronization is not
+       performed yet or is successful. Possible values are:

Second line is confusing (..for synced slots..). Shall we rephrase the
complete thing to:

The reason for the last slot synchronization skip. Slot
synchronization occurs only on standby servers and thus this column
has no meaning on the primary server. It is relevant mainly for
logical slots on standby servers whose synced field is true. NULL if
slot synchronization is successful.

2)
Also in the 'possible values section', at one place we are using a
'failover slot on primary' and at another place 'remote slot'. We can
make these words also consistent. Shall we use 'failover slot on
primary' everywhere (as failover is a more widely used term than
remote in existing doc)?

3)
GetSlotSyncSkipReasonName()

+ default:
+ break;
+ }
+
+ Assert(false);
+ return NULL;

Shall we have :

  default:
    elog(ERROR, "unexpected slotsync skip reason: %d", (int) reason);
    return NULL;
  }
}

See other such functions: handle_streamed_transaction(),
RelationCreateStorage().

thanks
Shveta

  61. RE: How can end users know the cause of LR slot sync delays?

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-11-26T08:53:34Z

    On Wednesday, November 26, 2025 2:29 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> I have also addressed the remaining comments and attached the updated
> patch.

Thanks for updating the patch, I have few comments:

1.
+/*
+ * Map a SlotSyncSkipReason enum to a human-readable string
+ */
+static char *
+GetSlotSyncSkipReasonName(SlotSyncSkipReason reason)

Shall we add a static array to map the Enum value to the reason name
instead of adding the following function ?

2.
+          <literal>remote_behind</literal> means that the slot is ahead of the
+          corresponding failover slot on the primary.

I think the current naming and doc is not easy for user to understand. So, I
suggest mentioning the explicit reason of this skip, e.g., the required WALs and
rows are removed or at the risk of removal. We can rename this reason to
"wal_or_rows_removed" and make the document similar to the content in
logicaldecoding.sgml.

Best Regards,
Hou zj

  62. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-26T09:51:17Z

    On Wed, 26 Nov 2025 at 14:23, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, November 26, 2025 2:29 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > I have also addressed the remaining comments and attached the updated
> > patch.
>
> Thanks for updating the patch, I have few comments:
>
> 1.
> +/*
> + * Map a SlotSyncSkipReason enum to a human-readable string
> + */
> +static char *
> +GetSlotSyncSkipReasonName(SlotSyncSkipReason reason)
>
> Shall we add a static array to map the Enum value to the reason name
> instead of adding the following function ?
>
I think static array would be more clean and consistent with
ConflictTypeNames and SlotInvalidationCauses.
Made the changes for same.

> 2.
> +          <literal>remote_behind</literal> means that the slot is ahead of the
> +          corresponding failover slot on the primary.
>
> I think the current naming and doc is not easy for user to understand. So, I
> suggest mentioning the explicit reason of this skip, e.g., the required WALs and
> rows are removed or at the risk of removal. We can rename this reason to
> "wal_or_rows_removed" and make the document similar to the content in
> logicaldecoding.sgml.
>
I agree. Included the changes for same.

I have also addressed the comments by Shveta in [1].
[1]: https://www.postgresql.org/message-id/CAJpy0uDKC0QubC0pL%3DbZ4Qnq3eQbykLnFu5x%3DwmDkOmL44QL7g%40mail.gmail.com

Thanks,
Shlok Kyal

  63. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-27T03:55:23Z

    On Wed, 26 Nov 2025 at 15:21, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Wed, 26 Nov 2025 at 14:23, Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wednesday, November 26, 2025 2:29 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > I have also addressed the remaining comments and attached the updated
> > > patch.
> >
> > Thanks for updating the patch, I have few comments:
> >
> > 1.
> > +/*
> > + * Map a SlotSyncSkipReason enum to a human-readable string
> > + */
> > +static char *
> > +GetSlotSyncSkipReasonName(SlotSyncSkipReason reason)
> >
> > Shall we add a static array to map the Enum value to the reason name
> > instead of adding the following function ?
> >
> I think static array would be more clean and consistent with
> ConflictTypeNames and SlotInvalidationCauses.
> Made the changes for same.
>
> > 2.
> > +          <literal>remote_behind</literal> means that the slot is ahead of the
> > +          corresponding failover slot on the primary.
> >
> > I think the current naming and doc is not easy for user to understand. So, I
> > suggest mentioning the explicit reason of this skip, e.g., the required WALs and
> > rows are removed or at the risk of removal. We can rename this reason to
> > "wal_or_rows_removed" and make the document similar to the content in
> > logicaldecoding.sgml.
> >
> I agree. Included the changes for same.
>
> I have also addressed the comments by Shveta in [1].
> [1]: https://www.postgresql.org/message-id/CAJpy0uDKC0QubC0pL%3DbZ4Qnq3eQbykLnFu5x%3DwmDkOmL44QL7g%40mail.gmail.com

I have made some minor changes in documentation and comments. Attached
the updated patch.

Thanks,
Shlok Kyal

  64. Re: How can end users know the cause of LR slot sync delays?

    shveta malik <shveta.malik@gmail.com> — 2025-11-27T05:12:05Z

    On Thu, Nov 27, 2025 at 9:25 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> I have made some minor changes in documentation and comments. Attached
> the updated patch.
>

1)
<literal>NULL</literal> if
       slot synchronization is successful.

We shall add 'It is' to maintain continuity as the previous sentence has it.

2)
Related to previous patch: pgstat_report_replslotsync() currently has
sanity check Assert(SlotIsLogical(slot));
Instead, shall we have Assert(slot->synced)? It will implicitly ensure
that the slot is logical, plus it is important to check that we are
updating stats of synced slot only.

thanks
Shveta

  65. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-11-27T05:38:00Z

    On Thu, 27 Nov 2025 at 10:42, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Nov 27, 2025 at 9:25 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > I have made some minor changes in documentation and comments. Attached
> > the updated patch.
> >
>
> 1)
> <literal>NULL</literal> if
>        slot synchronization is successful.
>
> We shall add 'It is' to maintain continuity as the previous sentence has it.
>
> 2)
> Related to previous patch: pgstat_report_replslotsync() currently has
> sanity check Assert(SlotIsLogical(slot));
> Instead, shall we have Assert(slot->synced)? It will implicitly ensure
> that the slot is logical, plus it is important to check that we are
> updating stats of synced slot only.
>
Hi Shveta,

I have addressed the comments and attached the updated patch.

Thanks,
Shlok Kyal

  66. Re: How can end users know the cause of LR slot sync delays?

    Michael Banck <mbanck@gmx.net> — 2025-11-28T09:15:52Z

    Hi,

On Fri, Nov 21, 2025 at 11:00:36AM +0530, shveta malik wrote:
> 2)
> +            s.slotsync_skip_count,
> +            s.last_slotsync_skip_at,
> 
> Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> way all slotsync related stats columns will have same prefix.

I am not sure that was a great change. AFAICT, we only have _at once in
the catalog, and that if for two_phase_at. There, AIUI, it marks the
specific timestamp two_phase was enabled(?) for logical replication, not
the last time something happened.

So I think using _at here as well is confusing as this one is about the
last time a slotsync was skipped. I think it should be renamed to
'last', as per our usual naming.

I agree that having the same prefix would be nice, but it looks like
almost all other columns are named last_something (except for
checksum_last_failure, but that's been around for a long time).

So I suggest to reopen the discussion about naming this (second) column.


Michael

  67. Re: How can end users know the cause of LR slot sync delays?

    Michael Banck <mbanck@gmx.net> — 2025-11-28T09:29:10Z

    Hi,

On Thu, Nov 27, 2025 at 11:08:00AM +0530, Shlok Kyal wrote:
> I have addressed the comments and attached the updated patch.

Maybe it would be better to break out the slotsync parts of
pg_replication_slots into a new view? I think people are using
pg_replication_slots a lot for monitoring and keeping an eye on
replication and having it much wider makes it somewhat less useful.

Just a suggestion.


Micheal

  68. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-11-28T10:00:48Z

    On Fri, Nov 28, 2025 at 2:45 PM Michael Banck <mbanck@gmx.net> wrote:
>
> Hi,
>
> On Fri, Nov 21, 2025 at 11:00:36AM +0530, shveta malik wrote:
> > 2)
> > +            s.slotsync_skip_count,
> > +            s.last_slotsync_skip_at,
> >
> > Shall we rename last_slotsync_skip_at to slotsync_last_skip_at. That
> > way all slotsync related stats columns will have same prefix.
>
> I am not sure that was a great change. AFAICT, we only have _at once in
> the catalog, and that if for two_phase_at. There, AIUI, it marks the
> specific timestamp two_phase was enabled(?) for logical replication, not
> the last time something happened.
>
> So I think using _at here as well is confusing as this one is about the
> last time a slotsync was skipped. I think it should be renamed to
> 'last', as per our usual naming.
>
> I agree that having the same prefix would be nice, but it looks like
> almost all other columns are named last_something (except for
> checksum_last_failure, but that's been around for a long time).
>

I suggested the current one because having the last was making the
column name bit longer, and anyway the description clarifies it, but I
see your point. So, the other options could be
slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
last_sync_skip_time . The first two makes it easier to query similar
columns (slotsync related) together and the later two makes it similar
to existing ones in other views. The current one keeps the name
shorter and makes it easily queryable with other slotsync columns.

-- 
With Regards,
Amit Kapila.

  69. Re: How can end users know the cause of LR slot sync delays?

    Michael Banck <mbanck@gmx.net> — 2025-11-28T10:07:30Z

    Hi,

On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> I suggested the current one because having the last was making the
> column name bit longer, and anyway the description clarifies it, but I
> see your point. So, the other options could be
> slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> last_sync_skip_time . 

I also noticed while going through src/backend/catalog/system_views.sql
that *_last_*_time is rare, so in terms of brevity, removing the _time
at the end would be ok. "last_" already conveys time/a timestamp.


Michael

  70. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-03T05:28:41Z

    On Fri, Nov 28, 2025 at 3:37 PM Michael Banck <mbanck@gmx.net> wrote:
>
> Hi,
>
> On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> > I suggested the current one because having the last was making the
> > column name bit longer, and anyway the description clarifies it, but I
> > see your point. So, the other options could be
> > slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> > last_sync_skip_time .
>
> I also noticed while going through src/backend/catalog/system_views.sql
> that *_last_*_time is rare, so in terms of brevity, removing the _time
> at the end would be ok. "last_" already conveys time/a timestamp.
>

I think it depends on case to case but having last in the similar
cases seems to be a common practice. So, again thinking about it based
on your suggestion and looking at existing fields, I suggest we should
rename slotsync_skip_at to slotsync_last_skip. This is similar to
checksum_last_failure. I think there is a value in keeping initials
the same for similar fields in the view as users could easily identify
the related columns while querying the view. For example,
checksum_failures and checksum_last_failure in pg_stat_database.

Anyone else have any opinion on the names proposed here?

-- 
With Regards,
Amit Kapila.

  71. Re: How can end users know the cause of LR slot sync delays?

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-12-04T03:24:23Z

    On Wed, Dec 3, 2025 at 10:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 28, 2025 at 3:37 PM Michael Banck <mbanck@gmx.net> wrote:
> >
> > Hi,
> >
> > On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> > > I suggested the current one because having the last was making the
> > > column name bit longer, and anyway the description clarifies it, but I
> > > see your point. So, the other options could be
> > > slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> > > last_sync_skip_time .
> >
> > I also noticed while going through src/backend/catalog/system_views.sql
> > that *_last_*_time is rare, so in terms of brevity, removing the _time
> > at the end would be ok. "last_" already conveys time/a timestamp.
> >
>
> I think it depends on case to case but having last in the similar
> cases seems to be a common practice. So, again thinking about it based
> on your suggestion and looking at existing fields, I suggest we should
> rename slotsync_skip_at to slotsync_last_skip. This is similar to
> checksum_last_failure. I think there is a value in keeping initials
> the same for similar fields in the view as users could easily identify
> the related columns while querying the view. For example,
> checksum_failures and checksum_last_failure in pg_stat_database.
>
> Anyone else have any opinion on the names proposed here?

IMHO keeping it 'slotsync_last_skip' makes more sense, so that we can
keep the *slotsync* prefix and the naming style also match with some
other usage e.g. 'checksum_last_failure' as well.  I see there are
more common examples where the name starts with 'last_' but I prefer
the 'slotsync_last_skip' name.

-- 
Regards,
Dilip Kumar
Google

  72. Re: How can end users know the cause of LR slot sync delays?

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-12-04T06:36:44Z

    On Thu, 4 Dec 2025 at 08:54, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Dec 3, 2025 at 10:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Nov 28, 2025 at 3:37 PM Michael Banck <mbanck@gmx.net> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> > > > I suggested the current one because having the last was making the
> > > > column name bit longer, and anyway the description clarifies it, but I
> > > > see your point. So, the other options could be
> > > > slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> > > > last_sync_skip_time .
> > >
> > > I also noticed while going through src/backend/catalog/system_views.sql
> > > that *_last_*_time is rare, so in terms of brevity, removing the _time
> > > at the end would be ok. "last_" already conveys time/a timestamp.
> > >
> >
> > I think it depends on case to case but having last in the similar
> > cases seems to be a common practice. So, again thinking about it based
> > on your suggestion and looking at existing fields, I suggest we should
> > rename slotsync_skip_at to slotsync_last_skip. This is similar to
> > checksum_last_failure. I think there is a value in keeping initials
> > the same for similar fields in the view as users could easily identify
> > the related columns while querying the view. For example,
> > checksum_failures and checksum_last_failure in pg_stat_database.
> >
> > Anyone else have any opinion on the names proposed here?
>
> IMHO keeping it 'slotsync_last_skip' makes more sense, so that we can
> keep the *slotsync* prefix and the naming style also match with some
> other usage e.g. 'checksum_last_failure' as well.  I see there are
> more common examples where the name starts with 'last_' but I prefer
> the 'slotsync_last_skip' name.

I also agree with Amit and Dilip. I have made the change and attached
the patch for the same.

Thanks,
Shlok Kyal

  73. Re: How can end users know the cause of LR slot sync delays?

    Michael Banck <mbanck@gmx.net> — 2025-12-04T12:29:43Z

    Hi,

On Thu, Dec 04, 2025 at 12:06:44PM +0530, Shlok Kyal wrote:
> On Thu, 4 Dec 2025 at 08:54, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > On Wed, Dec 3, 2025 at 10:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Fri, Nov 28, 2025 at 3:37 PM Michael Banck <mbanck@gmx.net> wrote:
> > > > On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> > > > > I suggested the current one because having the last was making the
> > > > > column name bit longer, and anyway the description clarifies it, but I
> > > > > see your point. So, the other options could be
> > > > > slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> > > > > last_sync_skip_time .
> > > >
> > > > I also noticed while going through src/backend/catalog/system_views.sql
> > > > that *_last_*_time is rare, so in terms of brevity, removing the _time
> > > > at the end would be ok. "last_" already conveys time/a timestamp.
> > > >
> > >
> > > I think it depends on case to case but having last in the similar
> > > cases seems to be a common practice. So, again thinking about it based
> > > on your suggestion and looking at existing fields, I suggest we should
> > > rename slotsync_skip_at to slotsync_last_skip. This is similar to
> > > checksum_last_failure. I think there is a value in keeping initials
> > > the same for similar fields in the view as users could easily identify
> > > the related columns while querying the view. For example,
> > > checksum_failures and checksum_last_failure in pg_stat_database.
> > >
> > > Anyone else have any opinion on the names proposed here?
> >
> > IMHO keeping it 'slotsync_last_skip' makes more sense, so that we can
> > keep the *slotsync* prefix and the naming style also match with some
> > other usage e.g. 'checksum_last_failure' as well.  I see there are
> > more common examples where the name starts with 'last_' but I prefer
> > the 'slotsync_last_skip' name.
> 
> I also agree with Amit and Dilip. I have made the change and attached
> the patch for the same.

LGTM.


Michael

  74. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-05T09:08:56Z

    On Thu, Dec 4, 2025 at 5:59 PM Michael Banck <mbanck@gmx.net> wrote:
>
> On Thu, Dec 04, 2025 at 12:06:44PM +0530, Shlok Kyal wrote:
> > On Thu, 4 Dec 2025 at 08:54, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > On Wed, Dec 3, 2025 at 10:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > On Fri, Nov 28, 2025 at 3:37 PM Michael Banck <mbanck@gmx.net> wrote:
> > > > > On Fri, Nov 28, 2025 at 03:30:48PM +0530, Amit Kapila wrote:
> > > > > > I suggested the current one because having the last was making the
> > > > > > column name bit longer, and anyway the description clarifies it, but I
> > > > > > see your point. So, the other options could be
> > > > > > slotsync_last_skip_time, sync_last_skip_time, last_slotsync_skip_time,
> > > > > > last_sync_skip_time .
> > > > >
> > > > > I also noticed while going through src/backend/catalog/system_views.sql
> > > > > that *_last_*_time is rare, so in terms of brevity, removing the _time
> > > > > at the end would be ok. "last_" already conveys time/a timestamp.
> > > > >
> > > >
> > > > I think it depends on case to case but having last in the similar
> > > > cases seems to be a common practice. So, again thinking about it based
> > > > on your suggestion and looking at existing fields, I suggest we should
> > > > rename slotsync_skip_at to slotsync_last_skip. This is similar to
> > > > checksum_last_failure. I think there is a value in keeping initials
> > > > the same for similar fields in the view as users could easily identify
> > > > the related columns while querying the view. For example,
> > > > checksum_failures and checksum_last_failure in pg_stat_database.
> > > >
> > > > Anyone else have any opinion on the names proposed here?
> > >
> > > IMHO keeping it 'slotsync_last_skip' makes more sense, so that we can
> > > keep the *slotsync* prefix and the naming style also match with some
> > > other usage e.g. 'checksum_last_failure' as well.  I see there are
> > > more common examples where the name starts with 'last_' but I prefer
> > > the 'slotsync_last_skip' name.
> >
> > I also agree with Amit and Dilip. I have made the change and attached
> > the patch for the same.
>
> LGTM.
>

Pushed.

-- 
With Regards,
Amit Kapila.

  75. Re: How can end users know the cause of LR slot sync delays?

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-05T09:10:54Z

    On Fri, Nov 28, 2025 at 2:59 PM Michael Banck <mbanck@gmx.net> wrote:
>
> On Thu, Nov 27, 2025 at 11:08:00AM +0530, Shlok Kyal wrote:
> > I have addressed the comments and attached the updated patch.
>
> Maybe it would be better to break out the slotsync parts of
> pg_replication_slots into a new view?
>

We have another view pg_stat_replication_slots, for the statistics of
slots, yet another might not make sense at this stage. If we decide to
have more statistics/monitoring columns for slots that require some
separation, then we can consider moving this one as well.

-- 
With Regards,
Amit Kapila.