On Thu, Feb 15, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Euler,
>
> Here are my minor comments for 17.
>
> 01.
> ```
> /* Options */
> static const char *progname;
>
> static char *primary_slot_name = NULL;
> static bool dry_run = false;
>
> static bool success = false;
>
> static LogicalRepInfo *dbinfo;
> static int num_dbs = 0;
> ```
>
> The comment seems out-of-date. There is only one option.
>
> 02. check_subscriber and check_publisher
>
> Missing pg_catalog prefix in some lines.
>
> 03. get_base_conninfo
>
> I think dbname would not be set. IIUC, dbname should be a pointer of the pointer.
>
>
> 04.
>
> I check the coverage and found two functions have been never called:
> - drop_subscription
> - drop_replication_slot
>
> Also, some cases were not tested. Below bullet showed notable ones for me.
> (Some of them would not be needed based on discussions)
>
> * -r is specified
> * -t is specified
> * -P option contains dbname
> * -d is not specified
> * GUC settings are wrong
> * primary_slot_name is specified on the standby
> * standby server is not working
>
> In feature level, we may able to check the server log is surely removed in case
> of success.
>
> So, which tests should be added? drop_subscription() is called only when the
> cleanup phase, so it may be difficult to test. According to others, it seems that
> -r and -t are not tested. GUC-settings have many test cases so not sure they
> should be. Based on this, others can be tested.
>
> PSA my top-up patch set.
>
> V18-0001: same as your patch, v17-0001.
> V18-0002: modify the alignment of codes.
> V18-0003: change an argument of get_base_conninfo. Per comment 3.
> === experimental patches ===
> V18-0004: Add testcases per comment 4.
> V18-0005: Remove -P option. I'm not sure it should be needed, but I made just in case.
I created a cascade Physical Replication system like
node1->node2->node3 and ran pg_createsubscriber for node2. After
running the script, I started the node2 again and found
pg_createsubscriber command was successful after which the physical
replication between node2 and node3 has been broken. I feel
pg_createsubscriber should check this scenario and throw an error in
this case to avoid breaking the cascaded replication setup. I have
attached the script which was used to verify this.
Thanks and Regards,
Shubham Khanna.