On 21.02.22 13:09, Euler Taveira wrote:
> A new tool called pg_subscriber does this conversion and is tightly
> integrated
> with Postgres.
Are we comfortable with the name pg_subscriber? It seems too general.
Are we planning other subscriber-related operations in the future? If
so, we should at least make this one use a --create option or
something like that.
doc/src/sgml/ref/pg_subscriber.sgml
Attached is a patch that reorganizes the man page a bit. I moved the
description of the steps to the Notes section and formatted it
differently. I think the steps are interesting but not essential for
the using of the program, so I wanted to get them out of the main
description.
src/bin/pg_subscriber/pg_subscriber.c
+ if (made_temp_replslot)
+ {
+ conn = connect_database(dbinfo[0].pubconninfo, true);
+ drop_replication_slot(conn, &dbinfo[0], temp_replslot);
+ disconnect_database(conn);
+ }
Temp slots don't need to be cleaned up.
+/*
+ * Obtain the system identifier from control file. It will be used to
compare
+ * if a data directory is a clone of another one. This routine is used
locally
+ * and avoids a replication connection.
+ */
+static char *
+get_control_from_datadir(const char *datadir)
This could return uint64 directly, without string conversion.
get_sysid_from_conn() could then convert to uint64 internally.
+ {"verbose", no_argument, NULL, 'v'},
I'm not sure if the --verbose option is all that useful.
+ {"stop-subscriber", no_argument, NULL, 1},
This option doesn't seem to be otherwise supported or documented.
+ pub_sysid = pg_malloc(32);
+ pub_sysid = get_sysid_from_conn(dbinfo[0].pubconninfo);
+ sub_sysid = pg_malloc(32);
+ sub_sysid = get_control_from_datadir(subscriber_dir);
These mallocs don't appears to be of any use.
+ dbname_conninfo = pg_malloc(NAMEDATALEN);
This seems wrong.
Overall, this code could use a little bit more structuring. There are
a lot of helper functions that don't seem to do a lot and are mostly
duplicate runs-this-SQL-command calls. But the main() function is
still huge. There is room for refinement.
src/bin/pg_subscriber/t/001_basic.pl
Good start, but obviously, we'll need some real test cases here also.
src/bin/initdb/initdb.c
src/bin/pg_ctl/pg_ctl.c
src/common/file_utils.c
src/include/common/file_utils.h
I recommend skipping this refactoring. The readfile() function from
pg_ctl is not general enough to warrant the pride of place of a
globally available function. Note that it is specifically geared
toward some of pg_ctl's requirements, for example that the underlying
file can change while it is being read.
The requirements of pg_subscriber can be satisfied more easily: Just
call pg_ctl to start the server. You are already using that in
pg_subscriber. Is there a reason it can't be used here as well?
