Re: Synchronizing slots from primary to standby

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>

To: shveta malik <shveta.malik@gmail.com>

Cc: Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Ajin Cherian <itsajin@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, Alvaro Herrera <alvherre@alvh.no-ip.org>

Date: 2024-02-02T04:20:17Z

Lists: pgsql-hackers

Here are some review comments for v750001.

======
Commit message

1.
This patch provides support for non-replication connection
in libpqrcv_connect().

~

1a.
/connection/connections/

~

1b.
Maybe there needs to be a few more sentences just to describe what you
mean by "non-replication connection".

~

1c.
IIUC although the 'replication' parameter is added, in this patch
AFAICT every call to the connect function is still passing that
argument as true. If that's correct, probably this patch comment
should emphasise that this patch doesn't change any functionality at
all but is just preparation for later patches which *will* pass false
for the replication arg.

~~~

2.
This patch also implements a new API libpqrcv_get_dbname_from_conninfo()
to extract database name from the given connection-info

~

/extract database name/the extract database name/

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

3.
+ * Apart from walreceiver, the libpq-specific routines here are now being used
+ * by logical replication worker as well.

/worker/workers/

~~~

4. libpqrcv_connect

 /*
- * Establish the connection to the primary server for XLOG streaming
+ * Establish the connection to the primary server.
+ *
+ * The connection established could be either a replication one or
+ * a non-replication one based on input argument 'replication'. And further
+ * if it is a replication connection, it could be either logical or physical
+ * based on input argument 'logical'.

That first comment ("could be either a replication one or...") seemed
a bit meaningless (e.g. it like saying "this boolean argument can be
true or false") because it doesn't describe what is the meaning of a
"replication connection" versus what is a "non-replication
connection".

~~~

5.
/* We can not have logical without replication */
Assert(replication || !logical);

if (replication)
{
keys[++i] = "replication";
vals[i] = logical ? "database" : "true";

if (!logical)
{
/*
* The database name is ignored by the server in replication mode,
* but specify "replication" for .pgpass lookup.
*/
keys[++i] = "dbname";
vals[i] = "replication";
}
}

keys[++i] = "fallback_application_name";
vals[i] = appname;
if (logical)
{
...
}

~

The Assert already says we cannot be 'logical' if not 'replication',
therefore IMO it seemed strange that the code was not refactored to
bring that 2nd "if (logical)" code to within the scope of the "if
(replication)".

e.g. Can't you do something like this:

Assert(replication || !logical);

if (replication)
{
  ...
  if (logical)
  {
    ...
  }
  else
  {
    ...
  }
}
keys[++i] = "fallback_application_name";
vals[i] = appname;

~~~

6. libpqrcv_get_dbname_from_conninfo

+ for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
+ {
+ /*
+ * If multiple dbnames are specified, then the last one will be
+ * returned
+ */
+ if (strcmp(opt->keyword, "dbname") == 0 && opt->val &&
+ opt->val[0] != '\0')
+ dbname = pstrdup(opt->val);
+ }

Should you also pfree the old dbname instead of gathering a bunch of
strdups if there happened to be multiple dbnames specified ?

SUGGESTION
if (strcmp(opt->keyword, "dbname") == 0 && opt->val && *opt->val)
{
   if (dbname)
    pfree(dbname);
   dbname = pstrdup(opt->val);
}

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

7.
/*
 * walrcv_connect_fn
 *
 * Establish connection to a cluster.  'logical' is true if the
 * connection is logical, and false if the connection is physical.
 * 'appname' is a name associated to the connection, to use for example
 * with fallback_application_name or application_name.  Returns the
 * details about the connection established, as defined by
 * WalReceiverConn for each WAL receiver module.  On error, NULL is
 * returned with 'err' including the error generated.
 */
typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo,
   bool replication,
   bool logical,
   bool must_use_password,
   const char *appname,
   char **err);

~

The comment is missing any description of the new parameter 'replication'.

~~~

8.
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);
+

/dbid/database name/

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

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