Subject: Re: Synchronizing slots from primary to standby

From: shveta malik <[email-masked]>
Date: Fri, 12 Jan 2024 17:49:57 +0530

On Fri, Jan 12, 2024 at 5:30 PM Masahiko Sawada <[email-masked]> wrote:
>
> On Thu, Jan 11, 2024 at 7:53 PM Amit Kapila <[email-masked]> wrote:
> >
> > On Tue, Jan 9, 2024 at 6:39 PM Amit Kapila <[email-masked]> wrote:
> > >
> > > +static bool
> > > +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot)
> > > {
> > > ...
> > > + /* Slot ready for sync, so sync it. */
> > > + else
> > > + {
> > > + /*
> > > + * Sanity check: With hot_standby_feedback enabled and
> > > + * invalidations handled appropriately as above, this should never
> > > + * happen.
> > > + */
> > > + if (remote_slot->restart_lsn < slot->data.restart_lsn)
> > > + elog(ERROR,
> > > + "cannot synchronize local slot \"%s\" LSN(%X/%X)"
> > > + " to remote slot's LSN(%X/%X) as synchronization"
> > > + " would move it backwards", remote_slot->name,
> > > + LSN_FORMAT_ARGS(slot->data.restart_lsn),
> > > + LSN_FORMAT_ARGS(remote_slot->restart_lsn));
> > > ...
> > > }
> > >
> > > I was thinking about the above code in the patch and as far as I can
> > > think this can only occur if the same name slot is re-created with
> > > prior restart_lsn after the existing slot is dropped. Normally, the
> > > newly created slot (with the same name) will have higher restart_lsn
> > > but one can mimic it by copying some older slot by using
> > > pg_copy_logical_replication_slot().
> > >
> > > I don't think as mentioned in comments even if hot_standby_feedback is
> > > temporarily set to off, the above shouldn't happen. It can only lead
> > > to invalidated slots on standby.
> > >
> > > To close the above race, I could think of the following ways:
> > > 1. Drop and re-create the slot.
> > > 2. Emit LOG/WARNING in this case and once remote_slot's LSN moves
> > > ahead of local_slot's LSN then we can update it; but as mentioned in
> > > your previous comment, we need to update all other fields as well. If
> > > we follow this then we probably need to have a check for catalog_xmin
> > > as well.
> > >
> >
> > The second point as mentioned is slightly misleading, so let me try to
> > rephrase it once again: Emit LOG/WARNING in this case and once
> > remote_slot's LSN moves ahead of local_slot's LSN then we can update
> > it; additionally, we need to update all other fields like two_phase as
> > well. If we follow this then we probably need to have a check for
> > catalog_xmin as well along remote_slot's restart_lsn.
> >
> > > Now, related to this the other case which needs some handling is what
> > > if the remote_slot's restart_lsn is greater than local_slot's
> > > restart_lsn but it is a re-created slot with the same name. In that
> > > case, I think the other properties like 'two_phase', 'plugin' could be
> > > different. So, is simply copying those sufficient or do we need to do
> > > something else as well?
> > >
> >
> > Bertrand, Dilip, Sawada-San, and others, please share your opinion on
> > this problem as I think it is important to handle this race condition.
>
> Is there any good use case of copying a failover slot in the first
> place? If it's not a normal use case and we can probably live without
> it, why not always disable failover during the copy? FYI we always
> disable two_phase on copied slots. It seems to me that copying a
> failover slot could lead to problems, as long as we synchronize slots
> based on their names. IIUC without the copy, this pass should never
> happen.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

There are multiple approaches discussed and tried when it comes to
starting a slot-sync worker. I am summarizing all here:

 1) Make slotsync worker as an Auxiliary Process (like checkpointer,
walwriter, walreceiver etc). The benefit this approach provides is, it
can control begin and stop in a more flexible way as each auxiliary
process could have different checks before starting and can have
different stop conditions. But it needs code duplication for process
management(start, stop, crash handling, signals etc) and currently it
does not support db-connection smoothly (none of the auxiliary process
has one so far)

We attempted to make slot-sync worker as an auxiliary process and
faced some challenges. The slot sync worker needs db-connection and
thus needs InitPostgres(). But AuxiliaryProcessMain() and
InitPostgres() are not compatible as both invoke common functions and
end up setting many callbacks functions twice (with different args).
Also InitPostgres() does 'MyBackendId' initialization (which further
triggers some stuff) which is not needed for AuxiliaryProcess and so
on. And thus in order to make slot-sync worker as an auxiliary
process, we need something similar to InitPostgres (trimmed down
working version) which needs further detailed analysis.

2) Make slotsync worker as a 'special' process like AutoVacLauncher
which is neither an Auxiliary process nor a bgworker one. It allows
db-connection and also provides flexibility to have start and stop
conditions for a process. But it needs a lot of code-duplication
around start, stop, fork (windows, non-windows), crash-management and
stuff.  It also needs to do many process-initialization stuff by
itself (which is otherwise done internally by Aux and bgworker infra).
And I am not sure if we should be adding a new process as a 'special'
one when postgres already provides bgworker and Auxiliary process
infrastructure.

3) Make slotysnc worker a bgworker. Here we just need to register our
process as a bgworker (RegisterBackgroundWorker()) by providing a
relevant start_time and restart_time and then the process management
is well taken care of. It does not need any code-duplication and
allows db-connection smoothly in registered process. The only thing it
lacks is that it does not provide flexibility of having
start-condition which then makes us to have 'enable_syncslot' as
PGC_POSTMASTER parameter rather than PGC_SIGHUP. Having said this, I
feel enable_syncslot is something which will not be changed frequently
and with the benefits provided by bgworker infra, it seems a
reasonably good option to choose this approach.

4) Another option is to have Logical Replication Launcher(or a new
process) to launch slot-sync worker. But going by the current design
where we have only 1 slotsync worker, it may be an overhead to have an
additional manager process maintained. Especially if we go by 'Logical
Replication Launcher', some extra changes will be needed there. It
will need start_time change from BgWorkerStart_RecoveryFinished to
BgWorkerStart_ConsistentState (doable but wanted to mention the
change). And provided the fact that 'Logical Replication Launcher'
does not have db-connection currently, in future if slotsync
validation-checks need to execute some sql query, it cannot do it
simply. It will either need the launcher to have db-connection or will
need new commands to be implemented for the same.

Thus weighing pros and cons of all these options, we have currently
implemented the bgworker approach (approach 3).  Any feedback is
welcome.

Thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Sat, 13 Jan 2024 12:53:50 +0530

> does not support db-connection smoothly (none of the auxiliary process

As slotsync worker needs to perform transactions and access syscache,
we can't make it an auxiliary process as that doesn't initialize the
required stuff like syscache. Also, see the comment "Auxiliary
processes don't run transactions ..." in AuxiliaryProcessMain() which
means this is not an option.

> conditions for a process.
>

Yeah, due to these reasons, I think this option is worth considering
and another plus point is that this allows us to make enable_syncslot
a PGC_SIGHUP GUC rather than a PGC_POSTMASTER.

> reasonably good option to choose this approach.

I agree but it may be better to make it a PGC_SIGHUP parameter.

> additional manager process maintained.
>

I don't see any good reason to have an additional launcher process here.

> implemented the bgworker approach (approach 3).  Any feedback is

I vote to go for (2) unless we face difficulties in doing so but (3)
is also okay especially if others also think so.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jan 2024 09:03:17 +0530

> is also okay especially if others also think so.

I am not against any of the approaches but I still feel that when we
have a standard way of doing things (bgworker) we should not keep
adding code to do things in a special way unless there is a strong
reason to do so. Now we need to decide if 'enable_syncslot' being
PGC_POSTMASTER is a strong reason to go the non-standard way? If yes,
then we should think of option 2 else option 3 seems better in my
understanding (which may be limited due to my short experience here),
so I am all ears to what others think on this.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Tue, 16 Jan 2024 09:37:37 +0530

On Tue, Jan 16, 2024 at 9:03 AM shveta malik <[email-masked]> wrote:
> PGC_POSTMASTER is a strong reason to go the non-standard way?
>

Agreed and as said earlier I think it is better to make it a
PGC_SIGHUP. Also, not sure we can say it is a non-standard way as
already autovacuum launcher is handled in the same way. One more minor
thing is it will save us for having a new bgworker state
BgWorkerStart_ConsistentState_HotStandby as introduced by this patch.

> If yes,

I also think it would be better if more people share their opinion on
this matter.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 16 Jan 2024 10:03:51 +0530

> BgWorkerStart_ConsistentState_HotStandby as introduced by this patch.

Yeah, it's not a nonstandard way.  But bgworker provides a lot of
inbuilt infrastructure which otherwise we would have to maintain by
ourselves if we opt for option 2.  I would have preferred option 3
from the simplicity point of view but I would prefer to make this
PGC_SIGHUP over simplicity.  But anyway, if there are issues in doing
so then we can keep it PGC_POSTMASTER but it's worth trying this out.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 16 Jan 2024 16:28:42 +0900

> BgWorkerStart_ConsistentState_HotStandby as introduced by this patch.

Why do we need to add a new BgWorkerStart_ConsistentState_HotStandby
for the slotsync worker? Isn't it sufficient that the slotsync worker
exits if not in hot standby mode?

Is there any technical difficulty or obstacle to make the slotsync
worker start using bgworker after reloading the config file?

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 16 Jan 2024 08:42:11 +0000

Hi,

> is also okay especially if others also think so.

Yeah, I think that (2) would be the "ideal" one but (3) is fine too. I think
that if we think/see that (2) is too "complicated"/long to implement maybe we
could do (3) initially and switch to (2) later. What I mean by that is that I
don't think that not doing (2) should be a blocker.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jan 2024 15:10:31 +0530

> exits if not in hot standby mode?

It is doable, but that will mean starting slot-sync worker even on
primary on every server restart which does not seem like a good idea.
We wanted to have a way where-in it does not start itself in
non-standby mode.

> worker start using bgworker after reloading the config file?

When we register slotsync worker as bgworker, we can only register the
bgworker before initializing shared memory, we cannot register
dynamically in the cycle of ServerLoop and thus we do not have
flexibility of registering/deregistering the bgworker  (or controlling
the bgworker start) based on config parameters each time they change.
We can always start slot-sync worker and let it check if
enable_syncslot is ON. If not, exit and retry the next time when
postmaster will restart it after restart_time(60sec). The downside of
this approach is, even if any user does not want slot-sync
functionality and thus has permanently disabled 'enable_syncslot', it
will keep on restarting and exiting there.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jan 2024 17:27:05 +0530

> will keep on restarting and exiting there.

PFA v62. Details:

v62-001: No change.

v62-002:
1) Addressed slotsync.c related comments by Peter in [1].
2) Addressed CFBot failure where there was a crash in 32 bit env while
accessing DatumGetLSN
3) Addressed another CFBot failure where the test for
'050_standby_failover_slots_sync.pl' was hanging. Thanks Hou-San for
this fix.

v62-003:
It is a new patch which attempts to implement slot-sync worker as a
special process which is neither a bgworker nor an Auxiliary process.
Here we get the benefit of converting enable_syncslot to a PGC_SIGHUP
Guc rather than PGC_POSTMASTER. We launch the slot-sync worker only if
it is hot-standby and 'enable_syncslot' is ON.

v62-004:
Small change in document.

v62-005: No change

v62-006:
Separated the failover-ready validation steps into this separate
doc-patch (which were earlier present in v61-002 and v61-003). Also
addressed some of the doc comments by Peter in [1].
Thanks Hou-San for providing this patch.

[1]: https://www.postgresql.org/message-id/CAHut%2BPteZVNx1jQ6Hs3mEdoC%3DDNALVpJJ2mZDYim7sU-04tiaw%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jan 2024 17:32:20 +0530

> is also okay especially if others also think so.

Okay. Attempted approach 2 as a separate patch in v62-0003. Approach 3
(bgworker) is still  maintained in v62-002.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jan 2024 11:42:01 +1100

About v62-0001:

As stated in the patch comment:
But note that this commit does not yet include the capability to
actually sync the replication slot; the next patch will address that.

~~~

Because of this, I think it might be prudent to separate the
documentation portion from this patch so that it can be pushed later
when the actual synchronize capability also gets pushed.

It would not be good for the PG documentation on HEAD to be describing
behaviour that does not yet exist. (e.g. if patch 0001 is pushed
early, but then there is some delay or problems getting the subsequent
patches committed).

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 17 Jan 2024 10:13:19 +0900

> We wanted to have a way where-in it does not start itself in

Understood.

Another idea would be that the startup process dynamically registers
the slotsync worker if hot_standby is enabled. But it doesn't seem
like the right approach.

> will keep on restarting and exiting there.

Thanks for the explanation. It sounds like it's not impossible but
would require some work. If allowing bgworkers to start also on SIGUP
is a general improvement, we can implement it later while having
enable_syncslot PGC_POSTMASTER at this time. Then, we will be able to
make the enable_syncslot PGC_SIGUP later.

BTW I think I found a race condition in the v61 patch to cause that
the slotsync worker continues working even after promotion (I've not
tested with v62 patch though). At the time when the startup shutdown
the slotsync worker in FinishWalRecovery(), the postmaster's pmState
is still PM_HOT_STANDBY. And if the slotsync worker is not running
when the startup process attempts to shutdown it, ShutDownSlotSync()
does nothing. Therefore, after the startup process doesn't shutdown
the slotsync worker, the postmaster could relaunch the slotsync worker
before its state transition to PM_RUN.

Regards,

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jan 2024 15:33:10 +1100

Here is a  review comment for the latest v62-0002 changes.

======
src/backend/replication/logical/slotsync.c

1.
+ if (namestrcmp(&slot->data.plugin, remote_slot->plugin) == 0 &&
+ slot->data.database == dbid &&
+ remote_slot->restart_lsn == slot->data.restart_lsn &&
+ remote_slot->catalog_xmin == slot->data.catalog_xmin &&
+ remote_slot->two_phase == slot->data.two_phase &&
+ remote_slot->failover == slot->data.failover &&
+ remote_slot->confirmed_lsn == slot->data.confirmed_flush)
+ return false;

For consistency, I think it would be better to always code the remote
slot value on the LHS and the local slot value on the RHS, instead of
the current random mix.

And rename 'dbid' to 'remote_dbid' for name consistency too.

SUGGESTION
if (namestrcmp(remote_slot->plugin, &slot->data.plugin) == 0 &&
  remote_dbid == slot->data.database &&
  remote_slot->restart_lsn == slot->data.restart_lsn &&
  remote_slot->catalog_xmin == slot->data.catalog_xmin &&
  remote_slot->two_phase == slot->data.two_phase &&
  remote_slot->failover == slot->data.failover &&
  remote_slot->confirmed_lsn == slot->data.confirmed_flush)
  return false;

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jan 2024 15:37:14 +1100

On Tue, Jan 16, 2024 at 10:57 PM shveta malik <[email-masked]> wrote:
...
> [1]: https://www.postgresql.org/message-id/CAHut%2BPteZVNx1jQ6Hs3mEdoC%3DDNALVpJJ2mZDYim7sU-04tiaw%40mail.gmail.com

Thanks for addressing my previous review in the new patch 0006. I
checked it again and below are a few more comments

======
1. GENERAL

I was wondering if some other documentation (like somewhere from
chapter 27, or maybe the pgctl promote docs?) should be referring back
to this new information about how to decide if the standby is ready
for promotion.

======
doc/src/sgml/logical-replication.sgml

2.
+
+  <para>
+   Because the slot synchronization logic copies asynchronously, it is
+   necessary to confirm that replication slots have been synced to the standby
+   server before the failover happens. Furthermore, to ensure a successful
+   failover, the standby server must not be lagging behind the subscriber. It
+   is highly recommended to use <varname>standby_slot_names</varname> to
+   prevent the subscriber from consuming changes faster than the hot standby.
+   To confirm that the standby server is indeed ready for failover, follow
+   these 2 steps:
+  </para>

For easier navigation, perhaps that standby_slot_names should include
a link back to where the standby_slot_names GUC is described.

~~~

3.
+    <substeps>
+     <step performance="required">
+      <para>
+       Firstly, on the subscriber node, use the following SQL to identify the
+       slot names that should be synced to the standby that we plan to promote.

Minor change to wording.

SUGGESTION
Firstly, on the subscriber node, use the following SQL to identify
which slots should be synced to the standby that we plan to promote.

~~~

4.
+<programlisting>
+test_sub=# SELECT
+               array_agg(slotname) AS slots
+           FROM
+           ((
+               SELECT r.srsubid AS subid, CONCAT('pg_' || srsubid ||
'_sync_' || srrelid || '_' || ctl.system_identifier) AS slotname
+               FROM pg_control_system() ctl, pg_subscription_rel r,
pg_subscription s
+               WHERE r.srsubstate = 'f' AND s.oid = r.srsubid AND s.subfailover
+           ) UNION (
+               SELECT s.oid AS subid, s.subslotname as slotname
+               FROM pg_subscription s
+               WHERE s.subfailover
+           ));
+ slots
+-------
+ {sub}
+(1 row)
+</programlisting></para>
+     </step>

I think the example might be better if the result shows > 1 slot.  e.g.
{sub1,sub2,sub3}

This would also make the next step 1.b. more clear.

~~~

5.
+</programlisting></para>
+     </step>
+     <step performance="required">
+      <para>
+       Next, check that the logical replication slots identified above exist on
+       the standby server. This step can be skipped if
+       <varname>standby_slot_names</varname> has been correctly configured.
+<programlisting>
+test_standby=# SELECT bool_and(synced AND NOT temporary AND
conflict_reason IS NULL) AS failover_ready
+               FROM pg_replication_slots
+               WHERE slot_name in ('sub');
+ failover_ready
+----------------
+ t
+(1 row)
+</programlisting></para>

5a.

(uppercase SQL keyword)

/in/IN/

~

5b.
I felt this might be easier to understand if the SQL gives a
two-column result instead of one all-of-nothing T/F where you might no
be sure which slot was the one giving a problem. e.g.

failover_ready | slot
---------------------
t              | sub1
t              | sub2
f              | sub3
...

~~~

6.
+       <para>
+        Firstly, on the subscriber node check the last replayed WAL. If the
+        query result is NULL, it indicates that the subscriber has not yet
+        replayed any WAL. Therefore, the next step can be skipped, as the
+        standby server must be ahead of the subscriber.

IMO all of that part "If the query result is NULL" does not really
belong here because it describes skipping the *next* step. So, it
would be better to say this in the next step. Something like:

SUGGESTION (for step 2b)
Next, on the standby server check that the last-received WAL location
is ahead of the replayed WAL location on the subscriber identified
above. If the above SQL result was NULL, it means the subscriber has
not yet replayed any WAL, so the standby server must be ahead of the
subscriber, and this step can be skipped.

~~~

7.
+<programlisting>
+test_sub=# SELECT
+               MAX(remote_lsn) AS remote_lsn_on_subscriber
+           FROM
+           ((
+               SELECT (CASE WHEN r.srsubstate = 'f' THEN
pg_replication_origin_progress(CONCAT('pg_' || r.srsubid || '_' ||
r.srrelid), false)
+                           WHEN r.srsubstate IN ('s', 'r') THEN
r.srsublsn END) as remote_lsn
+               FROM pg_subscription_rel r, pg_subscription s
+               WHERE r.srsubstate IN ('f', 's', 'r') AND s.oid =
r.srsubid AND s.subfailover
+           ) UNION (
+               SELECT pg_replication_origin_progress(CONCAT('pg_' ||
s.oid), false) AS remote_lsn
+               FROM pg_subscription s
+               WHERE subfailover
+           ));
+ remote_lsn_on_subscriber
+--------------------------
+ 0/3000388
+</programlisting></para>

7a.
(uppercase SQL keyword)

/as/AS/

~

7b.
missing table alias

/WHERE subfailover/WHERE s.subfailover/

~~~

8.
+    </step>
+    <step performance="required">
+     <para>
+      Next, on the standby server check that the last-received WAL location
+      is ahead of the replayed WAL location on the subscriber identified above.

See the review comment above (#6) which suggested adding some more info here.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Wed, 17 Jan 2024 10:18:46 +0530

> before its state transition to PM_RUN.

Yes, this race condition exists. We have attempted to fix this
race-condition in v62-003 by introducing 'stopSignaled' bool in
slot-sync worker shared memory. The startup process will set it to
true before shutting slotsync worker and if meanwhile postmaster ends
up restarting it, ReplSlotSyncWorkerMain() will exit seeing
'stopSignaled' set. This is on similar line of WalReceiver where
postmaster starts it and startup process shuts it down and it handles
similar race condition by having state-machinery in place (see
WALRCV_STOPPING, WALRCV_STOPPED in ShutdownWalRcv() and
WalReceiverMain()).

Having said above, I feel in slotysnc worker case:
--I need to pull that race-condition fix around 'stopSignaled' to
patch002 instead.
--Also I feel in ShutDownSlotSync(), I need to move setting
'stopSignaled' to true before we exit on finding 'SlotSyncWorker->pid'
is InvalidPid. This will take care of that scenario too, where there
was no slot-sync worker present and startup process tried to shut it
down. We need this flag set in that corner-case too.

thanks
Shveta

---

From: Nisha Moond <[email-masked]>
Date: Wed, 17 Jan 2024 10:41:42 +0530

A review on v62-006: failover-ready validation steps doc -

This query does not ensure that all the logical replication slots
exist on standby. Due to the 'IN ('slots')' check, it will return
'true' even if only one or a few slots exist.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 17 Jan 2024 09:38:45 +0000

Hi,

> PFA v62. Details:

Thanks! 

> it is hot-standby and 'enable_syncslot' is ON.

The implementation looks reasonable to me (from what I can see some parts is
copy/paste from an already existing "special" process and some parts are
"sync slot" specific) which makes fully sense.

A few remarks:

1 ===
+                * Was it the slot sycn worker?

Typo: sycn

2 ===
+                * ones), and no walwriter, autovac launcher or bgwriter or slot sync

Instead? "* ones), and no walwriter, autovac launcher, bgwriter or slot sync"

3 ===
+ * restarting slot slyc worker. If stopSignaled is set, the worker will

Typo: slyc

4 ===
+/* Flag to tell if we are in an slot sync worker process */

s/an/a/ ?

5 === (coming from v62-0002)
+       Assert(tuplestore_tuple_count(res->tuplestore) == 1);

Is it even possible for the related query to not return only one row? (I think the 
"count" ensures it).

6 ===
        if (conninfo_changed ||
                primary_slotname_changed ||
+               old_enable_syncslot != enable_syncslot ||
                (old_hot_standby_feedback != hot_standby_feedback))
        {
                ereport(LOG,
                                errmsg("slot sync worker will restart because of"
                                           " a parameter change"));

I don't think "slot sync worker will restart" is true if one change enable_syncslot
from on to off.

IMHO, v62-003 is in good shape and could be merged in v62-002 (that would ease
the review). But let's wait to see if others think differently.

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 17 Jan 2024 16:00:17 +0530

> the review). But let's wait to see if others think differently.

PFA v63.

--It addresses comments by Peter given in [1], [2], comment by Nisha
given in [3], comments by Bertrand given in [4]
--It also moves race-condition fix from patch003 to patch002 as
suggested by Swada-san offlist. Race-condition is mentioned in [5]

All the changes are in patch02, patch003 and patch006.

[1]: https://www.postgresql.org/message-id/CAHut%2BPuECB8fNBfXMdTHSMKF9kL%3D0XqPw1Am4NVahfJSSHzoYg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPt0uum%2B6Hg5UDofWMEJWhVEyArM1b0_B94UJmRcQmz7DA%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CABdArM73qdHyA0nteDLAQrfKNHRP%2B5Qq6p8uobg5bkE3EWiC%2Bg%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/ZaegJe9JpUiQeV%2BD%40ip-10-97-1-34.eu-west-3.compute.internal
[5]: https://www.postgresql.org/message-id/CAD21AoA5izeKpp9Ei4Cd745pKX3wn-TRvhhmPFEW9UY1nx%2B_aw%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 17 Jan 2024 16:06:19 +0530

> "sync slot" specific) which makes fully sense.

Thanks for the feedback. I have addressed the comments in v63 except 5th one.

> Is it even possible for the related query to not return only one row? (I think the

I think you are right. This assertion was added sometime back on the
basis of feedback on hackers. Let me review that again. I can consider
this comment in the next version.

> I don't think "slot sync worker will restart" is true if one change enable_syncslot

Yes, right. I have changed the log-msg in this specific case.

---

From: Peter Smith <[email-masked]>
Date: Thu, 18 Jan 2024 16:01:00 +1100

I have one question about the new code in v63-0002.

======

1. ReplSlotSyncWorkerMain

+ Assert(SlotSyncWorker->pid == InvalidPid);
+
+ /*
+ * Startup process signaled the slot sync worker to stop, so if meanwhile
+ * postmaster ended up starting the worker again, exit.
+ */
+ if (SlotSyncWorker->stopSignaled)
+ {
+ SpinLockRelease(&SlotSyncWorker->mutex);
+ proc_exit(0);
+ }

Can we be sure a worker crash can't occur (in ShutDownSlotSync?) in
such a way that SlotSyncWorker->stopSignaled was already assigned
true, but SlotSyncWorker->pid was not yet reset to InvalidPid;

e.g. Is the Assert above still OK?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Jan 2024 16:49:28 +0530

> PFA v63.

1.
+ /* User created slot with the same name exists, raise ERROR. */
+ if (!synced)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exiting from slot synchronization on receiving"
+    " the failover slot \"%s\" from the primary server",
+    remote_slot->name),
+ errdetail("A user-created slot with the same name already"
+   " exists on the standby."));

I think here primary error message should contain the reason for
failure. Something like: "exiting from slot synchronization because
same name slot already exists on standby" then we can add more details
in errdetail.

2.
+synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot)
{
...
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ xmin_horizon = GetOldestSafeDecodingTransactionId(true);
+ SpinLockAcquire(&slot->mutex);
+ slot->data.catalog_xmin = xmin_horizon;
+ SpinLockRelease(&slot->mutex);
...
}

Here, why slot->effective_catalog_xmin is not updated? The same is
required by a later call to ReplicationSlotsComputeRequiredXmin(). I
see that the prior version v60-0002 has the corresponding change but
it is missing in the latest version. Any reason?

3.
+ * Return true either if the slot is marked as RS_PERSISTENT (sync-ready) or
+ * is synced periodically (if it was already sync-ready). Return false
+ * otherwise.
+ */
+static bool
+update_and_persist_slot(RemoteSlot *remote_slot)

The second part of the above comment (or is synced periodically (if it
was already sync-ready)) is not clear to me. Does it intend to
describe the case when we try to update the already created temp slot
in the last call. If so, that is not very clear because periodically
sounds like it can be due to repeated sync for sync-ready slot.

4.
+update_and_persist_slot(RemoteSlot *remote_slot)
{
...
+ (void) local_slot_update(remote_slot);
...
}

Can we write a comment to state the reason why we don't care about the
return value here?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 18 Jan 2024 12:55:17 +0000

> PFA v63.

I analyzed the security of the slotsync worker and replication connection a bit,
and didn't find issue. Here is detail: 

1) data security

First, we are using the role used in primary_conninfo, the role used here is
requested to have REPLICATION or SUPERUSER privilege[1] which means it is
reasonable for the role to modify and read replication slots on the primary.

On the primary, the slotsync worker only queries the pg_replication_view which
doesn't contain any system or user table access, so I think it's safe.

On the standby server, the slot sync worker will not read/write any user table as
well, thus we don't have the risk of executing arbitrary codes in trigger.

2) privilege check

The SQL query of the slotsync worker will take common privilege check on the
primary. If I revoke the function execution privilege on
pg_get_replication_slots from replication user, then the slotsync worker
won't be able to query the pg_replication_slots view. Same is true for the
pg_is_in_recovery function. The slotsync worker will keep reporting ERROR after
revoking which is as expected.

Based on above, I didn't see some security issues for slotsync worker.

[1] https://www.postgresql.org/docs/16/runtime-config-replication.html#GUC-PRIMARY-CONNINFO

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Fri, 19 Jan 2024 10:11:20 +1100

On Tue, Jan 9, 2024 at 11:15 PM Zhijie Hou (Fujitsu)
<[email-masked]> wrote:
>
> On Tuesday, January 9, 2024 9:17 AM Peter Smith <[email-masked]> wrote:
> >
...
> >
> > 2. ALTER_REPLICATION_SLOT ... FAILOVER
> >
> > +      <variablelist>
> > +       <varlistentry>
> > +        <term><literal>FAILOVER [ <replaceable
> > class="parameter">boolean</replaceable> ]</literal></term>
> > +        <listitem>
> > +         <para>
> > +          If true, the slot is enabled to be synced to the physical
> > +          standbys so that logical replication can be resumed after failover.
> > +         </para>
> > +        </listitem>
> > +       </varlistentry>
> > +      </variablelist>
> >
> > This syntax says passing the boolean value is optional. So it needs to be
> > specified here in the docs that not passing a value would be the same as
> > passing the value true.
>
> The behavior that "not passing a value would be the same as passing the value
> true " is due to the rule of defGetBoolean(). And all the options of commands
> in this document behave the same in this case, therefore I think we'd better
> add document for it in a general place in a separate patch/thread instead of
> mentioning this in each option's paragraph.
>

Hi Hou-san,

I did as suggested and posted a patch for this in another thread [1].
Please see if it is OK.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPtDWSmW8uiRJF1LfGQJikmo7V2jdysLuRmtsanNZc7fNw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Jan 2024 09:11:16 +0530

> e.g. Is the Assert above still OK?

We are good with the Assert here. I tried below cases:

1) When slotsync worker is say killed using 'kill', it is considered
as SIGTERM; slot sync worker invokes 'slotsync_worker_onexit()' before
going down and thus sets SlotSyncWorker->pid = InvalidPid. This means
when it is restarted (considering we have put the breakpoints in such
a way that postmaster had already reached do_start_bgworker() before
promotion finished), it is able to see stopSignaled set but pid is
InvalidPid and thus we are good.

2) Another case is when we kill slot sync worker using 'kill -9' (or
say we make it crash), in such a case, postmaster signals each sibling
process to quit (including startup process) and cleans up the shared
memory used by each (including SlotSyncWorker). In such a case
promotion fails. And if slot sync worker is started again, it will
find pid as InvalidPid. So we are good.

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 19 Jan 2024 14:05:06 +0900

> suggested by Swada-san offlist. Race-condition is mentioned in [5]

Thank you for updating the patch. I have some comments:

---
+        latestWalEnd = GetWalRcvLatestWalEnd();
+        if (remote_slot->confirmed_lsn > latestWalEnd)
+        {
+                elog(ERROR, "exiting from slot synchronization as the
received slot sync"
+                         " LSN %X/%X for slot \"%s\" is ahead of the
standby position %X/%X",
+                         LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
+                         remote_slot->name,
+                         LSN_FORMAT_ARGS(latestWalEnd));
+        }

IIUC GetWalRcvLatestWalEnd () returns walrcv->latestWalEnd, which is
typically the primary server's flush position and doesn't mean the LSN
where the walreceiver received/flushed up to. Does it really happen
that the slot's confirmed_flush_lsn is higher than the primary's flush
lsn?

---
After dropping a database on the primary, I got the following LOG (PID
2978463 is the slotsync worker on the standby):

LOG:  still waiting for backend with PID 2978463 to accept ProcSignalBarrier
CONTEXT:  WAL redo at 0/301CE00 for Database/DROP: dir 1663/16384

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Jan 2024 11:22:51 +0530

> this comment in the next version.

OTOH, can't we keep the assert as it is but remove "= 1" from
"count(*) = 1" in the query. There shouldn't be more than one slot
with same name on the primary. Or, am I missing something?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Jan 2024 11:46:51 +0530

> with same name on the primary. Or, am I missing something?

There will be 1 record max and 0 record if the primary_slot_name is
invalid. Keeping 'count(*)=1' gives the benefit that it will straight
away give us true/false indicating if we are good or not wrt
primary_slot_name. I feel Assert can be removed and we can simply
have:

        if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
                elog(ERROR, "failed to fetch primary_slot_name tuple");

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Fri, 19 Jan 2024 17:18:20 +1100

Here are some review comments for patch v63-0003.

======
Commit Message

1.
This patch attempts to start slot-sync worker as a special process
which is neither a bgworker nor an Auxiliary process. The benefit
we get here is we can control the start-conditions of the worker which
further allows us to 'enable_syncslot' as PGC_SIGHUP which was otherwise
a PGC_POSTMASTER GUC when slotsync worker was registered as bgworker.

~

missing word?

/allows us to/allows us to define/

======
src/backend/postmaster/postmaster.c

2. process_pm_child_exit

+ /*
+ * Was it the slot sync worker? Normal exit or FATAL exit (FATAL can
+ * be caused by libpqwalreceiver on receiving shutdown request by the
+ * startup process during promotion) can be ignored; we'll start a new
+ * one at the next iteration of the postmaster's main loop, if
+ * necessary. Any other exit condition is treated as a crash.
+ */
+ if (pid == SlotSyncWorkerPID)
+ {
+ SlotSyncWorkerPID = 0;
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ HandleChildCrash(pid, exitstatus,
+ _("Slotsync worker process"));
+ continue;
+ }

2a.

I think the 2nd sentence is easier to read if written like:

Normal exit or FATAL exit can be ignored (FATAL can be caused by
libpqwalreceiver on receiving shutdown request by the startup process
during promotion);

~

2b.
All other names nearby are lowercase so maybe change "Slotsync worker
process" to ""slotsync worker process" or ""slot sync worker process".

======

3. check_primary_info

  if (!valid)
- ereport(ERROR,
+ {
+ *primary_slot_invalid = true;
+ ereport(LOG,
  errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("exiting from slot synchronization due to bad configuration"),
+ errmsg("skipping slot synchronization due to bad configuration"),
  /* translator: second %s is a GUC variable name */
  errdetail("The primary server slot \"%s\" specified by %s is not valid.",
    PrimarySlotName, "primary_slot_name"));
+ }

Somehow it seems more appropriate for the *caller* to decide what to
do (e.g. "skipping...") when the primary slot is invalid. See  also
the next review comment #4b -- maybe just change this LOG to say "bad
configuration for slot synchronization".

~~~

4.
 /*
  * Check that all necessary GUCs for slot synchronization are set
- * appropriately. If not, raise an ERROR.
+ * appropriately. If not, log the message and pass 'valid' as false
+ * to the caller.
  *
  * If all checks pass, extracts the dbname from the primary_conninfo GUC and
  * returns it.
  */
 static char *
-validate_parameters_and_get_dbname(void)
+validate_parameters_and_get_dbname(bool *valid)

4a.
This feels back-to-front. I think a "validate" function should return
boolean. It can return the dbname as a side-effect only when it is
valid.

SUGGESTION
static boolean
validate_parameters_and_get_dbname(char *dbname)

~

4b.
It was a bit different when there were ERRORs but now they are LOGs
somehow it seems wrong for this function to say what the *caller* will
do. Maybe you can rewrite all the errmsg so the don't say "skipping"
but they just say "bad configuration for slot synchronization"

If valid is false then you can LOG "skipping" at the caller...

~~~

5. wait_for_valid_params_and_get_dbname

+ dbname = validate_parameters_and_get_dbname(&valid);
+ if (valid)
+ break;
+ else

This code will be simpler when the function is change to return
boolean as suggested above in #4a.

Also the 'else' is unnecessary.

SUGGESTION
if (validate_parameters_and_get_dbname(&dbname)
break;
~

6.
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+
+ }
+ }

Unnecessary blank line.

~~~

7. slotsync_reread_config

+ if (old_enable_syncslot != enable_syncslot)
+ {
+ /*
+ * We have reached here, so old value must be true and new must be
+ * false.
+ */
+ Assert(old_enable_syncslot);
+ Assert(!enable_syncslot);

I felt it would be better just to say Assert(enable_syncslot); at the
top of this function (before the ProcessConfigFile). Then none of this
other comment/assert if really needed because it should be
self-evident.

~~~

8. StartSlotSyncWorker

int
StartSlotSyncWorker(void)
{
pid_t pid;

#ifdef EXEC_BACKEND
switch ((pid = slotsyncworker_forkexec()))
#else
switch ((pid = fork_process()))
#endif
{
case -1:
ereport(LOG,
(errmsg("could not fork slot sync worker process: %m")));
return 0;

#ifndef EXEC_BACKEND
case 0:
/* in postmaster child ... */
InitPostmasterChild();

/* Close the postmaster's sockets */
ClosePostmasterPorts(false);

ReplSlotSyncWorkerMain(0, NULL);
break;
#endif
default:
return (int) pid;
}

/* shouldn't get here */
return 0;
}

The switch code can be rearranged so you don't need the #ifndef

SUGGESTION
#ifdef EXEC_BACKEND
switch ((pid = slotsyncworker_forkexec()))
{
#else
switch ((pid = fork_process()))
{
case 0:
/* in postmaster child ... */
InitPostmasterChild();

/* Close the postmaster's sockets */
ClosePostmasterPorts(false);

ReplSlotSyncWorkerMain(0, NULL);
break;
#endif
case -1:
ereport(LOG,
(errmsg("could not fork slot sync worker process: %m")));
return 0;
default:
return (int) pid;
}

======
src/backend/storage/lmgr/proc.c

9. InitProcess

  * this; it probably should.)
+ *
+ * Slot sync worker does not participate in it, see comments atop Backend.
  */
- if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
+ if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess() &&
+ !IsLogicalSlotSyncWorker())
  MarkPostmasterChildActive();

9a.
/does not participate in it/also does not participate in it/

~

9b.
It's not clear where "atop Backend" is referring to.

~~~

10.
  * way, so tell the postmaster we've cleaned up acceptably well. (XXX
  * autovac launcher should be included here someday)
  */
- if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
+ if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess() &&
+ !IsLogicalSlotSyncWorker())
  MarkPostmasterChildInactive();

Should this comment also be updated to mention slot sync worker?

======
src/backend/utils/activity/pgstat_io.c

11. pgstat_tracks_io_bktype

  case B_WAL_SENDER:
+ case B_SLOTSYNC_WORKER:
  return true;
  }

Notice all the other enums were arrange in alphabetical order, so do
the same here.

======
src/backend/utils/init/miscinit.c

12. GetBackendTypeDesc

+ case B_SLOTSYNC_WORKER:
+ backendDesc = "slotsyncworker";
+ break;
  }
All the other case are in alphabetical order, same as the enum values,
so do the same here.

~~~

13. InitializeSessionUserIdStandalone

  * This function should only be called in single-user mode, in autovacuum
  * workers, and in background workers.
  */
- Assert(!IsUnderPostmaster || IsAutoVacuumWorkerProcess() ||
IsBackgroundWorker);
+ Assert(!IsUnderPostmaster || IsAutoVacuumWorkerProcess() ||
+    IsLogicalSlotSyncWorker() || IsBackgroundWorker);

Looks like this Assert has a stale comment that should be updated.

======
src/include/miscadmin.h

14. GetBackendTypeDesc

  B_WAL_SUMMARIZER,
+ B_SLOTSYNC_WORKER,
  B_WAL_WRITER,
 } BackendType;

It seems strange to jam this new value among the other B_WAL enums.
Anyway, it looks like everything else is in alphabetical order, so we
do that too.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 19 Jan 2024 08:12:33 +0000

Hi,

On Fri, Jan 19, 2024 at 11:46:51AM +0530, shveta malik wrote:
> invalid. 

I think we'd have exactly one record in all the cases (due to the count):

postgres=# SELECT pg_is_in_recovery(), count(*) FROM pg_replication_slots WHERE 1 = 2;
 pg_is_in_recovery | count
-------------------+-------
 f                 |     0
(1 row)

postgres=# SELECT pg_is_in_recovery(), count(*) FROM pg_replication_slots WHERE 1 = 1;
 pg_is_in_recovery | count
-------------------+-------
 f                 |     1
(1 row)

> Keeping 'count(*)=1' gives the benefit that it will straight

I'd also vote for keeping it as it is and remove the Assert.

Regards,

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Jan 2024 15:55:24 +0530

On Fri, Jan 19, 2024 at 10:35 AM Masahiko Sawada <[email-masked]> wrote:
> where the walreceiver received/flushed up to.

yes. I think it makes more sense to use something which actually tells
flushed-position. I gave it a try by replacing GetWalRcvLatestWalEnd()
with GetWalRcvFlushRecPtr() but I see a problem here. Lets say I have
enabled the slot-sync feature in a running standby, in that case we
are all good (flushedUpto is the same as actual flush-position
indicated by LogstreamResult.Flush). But if I restart standby, then I
observed that the startup process sets flushedUpto to some value 'x'
(see [1]) while when the wal-receiver starts, it sets
'LogstreamResult.Flush' to another value (see [2]) which is always
greater than 'x'. And we do not update flushedUpto with the
'LogstreamResult.Flush' value in walreceiver until we actually do an
operation on primary. Performing a data change on primary sends WALs
to standby which then hits XLogWalRcvFlush() and updates flushedUpto
same as LogstreamResult.Flush. Until then we have a situation where
slots received on standby are ahead of flushedUpto and thus slotsync
worker keeps one erroring out. I am yet to find out why flushedUpto is
set to a lower value than 'LogstreamResult.Flush' at the start of
standby.  Or maybe am I using the wrong function
GetWalRcvFlushRecPtr() and should be using something else instead?

[1]:
Startup process sets 'flushedUpto' here:
ReadPageInternal-->XLogPageRead-->WaitForWALToBecomeAvailable-->RequestXLogStreaming

[2]:
Walreceiver sets 'LogstreamResult.Flush' here but do not update
'flushedUpto' here:
WalReceiverMain():  LogstreamResult.Write = LogstreamResult.Flush =
GetXLogReplayRecPtr(NULL)

> Does it really happen

It may happen if we have not configured standby_slot_names on primary.
In such a case, slots may get updated w/o confirming that standby has
taken the change and thus slot-sync worker may fetch the slots which
have lsns ahead of the latest WAL position on standby.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Jan 2024 16:18:51 +0530

> I'd also vote for keeping it as it is and remove the Assert.

Sure, retained the query as is. Removed Assert.

PFA v64. Changes are:
1) Addressed comments by Amit in [1].
2) Addressed offlist comments given by Peter for documentation patch06.
3) Moved some docs to patch04 which were wrongly placed in patch02.
4) Addressed 1 pending comment from Bertrand (as stated above) to
remove redundant Assert from check_primary_info()

TODO:
Address comments by Peter given in [2]

[1]: https://www.postgresql.org/message-id/CAA4eK1LBnCjxBi7vPam0OfxsTEyHdvqx7goKxi1ePU45oz%3Dkhg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPt5Pk_xJkb54oahR%2Bf9oawgfnmbpewvkZPgnRhoJ3gkYg%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Jan 2024 16:25:06 +0530

> it is missing in the latest version. Any reason?

I think it was a mistake in v61. Added it back in v64..

> sounds like it can be due to repeated sync for sync-ready slot.

The comment was as per old functionality where this function was doing
persist and save both. In v61 code changed, but comment was not
updated. I have changed it now in v64.

> Can we write a comment to state the reason why we don't care about the

Since it is the first time 'local_slot_update' is happening on any
slot, the return value must be true i.e. local_slot_update() should
not skip the update. I have thus added an Assert on return value now
(in v64).

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Jan 2024 17:23:53 +0530

On Wed, Jan 17, 2024 at 4:00 PM shveta malik <[email-masked]> wrote:

I had some off-list discussions with Sawada-San, Hou-San, and Shveta
on the topic of extending replication commands instead of using the
current model where we fetch the required slot information via SQL
using a database connection. I would like to summarize the discussion
and would like to know the thoughts of others on this topic.

In the current patch, we launch the slotsync worker on physical
standby which connects to the specified database (currently we let
users specify the required dbname in primary_conninfo) on the primary.
It then fetches the required information for failover marked slots
from the primary and also does some primitive checks on the upstream
node via SQL (the additional checks are like whether the upstream node
has a specified physical slot or whether the upstream node is a
primary node or a standby node). To fetch the required information it
uses a libpqwalreciever API which is mostly apt for this purpose as it
supports SQL execution but for this patch, we don't need a replication
connection, so we extend the libpqwalreciever connect API.

Now, the concerns related to this could be that users would probably
need to change existing mechanisms/tools to update priamry_conninfo
and one of the alternatives proposed is to have an additional GUC like
slot_sync_dbname. Users won't be able to drop the database this worker
is connected to aka whatever is specified in slot_sync_dbname but as
the user herself sets up the configuration it shouldn't be a big deal.
Then we also discussed whether extending libpqwalreceiver's connect
API is a good idea and whether we need to further extend it in the
future. As far as I can see, slotsync worker's primary requirement is
to execute SQL queries which the current API is sufficient, and don't
see something that needs any drastic change in this API. Note that
tablesync worker that executes SQL also uses these APIs, so we may
need something in the future for either of those. Then finally we need
a slotsync worker to also connect to a database to use SQL and fetch
results.

Now, let us consider if we extend the replication commands like
READ_REPLICATION_SLOT and or introduce a new set of replication
commands to fetch the required information then we don't need a DB
connection with primary or a connection in slotsync worker. As per my
current understanding, it is quite doable but I think we will slowly
go in the direction of making replication commands something like SQL
because today we need to extend it to fetch all slots info that have
failover marked as true, the existence of a particular replication,
etc. Then tomorrow, if we want to extend this work to have multiple
slotsync workers say workers perdb then we have to extend the
replication command to fetch per-database failover marked slots. To
me, it sounds more like we are slowly adding SQL-like features to
replication commands.

Apart from this when we are reading per-db replication slots without
connecting to a database, we probably need some additional protection
mechanism so that the database won't get dropped.

Considering all this it seems that for now probably extending
replication commands can simplify a few things like mentioned above
but using SQL's with db-connection is more extendable.

Thoughts?

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 20 Jan 2024 10:51:49 +0530

> connection, so we extend the libpqwalreciever connect API.

What sort of extension we have done to 'libpqwalreciever'? Is it
something like by default this supports replication connections so we
have done an extension to the API so that we can provide an option
whether to create a replication connection or a normal connection?

> the user herself sets up the configuration it shouldn't be a big deal.

Yeah for this purpose users may use template1 or so which they
generally don't plan to drop.  So in case the user wants to drop that
database user needs to turn off the slot syncing option and then it
can be done?

> a slotsync worker to also connect to a database to use SQL and fetch

While looking into the patch v64-0002 I could not exactly point out
what sort of extensions are there in libpqwalreceiver.c, I just saw
one extra API for fetching the dbname from connection info?

> mechanism so that the database won't get dropped.

Something like locking the database only while fetching the slots?

> but using SQL's with db-connection is more extendable.

Even I have similar thoughts.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 20 Jan 2024 16:14:16 +0530

> whether to create a replication connection or a normal connection?

Yeah and in the future there could be more as well. The other function
added walrcv_get_dbname_from_conninfo doesn't appear to be a problem
either for now.

> generally don't plan to drop.
>

Using template1 has other problems like users won't be able to create
a new database. See [2] (point number 2.2)

>
>  So in case the user wants to drop that

Right.

> one extra API for fetching the dbname from connection info?

Right, the worry was that we may need it in the future.

> Something like locking the database only while fetching the slots?

Possible, but can we lock the database from an auxiliary process?

> Even I have similar thoughts.

Thanks.

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

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 22 Jan 2024 09:06:07 +0530

On Fri, Jan 19, 2024 at 4:18 PM shveta malik <[email-masked]> wrote:
> PFA v64.

V64 fails to apply to HEAD due to a recent commit. Rebased it. PFA
v64_2. It has no new changes.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Mon, 22 Jan 2024 09:49:26 +0530

> but using SQL's with db-connection is more extendable.

Bertrand, and others, do you have an opinion on this matter?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 22 Jan 2024 12:28:30 +0530

> GetWalRcvFlushRecPtr() and should be using something else instead?

Can we think of using GetStandbyFlushRecPtr()? We probably need to
expose this function, if this works for the required purpose.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 22 Jan 2024 07:41:19 +0000

Hi,

> need to change existing mechanisms/tools to update priamry_conninfo

Yeah, for the ones that want the sync slot feature.

> the user herself sets up the configuration it shouldn't be a big deal.

Same point of view here.

> a slotsync worker to also connect to a database to use SQL and fetch

On my side the nits concerns about using the libpqrcv_connect / walrcv_connect are:

- cosmetic: the "rcv" do not really align with the sync slot worker
- we're using a WalReceiverConn, while a PGconn should suffice. From what I can
see the "overhead" is (1 byte + 7 bytes hole + 8 bytes). I don't think that's a
big deal even if we switch to a multi sync slot worker design later on.

Those have already been discussed in [1] and I'm fine with them.

> me, it sounds more like we are slowly adding SQL-like features to

Agree. Also it seems to me that extending the replication commands is more like
a one-way door change.

> but using SQL's with db-connection is more extendable.

I'd vote for using a SQL db-connection (like we are doing currently).
It seems more extendable and more a two-way door (as compared to extending the
replication commands): I think it still gives us the flexibility to switch to
extending the replication commands if we want to in the future.

[1]: https://www.postgresql.org/message-id/ZZe6sok7IWmhKReU%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 22 Jan 2024 08:49:11 +0000

On Monday, January 22, 2024 11:36 AM shveta malik <[email-masked]> wrote:

Hi,

> V64 fails to apply to HEAD due to a recent commit. Rebased it. PFA v64_2. It has
> no new changes.

I noticed few things while analyzing the patch.

1.
sleep_ms = Min(sleep_ms * 2, MAX_WORKER_NAPTIME_MS);

The initial value for sleep_ms is 0(default value for static variable) which
will not be advanced in this expression. We should initialize sleep_ms to a positive
number.

2.
		/ Wait a bit, we don't expect to have to wait long /
		rc = WaitLatch(MyLatch,
					   WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
					   10L, WAIT_EVENT_BGWORKER_SHUTDOWN);

The slotsync worker is not a bgworker anymore after 0003 patch, so a new event
is needed I think.

3.
slot->effective_catalog_xmin = xmin_horizon;

The assignment is also needed in local_slot_update() to make 
ReplicationSlotsComputeRequiredXmin work.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Mon, 22 Jan 2024 15:10:26 +0530

> Hi,

Thanks for sharing the feedback.

> - cosmetic: the "rcv" do not really align with the sync slot worker

But note that the same API is even used for apply worker as well. One
can think that this is a connection used to receive WAL or slot_info.

minor comments on the patch:
=======================
1.
+ /* First time slot update, the function must return true */
+ Assert(local_slot_update(remote_slot));

Isn't moving this code to Assert in update_and_persist_slot() wrong?
It will make this function call no-op in non-assert builds?

2.
+ ereport(LOG,
+ errmsg("newly locally created slot \"%s\" is sync-ready now",

I think even without 'locally' in the above LOG message, it is clear.

3.
+/*
+ * walrcv_get_dbinfo_for_failover_slots_fn
+ *
+ * Run LIST_DBID_FOR_FAILOVER_SLOTS on primary server to get the
+ * list of unique DBIDs for failover logical slots
+ */
+typedef List *(*walrcv_get_dbinfo_for_failover_slots_fn)
(WalReceiverConn *conn);

This looks like a leftover from the previous version of the patch.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 22 Jan 2024 17:00:12 +0530

> minor comments on the patch:

PFA v65 addressing the comments.

Addressed comments by Peter in [1], comments by Hou-San in [2],
comments by Amit in [3] and [4]

TODO:
Analyze the issue reported by Swada-san in [5] (pt 2)
Disallow subscription creation on standby with failover=true (as we do
not support sync on cascading standbys)

[1]: https://www.postgresql.org/message-id/CAHut%2BPt5Pk_xJkb54oahR%2Bf9oawgfnmbpewvkZPgnRhoJ3gkYg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/OS0PR01MB57160C7184E17C6765AAE38294752%40OS0PR01MB5716.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/message-id/CAA4eK1JPB-zpGYTbVOP5Qp26tNQPMjDuYzNZ%2Ba9RFiN5nE1tEA%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/CAA4eK1Jhy1-bsu6vc0%3DNja7aw5-EK_%3D101pnnuM3ATqTA8%2B%3DSg%40mail.gmail.com
[5]: https://www.postgresql.org/message-id/CAD21AoBgzONdt3o5mzbQ4MtqAE%3DWseiXUOq0LMqne-nWGjZBsA%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 22 Jan 2024 17:03:52 +0530

> expose this function, if this works for the required purpose.

I think we can. For the records, the problem while using flushedUpto
(or GetWalRcvFlushRecPtr()) directly is that it is not set to the
latest flushed position immediately after startup.  It points to some
prior location (perhaps segment or page start) after startup until
some data  is flushed next which then updates it to the latest flushed
position, thus we can not use it directly.  GetStandbyFlushRecPtr()
OTOH takes care of it i.e. it returns correct flushed-location at any
point of time. I have changed v65 to use this one.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 22 Jan 2024 17:07:05 +0530

> Here are some review comments for patch v63-0003.

Thanks Peter. I have addressed all in v65.

> If valid is false then you can LOG "skipping" at the caller...

I have made this change but now in the log file we see 3 logs like
below, does it seem apt? Was the earlier one better where we get the
info in 2 lines?

[34416] LOG:  bad configuration for slot synchronization
[34416] HINT:  hot_standby_feedback must be enabled.
[34416] LOG:  skipping slot synchronization

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 22 Jan 2024 20:58:05 +0900

> > > need to change existing mechanisms/tools to update priamry_conninfo

I'm concerned about this. In fact, a primary_conninfo value generated
by pg_basebackup does not work with enable_syncslot.

> > database user needs to turn off the slot syncing option and then it

If the user wants to continue using slot syncing, they need to switch
the database to connect. Which requires modifying primary_conninfo and
reloading the configuration file. Which further leads to restarting
the physical replication. If they use synchronous replication, it
means the application temporarily stops during that.

> Right, the worry was that we may need it in the future.

Yes. IIUC the slotsync worker uses libpqwalreceiver to establish a
non-replication connection and to execute SQL query. But neither of
them are relevant with replication. I'm a bit concerned that when we
need to extend the slotsync feature in the future we will end up
extending libpqwalreceiver, even if the new feature is not also
relevant with replication.

> > > me, it sounds more like we are slowly adding SQL-like features to

Right. How about filtering slots on the standby side? That is, for
example, the LIST_SLOT command returns all slots and the slotsync
worker filters out non-failover slots. Also such command could
potentially be used also in client tools like pg_basebackup,
pg_receivewal, and pg_recvlogical to list the available replication
slots to specify.

> > > but using SQL's with db-connection is more extendable.

Agreed.

Having said that, considering Amit, Bertrand, and Dilip already agreed
with the current design (using SQL's with db-connection), I might be
worrying too much. So we can probably go with the current design and
improve it if we find some problems.

Regards,

--
Masahiko Sawada

---

From: Amit Kapila <[email-masked]>
Date: Mon, 22 Jan 2024 17:56:16 +0530

> by pg_basebackup does not work with enable_syncslot.

Right, but if we want can't we extend pg_basebackup to do that? It is
just that I am not sure that it is a good idea to extend pg_basebackup
in the first version.

> means the application temporarily stops during that.

Yes, that would be an inconvenience but the point is we don't expect
this to change often.

> them are relevant with replication.
>

But we are already using libpqwalreceiver to execute SQL queries via
tablesync worker.

 I'm a bit concerned that when we
> worker filters out non-failover slots.
>

Yeah, we can do that but it could be unnecessary network overhead when
there are very few failover slots. And it may not be just one time, we
need to fetch slot information periodically.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 23 Jan 2024 00:12:04 +0900

> just that I am not sure that it is a good idea to extend pg_basebackup

Okay.

> But we are already using libpqwalreceiver to execute SQL queries via

IIUC tablesync workers do both SQL queries and replication commands. I
think the slotsync worker is the first background process who does
only SQL queries in a non-replication command ( using
libpqwalreceiver).

> need to fetch slot information periodically.

True.

Regards,

---

From: Peter Smith <[email-masked]>
Date: Tue, 23 Jan 2024 15:14:42 +1100

Here are some review comments for v65-0002

======
0. General - GUCs in messages

I think it would be better for the GUC names to all be quoted. It's
not a rule (yet), but OTOH it seems to be the consensus most people
want. See [1].

This might impact the following messages:

0.1
+ ereport(ERROR,
+ errmsg("could not fetch primary_slot_name \"%s\" info from the"
+    " primary server: %s", PrimarySlotName, res->err));

SUGGESTION
errmsg("could not fetch primary server slot \"%s\" info from the
primary server: %s", ...)
errhint("Check if \"primary_slot_name\" is configured correctly.");

~~~

0.2
+ if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ elog(ERROR, "failed to fetch primary_slot_name tuple");

SUGGESTION
elog(ERROR, "failed to fetch tuple for the primary server slot
specified by \"primary_slot_name\"");

~~~

0.3
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ /* translator: second %s is a GUC variable name */
+ errdetail("The primary server slot \"%s\" specified by %s is not valid.",
+   PrimarySlotName, "primary_slot_name"));

/specified by %s/specified by \"%s\"/

~~~

0.4
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_slot_name"));

/%s must be defined./\"%s\" must be defined./

~~~

0.5
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be enabled.", "hot_standby_feedback"));

/%s must be enabled./\"%s\" must be enabled./

~~~

0.6
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("wal_level must be >= logical."));

errhint("\"wal_level\" must be >= logical."))

~~~

0.7
+ if (PrimaryConnInfo == NULL || strcmp(PrimaryConnInfo, "") == 0)
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_conninfo"));

/%s must be defined./\"%s\" must be defined./

~~~

0.8
+ ereport(ERROR,
+
+ /*
+ * translator: 'dbname' is a specific option; %s is a GUC variable
+ * name
+ */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("'dbname' must be specified in %s.", "primary_conninfo"));

/must be specified in %s./must be specified in \"%s\"./

~~~

0.9
+ ereport(LOG,
+ errmsg("skipping slot synchronization"),
+ errdetail("enable_syncslot is disabled."));

errdetail("\"enable_syncslot\" is disabled."));

======

1.
+/* Min and Max sleep time for slot sync worker */
+#define MIN_WORKER_NAPTIME_MS  200
+#define MAX_WORKER_NAPTIME_MS  30000 /* 30s */
+
+/*
+ * Sleep time in ms between slot-sync cycles.
+ * See wait_for_slot_activity() for how we adjust this
+ */
+static long sleep_ms = MIN_WORKER_NAPTIME_MS;

These all belong together, so I think they share a combined comment like:

SUGGESTION
The sleep time (ms) between slot-sync cycles varies dynamically
(within a MIN/MAX range) according to slot activity. See
wait_for_slot_activity() for details.

~~~

2. update_and_persist_slot

+ /* First time slot update, the function must return true */
+ if(!local_slot_update(remote_slot))
+ elog(ERROR, "failed to update slot");

Missing whitespace after 'if'

~~~

3. synchronize_one_slot

+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exiting from slot synchronization because same"
+    " name slot \"%s\" already exists on standby",
+    remote_slot->name),
+ errdetail("A user-created slot with the same name as"
+   " failover slot already exists on the standby."));

3a.
/on standby/on the standby/

~

3b.
Now the errmsg is changed, the errdetail doesn't seem so useful. Isn't
it repeating pretty much the same information as in the errmsg?

======
src/backend/replication/walsender.c

4. GetStandbyFlushRecPtr

 /*
- * Returns the latest point in WAL that has been safely flushed to disk, and
- * can be sent to the standby. This should only be called when in recovery,
- * ie. we're streaming to a cascaded standby.
+ * Returns the latest point in WAL that has been safely flushed to disk.
+ * This should only be called when in recovery.
+ *

Since it says "This should only be called when in recovery", should
there also be a check for that (e.g. RecoveryInProgress) in the added
Assert?

======
src/include/replication/walreceiver.h

5.
 typedef char *(*walrcv_identify_system_fn) (WalReceiverConn *conn,
  TimeLineID *primary_tli);
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);

It looks like a blank line that previously existed has been lost.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPsf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Tue, 23 Jan 2024 11:09:02 +0530

> only SQL queries in a non-replication command ( using

Yes, I agree but till now we didn't saw any problem with the same.

-- 
With Regards,
Amit Kapila.

---

From: Ajin Cherian <[email-masked]>
Date: Tue, 23 Jan 2024 20:07:50 +1100

> not support sync on cascading standbys)
https://www.postgresql.org/message-id/CAHut%2BPt5Pk_xJkb54oahR%2Bf9oawgfnmbpewvkZPgnRhoJ3gkYg%40mail.gmail.com
https://www.postgresql.org/message-id/OS0PR01MB57160C7184E17C6765AAE38294752%40OS0PR01MB5716.jpnprd01.prod.outlook.com
> [3]:
https://www.postgresql.org/message-id/CAA4eK1JPB-zpGYTbVOP5Qp26tNQPMjDuYzNZ%2Ba9RFiN5nE1tEA%40mail.gmail.com
> [4]:
https://www.postgresql.org/message-id/CAA4eK1Jhy1-bsu6vc0%3DNja7aw5-EK_%3D101pnnuM3ATqTA8%2B%3DSg%40mail.gmail.com
> [5]:
https://www.postgresql.org/message-id/CAD21AoBgzONdt3o5mzbQ4MtqAE%3DWseiXUOq0LMqne-nWGjZBsA%40mail.gmail.com
I was doing some testing on this. What I noticed is that creating
subscriptions with failover enabled is taking a lot longer compared with a
subscription with failover disabled. The setup has primary configured with
standby_slot_names and that standby is enabled with enable_synclot turned
on.

Publisher has one publication, no tables.
subscriber:
postgres=# \timing
Timing is on.
postgres=# CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres
host=localhost port=6972' PUBLICATION pub with (failover = true);
NOTICE:  created replication slot "sub" on publisher
CREATE SUBSCRIPTION
Time: 10011.829 ms (00:10.012)

== drop the sub

postgres=# CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres
host=localhost port=6972' PUBLICATION pub with (failover = false);
NOTICE:  created replication slot "sub" on publisher
CREATE SUBSCRIPTION
Time: 46.317 ms

With failover=true, it takes 10011 ms while failover=false takes 46 ms.

I don't see a similar delay when creating slot on the primary with
pg_create_logical_replication_slot() with failover flag enabled.

Then on primary:
postgres=# SELECT 'init' FROM
pg_create_logical_replication_slot('lsub2_slot', 'pgoutput', false, false,
true);
?column?
----------
init
(1 row)

Time: 36.125 ms
postgres=# SELECT 'init' FROM
pg_create_logical_replication_slot('lsub1_slot', 'pgoutput', false, false,
false);
?column?
----------
init
(1 row)

Time: 53.981 ms

regards,
Ajin Cherian
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Tue, 23 Jan 2024 17:13:12 +0530

On Tue, Jan 23, 2024 at 2:38 PM Ajin Cherian <[email-masked]> wrote:
>
> I was doing some testing on this. What I noticed is that creating subscriptions with failover enabled is taking a lot longer compared with a subscription with failover disabled. The setup has primary configured with standby_slot_names and that standby is enabled with enable_synclot turned on.
>

Thanks Ajin for testing the patch. PFA v66 which fixes this issue.

The overall changes in this version are:

patch 001
1) Restricted enabling failover for user created slots on standby.
2) Fixed a wrong NOTICE during alter-sub which was always saying that
'changed the failover state to false' even if it was switched to true.

patch 002:
3) Addressed Peter's comment in [1]

patch 003:
4) Fixed the drop-db issue reported by Swada-San in [2]
5) Added other signal-handlers.
6) Fixed CFBot Windows compilation failure.

patch 004:
7) Fixed the issue reported by Ajin above in [3]. The performance
issue was due to the additional wait in WalSndWaitForWal() for
failover slots. Create Subscription calls
DecodingContextFindStartpoint() which then reads WALs to build the
initial snapshot which ends up calling WalSndWaitForWal() which waits
for standby confirmation for the case of failover slots. Addressed it
by skipping the wait during Create Sub as it is not needed there. We
now wait only if 'replication_active' is true.

Thanks Nisha for reporting the NOTICE issue (addressed in 2) and
working on issue #6.

Thanks Hou-San for working on #7.

[1]: https://www.postgresql.org/message-id/CAHut%2BPs6p6Km8_Hfy6X0KTuyqBKkhC84u23sQnnkhqkHuDL%2BDQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAD21AoBgzONdt3o5mzbQ4MtqAE%3DWseiXUOq0LMqne-nWGjZBsA%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAFPTHDbsZ%2BpxAubb9d9BwVNt5OB3_2s77bG6nHcAgUPPhEVmMQ%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 23 Jan 2024 17:20:02 +0530

> Here are some review comments for v65-0002

Thanks Peter for the feedback. I have addressed these in v66.

> there also be a check for that (e.g. RecoveryInProgress) in the added

Since 'am_cascading_walsender' and  'IsLogicalSlotSyncWorker' makes
sense 'in-recovery' only, I think explicit check for
'RecoveryInProgress' is not needed here. But I can add if others also
think it is needed.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Wed, 24 Jan 2024 14:21:53 +1100

Here are some comments for patch v66-0001.

======
doc/src/sgml/catalogs.sgml

1.
+      <para>
+       If true, the associated replication slots (i.e. the main slot and the
+       table sync slots) in the upstream database are enabled to be
+       synchronized to the physical standbys
+      </para></entry>

/physical standbys/physical standby/

I wondered if it is better just to say singular "standby" instead of
"standbys" in places like this; e.g. plural might imply cascading for
some readers.

There are a number of examples like this, so I've repeated the same
comment multiple times below. If you disagree, please just ignore all
of them.

======
doc/src/sgml/func.sgml

2.
         that the decoding of prepared transactions is enabled for this
-        slot. A call to this function has the same effect as the replication
-        protocol command <literal>CREATE_REPLICATION_SLOT ...
LOGICAL</literal>.
+        slot. The optional fifth parameter,
+        <parameter>failover</parameter>, when set to true,
+        specifies that this slot is enabled to be synced to the
+        physical standbys so that logical replication can be resumed
+        after failover. A call to this function has the same effect as
+        the replication protocol command
+        <literal>CREATE_REPLICATION_SLOT ... LOGICAL</literal>.
        </para></entry>

(same as above)

/physical standbys/physical standby/

Also, I don't see anything else on this page using plural "standbys".

======
doc/src/sgml/protocol.sgml

3. CREATE_REPLICATION_SLOT

+       <varlistentry>
+          The default is false.
+         </para>
+        </listitem>
+       </varlistentry>

(same as above)

/physical standbys/physical standby/

~~~

4. ALTER_REPLICATION_SLOT

+      <variablelist>
+       <varlistentry>

(same as above)

/physical standbys/physical standby/

======
doc/src/sgml/ref/create_subscription.sgml

5.
+       <varlistentry id="sql-createsubscription-params-with-failover">
+        <term><literal>failover</literal> (<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether the replication slots associated with the
subscription
+          are enabled to be synced to the physical standbys so that logical
+          replication can be resumed from the new primary after failover.
+          The default is <literal>false</literal>.
+         </para>
+        </listitem>
+       </varlistentry>

(same as above)

/physical standbys/physical standby/

======
doc/src/sgml/system-views.sgml

6.
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>failover</structfield> <type>bool</type>
+      </para>
+      <para>
+       True if this is a logical slot enabled to be synced to the physical
+       standbys so that logical replication can be resumed from the new primary
+       after failover. Always false for physical slots.
+      </para></entry>
+     </row>

(same as above)

/physical standbys/physical standby/

======
src/backend/commands/subscriptioncmds.c

7.
+ if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
+ {
+ if (!sub->slotname)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set failover for a subscription that does not have a
slot name")));
+
+ /*
+ * Do not allow changing the failover state if the
+ * subscription is enabled. This is because the failover
+ * state of the slot on the publisher cannot be modified if
+ * the slot is currently acquired by the apply worker.
+ */
+ if (sub->enabled)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set %s for enabled subscription",
+ "failover")));
+
+ values[Anum_pg_subscription_subfailover - 1] =
+ BoolGetDatum(opts.failover);
+ replaces[Anum_pg_subscription_subfailover - 1] = true;
+ }

The first message is not consistent with the second. The "failover"
option maybe should be extracted so it won't be translated.

SUGGESTION
errmsg("cannot set %s for a subscription that does not have a slot
name", "failover")

~~~

8. AlterSubscription

+ if (!wrconn)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not connect to the publisher: %s", err)));
+

Need to keep an eye on the patch proposed by Nisha [1] for messages
similar to this one, so in case that gets pushed this code should be
changed appropriately.

======
src/backend/replication/slot.c

9.
  *     during getting changes, if the two_phase option is enabled it can skip
  *     prepare because by that time start decoding point has been moved. So the
  *     user will only get commit prepared.
+ * failover: If enabled, allows the slot to be synced to physical standbys so
+ *     that logical replication can be resumed after failover.
  */

(same as earlier)

/physical standbys/physical standby/

~~~

10.
+ /*
+ * Do not allow users to alter slots to enable failover on the standby
+ * as we do not support sync to the cascading standby.
+ */
+ if (RecoveryInProgress() && failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter replication slot to have failover"
+    " enabled on the standby"));

I felt the errmsg could be expressed with less ambiguity:

SUGGESTION:
cannot enable failover for a replication slot on the standby

======
src/backend/replication/slotfuncs.c

11. create_physical_replication_slot

  /* acquire replication slot, this will check for conflicting names */
  ReplicationSlotCreate(name, false,
-   temporary ? RS_TEMPORARY : RS_PERSISTENT, false);
+   temporary ? RS_TEMPORARY : RS_PERSISTENT, false,
+   false);

Having an inline comment might be helpful here instead of passing "false,false"

SUGGESTION
ReplicationSlotCreate(name, false,
 temporary ? RS_TEMPORARY : RS_PERSISTENT, false,
 false /* failover */);

~~~

12. create_logical_replication_slot

+ /*
+ * Do not allow users to create the slots with failover enabled on the
+ * standby as we do not support sync to the cascading standby.
+ */
+ if (RecoveryInProgress() && failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot create replication slot with failover"
+    " enabled on the standby"));

(similar to previous comment)

SUGGESTION:
cannot enable failover for a replication slot created on the standby

~~~

13. copy_replication_slot

  * hence pass find_startpoint false.  confirmed_flush will be set
  * below, by copying from the source slot.
+ *
+ * To avoid potential issues with the slotsync worker when the
+ * restart_lsn of a replication slot goes backwards, we set the
+ * failover option to false here. This situation occurs when a slot on
+ * the primary server is dropped and immediately replaced with a new
+ * slot of the same name, created by copying from another existing
+ * slot. However, the slotsync worker will only observe the restart_lsn
+ * of the same slot going backwards.
  */
  create_logical_replication_slot(NameStr(*dst_name),
  plugin,
  temporary,
  false,
+ false,
  src_restart_lsn,
  false);
(similar to an earlier comment)

Having an inline comment might be helpful here.

e.g. false /* failover */,

======
src/backend/replication/walreceiver.c

14.
- walrcv_create_slot(wrconn, slotname, true, false, 0, NULL);
+ walrcv_create_slot(wrconn, slotname, true, false, false, 0, NULL);

(similar to an earlier comment)

Having an inline comment might be helpful here:

SUGGESTION
walrcv_create_slot(wrconn, slotname, true, false, false /* failover
*/, 0, NULL);

======

15. CreateReplicationSlot

  ReplicationSlotCreate(cmd->slotname, false,
    cmd->temporary ? RS_TEMPORARY : RS_PERSISTENT,
-   false);
+   false, false);

(similar to an earlier comment)

Having an inline comment might be helpful here.

e.g. false /* failover */,

~~~

16. CreateReplicationSlot

+ /*
+ * Do not allow users to create the slots with failover enabled on the
+ * standby as we do not support sync to the cascading standby.
+ */
+ if (RecoveryInProgress() && failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot create replication slot with failover"
+    " enabled on the standby"));
+
  /*
  * Initially create persistent slot as ephemeral - that allows us to
  * nicely handle errors during initialization because it'll get
@@ -1243,7 +1265,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
  */
  ReplicationSlotCreate(cmd->slotname, true,
    cmd->temporary ? RS_TEMPORARY : RS_EPHEMERAL,
-   two_phase);
+   two_phase, failover);

This errmsg seems to be repeated in a few places, so I wondered if
this code can be refactored to call direct to
create_logical_replication_slot() so the errmsg can be just once in a
common place.

OTOH, if it cannot be refactored, then needs to be using same errmsg
as suggested by earlier review comments (see above).

======
src/include/catalog/pg_subscription.h

17.
+ bool subfailover; /* True if the associated replication slots
+ * (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * to be synchronized to the physical
+ * standbys. */

(same as earlier)

/physical standbys/physical standby/

~~~

18.
+ bool failover; /* True if the associated replication slots
+ * (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * to be synchronized to the physical
+ * standbys. */

(same as earlier)

/physical standbys/physical standby/

======
src/include/replication/slot.h

19.
+
+ /*
+ * Is this a failover slot (sync candidate for physical standbys)? Only
+ * relevant for logical slots on the primary server.
+ */
+ bool failover;

(same as earlier)

/physical standbys/physical standby/

======
[1] Nisha errmsg -
https://www.postgresql.org/message-id/CABdArM5-VR4Akt_AHap_0Ofne0cTcsdnN6FcNe%2BMU8eXsa_ERQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Wed, 24 Jan 2024 09:39:01 +0530

> "standbys" in places like this; e.g. plural might imply cascading for

I don't think it is confusing as we used in a similar way in docs. We
can probably avoid using physical in places similar to above as that
is implied

>  temporary ? RS_TEMPORARY : RS_PERSISTENT, false,

I don't think we follow to use of inline comments. I feel that
sometimes makes code difficult to read considering when we have
multiple such parameters.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 24 Jan 2024 14:10:41 +0900

> expose this function, if this works for the required purpose.

GetStandbyFlushRecPtr() seems good. But do we really want to raise an
ERROR in this case? IIUC this case could happen often when the slot
used by the standby is not listed in standby_slot_names. I think we
can just skip such a slot to synchronize and check it the next time.

Here are random comments on slotsyncworker.c (v66):

---
The postmaster relaunches the slotsync worker without intervals. So if
a connection string in primary_conninfo is not correct, many errors
are emitted.

---
+/* GUC variable */
+bool       enable_syncslot = false;

Is enable_syncslot a really good name? We use "enable" prefix only for
planner parameters such as enable_seqscan, and it seems to me that
"slot" is not specific. Other candidates are:

* synchronize_replication_slots = on|off
* synchronize_failover_slots = on|off

---
+               elog(ERROR,

Many error messages in slotsync.c are splitted into several lines, but
I think it would reduce the greppability when the user looks for the
error message in the source code.

---
+       SpinLockAcquire(&slot->mutex);
+       slot->data.database = get_database_oid(remote_slot->database, false);
+       namestrcpy(&slot->data.plugin, remote_slot->plugin);

We should not access syscaches while holding a spinlock.

---
+       SpinLockAcquire(&slot->mutex);
+       slot->data.database = get_database_oid(remote_slot->database, false);
+       namestrcpy(&slot->data.plugin, remote_slot->plugin);
+       SpinLockRelease(&slot->mutex);

Similarly, it's better to avoid calling namestrcpy() while holding a
spinlock, as we do in CreateInitDecodingContext().

---
+   SpinLockAcquire(&SlotSyncWorker->mutex);
+
+   SlotSyncWorker->stopSignaled = true;
+
+   if (SlotSyncWorker->pid == InvalidPid)
+   {
+       SpinLockRelease(&SlotSyncWorker->mutex);
+       return;
+   }
+
+   kill(SlotSyncWorker->pid, SIGINT);
+
+   SpinLockRelease(&SlotSyncWorker->mutex);

It's better to avoid calling a system call while holding a spin lock.

---
+   BackgroundWorkerUnblockSignals();

I think it's no longer necessary.

---
+       ereport(LOG,
+       /* translator: %s is a GUC variable name */
+               errmsg("bad configuration for slot synchronization"),
+               errhint("\"wal_level\" must be >= logical."));

There is no '%s' in errmsg string.

---
+/*
+ * Cleanup function for logical replication launcher.
+ *
+ * Called on logical replication launcher exit.
+ */

IIUC this function is never called by logical replication launcher.

---
+   /*
+    * The slot sync worker can not get here because it will only stop when it
+    * receives a SIGINT from the logical replication launcher, or when there
+    * is an error.
+    */
+   Assert(false);

This comment is not correct. IIUC the slotsync worker receives a
SIGINT from the startup process.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 24 Jan 2024 11:13:31 +0530

On Wed, Jan 24, 2024 at 10:41 AM Masahiko Sawada <[email-masked]> wrote:
> used by the standby is not listed in standby_slot_names.
>

or it can be due to some bug in the code as well.

> I think we

How about logging the message and then skipping the sync step? This
will at least make users aware that they could be missing to set
standby_slot_names.

> * synchronize_failover_slots = on|off

I would prefer the second one. Would it be better to just say
sync_failover_slots?

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 24 Jan 2024 14:53:44 +0900

> will at least make users aware that they could be missing to set

+1

> I would prefer the second one. Would it be better to just say

Works for me. But if we want to extend this option for non-failover
slots as well in the future, synchronize_replication_slots (or
sync_replication_slots) seems better. We can extend it by having an
enum later. For example, the values can be on, off, or failover etc.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 24 Jan 2024 13:51:54 +0530

> enum later. For example, the values can be on, off, or failover etc.

I see your point. Let us see if others have any suggestions on this.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 24 Jan 2024 09:08:00 +0000

Hi,

> I see your point. Let us see if others have any suggestions on this.

I also see Sawada-San's point and I'd vote for "sync_replication_slots". Then for
the current feature I think "failover" and "on" should be the values to turn the
feature on (assuming "on" would mean "all kind of supported slots").

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 24 Jan 2024 16:00:56 +0530

> Thanks Ajin for testing the patch. PFA v66 which fixes this issue.

I think we should try to commit the patch as all of the design
concerns are resolved now. To achieve that, can we split the failover
setting patch into the following: (a) setting failover property via
SQL commands and display it in pg_replication_slots (b) replication
protocol command (c) failover property via subscription commands?

It will make each patch smaller and it would be easier to detect any
problem in the same after commit.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 24 Jan 2024 16:09:15 +0530

> feature on (assuming "on" would mean "all kind of supported slots").

Even if others agree and we change this GUC name to
"sync_replication_slots", I feel we should keep the values as "on" and
"off" currently, where "on" would mean 'sync failover slots' (docs can
state that clearly).  I do not think we should support sync of "all
kinds of supported slots" in the first version. Maybe we can think
about it for future versions.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 24 Jan 2024 17:17:01 +0530

> about it for future versions.

PFA v67. Note that the GUC (enable_syncslot) name is unchanged. Once
we have final agreement on the name, we can make the change in the
next version.

Changes in v67 are:

1) Addressed comments by Peter given in [1].
2) Addressed comments by Swada-San given in [2].
3) Removed syncing 'failover' on standby from remote_slot. The
'failover' field will be false for synced slots. Since we do not
support sync to cascading standbys yet, thus failover=true was
misleading and unused there.

Thanks Hou-San for contributing in 2.

Changes are split across patch001,002 and 003.

TODO:
--Split patch-001 as suggested in [3].
--Change GUC name.

[1]: https://www.postgresql.org/message-id/CAHut%2BPu_uK%3D%3DM%2BVmCMug7m7O6LAwpC05A%3DT7zP8c4G2-hS%2Bbdg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAD21AoApGoTZu7D_7%3DbVYQqKnj%2BPZ2Rz%2Bnc8Ky1HPQMS_XL6%2BA%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAA4eK1Lxvfq9RwOEsguiMCrKPUc1He9UGz1_wi0N0cJaXFa4Eg%40mail.gmail.com

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Thu, 25 Jan 2024 12:37:09 +1100

Here are some review comments for the patch v67-0001.

======
1.
There are a couple of places checking for failover usage on a standby.

+ if (RecoveryInProgress() && failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable failover for a replication slot"
+    " created on the standby"));

and

+ if (RecoveryInProgress() && failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable failover for a replication slot"
+    " on the standby"));

IMO the conditions should be written the other way around (failover &&
RecoveryInProgress()) to avoid the unnecessary function calls when
'failover' flag is probably mostly default false anyway.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 25 Jan 2024 02:57:30 +0000

On Wednesday, January 24, 2024 6:31 PM Amit Kapila <[email-masked]> wrote:
> I think we should try to commit the patch as all of the design concerns are
> resolved now. To achieve that, can we split the failover setting patch into the
> following: (a) setting failover property via SQL commands and display it in
> pg_replication_slots (b) replication protocol command (c) failover property via
> subscription commands?
> 
> It will make each patch smaller and it would be easier to detect any problem in
> the same after commit.

Agreed. I split the original 0001 patch into 3 patches as suggested.
Here is the V68 patch set.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Thu, 25 Jan 2024 09:13:05 +0530

> misleading and unused there.

But what will happen after the standby is promoted? After promotion,
ideally, it should have failover enabled, so that the slots can be
synced. Also, note that corresponding subscriptions still have the
failover flag enabled. I think we should copy the 'failover' option
for the synced slots.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 25 Jan 2024 04:24:58 +0000

Thanks for the comments:

> Many error messages in slotsync.c are splitted into several lines, but I think it
> would reduce the greppability when the user looks for the error message in the
> source code.

Thanks for the suggestion! we combined most of the messages in the new version
patch. Although some messages including the above one were kept splitted,
because It's too long(> 120 col including the indent) to fit into the screen,
so I feel it's better to keep these messages splitted.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Thu, 25 Jan 2024 16:08:48 +1100

Here are some review comments for v67-0002.

======

1.
+/* The sleep time (ms) between slot-sync cycles varies dynamically
+ * (within a MIN/MAX range) according to slot activity. See
+ * wait_for_slot_activity() for details.
+ */
+#define MIN_WORKER_NAPTIME_MS  200
+#define MAX_WORKER_NAPTIME_MS  30000 /* 30s */
+
+static long sleep_ms = MIN_WORKER_NAPTIME_MS;

In my previous review for this, I meant for there to be no whitespace
between the #defines and the static long sleep_ms so the prior comment
then clearly belongs to all 3 lines

~~~

2. synchronize_one_slot

+ /*
+ * Sanity check: Make sure that concerned WAL is received and flushed
+ * before syncing slot to target lsn received from the primary server.
+ *
+ * This check should never pass as on the primary server, we have waited
+ * for the standby's confirmation before updating the logical slot.
+ */
+ latestFlushPtr = GetStandbyFlushRecPtr(NULL);
+ if (remote_slot->confirmed_lsn > latestFlushPtr)
+ {
+ ereport(LOG,
+ errmsg("skipping slot synchronization as the received slot sync"
+    " LSN %X/%X for slot \"%s\" is ahead of the standby position %X/%X",
+    LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
+    remote_slot->name,
+    LSN_FORMAT_ARGS(latestFlushPtr)));
+
+ return false;
+ }

Previously in v65 this was an elog, but now it is an ereport. But
since this is a sanity check condition that "should never pass" wasn't
the elog the more appropriate choice?

~~~

3. synchronize_one_slot

+ /*
+ * We don't want any complicated code while holding a spinlock, so do
+ * namestrcpy() and get_database_oid() outside.
+ */
+ namestrcpy(&plugin_name, remote_slot->plugin);
+ dbid = get_database_oid(remote_slot->database, false);

IMO just simplify the whole comment, here and for the other similar
comment in local_slot_update().

SUGGESTION
/* Avoid expensive operations while holding a spinlock. */

~~~

4. synchronize_slots

+ /* Construct the remote_slot tuple and synchronize each slot locally */
+ tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ while (tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ {
+ bool isnull;
+ RemoteSlot *remote_slot = palloc0(sizeof(RemoteSlot));
+ Datum d;
+
+ remote_slot->name = TextDatumGetCString(slot_getattr(tupslot, 1, &isnull));
+ Assert(!isnull);
+
+ remote_slot->plugin = TextDatumGetCString(slot_getattr(tupslot, 2, &isnull));
+ Assert(!isnull);
+
+ /*
+ * It is possible to get null values for LSN and Xmin if slot is
+ * invalidated on the primary server, so handle accordingly.
+ */
+ d = slot_getattr(tupslot, 3, &isnull);
+ remote_slot->confirmed_lsn = isnull ? InvalidXLogRecPtr :
+ DatumGetLSN(d);
+
+ d = slot_getattr(tupslot, 4, &isnull);
+ remote_slot->restart_lsn = isnull ? InvalidXLogRecPtr : DatumGetLSN(d);
+
+ d = slot_getattr(tupslot, 5, &isnull);
+ remote_slot->catalog_xmin = isnull ? InvalidTransactionId :
+ DatumGetTransactionId(d);
+
+ remote_slot->two_phase = DatumGetBool(slot_getattr(tupslot, 6, &isnull));
+ Assert(!isnull);
+
+ remote_slot->database = TextDatumGetCString(slot_getattr(tupslot,
+ 7, &isnull));
+ Assert(!isnull);
+
+ d = slot_getattr(tupslot, 8, &isnull);
+ remote_slot->invalidated = isnull ? RS_INVAL_NONE :
+ get_slot_invalidation_cause(TextDatumGetCString(d));

Would it be better to get rid of the hardwired column numbers and then
be able to use the SLOTSYNC_COLUMN_COUNT already defined as a sanity
check?

SUGGESTION
int col = 0;
...
remote_slot->name = TextDatumGetCString(slot_getattr(tupslot, ++col, &isnull));
...
remote_slot->plugin = TextDatumGetCString(slot_getattr(tupslot, ++col,
&isnull));
...
d = slot_getattr(tupslot, ++col, &isnull);
remote_slot->confirmed_lsn = isnull ? InvalidXLogRecPtr : DatumGetLSN(d);
...
d = slot_getattr(tupslot, ++col, &isnull);
remote_slot->restart_lsn = isnull ? InvalidXLogRecPtr : DatumGetLSN(d);
...
d = slot_getattr(tupslot, ++col, &isnull);
remote_slot->catalog_xmin = isnull ? InvalidTransactionId :
DatumGetTransactionId(d);
...
remote_slot->two_phase = DatumGetBool(slot_getattr(tupslot, ++col, &isnull));
...
remote_slot->database = TextDatumGetCString(slot_getattr(tupslot,
++col, &isnull));
...
d = slot_getattr(tupslot, ++col, &isnull);
remote_slot->invalidated = isnull ? RS_INVAL_NONE :
get_slot_invalidation_cause(TextDatumGetCString(d));

/* Sanity check */
Asert(col == SLOTSYNC_COLUMN_COUNT);

~~~

5.
+static char *
+validate_parameters_and_get_dbname(void)
+{
+ char    *dbname;

These are configuration issues, so probably all these ereports could
also set errcode(ERRCODE_INVALID_PARAMETER_VALUE).

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 25 Jan 2024 05:56:49 +0000

Hi,

On Wed, Jan 24, 2024 at 04:09:15PM +0530, shveta malik wrote:
> state that clearly).

I gave more thoughts on it and I think the values should only be "failover" or
"off".

The reason is that if we allow "on" and change the "on" behavior in future
versions (to support more than failover slots) then that would change the behavior 
for the ones that used "on".

That's right that we can mention it in the docs, but there is still the risk of
users not reading the doc (that's why I think that it would be good if we can put 
this extra "safety" in the code too).

>  I do not think we should support sync of "all

Yeah I think the same (I was mentioning the future "on" behavior up-thread).

Regards,

---

From: shveta malik <[email-masked]>
Date: Thu, 25 Jan 2024 11:41:06 +0530

> failover flag enabled. I think we should copy the 'failover' option

Yes, right, missed this point earlier. I will make the change in the
next version.

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 25 Jan 2024 07:55:32 +0000

Hi,

Thanks!

Some comments.

Looking at 0002:

1 ===

+      <para>The following options are supported:</para>

What about "The following option is supported"? (as currently only the "FAILOVER"
is)

2 ===

What about adding some TAP tests too? (I can see that ALTER_REPLICATION_SLOT test
is added in v68-0004 but I think having some in 0002 would make sense too).

Looking at 0003:

1 ===

+      parameter specified in the subscription. When creating the slot,
+      ensure the slot <literal>failover</literal> property matches the
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      parameter value of the subscription.

What about explaining what would be the consequence of not doing so?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 25 Jan 2024 15:54:45 +0530

> > Here is the V68 patch set.

Thanks, I have pushed 0001.

> is added in v68-0004 but I think having some in 0002 would make sense too).

The subscription tests in v68-0003 will test this functionality. The
one advantage of adding separate tests for this is that if in the
future we extend this replication command, it could be convenient to
test various options. However, the same could be said about existing
replication commands as well. But is it worth having extra tests which
will be anyway covered in the next commit in a few days?

I understand that it is a good idea and makes one comfortable to have
tests for each separate commit but OTOH, in the longer term it will
just be adding more test time without achieving much benefit. I think
we can tell explicitly in the commit message of this patch that the
subsequent commit will cover the tests for this functionality

One minor comment on 0002:
+          so that logical replication can be resumed after failover.
+         </para>

Can we move this and similar comments or doc changes to the later 0004
patch where we are syncing the slots?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Thu, 25 Jan 2024 17:17:14 +0530

> the elog the more appropriate choice?

We realized that this scenario can be frequently hit when the user has
not set standby_slot_names on primary. And thus ereport makes more
sense. But I agree that this comment is misleading. We will adjust the
comment in the next version.

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 25 Jan 2024 11:51:28 +0000

Hi,

On Thu, Jan 25, 2024 at 03:54:45PM +0530, Amit Kapila wrote:
> replication commands as well.

I initially did check for "START_REPLICATION" and I saw it's part of 
006_logical_decoding.pl (but did not check all the "REPLICATION" commands).

That said, it's more a Nit and I think it's fine with having the test in v68-0004
(as it is currently done) + the ones in v68-0003.

> But is it worth having extra tests which

Yeah, I think that's enough (at least someone reading the commit message, the 
diff changes and not following this dedicated thread closely would know the lack
of test is not a miss).

> patch where we are syncing the slots?

Sure.

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 25 Jan 2024 13:11:50 +0000

On Thursday, January 25, 2024 6:25 PM Amit Kapila <[email-masked]> wrote:
> > What about adding some TAP tests too? (I can see that
> > ALTER_REPLICATION_SLOT test is added in v68-0004 but I think having some
> in 0002 would make sense too).
> >
> 
> The subscription tests in v68-0003 will test this functionality. The one
> advantage of adding separate tests for this is that if in the future we extend this
> replication command, it could be convenient to test various options. However,
> the same could be said about existing replication commands as well. But is it
> worth having extra tests which will be anyway covered in the next commit in a
> few days?
> 
> I understand that it is a good idea and makes one comfortable to have tests for
> each separate commit but OTOH, in the longer term it will just be adding more
> test time without achieving much benefit. I think we can tell explicitly in the
> commit message of this patch that the subsequent commit will cover the tests
> for this functionality

Agreed.

> Can we move this and similar comments or doc changes to the later 0004 patch
> where we are syncing the slots?

Thanks for the comment.

Here is the V69 patch set which includes the following changes.

V69-0001, V69-0002
1) Addressed Bertrand's comments[1].

V69-0003
1) Addressed Peter's comment in [2], [3]
2) Addressed Amit's comment in [4] and above.
3) Fixed one issue that the startup process may report ERROR if it tries to drop
the same slot that the slotsync worker is acquiring. Now we take shared lock on
db in slot-sync worker before we create, update or drop any of its slots. This
is done to prevent potential conflict with ReplicationSlotsDropDBSlots() in
case that database is dropped in parallel.

V69-0004
1) Rebased and fixed one CFbot failure.

V69-0005, V69-0006, V69-0007
1) Rebased.

Thanks Shveta for rebasing and working for the changes on 0003~0007.

[1] https://www.postgresql.org/message-id/ZbIT9Kj3d8TFD8h6%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/CAHut%2BPt2oLfxv_%3DGN23dOOduKHBHdAkCvwSZiwSbtTJFFbQm-w%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAHut%2BPtsDYPbg7qM1nGWtJcSQBQ5JH%3DLmgyqwqBPL9k%2Bz8f5Ew%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/CAA4eK1%2B4PhO-f4%2B2fForG6MOEj3jbtee_PYPtwtgww%3DonC5DSQ%40mail.gmail.com

Best Regards,
Hou zj

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 26 Jan 2024 08:31:18 +0000

Hi,

Thanks!

V69-0001 LGTM.

As far V69-0002 I just have one more last remark:

+                                        */
+                                       if (sub->enabled)
+                                               ereport(ERROR,

Worth to add a test for it in 050_standby_failover_slots_sync.pl? (I had a quick
look and it does not seem to be covered).

Remarks,

---

From: Amit Kapila <[email-masked]>
Date: Sat, 27 Jan 2024 09:13:15 +0530

> Here is the V69 patch set which includes the following changes.

Few minor comments on v69-0001
1. In libpqrcv_create_slot(), I see we are using two types of syntaxes
based on 'use_new_options_syntax' (aka server_version >= 15) whereas
this new 'failover' option doesn't follow that. What is the reason of
the same? I thought it is because older versions anyway won't support
this option. However, I guess we should follow the syntax of the old
server and let it error out. BTW, did you test this patch with old
server versions (say < 15 and >=15) by directly using replication
commands, if so, what is the behavior of same?

2.
  }
-
+ if (failover)
+ appendStringInfoString(&cmd, "FAILOVER, ");

Spurious line removal. Also, to follow a coding pattern similar to
nearby code, let's have one empty line after handling of failover.

3.
+/* ALTER_REPLICATION_SLOT slot */
+alter_replication_slot:
+ K_ALTER_REPLICATION_SLOT IDENT '(' generic_option_list ')'

I think it would be better if we follow the create style by specifying
syntax in comments as that can make the code easier to understand
after future extensions to this command if any. See
create_replication_slot:
/* CREATE_REPLICATION_SLOT slot [TEMPORARY] PHYSICAL [options] */
K_CREATE_REPLICATION_SLOT IDENT opt_temporary K_PHYSICAL create_slot_options

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 27 Jan 2024 06:31:52 +0000

On Saturday, January 27, 2024 11:43 AM Amit Kapila <[email-masked]> wrote:
> 
> On Thu, Jan 25, 2024 at 6:42 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> 1. In libpqrcv_create_slot(), I see we are using two types of syntaxes based on
> 'use_new_options_syntax' (aka server_version >= 15) whereas this new 'failover'
> option doesn't follow that. What is the reason of the same? I thought it is
> because older versions anyway won't support this option. However, I guess we
> should follow the syntax of the old server and let it error out.

Changed as suggested.

> BTW, did you test
> this patch with old server versions (say < 15 and >=15) by directly using
> replication commands, if so, what is the behavior of same?

Yes, I tested it. We cannot use new failover option or new
alter_replication_slot on server <17, the errors we will get are as follows:

Using failover option in create_replication_slot on server 15 ~ 16
	ERROR:  unrecognized option: failover
Using failover option in create_replication_slot on server < 15
	ERROR:  syntax error
Alter_replication_slot on server < 17
	ERROR:  syntax error at or near "ALTER_REPLICATION_SLOT"

> Spurious line removal. Also, to follow a coding pattern similar to nearby code,
> let's have one empty line after handling of failover.

Changed.

> I think it would be better if we follow the create style by specifying syntax in
> comments as that can make the code easier to understand after future
> extensions to this command if any. See
> create_replication_slot:

Changed.

Attach the V70 patch set which addressed above comments and Bertrand's comments in [1]

[1] https://www.postgresql.org/message-id/ZbNt1oRZRcdIAw2c%40ip-10-97-1-34.eu-west-3.compute.internal

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Mon, 29 Jan 2024 14:50:52 +1100

Here are some review comments for v70-0001.

======

1.
Related to this, please also review my other patch to the same docs
page protocol.sgml [1].

======
src/backend/replication/logical/tablesync.c

2.
  walrcv_create_slot(LogRepWorkerWalRcvConn,
     slotname, false /* permanent */ , false /* two_phase */ ,
+    false,
     CRS_USE_SNAPSHOT, origin_startpos);

I know it was previously mentioned in this thread that inline
parameter comments are unnecessary, but here they are already in the
existing code so shouldn't we do the same?

======
src/backend/replication/repl_gram.y

3.
+/* ALTER_REPLICATION_SLOT slot options */
+alter_replication_slot:
+ K_ALTER_REPLICATION_SLOT IDENT '(' generic_option_list ')'
+ {
+ AlterReplicationSlotCmd *cmd;
+ cmd = makeNode(AlterReplicationSlotCmd);
+ cmd->slotname = $2;
+ cmd->options = $4;
+ $$ = (Node *) cmd;
+ }
+ ;
+

IMO write that comment with parentheses, so it matches the code.

SUGGESTION
ALTER_REPLICATION_SLOT slot ( options )

======

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Jan 2024 09:35:44 +0530

> Related to this, please also review my other patch to the same docs

We can check that separately.

> existing code so shouldn't we do the same?

I think it is better to remove the even existing ones as those many
times make code difficult to read.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 29 Jan 2024 10:24:11 +0530

> Attach the V70 patch set which addressed above comments and Bertrand's comments in [1]

Since v70-0001 is pushed, rebased and attached v70_2 patches.  There
are no new changes.

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 29 Jan 2024 08:52:14 +0000

Hi,

Thanks!

Looking at 0001:

+      When altering the
+      <link linkend="sql-createsubscription-params-with-slot-name"><literal>slot_name</literal></link>,
+      the <literal>failover</literal> property value of the named slot may differ from the
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      parameter specified in the subscription. When creating the slot,
+      ensure the slot <literal>failover</literal> property matches the
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      parameter value of the subscription. Otherwise, the slot on the publisher may
+      not be enabled to be synced to standbys.

Not related to this patch series but while at it shouldn't we also add a few
words about two_phase too? (I mean ensure the slot property matchs the
subscription one).

Or would it be better to create a dedicated patch (outside of this thread) for
the "two_phase" remark? (If so I can take care of it).

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Jan 2024 14:35:52 +0530

> the "two_phase" remark? (If so I can take care of it).

I think it is better to create a separate patch for two_phase after
this patch gets committed.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 29 Jan 2024 09:15:57 +0000

Hi,

> this patch gets committed.

Yeah, makes sense, will do, thanks!

Regards,

---

From: shveta malik <[email-masked]>
Date: Mon, 29 Jan 2024 15:11:16 +0530

> times make code difficult to read.

I had earlier added inline comments in callers of
ReplicationSlotCreate() and walrcv_connect() for new args 'synced' and
'replication' respectively, removing those changes from pacth002 and
patch005 now.
Also improved alter-sub doc in patch001 as suggested by Peter offlist.

PFA v71 patch set with above changes.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Jan 2024 17:00:29 +0530

> PFA v71 patch set with above changes.

Few comments on 0001
===================
1.
parse_subscription_options()
{
...
/*
* We've been explicitly asked to not connect, that requires some
* additional processing.
*/
if (!opts->connect && IsSet(supported_opts, SUBOPT_CONNECT))
{

Here, along with other options, we need an explicit check for
failover, so that if connect=false and failover=true, the statement
should give error. I was expecting the below statement to fail but it
passed with WARNING.
postgres=# create subscription sub2 connection 'dbname=postgres'
publication pub2 with(connect=false, failover=true);
WARNING:  subscription was created, but is not connected
HINT:  To initiate replication, you must manually create the
replication slot, enable the subscription, and refresh the
subscription.
CREATE SUBSCRIPTION

2.
@@ -148,6 +153,10 @@ typedef struct Subscription
  List    *publications; /* List of publication names to subscribe to */
  char    *origin; /* Only publish data originating from the
  * specified origin */
+ bool failover; /* True if the associated replication slots
+ * (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * to be synchronized to the standbys. */
 } Subscription;

Let's add this new field immediately after "bool runasowner;" as is
done for other boolean members. This will help avoid increasing the
size of the structure due to alignment when we add any new pointer
field in the future. Also, that would be consistent with what we do
for other new boolean members.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 29 Jan 2024 13:17:28 +0000

> > PFA v71 patch set with above changes.

Thanks for the comments.

> if (!opts->connect && IsSet(supported_opts, SUBOPT_CONNECT)) {
> 
> Here, along with other options, we need an explicit check for failover, so that if
> connect=false and failover=true, the statement should give error. I was
> expecting the below statement to fail but it passed with WARNING.
> HINT:  To initiate replication, you must manually create the replication slot,
> enable the subscription, and refresh the subscription.
> CREATE SUBSCRIPTION

Added.

> Let's add this new field immediately after "bool runasowner;" as is done for
> other boolean members. This will help avoid increasing the size of the structure
> due to alignment when we add any new pointer field in the future. Also, that
> would be consistent with what we do for other new boolean members.

Moved this field as suggested.

Attach the V72-0001 which addressed above comments, other patches will be
rebased and posted after pushing first patch. Thanks Shveta for helping address
the comments.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 29 Jan 2024 13:47:35 +0000

On Monday, January 29, 2024 9:17 PM Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> > should give error. I was expecting the below statement to fail but it passed
> with WARNING.
> > replication slot, enable the subscription, and refresh the subscription.
> > CREATE SUBSCRIPTION
> 
> Added.
> 
> >
> > 2.
> > field in the future. Also, that would be consistent with what we do for other
> new boolean members.
> 
> rebased and posted after pushing first patch. Thanks Shveta for helping
> address the comments.

Apart from above comments. The new V72 patch also includes the followings changes.

1. Moved the test 'altering failover for enabled sub' to the tap-test where most
of the alter-sub behaviors are tested.

2. Rename the tap-test from 050_standby_failover_slots_sync.pl to
040_standby_failover_slots_sync.pl (the big number 050 was used to avoid
conflict with other newly committed tests). And add the test into meson.build
which was missed.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Tue, 30 Jan 2024 12:59:04 +1100

Here are some review comments for v72-0001

======
doc/src/sgml/ref/alter_subscription.sgml

1.
+      parameter value of the subscription. Otherwise, the slot on the
+      publisher may behave differently from what subscription's
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option says. The slot on the publisher could either be
+      synced to the standbys even when the subscription's
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option is disabled or could be disabled for sync
+      even when the subscription's
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option is enabled.
+     </para>

It is a bit wordy to keep saying "disabled/enabled"

BEFORE
The slot on the publisher could either be synced to the standbys even
when the subscription's failover option is disabled or could be
disabled for sync even when the subscription's failover option is
enabled.

SUGGESTION
The slot on the publisher could be synced to the standbys even when
the subscription's failover = false or may not be syncing even when
the subscription's failover = true.

======
.../t/040_standby_failover_slots_sync.pl

2.
+# Enable subscription
+$subscriber1->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub1 ENABLE");
+
+# Disable failover for enabled subscription
+my ($result, $stdout, $stderr) = $subscriber1->psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub1 SET (failover = false)");
+ok( $stderr =~ /ERROR:  cannot set failover for enabled subscription/,
+ "altering failover is not allowed for enabled subscription");
+

Currently, those tests are under scope the big comment:

+##################################################
+# Test that changing the failover property of a subscription updates the
+# corresponding failover property of the slot.
+##################################################

But that comment is not quite relevant to these tests. So, add another
one just these:

SUGGESTION:
##################################################
# Test that cannot modify the failover option for enabled subscriptions.
##################################################

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Tue, 30 Jan 2024 09:36:21 +0530

> the subscription's failover = true.

I think it is a matter of personal preference because I find the
existing wording in the patch easier to follow. So, I would like to
retain that as it is.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 30 Jan 2024 11:31:18 +0530

> > enable the subscription, and refresh the subscription.

In this regard, I feel we don't need to dump/restore the 'FAILOVER'
option non-binary upgrade paths similar to the 'ENABLE' option. For
binary upgrade, if the failover option is enabled, then we can enable
it using Alter Subscription SET (failover=true). Let's add one test
corresponding to this behavior in
postgresql\src\bin\pg_upgrade\t\004_subscription.

Additionally, we need to update the pg_dump docs for the 'failover'
option. See "When dumping logical replication subscriptions, .." [1].
I think we also need to update the connect option docs in CREATE
SUBSCRIPTION [2].

[1] - https://www.postgresql.org/docs/devel/app-pgdump.html
[2] - https://www.postgresql.org/docs/devel/sql-createsubscription.html

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 30 Jan 2024 16:06:10 +0530

> postgresql\src\bin\pg_upgrade\t\004_subscription.

Changed pg_dump behaviour as suggested and added additional test.

> I think we also need to update the connect option docs in CREATE

Updated docs.

> [2] - https://www.postgresql.org/docs/devel/sql-createsubscription.html

PFA v73-0001 which addresses the above comments. Other patches will be
rebased and posted after pushing this one. Thanks Hou-San for adding
pg_upgrade test for failover.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 30 Jan 2024 18:22:54 +0530

On Tue, Jan 30, 2024 at 4:06 PM shveta malik <[email-masked]> wrote:
> rebased and posted after pushing this one.

Since v73-0001 is pushed, PFA  rest of the patches. Changes are:

1) Rebased the patches.
2) Ran pg_indent on all.
3) patch001: Updated logicaldecoding.sgml for dbname requirement in
primary_conninfo for slot-synchronization.

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 30 Jan 2024 15:45:00 +0000

Hi,

> Yeah, makes sense, will do, thanks!

It's done in [1].

[1]: https://www.postgresql.org/message-id/ZbkYrLPhH%2BRxpZlW%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

---

From: Peter Smith <[email-masked]>
Date: Wed, 31 Jan 2024 12:57:24 +1100

Hi,

I saw that v73-0001 was pushed, but it included some last-minute
changes that I did not get a chance to check yesterday.

Here are some review comments for the new parts of that patch.

======

1.
connect (boolean)

    Specifies whether the CREATE SUBSCRIPTION command should connect
to the publisher at all. The default is true. Setting this to false
will force the values of create_slot, enabled, copy_data, and failover
to false. (You cannot combine setting connect to false with setting
create_slot, enabled, copy_data, or failover to true.)

~

I don't think the first part "Setting this to false will force the
values ... failover to false." is strictly correct.

I think is correct to say all those *other* properties (create_slot,
enabled, copy_data) are forced to false because those otherwise have
default true values. But the 'failover' has default false, so it
cannot get force-changed at all because you can't set connect to false
when failover is true as the second part ("You cannot combine...")
explains.

IMO remove 'failover' from that first sentence.

~~~

2.
          <para>
           Since no connection is made when this option is
           <literal>false</literal>, no tables are subscribed. To initiate
           replication, you must manually create the replication slot, enable
-          the subscription, and refresh the subscription. See
+          the failover if required, enable the subscription, and refresh the
+          subscription. See
           <xref
linkend="logical-replication-subscription-examples-deferred-slot"/>
           for examples.
          </para>

IMO "see the failover if required" is very vague. See what failover?
The slot property failover, or the subscription option failover? And
"see" it for what purpose?

I think the intention was probably to say something like "ensure the
manually created slot has the same matching failover property value as
the subscriber failover option", but that is not clear from the
current text.

======
doc/src/sgml/ref/pg_dump.sgml

3.
    dump can be restored without requiring network access to the remote
    servers.  It is then up to the user to reactivate the subscriptions in a
    suitable way.  If the involved hosts have changed, the connection
-   information might have to be changed.  It might also be appropriate to
+   information might have to be changed.  If the subscription needs to
+   be enabled for
+   <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>,
+   then same needs to be done by executing
+   <link linkend="sql-altersubscription-params-set">
+   <literal>ALTER SUBSCRIPTION ... SET(failover = true)</literal></link>
+   after the slot has been created.  It might also be appropriate to

"then same needs to be done" (English?)

BEFORE
If the subscription needs to be enabled for failover, then same needs
to be done by executing ALTER SUBSCRIPTION ... SET(failover = true)
after the slot has been created.

SUGGESTION
If the subscription needs to be enabled for failover, execute ALTER
SUBSCRIPTION ... SET(failover = true) after the slot has been created.

======

4.
 #define SUBOPT_RUN_AS_OWNER 0x00001000
-#define SUBOPT_LSN 0x00002000
-#define SUBOPT_ORIGIN 0x00004000
+#define SUBOPT_FAILOVER 0x00002000
+#define SUBOPT_LSN 0x00004000
+#define SUBOPT_ORIGIN 0x00008000
+

A spurious blank line was added.

======
src/bin/pg_upgrade/t/004_subscription.pl

5.
-# The subscription's running status should be preserved. Old subscription
-# regress_sub1 should be enabled and old subscription regress_sub2 should be
-# disabled.
+# The subscription's running status and failover option should be preserved.
+# Old subscription regress_sub1 should have enabled and failover as true while
+# old subscription regress_sub2 should have enabled and failover as false.
 $result =
   $new_sub->safe_psql('postgres',
- "SELECT subname, subenabled FROM pg_subscription ORDER BY subname");
-is( $result, qq(regress_sub1|t
-regress_sub2|f),
+ "SELECT subname, subenabled, subfailover FROM pg_subscription ORDER
BY subname");
+is( $result, qq(regress_sub1|t|t
+regress_sub2|f|f),
  "check that the subscription's running status are preserved");

~

Calling those "old subscriptions" seems misleading. Aren't these the
new/upgraded subscriptions being checked here?

Should the comment be more like:

# The subscription's running status and failover option should be preserved.
# Upgraded regress_sub1 should still have enabled and failover as true.
# Upgraded regress_sub2 should still have enabled and failover as false.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 31 Jan 2024 08:48:40 +0530

On Wed, Jan 31, 2024 at 7:27 AM Peter Smith <[email-masked]> wrote:
> default true values.

So, won't when connect=false, the user has to explicitly provide such
values (create_slot, enabled, etc.) as false? If so, is using 'force'
strictly correct?

> IMO "see the failover if required" is very vague. See what failover?

AFAICS, the committed docs says: "To initiate replication, you must
manually create the replication slot, enable the failover if required,
...". I am not sure what you are referring to.

> new/upgraded subscriptions being checked here?

Again the quoted wording is not introduced by this patch. But, I see
your point and it is better if you can start a separate thread for it.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Wed, 31 Jan 2024 16:10:16 +1100

> values (create_slot, enabled, etc.) as false? If so, is using 'force'

Perhaps the original docs text could be worded differently; I think
the word "force" here just meant setting connection=false
forces/causes/makes those other options behave "as if" they had been
set to false without the user explicitly doing anything to them. The
point is they are made to behave *differently* to their normal
defaults.

So,
connect=false ==> this actually sets enabled=false (you can see this
with \dRs+), which is different to the default setting of 'enabled'
connect=false ==> will not create a slot (because there is no
connection), which is different to the default behaviour for
'create-slot'
connect=false ==> will not copy tables (because there is no
connection), which is different to the default behaviour for
'copy_data;'

OTOH, failover is different
connect=false ==> failover is not possible (because there is no
connection), but the 'failover' default is false anyway, so no change
to the default behaviour for this one.

> ...". I am not sure what you are referring to.

My mistake. I was misreading the patch code without applying the
patch. Sorry for the noise.

> your point and it is better if you can start a separate thread for it.

OK. I created a separate thread for this [1]

======
[1] https://www.postgresql.org/message-id/[email-masked]

Kind Regards,
Peter Smith.
Fujitsu Australia.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 31 Jan 2024 07:23:19 +0000

On Wednesday, January 31, 2024 9:57 AM Peter Smith <[email-masked]> wrote:
> +   <link
> linkend="sql-createsubscription-params-with-failover"><literal>failover</lit
> eral></link>,

Here is a small patch to address the comment 3 and 4.
The discussion for comment 1 is still going on, so we can
update the patch once it's concluded.

Best Regards,
Hou zj

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 31 Jan 2024 17:32:00 +0900

> primary_conninfo for slot-synchronization.

Thank you for updating the patches. As for the slotsync worker patch,
is there any reason why 0001, 0002, and 0004 patches are still
separated?

Beside, here are some comments on v74 0001, 0002, and 0004 patches:

---
+static char *
+wait_for_valid_params_and_get_dbname(void)
+{
+   char       *dbname;
+   int         rc;
+
+   /* Sanity check. */
+   Assert(enable_syncslot);
+
+   for (;;)
+   {
+       if (validate_parameters_and_get_dbname(&dbname))
+           break;
+       ereport(LOG, errmsg("skipping slot synchronization"));
+
+       ProcessSlotSyncInterrupts(NULL);

When reading this function, I expected that the slotsync worker would
resume working once the parameters became valid, but it was not
correct. For example, if I changed hot_standby_feedback from off to
on, the slotsync worker reads the config file, exits, and then
restarts. Given that the slotsync worker ends up exiting on parameter
changes anyway, why do we want to have it wait for parameters to
become valid? IIUC even if the slotsync worker exits when a parameter
is not valid, it restarts at some intervals.

---
+bool
+SlotSyncWorkerCanRestart(void)
+{
+#define SLOTSYNC_RESTART_INTERVAL_SEC 10
+

IIUC depending on how busy the postmaster is and the timing, the user
could wait for 1 min to re-launch the slotsync worker. But I think the
user might want to re-launch the slotsync worker more quickly for
example when the slotsync worker restarts due to parameter changes.
IIUC SloSyncWorkerCanRestart() doesn't consider the fact that the
slotsync worker previously exited with 0 or 1.

---
+       /* We are a normal standby */
+       valid = DatumGetBool(slot_getattr(tupslot, 2, &isnull));
+       Assert(!isnull);

What do you mean by "normal standby"?

---
+   appendStringInfo(&cmd,
+                    "SELECT pg_is_in_recovery(), count(*) = 1"
+                    " FROM pg_replication_slots"
+                    " WHERE slot_type='physical' AND slot_name=%s",
+                    quote_literal_cstr(PrimarySlotName));

I think we need to make "pg_replication_slots" schema-qualified.

---
+                   errdetail("The primary server slot \"%s\" specified by"
+                             " \"%s\" is not valid.",
+                             PrimarySlotName, "primary_slot_name"));

and

+               errmsg("slot sync worker will shutdown because"
+                      " %s is disabled", "enable_syncslot"));

It's better to write it in one line for better greppability.

---
When I dropped a database on the primary that has a failover slot, I
got the following logs on the standby:

2024-01-31 17:25:21.750 JST [1103933] FATAL:  replication slot "s" is
active for PID 1103935
2024-01-31 17:25:21.750 JST [1103933] CONTEXT:  WAL redo at 0/3020D20
for Database/DROP: dir 1663/16384
2024-01-31 17:25:21.751 JST [1103930] LOG:  startup process (PID
1103933) exited with exit code 1

It seems that because the slotsync worker created the slot on the
standby, the slot's active_pid is still valid. That is why the startup
process could not drop the slot.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 31 Jan 2024 16:12:25 +0530

> is there any reason why 0001, 0002, and 0004 patches are still

No specific reason, it could be easier to review those parts.

> become valid?

Right, the reason for waiting is to avoid repeated re-start of
slotsync worker if the required parameter is not changed. To follow
that, I think we should simply continue when the required parameter is
changed and is valid. But, I think during actual slotsync, if
connection_info is changed then there is no option but to restart.

> slotsync worker previously exited with 0 or 1.

Considering my previous where we don't want to restart for a required
parameter change, isn't it better to avoid repeated restart (say when
the user gave an invalid dbname)? BTW, I think this restart interval
is added based on your previous complaint [1].

> standby, the slot's active_pid is still valid.
>

But we release the slot after sync. And we do take a shared lock on
the database to make the startup process wait for slotsync. There is
one gap which is that we don't reset active_pid for temp slots in
ReplicationSlotRelease(), so for temp slots such an error can occur
but OTOH, we immediately make the slot persistent after sync. As per
my understanding, it is only possible to get this error if the initial
sync doesn't happen and the slot remains temporary. Is that your case?
How did reproduce this?

 That is why the startup
> process could not drop the slot.

[1] - https://www.postgresql.org/message-id/CAD21AoApGoTZu7D_7%3DbVYQqKnj%2BPZ2Rz%2Bnc8Ky1HPQMS_XL6%2BA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 1 Feb 2024 00:49:37 +0900

> No specific reason, it could be easier to review those parts.

Okay, I think we can merge 0001 and 0002 at least as we don't need
bgworker codes.

Agreed.

> is added based on your previous complaint [1].

I think it's useful that the slotsync worker restarts immediately when
a required parameter is changed but waits to restart when it exits
with an error. IIUC the apply worker does so; if it restarts due to a
subscription parameter change, it resets the last-start time so that
the launcher will restart it without waiting. But if it exits with an
error, the launcher waits for wal_retrieve_retry_interval. I don't
think the slotsync worker must follow this behavior but I feel it's
useful behavior.

> sync doesn't happen and the slot remains temporary. Is that your case?

I created a failover slot manually on the primary and dropped the
database where the failover slot is created. So this would not happen
in normal cases.

BTW I've tested the following switch/fail-back scenario but it seems
not to work fine. Am I missing something?

Setup:
node1 is the primary, node2 is the physical standby for node1, and
node3 is the subscriber connecting to node1.

Steps:
1. [node1]: create a table and a publication for the table.
2. [node2]: set enable_syncslot = on and start (to receive WALs from node1).
3. [node3]: create a subscription with failover = true for the publication.
4. [node2]: promote to the new standby.
5. [node3]: alter subscription to connect the new primary, node2.
6. [node1]: stop, set enable_syncslot = on (and other required
parameters), then start as a new standby.

Then I got the error "exiting from slot synchronization because same
name slot "test_sub" already exists on the standby".

The logical replication slot that was created on the old primary
(node1) has been synchronized to the old standby (node2). Therefore on
node2, the slot's "synced" field is true. However, once node1 starts
as the new standby with slot synchronization, the slotsync worker
cannot synchronize the slot because the slot's "synced" field on the
primary is false.

Regards,