On 18.03.24 06:43, Hayato Kuroda (Fujitsu) wrote:
02.
"The main difference between the logical replication setup and
pg_createsubscriber
is the initial data copy."
Grammarly suggests:
"The initial data copy is the main difference between the logical replication
setup and pg_createsubscriber."
I think that change is worse.
09.
"The source server must accept connections from the target server. The source server
must not be in recovery."
Grammarly suggests:
"The source server must accept connections from the target server and not be in
recovery."
I think the previous version is better.
17.
"specifies promote"
We can do double-quote for the word promote.
The v30 patch has <literal>promote</literal>, which I think is adequate.
19.
"is accepting read-write transactions"
Grammarly suggests:
"accepts read-write transactions"
I like the first one better.
20.
New options must be also documented as well. This helps not only users but also
reviewers.
(Sometimes we cannot identify that the implementation is intentinal or not.)
Which ones are missing?
21.
Also, not sure the specification is good. I preferred to specify them by format
string. Because it can reduce the number of arguments and I cannot find use
cases
which user want to control the name of objects.
However, your approach has a benefit which users can easily identify the
generated
objects by pg_createsubscriber. How do other think?
I think listing them explicitly is better for the first version. It's
simpler to implement and more flexible.
22.
```
#define BASE_OUTPUT_DIR "pg_createsubscriber_output.d"
```
No one refers the define.
This is gone in v30.
23.
```
} CreateSubscriberOptions;
...
} LogicalRepInfo;
```
Declarations after the "{" are not needed, because we do not do typedef.
Yeah, this is actually wrong, because as it is written now, it defines
global variables.
22.
While seeing definitions of functions, I found that some pointers are declared
as const, but others are not. E.g., "char *lsn" in setup_recovery() won' be
changed but not the constant. Is it just missing or is there another rule?
Yes, more could be done here. I have attached a patch for this. (This
also requires the just-committed 48018f1d8c.)
24.
```
/* standby / subscriber data directory */
static char *subscriber_dir = NULL;
```
It is bit strange that only subscriber_dir is a global variable. Caller requires
the CreateSubscriberOptions as an argument, except cleanup_objects_atexit() and
main. So, how about makeing `CreateSubscriberOptions opt` to global one?
Fewer global variables seem preferable. Only make global as needed.
30.
```
if (num_replslots == 0)
dbinfo[i].replslotname = pg_strdup(genname);
```
I think the straightforward way is to use the name of subscription if no name
is specified. This follows the rule for CREATE SUBSCRIPTION.
right
31.
```
/* Create replication slot on publisher */
if (lsn)
pg_free(lsn);
```
I think allocating/freeing memory is not so efficient.
Can we add a flag to create_logical_replication_slot() for controlling the
returning value (NULL or duplicated string)? We can use the condition (i ==
num_dbs-1)
as flag.
Nothing is even using the return value of
create_logical_replication_slot(). I think this can be removed altogether.
34.
```
{"config-file", required_argument, NULL, 1},
{"publication", required_argument, NULL, 2},
{"replication-slot", required_argument, NULL, 3},
{"subscription", required_argument, NULL, 4},
```
The ordering looks strange for me. According to pg_upgarade and pg_basebackup,
options which do not have short notation are listed behind.
35.
```
opt.sub_port = palloc(16);
```
Per other lines, pg_alloc() should be used.
Even better psprintf().
37.
```
/* Register a function to clean up objects in case of failure */
atexit(cleanup_objects_atexit);
```
Sorry if we have already discussed. I think the registration can be moved just
before the boot of the standby. Before that, the callback will be no-op.
But it can also stay where it is. What is the advantage of moving it later?
40.
```
* XXX this code was extracted from BootStrapXLOG().
```
So, can we extract the common part to somewhere? Since system identifier is
related
with the controldata file, I think it can be located in controldata_util.c.
Let's leave it as is for this PG release.
From d951a9f186b2162aa241f7554908b236c718154f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 18 Mar 2024 11:54:03 +0100
Subject: [PATCH] fixup! pg_createsubscriber: creates a new logical replica
from a standby server
Add const decorations.
---
src/bin/pg_basebackup/pg_createsubscriber.c | 54 ++++++++++-----------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c
b/src/bin/pg_basebackup/pg_createsubscriber.c
index 6cc1c341214..e24ed7ef506 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -61,46 +61,46 @@ struct LogicalRepInfo
static void cleanup_objects_atexit(void);
static void usage();
-static char *get_base_conninfo(char *conninfo, char **dbname);
-static char *get_sub_conninfo(struct CreateSubscriberOptions *opt);
+static char *get_base_conninfo(const char *conninfo, char **dbname);
+static char *get_sub_conninfo(const struct CreateSubscriberOptions *opt);
static char *get_exec_path(const char *argv0, const char *progname);
static void check_data_directory(const char *datadir);
static char *concat_conninfo_dbname(const char *conninfo, const char *dbname);
-static struct LogicalRepInfo *store_pub_sub_info(struct
CreateSubscriberOptions *opt,
+static struct LogicalRepInfo *store_pub_sub_info(const struct
CreateSubscriberOptions *opt,
const char *pub_base_conninfo,
const char *sub_base_conninfo);
static PGconn *connect_database(const char *conninfo, bool exit_on_error);
static void disconnect_database(PGconn *conn, bool exit_on_error);
static uint64 get_primary_sysid(const char *conninfo);
static uint64 get_standby_sysid(const char *datadir);
-static void modify_subscriber_sysid(struct CreateSubscriberOptions *opt);
+static void modify_subscriber_sysid(const struct CreateSubscriberOptions *opt);
static bool server_is_in_recovery(PGconn *conn);
static char *generate_object_name(PGconn *conn);
-static void check_publisher(struct LogicalRepInfo *dbinfo);
+static void check_publisher(const struct LogicalRepInfo *dbinfo);
static char *setup_publisher(struct LogicalRepInfo *dbinfo);
-static void check_subscriber(struct LogicalRepInfo *dbinfo);
+static void check_subscriber(const struct LogicalRepInfo *dbinfo);
static void setup_subscriber(struct LogicalRepInfo *dbinfo,
const char
*consistent_lsn);
-static void setup_recovery(struct LogicalRepInfo *dbinfo, char *datadir,
- char *lsn);
+static void setup_recovery(const struct LogicalRepInfo *dbinfo, const char
*datadir,
+ const char *lsn);
static void drop_primary_replication_slot(struct LogicalRepInfo *dbinfo,
-
char *slotname);
+
const char *slotname);
static char *create_logical_replication_slot(PGconn *conn,
struct LogicalRepInfo *dbinfo);
static void drop_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo,
const char
*slot_name);
static void pg_ctl_status(const char *pg_ctl_cmd, int rc);
-static void start_standby_server(struct CreateSubscriberOptions *opt,
+static void start_standby_server(const struct CreateSubscriberOptions *opt,
bool
restricted_access);
static void stop_standby_server(const char *datadir);
static void wait_for_end_recovery(const char *conninfo,
- struct
CreateSubscriberOptions *opt);
+ const struct
CreateSubscriberOptions *opt);
static void create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
static void drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
-static void create_subscription(PGconn *conn, struct LogicalRepInfo *dbinfo);
-static void set_replication_progress(PGconn *conn, struct LogicalRepInfo
*dbinfo,
+static void create_subscription(PGconn *conn, const struct LogicalRepInfo
*dbinfo);
+static void set_replication_progress(PGconn *conn, const struct LogicalRepInfo
*dbinfo,
const
char *lsn);
-static void enable_subscription(PGconn *conn, struct LogicalRepInfo *dbinfo);
+static void enable_subscription(PGconn *conn, const struct LogicalRepInfo
*dbinfo);
#define USEC_PER_SEC 1000000
#define WAIT_INTERVAL 1 /* 1 second */
@@ -244,7 +244,7 @@ usage(void)
* dbname.
*/
static char *
-get_base_conninfo(char *conninfo, char **dbname)
+get_base_conninfo(const char *conninfo, char **dbname)
{
PQExpBuffer buf = createPQExpBuffer();
PQconninfoOption *conn_opts = NULL;
@@ -292,7 +292,7 @@ get_base_conninfo(char *conninfo, char **dbname)
* since it starts a server with restricted access.
*/
static char *
-get_sub_conninfo(struct CreateSubscriberOptions *opt)
+get_sub_conninfo(const struct CreateSubscriberOptions *opt)
{
PQExpBuffer buf = createPQExpBuffer();
char *ret;
@@ -411,7 +411,7 @@ concat_conninfo_dbname(const char *conninfo, const char
*dbname)
* setup_publisher().
*/
static struct LogicalRepInfo *
-store_pub_sub_info(struct CreateSubscriberOptions *opt, const char
*pub_base_conninfo,
+store_pub_sub_info(const struct CreateSubscriberOptions *opt, const char
*pub_base_conninfo,
const char *sub_base_conninfo)
{
struct LogicalRepInfo *dbinfo;
@@ -603,7 +603,7 @@ get_standby_sysid(const char *datadir)
* files from one of the systems might be used in the other one.
*/
static void
-modify_subscriber_sysid(struct CreateSubscriberOptions *opt)
+modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
{
ControlFileData *cf;
bool crc_ok;
@@ -791,7 +791,7 @@ server_is_in_recovery(PGconn *conn)
* XXX Does it not allow a synchronous replica?
*/
static void
-check_publisher(struct LogicalRepInfo *dbinfo)
+check_publisher(const struct LogicalRepInfo *dbinfo)
{
PGconn *conn;
PGresult *res;
@@ -947,7 +947,7 @@ check_publisher(struct LogicalRepInfo *dbinfo)
* will be broken at the end of this process (due to pg_resetwal).
*/
static void
-check_subscriber(struct LogicalRepInfo *dbinfo)
+check_subscriber(const struct LogicalRepInfo *dbinfo)
{
PGconn *conn;
PGresult *res;
@@ -1124,7 +1124,7 @@ setup_subscriber(struct LogicalRepInfo *dbinfo, const
char *consistent_lsn)
* Write the required recovery parameters.
*/
static void
-setup_recovery(struct LogicalRepInfo *dbinfo, char *datadir, char *lsn)
+setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const
char *lsn)
{
PGconn *conn;
PQExpBuffer recoveryconfcontents;
@@ -1184,7 +1184,7 @@ setup_recovery(struct LogicalRepInfo *dbinfo, char
*datadir, char *lsn)
* eventually drops this replication slot later.
*/
static void
-drop_primary_replication_slot(struct LogicalRepInfo *dbinfo, char *slotname)
+drop_primary_replication_slot(struct LogicalRepInfo *dbinfo, const char
*slotname)
{
PGconn *conn;
@@ -1322,7 +1322,7 @@ pg_ctl_status(const char *pg_ctl_cmd, int rc)
}
static void
-start_standby_server(struct CreateSubscriberOptions *opt, bool
restricted_access)
+start_standby_server(const struct CreateSubscriberOptions *opt, bool
restricted_access)
{
PQExpBuffer pg_ctl_cmd = createPQExpBuffer();
int rc;
@@ -1380,7 +1380,7 @@ stop_standby_server(const char *datadir)
* the recovery process. By default, it waits forever.
*/
static void
-wait_for_end_recovery(const char *conninfo, struct CreateSubscriberOptions
*opt)
+wait_for_end_recovery(const char *conninfo, const struct
CreateSubscriberOptions *opt)
{
PGconn *conn;
int status = POSTMASTER_STILL_STARTING;
@@ -1575,7 +1575,7 @@ drop_publication(PGconn *conn, struct LogicalRepInfo
*dbinfo)
* initial location.
*/
static void
-create_subscription(PGconn *conn, struct LogicalRepInfo *dbinfo)
+create_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
{
PQExpBuffer str = createPQExpBuffer();
PGresult *res;
@@ -1620,7 +1620,7 @@ create_subscription(PGconn *conn, struct LogicalRepInfo
*dbinfo)
* and LSN are set to invalid values for printing purposes.
*/
static void
-set_replication_progress(PGconn *conn, struct LogicalRepInfo *dbinfo, const
char *lsn)
+set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo,
const char *lsn)
{
PQExpBuffer str = createPQExpBuffer();
PGresult *res;
@@ -1702,7 +1702,7 @@ set_replication_progress(PGconn *conn, struct
LogicalRepInfo *dbinfo, const char
* adjusting the initial logical replication location, enable the subscription.
*/
static void
-enable_subscription(PGconn *conn, struct LogicalRepInfo *dbinfo)
+enable_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
{
PQExpBuffer str = createPQExpBuffer();
PGresult *res;
base-commit: 48018f1d8c12d42b53c2a855626ee1ceb7f4ca71
prerequisite-patch-id: e4c7223061ca1e12dfa84f4a5ccdc3a9ab0c268a
--
2.44.0
