On 16.03.24 16:42, Euler Taveira wrote:
> I'm attaching a new version (v30) that adds:
I have some review comments and attached a patch with some smaller
fixups (mainly message wording and avoid fixed-size string buffers).
* doc/src/sgml/ref/pg_createsubscriber.sgml
I would remove the "How It Works" section. This is not relevant to
users, and it is very detailed and will require updating whenever the
implementation changes. It could be a source code comment instead.
* src/bin/pg_basebackup/pg_createsubscriber.c
I think the connection string handling is not robust against funny
characters, like spaces, in database names etc.
Most SQL commands need to be amended for proper identifier or string
literal quoting and/or escaping.
In check_subscriber(): All these permissions checks seem problematic
to me. We shouldn't reimplement our own copy of the server's
permission checks. The server can check the permissions. And if the
permission checking in the server ever changes, then we have
inconsistencies to take care of. Also, the error messages "permission
denied" are inappropriate, because we are not doing the actual thing.
Maybe we want to do a dry-run for the benefit of the user, but then we
should do the actual thing, like try to create a replication slot, or
whatever. But I would rather just remove all this, it seems too
problematic.
In main(): The first check if the standby is running is problematic.
I think it would be better to require that the standby is initially
shut down. Consider, the standby might be running under systemd.
This tool will try to stop it, systemd will try to restart it. Let's
avoid these kinds of battles. It's also safer if we don't try to
touch running servers.
The -p option (--subscriber-port) doesn't seem to do anything. In my
testing, it always uses the compiled-in default port.
Printing all the server log lines to the terminal doesn't seem very
user-friendly. Not sure what to do about that, short of keeping a
pg_upgrade-style directory of log files. But it's ugly.