On Mon, Feb 19, 2024, at 6:47 AM, Peter Eisentraut wrote: > Some review of the v21 patch:
Thanks for checking. > - commit message > > Mention pg_createsubscriber in the commit message title. That's the > most important thing that someone doing git log searches in the future > will be looking for. Right. Done. > - doc/src/sgml/ref/allfiles.sgml > > Move the new entry to alphabetical order. Done. > > - doc/src/sgml/ref/pg_createsubscriber.sgml > > + <para> > + The <application>pg_createsubscriber</application> should be run at > the target > + server. The source server (known as publisher server) should accept > logical > + replication connections from the target server (known as subscriber > server). > + The target server should accept local logical replication connection. > + </para> > > "should" -> "must" ? Done. > + <refsect1> > + <title>Options</title> > > Sort options alphabetically. Done. > It would be good to indicate somewhere which options are mandatory. I'll add this information in the option description. AFAICT the synopsis kind of indicates it. > + <refsect1> > + <title>Examples</title> > > I suggest including a pg_basebackup call into this example, so it's > easier for readers to get the context of how this is supposed to be > used. You can add that pg_basebackup in this example is just an example > and that other base backups can also be used. We can certainly add it but creating a standby isn't out of scope here? I will make sure to include references to pg_basebackup and the "Setting up a Standby Server" section. > - doc/src/sgml/reference.sgml > > Move the new entry to alphabetical order. Done. > - src/bin/pg_basebackup/Makefile > > Move the new sections to alphabetical order. Done. > - src/bin/pg_basebackup/meson.build > > Move the new sections to alphabetical order. Done. > > - src/bin/pg_basebackup/pg_createsubscriber.c > > +typedef struct CreateSubscriberOptions > +typedef struct LogicalRepInfo > > I think these kinds of local-use struct don't need to be typedef'ed. > (Then you also don't need to update typdefs.list.) Done. > +static void > +usage(void) > > Sort the options alphabetically. Are you referring to s/options/functions/? > +static char * > +get_exec_path(const char *argv0, const char *progname) > > Can this not use find_my_exec() and find_other_exec()? It is indeed using it. I created this function because it needs to run the same code path twice (pg_ctl and pg_resetwal). > +int > +main(int argc, char **argv) > > Sort the options alphabetically (long_options struct, getopt_long() > argument, switch cases). Done. > - .../t/040_pg_createsubscriber.pl > - .../t/041_pg_createsubscriber_standby.pl > > These two files could be combined into one. Done. > +# Force it to initialize a new cluster instead of copying a > +# previously initdb'd cluster. > > Explain why? Ok. It needs a new cluster because it will have a different system identifier so we can make sure the target cluster is a copy of the source server. > +$node_s->append_conf( > + 'postgresql.conf', qq[ > +log_min_messages = debug2 > > Is this setting necessary for the test? No. It is here as a debugging aid. Better to include it in a separate patch. There are a few messages that I don't intend to include in the final patch. All of these modifications will be included in the next patch. I'm finishing to integrate patches proposed by Hayato [1] and some additional fixes and refactors. [1] https://www.postgresql.org/message-id/TYCPR01MB12077A8421685E5515DE408EEF5512%40TYCPR01MB12077.jpnprd01.prod.outlook.com -- Euler Taveira EDB https://www.enterprisedb.com/
