Обсуждение: How can end users know the cause of LR slot sync delays?

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
orvice versa, and the slot sync worker has to wait for one to catch up with the other. During this waiting period,
usersquerying 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
forthe delay. 
>
> Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions
tospeed 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.

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
standbyor 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
alreadysynchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the
reasonfor the delay. 
> >
> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate actions
tospeed 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

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
standbyor 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
alreadysynchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the
reasonfor the delay. 
> > >
> > > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate
actionsto 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.

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
standbyor 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
alreadysynchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the
reasonfor the delay. 
>> >
>> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate
actionsto 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_slotor update_and_persist_local_synced_slot. This value will can later be persisted in the
pg_replication_slotscatalog. 
>
> --
>
> 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

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
standbyor 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
alreadysynchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the
reasonfor the delay. 
>> >> >
>> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate
actionsto speed it up? 
>> >> >
>> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to
checkregularly. 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
inpg_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.

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
standbyor 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
alreadysynchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand the
reasonfor the delay. 
> >> >> >
> >> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate
actionsto speed it up? 
> >> >> >
> >> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to
checkregularly. 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
inpg_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.

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
failoverstandby or vice versa, and the slot sync worker has to wait for one to catch up with the other. During this
waitingperiod, users querying pg_replication_slots can only see whether the slot has been synchronized or not. If it
hasalready synchronized, that’s fine, but if synchronization is taking longer, users would naturally want to understand
thereason for the delay. 
> > >> >> >
> > >> >> > Is there a way for end users to know the cause of slot synchronization delays, so they can take appropriate
actionsto speed it up? 
> > >> >> >
> > >> >> > I understand that server logs are emitted in such cases, but logs are not something end users would want to
checkregularly. 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
storedin 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.

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.

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.

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

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

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.

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,

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.

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.

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.

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

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.

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

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

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

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

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

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

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

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

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

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

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

>
> 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

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

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

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

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

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