Subject: RE: Synchronizing slots from primary to standby

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 18 Dec 2023 12:12:26 +0000

On Friday, December 15, 2023 1:32 PM shveta malik <[email-masked]> wrote:
> 
> TODO:
> --Address the test comments in [1] for 050_standby_failover_slots_sync.pl
> --Review the feasibility of addressing one pending comment (comment 13 in
> [5]) of 'r'->'n' conversion.

Here is the V49 patch set which addressed above TODO items.

The patch also includes the following changes:

V49-0001

1) added some documents to mention it's user responsibility to ensure the table
   sync is completed before subscriber to the new primary.
2) fix one CFbot failure in 050_standby_failover_slots_sync.pl.

V49-0002
1) added few comments to mention why we retain the READY state after promotion.
2) Prevent user from altering the slots that is being synced.
3) fix one CFbot failure in 050_standby_failover_slots_sync.pl.
4) Improve the 050_standby_failover_slots_sync.pl to remove some unnecessary
   operations.

V49-0003

There is one unstable test in V48-0002 which is to validate the restart_lsn of
synced slot. We test it by checking "'$primary_restart_lsn' <= restart_lsn"
which would wrongly allow the standby to go ahead of primary. And it may fail
randomly as standby may still be lagging behind primary if the slot-sync worker
has gone to longer nap (10 sec) and has not taken the slots-changes yet.  And
we cannot put sleep of 10sec here.

We may consider removing this test as it may be enough to test
that logical replication is proceeding well from the synced slots on new
primary. So, I temporarily move it into a separately patch for review.

Thanks Ajin for working on the testcases improvement.

> 
> [1]:
> https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTu
> D3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com
> [5]:
> https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%
> 2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Tue, 19 Dec 2023 12:27:34 +1100

Here are some comments for the patch v49-0002.

(This is in addition to my review comments for v48-0002 [1])

======
src/backend/access/transam/xlogrecovery.c

1. FinishWalRecovery

+ *
+ * We do not update the sync_state from READY to NONE here, as any failed
+ * update could leave some slots in the 'NONE' state, causing issues during
+ * slot sync after restarting the server as a standby. While updating after
+ * switching to the new timeline is an option, it does not simplify the
+ * handling for both READY and NONE state slots. Therefore, we retain the
+ * READY state slots after promotion as they can provide useful information
+ * about their origin.
+ */

Do you know if that wording is correct? e.g., If you were updating
from READY to NONE and there was a failed update, that would leave
some slots still in a READY state, right? So why does the comment say
"could leave some slots in the 'NONE' state"?

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

2. ReplicationSlotAlter

+ /*
+ * Do not allow users to drop the slots which are currently being synced
+ * from the primary to the standby.
+ */
+ if (RecoveryInProgress() &&
+ MyReplicationSlot->data.sync_state != SYNCSLOT_STATE_NONE)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter replication slot \"%s\"", name),
+ errdetail("This slot is being synced from the primary server.")));
+

The comment looks wrong -- should say "Do not allow users to alter..."

======

3.
+##################################################
+# Test that synchronized slot can neither be decoded nor dropped by the user
+##################################################
+

3a,
/Test that synchronized slot/Test that a synchronized slot/

3b.
Isn't there a missing test? Should this part also check that it cannot
ALTER the replication slot being synced? e.g. test for the new v49
error message that was added in ReplicationSlotAlter()

~~~

4.
+# Disable hot_standby_feedback
+$standby1->safe_psql('postgres', 'ALTER SYSTEM SET
hot_standby_feedback = off;');
+$standby1->restart;
+

Can there be a comment added to explain why you are doing the
'hot_standby_feedback' toggle?

~~~

5.
+##################################################
+# Promote the standby1 to primary. Confirm that:
+# a) the sync-ready('r') slot 'lsub1_slot' is retained on the new primary
+# b) the initiated('i') slot 'logical_slot' is dropped on promotion
+# c) logical replication for regress_mysub1 is resumed succesfully
after failover
+##################################################

/succesfully/successfully/

~~~

6.
+
+# Confirm that data in tab_mypub3 replicated on subscriber
+is( $subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}),
+ "$primary_row_count",
+ 'data replicated from the new primary');

The comment is wrong -- it names a different table ('tab_mypub3' ?) to
what the SQL says.

======
[1] My v48-0002 review comments.
https://www.postgresql.org/message-id/CAHut%2BPsyZQZ1A4XcKw-D%3DvcTg16pN9Dw0PzE8W_X7Yz_bv00rQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Tue, 19 Dec 2023 17:30:09 +0530

> Here are some comments for the patch v49-0002.

Thanks for reviewing. I have addressed these in v50.

> "could leave some slots in the 'NONE' state"?

yes, it the comment is correct as stated in [1]

[1]: https://www.postgresql.org/message-id/CAA4eK1LoJSbFJwa%3D97_5qHNAVfOkmfc40W_SFMVBbm6r0%3DPXHQ%40mail.gmail.com

---

From: Amit Kapila <[email-masked]>
Date: Wed, 20 Dec 2023 09:12:00 +0530

> Thanks for reviewing. I have addressed these in v50.

I was looking at this patch to see if something smaller could be
independently committable. I think we can extract
pg_get_slot_invalidation_cause() and commit it as that function could
be independently useful as well. What do you think?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 20 Dec 2023 15:29:31 +0530

> be independently useful as well. What do you think?

Sure, forked another thread [1]
[1]: https://www.postgresql.org/message-id/CAJpy0uBpr0ym12%2B0mXpjcRFA6N%3DanX%2BYk9aGU4EJhHNu%3DfWykQ%40mail.gmail.com

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 20 Dec 2023 17:06:51 +0530

> [1]: https://www.postgresql.org/message-id/CAJpy0uBpr0ym12%2B0mXpjcRFA6N%3DanX%2BYk9aGU4EJhHNu%3DfWykQ%40mail.gmail.com

Thanks, thinking more, we can split the patch into the following three
patches which can be committed separately (a) Allowing the failover
property to be set for a slot via SQL API and subscription commands
(b) sync slot worker infrastructure (c) GUC standby_slot_names and the
the corresponding wait logic in server-side.

Thoughts?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 26 Dec 2023 11:09:57 +0000

On Wednesday, December 20, 2023 7:37 PM Amit Kapila <[email-masked]> wrote:
> > > pg_get_slot_invalidation_cause() and commit it as that function
> > > could be independently useful as well. What do you think?
> > >
> >
> https://www.postgresql.org/message-id/CAJpy0uBpr0ym12%2B0mXpjcRFA6
> N%3D
> > anX%2BYk9aGU4EJhHNu%3DfWykQ%40mail.gmail.com
> >
> 
> Thanks, thinking more, we can split the patch into the following three patches
> which can be committed separately (a) Allowing the failover property to be set
> for a slot via SQL API and subscription commands
> (b) sync slot worker infrastructure (c) GUC standby_slot_names and the the
> corresponding wait logic in server-side.
> 
> Thoughts?

I agree. Here is the V54 patch set which was split based on the suggestion.
The commit message in each patch is also improved.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Tue, 26 Dec 2023 17:57:19 +0530

> The commit message in each patch is also improved.

I would like to revisit the current dependency of slotsync worker on
dbname used in 002 patch. Currently we accept dbname in
primary_conninfo and thus the user has to make sure to provide one (by
manually altering it) even in case of a conf file auto-generated by
"pg_basebackup -R".
Thus I would like to discuss if there are better ways to do it.
Complete background is as follow:

We need dbname for 2 purposes:

1) to connect to remote db in order to run SELECT queries to fetch the
info needed by slotsync worker.
2) to make connection in slot-sync worker itself in order to be able
to use libpq APIs for 1)

We run 3 kind of select queries in slot-sync worker currently:

a) To fetch all failover slots (logical slots) info at once in
synchronize_slots().
b) To fetch a particular slot info during
wait_for_primary_slot_catchup() logic (logical slot).
c) To validate primary slot (physical one) and also to distinguish
between standby and cascading standby by running pg_is_in_recovery().

 1) One approach to avoid dependency on dbname is using commands
instead of SELECT.  This will need implementing LIST_SLOTS command for
a), and for b) we can use LIST_SLOTS and fetch everything (even though
it is not needed) or have LIST_SLOTS with a filter on slot-name or
extend READ_REPLICATION_SLOT,  and for c) we can have some other
command to get pg_is_in_recovery() info. But, I feel by relying on
commands we will be making the extension of the slot-sync feature
difficult. In future, if there is some more requirement to fetch any
other info,
then there too we have to implement a command. I am not sure if it is
good and extensible approach.

2) Another way to avoid asking for a dbname in primary_conninfo is to
use the default dbname internally. This brings us to two questions:
'How' and 'Which default db'?

2.1) To answer 'How':
Using default dbname is simpler for the purpose of slot-sync worker
having its own db-connection, but is a little tricky for the purpose
of connection to remote_db. This is because we have to inject this
dbname internally in our connection-info.

2.1.1) Say we use primary_conninfo (i.e. original one w/o dbname),
then currently it could have 2 formats:

a) The simple "=" format for key-value pairs, example:
'user=replication host=127.0.0.1 port=5433 dbname=postgres'.
b) URI format, example:
postgresql://other@localhost/otherdb?connect_timeout=10&application_name=myapp

We can distinguish between the 2 formats using 'uri_prefix_length' but
injecting the dbname part will be messy specially for URI format.  If
we want to do it w/o injecting and only by changing libpq interfaces
to accept dbname separately apart from conninfo, then there is no
current simpler way available. It will need a good amount of changes
in libpq.

2.1.2) Another way is to not rely on primary_conninfo directly but
rely on 'WalRcv->conninfo' in order to connect to remote_db. This is
because the latter is never URI format, it is some parsed format and
appending may work. As an example, primary_conninfo =
'postgresql://replication@localhost:5433', WalRcv->conninfo loaded
internally is:
"user=replication passfile=/home/shveta/.pgpass channel_binding=prefer
dbname=replication host=localhost port=5433
fallback_application_name=walreceiver sslmode=prefer sslcompression=0
sslcertmode=allow sslsni=1 ssl_min_protocol_version=TLSv1.2
gssencmode=disable krbsrvname=postgres gssdelegation=0
target_session_attrs=any load_balance_hosts=disable", '\000'

So we can try appending our default dbname to this. But all the
defaults loaded in WalRcv->conninfo need some careful analysis to
figure out if they work for slot-sync worker case.

2.2) Now coming to 'Which default db':

2.2.1) If we use 'template1' as default db, it may block 'create db'
operations on primary for the time when the slot-sync worker is
connected to remote using this dbname. Example:

postgres=# create database newdb1;
ERROR:  source database "template1" is being accessed by other users
DETAIL:  There is 1 other session using the database.

2.2.2) If we use 'postgres' as default db, there are chances that it
can be dropped as unlike 'template1', it is allowed to be dropped by
user, and if slotsync worker is connected to it, user may see:
newdb1=# drop database postgres;
ERROR:  database "postgres" is being accessed by other users
DETAIL:  There is 1 other session using the database.

But once the slot-sync worker or standby goes down, user can always
drop this and next time slot-sync worker may not be able to come up.

================

As explained, there is no clean approach to avoid dbname dependency
and thus making us implement it this way where we ask dbname in
primary_conninfo.  It will be good to know what others think on this
and if there are better ways to do it.

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 27 Dec 2023 15:06:13 +0900

Hi,

Thank you for working on this.

> drop this and next time slot-sync worker may not be able to come up.

Other random ideas for discussion are:

3) The slotsync worker uses primary_conninfo but also uses a new GUC
parameter, say slot_sync_dbname, to specify the database to connect.
The slot_sync_dbname overwrites the dbname if primary_conninfo also
specifies it. If both don't have a dbname, raise an error.

4) The slotsync worker uses a new GUC parameter, say
slot_sync_conninfo, to specify the connection string to the primary
aside from primary_conninfo. And pg_basebackup -R generates
slot_sync_conninfo as well if required (new option required).

BTW given that the slotsync worker executes only normal SQL queries,
is there any reason why it uses a replication connection? It's
slightly odd to me that the pg_stat_replication view shows one entry
that remains in the "startup" state.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Dec 2023 15:43:44 +0530

On Wed, Dec 27, 2023 at 11:36 AM Masahiko Sawada <[email-masked]> wrote:
> is there any reason why it uses a replication connection?

Thank You for the feedback.
Do you mean why are we using libpqwalreceiver.c APIs instead of using
libpq directly? I was not aware if there is any way to connect if we
want to run SQL queries. I initially tried using 'PQconnectdbParams'
but couldn't make it work. Perhaps it is to be used only by front-end
and extensions as the header files indicate as well:
 * libpq-fe.h :      This file contains definitions for structures and
 externs for functions used by frontend postgres applications.
 * libpq-be-fe-helpers.h:   Helper functions for using libpq in
extensions . Code built directly into the backend is not allowed to
link to libpq directly.

Do you mean some other kind of connection here?

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 Dec 2023 16:12:53 +0530

> specifies it. If both don't have a dbname, raise an error.

Would the users prefer to provide a value for a separate GUC instead
of changing primary_conninfo? It is possible that we can have some
users prefer to use one GUC and others prefer a separate GUC but we
should add a new GUC if we are sure that is what users would prefer.
Also, even if have to consider this option, I think we can easily
later add a new GUC to provide a dbname in addition to having the
provision of giving it in primary_conninfo.

Also, I think having a separate GUC for dbanme has some complexity in
terms of appending the dbname to primary_conninfo as pointed out by
Shveta.

> slot_sync_conninfo as well if required (new option required).

Yeah, this is worth considering but won't slot_sync_conninfo be mostly
a duplicate of primary_conninfo apart from dbname? I am not sure if
the benefit outweighs the disadvantage of having mostly similar
information in two GUCs.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Thu, 28 Dec 2023 16:03:09 +0530

> the benefit outweighs the disadvantage of having mostly similar

PFA v55. It has fixes for 2 CFBot failures seen on v53 and 1 CFBot
failure seen on v54.

patch002:
1) In 32-bit env, a Datum for int64 is treated  as a pointer, and thus
below leads to NULL pointer access if the concerned attribute is NULL.
Corrected it now.
DatumGetLSN(slot_getattr(tupslot, 3, &isnull));

2)During slot-creation on standby it is possible to get NULL
confirmed_lsn from primary even for a valid slot with valid
restart_lsn. This may happen when a slot is just created on primary
with valid restart_lsn and slot-sync worker has fetched it before
primary could set valid confirmed_lsn. And thus along with
remote_slot's restart_lsn to catch up, we also need to check for
non-null confirmed_lsn of remote_slot.

patch003:
3) Another intermittent failure was due to an unstable test added in
050_standby_failover_slots_sync.pl. It has now been removed.  The
other tests already have the coverage which the problematic test was
trying to achieve. Thank You Hou-san for working on this.

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 29 Dec 2023 10:29:21 +0900

> provision of giving it in primary_conninfo.

I think having two separate GUCs is more flexible for example when
users want to change the dbname to connect. It makes sense that the
slotsync worker wants to use the same connection string as the
walreceiver uses. But I guess today most primary_conninfo settings
that are set manually or are generated by tools such as pg_basebackup
don't have dbname. If we require a dbname in primary_conninfo, many
tools will need to be changed. Once the connection string is
generated, it would be tricky to change the dbname in it, as Shveta
mentioned. The users will have to carefully select the database to
connect when taking a base backup.

> terms of appending the dbname to primary_conninfo as pointed out by

I think we don't necessarily need to append the dbname to the
connection string in order to specify/change the database to connect.
PQconnectdbParams() overrides the database name to connect if the
dbname parameter appears twice in the connection keyword. The
documentation[1] says:

When expand_dbname is non-zero, the value for the first dbname key
word is checked to see if it is a connection string. If so, it is
“expanded” into the individual connection parameters extracted from
the string. The value is considered to be a connection string, rather
than just a database name, if it contains an equal sign (=) or it
begins with a URI scheme designator. (More details on connection
string formats appear in Section 33.1.1.) Only the first occurrence of
dbname is treated in this way; any subsequent dbname parameter is
processed as a plain database name.

In general the parameter arrays are processed from start to end. If
any key word is repeated, the last value (that is not NULL or empty)
is used. This rule applies in particular when a key word found in a
connection string conflicts with one appearing in the keywords array.
Thus, the programmer may determine whether array entries can override
or be overridden by values taken from a connection string. Array
entries appearing before an expanded dbname entry can be overridden by
fields of the connection string, and in turn those fields are
overridden by array entries appearing after dbname (but, again, only
if those entries supply non-empty values).

If the slotsync worker needs to use libpqwalreceiver to connect the
primary, we will need to change libpqrcv_connect(). But we have the
infrastructure to change the database name to connect without changing
the connection string, at least.

> the benefit outweighs the disadvantage of having mostly similar

Agreed.

Regards,

[1] https://www.postgresql.org/docs/devel/libpq-connect.html#LIBPQ-PQCONNECTDBPARAMS

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 29 Dec 2023 10:47:39 +0900

On Wed, Dec 27, 2023 at 7:13 PM shveta malik <[email-masked]> wrote:
> libpq directly?

Yes, I meant to use libpq directly, to connect a backend process but
not a walsender process.

>  I was not aware if there is any way to connect if we

Oh I didn't know that. Thank you for pointing it out.

But I'm still concerned it could confuse users that
pg_stat_replication keeps showing one entry that remains as "startup"
state. It has the same application_name as the walreceiver uses. For
example, when users want to check the particular replication
connection, it's common to filter the entries by the application name.
But it will end up having duplicate entries having different states.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 Dec 2023 10:25:20 +0530

> But it will end up having duplicate entries having different states.

Valid point. The main reason for using cluster_name is that if
multiple standby's connect to the same primary, all will have the same
application_name as 'slotsyncworker'. The other alternative could be
to use {cluster_name}_slotsyncworker, which will probably address your
concern and we can have to provision to differentiate among
slotsyncworkers from different standby's.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 Dec 2023 12:32:08 +0530

> connect when taking a base backup.

I see your point and agree that users need to be careful. I was trying
to compare it with other places like the conninfo used with a
subscription where no separate dbname needs to be provided. Now, here
the situation is not the same because the same conninfo is used for
different purposes (walreceiver doesn't require dbname (dbname is
ignored even if present) whereas slotsyncworker requires dbname). I
was just trying to see if we can avoid having a new GUC for this
purpose. Does anyone else have an opinion on this matter?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 2 Jan 2024 14:53:09 +0530

> purpose. Does anyone else have an opinion on this matter?

Attaching the rebased patches. A recent commit 9a17be1e2 has resulted
in conflicts in pg_dump changes.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 2 Jan 2024 16:01:48 +0530

On Fri, Dec 29, 2023 at 10:25 AM Amit Kapila <[email-masked]> wrote:
> > state.

Okay. I understand your concern. I have attached PoC patch
(v55_02-0004) which attempts to implement non-replication connection
in slotsync worker. By doing so, pg_stat_replication will not show its
entry while pg_stat_activity will still show it with 'state' as either
"active" or "idle". Currently, since we are not using any of the
replication cmds, thus non-replication connection suits well. But in
future, if there is a requirement to execute existing (or new) cmd in
slotsync worker, then that can not be done simply in non-replication
connection; it will need some changes in non-replication  or will need
the replication connection itself.

>> It has the same application_name as the walreceiver uses. For

The topup patch has also changed app_name to
{cluster_name}_slotsyncworker so that we do not confuse between
walreceiver and slotsyncworker entry.

Please note that there is no change in rest of the patches, changes
are in additional 0004 patch alone.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 3 Jan 2024 16:20:03 +0530

> purpose. Does anyone else have an opinion on this matter?

Bertrand, Dilip, and others involved in this thread or otherwise, see
if you can share an opinion on the above point because it would be
good to get some more opinions before we decide to add a new GUC (for
dbname) for slotsync worker.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 3 Jan 2024 11:26:57 +0000

Hi,

> dbname) for slotsync worker.

I think that as long as enable_syncslot is off then there is no need to add the
dbname in primary_conninfo (means there is no need to change an existing primary_conninfo
for the ones that don't use the sync slot feature).

So given that primary_conninfo does not necessary need to be changed (for ones that
don't use the sync slot feature) and that adding a new GUC looks more a one-way door
change to me, I'd vote to keep the patch as it is (we can still revisit this later
on and add a new GUC if we feel the need based on user's feedback).

Regards,

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

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 3 Jan 2024 13:02:50 +0000

On Tuesday, January 2, 2024 6:32 PM shveta malik <[email-masked]> wrote:
> On Fri, Dec 29, 2023 at 10:25 AM Amit Kapila <[email-masked]>
> 
> {cluster_name}_slotsyncworker so that we do not confuse between walreceiver
> and slotsyncworker entry.
> 
> Please note that there is no change in rest of the patches, changes are in
> additional 0004 patch alone.

Attach the V56 patch set which supports ALTER SUBSCRIPTION SET (failover).
This is useful when user want to refresh the publication tables, they can now alter the
failover option to false and then execute the refresh command.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jan 2024 09:18:46 +0530

> failover option to false and then execute the refresh command.

The patches no longer apply to HEAD due to a recent commit 007693f. I
am working on rebasing and will post the new patches soon

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jan 2024 10:27:31 +0530

> am working on rebasing and will post the new patches soon

Commit 007693f has changed 'conflicting' to 'conflict_reason', so
adjusted the code around that in the slotsync worker.

Also removed function 'pg_get_slot_invalidation_cause' as now
conflict_reason tells the same.

PFA rebased patches with above changes.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 4 Jan 2024 11:48:50 +0530

> dbname) for slotsync worker.

IMHO, as of now we can only use the primary_coninfo and let the user
modify this and add the dbname to this.  In the future, if this
creates some discomfort or we see some complaints about the usage then
we can expand the behavior by providing an additional GUC with dbname.

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Jan 2024 14:22:39 +0530

> on and add a new GUC if we feel the need based on user's feedback).

Okay, thanks for the feedback. Dilip also shares the same opinion, so
let's wait and see if there is any strong argument to add this new
GUC.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 4 Jan 2024 13:54:44 +0000

Hi,

> PFA rebased patches with above changes.

Thanks!

Looking at 0004:

1 ====

-libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
-                                const char *appname, char **err)
+libpqrcv_connect(const char *conninfo, bool replication, bool logical,
+                                bool must_use_password, const char *appname, char **err)

What about adjusting the preceding comment a bit to describe what the new replication
parameter is for?

2 ====

+       /* We can not have logical w/o replication */

what about replacing w/o by without?

3 ===

+       if(!replication)
+               Assert(!logical);
+
+       if (replication)
        {

what about using "if () else" instead (to avoid unnecessary test)?

Having said that the patch seems a reasonable way to implement non-replication
connection in slotsync worker.

4 ===

Looking closer, the only place where walrcv_connect() is called with replication
set to false and logical set to false is in ReplSlotSyncWorkerMain().

That does make sense, but what do you think about creating dedicated libpqslotsyncwrkr_connect
and slotsyncwrkr_connect (instead of using the libpqrcv_connect / walrcv_connect ones)?

That way we could make use of slotsyncwrkr_connect() in ReplSlotSyncWorkerMain()
as I think it's confusing to use "rcv" functions while the process using them is
not of backend type walreceiver.

I'm not sure that worth the extra complexity though, what do you think?

Regards,

---

From: Peter Smith <[email-masked]>
Date: Tue, 9 Jan 2024 12:17:27 +1100

Here are some review comments for patch v57-0001.

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

1. CREATE_REPLICATION_SLOT ... FAILOVER

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

This syntax says passing the boolean value is optional. So the default
needs to the specified here in the docs (like what the TWO_PHASE
option does).

~~~

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.

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

3.
+   If <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+   is enabled, you can temporarily disable it in order to execute
these commands.

/in order to/to/

~~~

4.
+     <para>
+      When altering the
+      <link linkend="sql-createsubscription-params-with-slot-name"><literal>slot_name</literal></link>,
+      the <literal>failover</literal> property of the new 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 failover property matches the
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      parameter value of the subscription.
+     </para>

4a.
the <literal>failover</literal> property of the new slot may differ

Maybe it would be more clear if that said "the failover property value
of the named slot...".

~

4b.
In the "failover property matches" part should that failover also be
rendered as <literal> like before in the same paragraph?

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

5.
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>failover</structfield> <type>bool</type>
+      </para>
+      <para>
+       True if this logical slot is 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>

/True if this logical slot is enabled.../True if this is a logical
slot enabled.../

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

6. CreateSubscription

+ /*
+ * Even if failover is set, don't create the slot with failover
+ * enabled. Will enable it once all the tables are synced and
+ * ready. The intention is that if failover happens at the time of
+ * table-sync, user should re-launch the subscription instead of
+ * relying on main slot (if synced) with no table-sync data
+ * present. When the subscription has no tables, leave failover as
+ * false to allow ALTER SUBSCRIPTION ... REFRESH PUBLICATION to
+ * work.
+ */
+ if (opts.failover && !opts.copy_data && tables != NIL)
+ failover_enabled = true;

AFAICT it might be possible for this to set failover_enabled = true if
copy_data is false. So failover_enabled would be true when later
calling:
  walrcv_create_slot(wrconn, opts.slot_name, false, twophase_enabled,
    failover_enabled, CRS_NOEXPORT_SNAPSHOT, NULL);

Isn't that contrary to what this comment said: "Even if failover is
set, don't create the slot with failover enabled"

~~~

7. AlterSubscription. case ALTER_SUBSCRIPTION_OPTIONS:

+ if (!sub->slotname)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set failover for subscription that does not have slot name")));

/for subscription that does not have slot name/for a subscription that
does not have a slot name/

======
.../libpqwalreceiver/libpqwalreceiver.c

8.
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("could not alter replication slot \"%s\"",
+ slotname)));

This used to display the error message like
pchomp(PQerrorMessage(conn->streamConn)) but it was removed. Is it OK?

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

9.
+ if (MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING)
+ ereport(LOG,
+ /* translator: %s is a subscription option */
+ (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that %s can be enabled",
+ MySubscription->name, "two_phase")));
+
+ if (MySubscription->failoverstate == LOGICALREP_FAILOVER_STATE_PENDING)
+ ereport(LOG,
+ /* translator: %s is a subscription option */
+ (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that %s can be enabled",
+ MySubscription->name, "failover")));

Those errors have multiple %s, so the translator's comment should say
"the 2nd %s is a..."

~~~

10.
 void
-UpdateTwoPhaseState(Oid suboid, char new_state)
+EnableTwoPhaseFailoverTriState(Oid suboid, bool enable_twophase,
+    bool enable_failover)

I felt the function name was a bit confusing. Maybe it is simpler to
call it like "EnableTriState" or "EnableSubTriState" -- the parameters
anyway specify what actual state(s) will be set.

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

11.
+ /* Update twophase and/or failover */
+ EnableTwoPhaseFailoverTriState(MySubscription->oid, twophase_pending,
+    failover_pending);
+ if (twophase_pending)
+ MySubscription->twophasestate = LOGICALREP_TWOPHASE_STATE_ENABLED;
+
+ if (failover_pending)
+ MySubscription->failoverstate = LOGICALREP_FAILOVER_STATE_ENABLED;

Can't you pass the MySubscription as a parameter and then the
EnableTwoPhaseFailoverTriState can also set these
LOGICALREP_TWOPHASE_STATE_ENABLED/LOGICALREP_FAILOVER_STATE_ENABLED
states within the Enable* function?

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

12.
 %token K_CREATE_REPLICATION_SLOT
 %token K_DROP_REPLICATION_SLOT
+%token K_ALTER_REPLICATION_SLOT

and

+ create_replication_slot drop_replication_slot
+ alter_replication_slot identify_system read_replication_slot
+ timeline_history show upload_manifest

and

  | create_replication_slot
  | drop_replication_slot
+ | alter_replication_slot

and

  | K_CREATE_REPLICATION_SLOT { $$ = "create_replication_slot"; }
  | K_DROP_REPLICATION_SLOT { $$ = "drop_replication_slot"; }
+ | K_ALTER_REPLICATION_SLOT { $$ = "alter_replication_slot"; }

etc.

~

Although it makes no difference IMO it is more natural to code
everything in the order: create, alter, drop.

======
src/backend/replication/repl_scanner.l

13.
 CREATE_REPLICATION_SLOT { return K_CREATE_REPLICATION_SLOT; }
 DROP_REPLICATION_SLOT { return K_DROP_REPLICATION_SLOT; }
+ALTER_REPLICATION_SLOT { return K_ALTER_REPLICATION_SLOT; }

and

  case K_CREATE_REPLICATION_SLOT:
  case K_DROP_REPLICATION_SLOT:
+ case K_ALTER_REPLICATION_SLOT:

Although it makes no difference IMO it is more natural to code
everything in the order: create, alter, drop.

======

14.
+ if (SlotIsPhysical(MyReplicationSlot))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use %s with a physical replication slot",
+    "ALTER_REPLICATION_SLOT"));

/with a/for a/

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

15.
+static void
+ParseAlterReplSlotOptions(AlterReplicationSlotCmd *cmd, bool *failover)
+{
+ ListCell   *lc;
+ bool failover_given = false;
+
+ /* Parse options */
+ foreach(lc, cmd->options)
+ {
+ DefElem    *defel = (DefElem *) lfirst(lc);

AFAIK there are some new-style macros now you can use for this code.
e.g. foreach_ptr? See [1].

~~~

16.
+ if (strcmp(defel->defname, "failover") == 0)
+ {
+ if (failover_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));
+ failover_given = true;
+ *failover = defGetBoolean(defel);
+ }

The documented syntax showed that passing the boolean value for the
FAILOVER option is not mandatory. Does this code work if the boolean
value is not passed?

======
src/bin/psql/tab-complete.c

17.
I think "ALTER SUBSCRIPTION ... SET (failover)" is possible, but the
ALTER SUBSCRIPTION tab completion code is missing.

======
src/include/nodes/replnodes.h

18.
+/* ----------------------
+ * ALTER_REPLICATION_SLOT command
+ * ----------------------
+ */
+typedef struct AlterReplicationSlotCmd
+{
+ NodeTag type;
+ char    *slotname;
+ List    *options;
+} AlterReplicationSlotCmd;
+
+

Same as an earlier comment. Although it makes no difference IMO it is
more natural to define these structs in the order:
CreateReplicationSlotCmd, then AlterReplicationSlotCmd, then
DropReplicationSlotCmd.

======
.../t/050_standby_failover_slots_sync.pl

19.
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+

/2023/2024/

~~~

20.
+# Create another subscription (using the same slot created above) that enables
+# failover.
+$subscriber1->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE tab_int (a int PRIMARY KEY);
+ CREATE SUBSCRIPTION regress_mysub1 CONNECTION '$publisher_connstr'
PUBLICATION regress_mypub WITH (slot_name = lsub1_slot,
copy_data=false, failover = true, create_slot = false);

The comment should not say "Create another subscription" because this
is the first subscription being created.

/another/a/

~~~

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

/Test if/Test that/

======
src/test/regress/sql/subscription.sql

22.
+CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, failover = true);
+
+\dRs+

This is currently only testing the explicit "failover=true".

Maybe you can also test the other kinds work as expected:
- explicit "SET (failover=false)"
- explicit "SET (failover)" with no value specified

======
[1] https://github.com/postgres/postgres/commit/14dd0f27d7cd56ffae9ecdbe324965073d01a9ff

Kind Regards,
Peter Smith.
Fujitsu Australia