On Wed, Mar 6, 2024, at 8:24 AM, vignesh C wrote:
> Few comments:
Thanks for your review. Some changes are included in v26.
> 1) Can we use strdup here instead of atoi, as we do similarly in
> case of pg_dump too, else we will do double conversion, convert using
> atoi and again to string while forming the connection string:
> + case 'p':
> + if ((opt.sub_port = atoi(optarg)) <= 0)
> + pg_fatal("invalid subscriber
> port number");
> + break;
I don't have a strong preference but decided to provide a patch for it. See
v26-0003.
> 2) We can have some valid range for this, else we will end up in some
> unexpected values when a higher number is specified:
> + case 't':
> + opt.recovery_timeout = atoi(optarg);
> + break;
I wouldn't like to add an arbitrary value. Suggestions?
> 3) Now that we have addressed most of the items, can we handle this TODO:
> + /*
> + * TODO use primary_conninfo (if available) from subscriber
> and
> + * extract publisher connection string. Assume that there are
> + * identical entries for physical and logical
> replication. If there is
> + * not, we would fail anyway.
> + */
> + pg_log_error("no publisher connection string specified");
> + pg_log_error_hint("Try \"%s --help\" for more
> information.", progname);
> + exit(1);
It is not in my top priority at the moment.
> 4) By default the log level as info here, I was not sure how to set
> it to debug level to get these error messages:
> + pg_log_debug("publisher(%d): connection string: %s",
> i, dbinfo[i].pubconninfo);
> + pg_log_debug("subscriber(%d): connection string: %s",
> i, dbinfo[i].subconninfo);
<term><option>-v</option></term>
<term><option>--verbose</option></term>
<listitem>
<para>
Enables verbose mode. This will cause
<application>pg_createsubscriber</application> to output progress
messages
and detailed information about each step to standard error.
Repeating the option causes additional debug-level messages to appear on
standard error.
</para>
> 5) Currently in non verbose mode there are no messages printed on
> console, we could have a few of them printed irrespective of verbose
> or not like the following:
> a) creating publication
> b) creating replication slot
> c) waiting for the target server to reach the consistent state
> d) If pg_createsubscriber fails after this point, you must recreate
> the physical replica before continuing.
> e) creating subscription
That's the idea. Quiet mode by default.
> 6) The message should be "waiting for the target server to reach the
> consistent state":
> +#define NUM_CONN_ATTEMPTS 5
> +
> + pg_log_info("waiting the target server to reach the consistent
> state");
> +
> + conn = connect_database(conninfo, true);
Fixed.
--
Euler Taveira
EDB https://www.enterprisedb.com/
