Re: Synchronizing slots from primary to standby
Amit Kapila <amit.kapila16@gmail.com>
On Thu, Nov 16, 2023 at 5:34 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> PFA v35.
>
Review v35-0002*
==============
1.
As quoted in the commit message,
>
If a logical slot is invalidated on the primary, slot on the standby is also
invalidated. If a logical slot on the primary is valid but is invalidated
on the standby due to conflict (say required rows removed on the primary),
then that slot is dropped and recreated on the standby in next sync-cycle.
It is okay to recreate such slots as long as these are not consumable on the
standby (which is the case currently).
>
I think this won't happen normally because of the physical slot and
hot_standby_feedback but probably can occur in cases like if the user
temporarily switches hot_standby_feedback from on to off. Are there
any other reasons? I think we can mention the cases along with it as
well at least for now. Additionally, I think this should be covered in
code comments as well.
2.
#include "postgres.h"
-
+#include "access/genam.h"
Spurious line removal.
3.
A password needs to be provided too, if the sender demands password
authentication. It can be provided in the
<varname>primary_conninfo</varname> string, or in a separate
- <filename>~/.pgpass</filename> file on the standby server (use
- <literal>replication</literal> as the database name).
- Do not specify a database name in the
- <varname>primary_conninfo</varname> string.
+ <filename>~/.pgpass</filename> file on the standby server.
+ </para>
+ <para>
+ Specify <literal>dbname</literal> in
+ <varname>primary_conninfo</varname> string to allow synchronization
+ of slots from the primary server to the standby server.
+ This will only be used for slot synchronization. It is ignored
+ for streaming.
Is there a reason to remove part of the earlier sentence "use
<literal>replication</literal> as the database name"?
4.
+ <primary><varname>enable_syncslot</varname> configuration
parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ It enables a physical standby to synchronize logical failover slots
+ from the primary server so that logical subscribers are not blocked
+ after failover.
+ </para>
+ <para>
+ It is enabled by default. This parameter can only be set in the
+ <filename>postgresql.conf</filename> file or on the server
command line.
+ </para>
I think you forgot to update the documentation for the default value
of this variable.
5.
+ * a) start the logical replication workers for every enabled subscription
+ * when not in standby_mode
+ * b) start the slot-sync worker for logical failover slots synchronization
+ * from the primary server when in standby_mode.
Either use a full stop after both lines or none of these.
6.
+static void slotsync_worker_cleanup(SlotSyncWorkerInfo * worker);
There shouldn't be space between * and the worker.
7.
+ if (!SlotSyncWorker->hdr.in_use)
+ {
+ LWLockRelease(SlotSyncWorkerLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("replication slot-sync worker not initialized, "
+ "cannot attach")));
+ }
+
+ if (SlotSyncWorker->hdr.proc)
+ {
+ LWLockRelease(SlotSyncWorkerLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("replication slot-sync worker is "
+ "already running, cannot attach")));
+ }
Using slot-sync in the error messages looks a bit odd to me. Can we
use "replication slot sync worker ..." in both these and other
similar messages? I think it would be better if we don't split the
messages into multiple lines in these cases as messages don't appear
too long to me.
8.
+/*
+ * Detach the worker from DSM and update 'proc' and 'in_use'.
+ * Logical replication launcher will come to know using these
+ * that the worker has shutdown.
+ */
+void
+slotsync_worker_detach(int code, Datum arg)
+{
I think the reference to DSM is leftover from the previous version of
the patch. Can we change the above comments as per the new code?
9.
+static bool
+slotsync_worker_launch()
{
...
+ /* TODO: do we really need 'generation', analyse more here */
+ worker->hdr.generation++;
We should do something about this TODO. As per my understanding, we
don't need a generation number for the slot sync worker as we have one
such worker but I guess the patch requires it because we are using
existing logical replication worker infrastructure. This brings the
question of whether we really need a separate SlotSyncWorkerInfo or if
we can use existing LogicalRepWorker and distinguish it with
LogicalRepWorkerType? I guess you didn't use it because most of the
fields in LogicalRepWorker will be unused for slot sync worker.
10.
+ * Can't use existing functions like 'get_database_oid' from dbcommands.c for
+ * validity purpose as they need db connection.
+ */
+static bool
+validate_dbname(const char *dbname)
I don't know how important it is to validate the dbname before
launching the sync slot worker because anyway after launching, it will
give an error while initializing the connection if the dbname is
invalid. But, if we think it is really required, did you consider
using GetDatabaseTuple()?
--
With Regards,
Amit Kapila.
Commits
-
Doc: Add the new section "Logical Replication Failover".
- b560a98a17ae 17.0 landed
-
Fix the review comments and a bug in the slot sync code.
- 3741f2a09d52 17.0 landed
-
Fix a test in failover slots regression test.
- 7e85d1c75f29 17.0 landed
-
Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync.
- 6f3d8d5e7cc2 17.0 landed
-
Split XLogCtl->LogwrtResult into separate struct members
- c9920a9068ea 17.0 cited
-
Ensure that the sync slots reach a consistent state after promotion without losing data.
- 2ec005b4e297 17.0 landed
-
Introduce a new GUC 'standby_slot_names'.
- bf279ddd1c28 17.0 landed
-
Fix BF failure introduced by commit b3f6b14cf4.
- def0ce337068 17.0 landed
-
Fixups for commit 93db6cbda0.
- b3f6b14cf48f 17.0 landed
-
Fix BF failure in commit 93db6cbda0.
- d13ff82319cc 17.0 landed
-
Add a new slot sync worker to synchronize logical slots.
- 93db6cbda037 17.0 landed
-
Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48.
- 801792e528d6 17.0 landed
-
Disable autovacuum on primary in 040_standby_failover_slots_sync test.
- b7bdade6a42f 17.0 landed
-
Fix the incorrect format specifier used in commit 7a424ece48.
- b987be39c3c7 17.0 landed
-
Change the LOG level in 040_standby_failover_slots_sync.pl to DEBUG2.
- d9e225f275b7 17.0 landed
-
Add more LOG and DEBUG messages for slot synchronization.
- 7a424ece4828 17.0 landed
-
Another try to fix BF failure introduced in commit ddd5f4f54a.
- 9bc1eee988c3 17.0 landed
-
Fix BF introduced in commit ddd5f4f54a.
- bd8fc1677b88 17.0 landed
-
Add a slot synchronization function.
- ddd5f4f54a02 17.0 landed
-
Clean-ups for 776621a5e4 and 7329240437.
- 22f7e61a6330 17.0 landed
-
Enhance libpqrcv APIs to support slot synchronization.
- dafbfed9efbe 17.0 landed
-
Add a failover option to subscriptions.
- 776621a5e479 17.0 landed
-
Allow setting failover property in the replication command.
- 73292404370c 17.0 landed
-
Allow to enable failover property for replication slots via SQL API.
- c393308b69d2 17.0 landed
-
Add macros for looping through a List without a ListCell.
- 14dd0f27d7cd 17.0 cited
-
Track conflict_reason in pg_replication_slots.
- 007693f2a3ac 17.0 cited
-
Allow upgrades to preserve the full subscription's state.
- 9a17be1e244a 17.0 cited
-
Add support for incremental backup.
- dc212340058b 17.0 cited
-
Simplify some logic in CreateReplicationSlot()
- e83aa9f92fdd 17.0 cited
-
Fix uninitialized access to InitialRunningXacts during decoding after ERROR.
- c7256e6564fa 15.5 cited
-
Flexible options for CREATE_REPLICATION_SLOT.
- 0266e98c6b86 15.0 cited
-
Empty search_path in logical replication apply worker and walsender.
- 11da97024abb 14.0 cited
-
Fix oldest xmin and LSN computation across repslots after advancing
- b48df818dcbd 14.0 cited
-
Require C99 (and thus MSCV 2013 upwards).
- d9dd406fe281 12.0 cited