On Thu, Feb 15, 2024 at 10:37 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@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.
#!/bin/bash # # This script tests a case of cascading replication. # # Creating system,: # node1-(physical replication)->node2-(physical replication)->node3 # # Stop instances pg_ctl stop -D node1 pg_ctl stop -D node2 pg_ctl stop -D node3 # Remove old files rm -rf node1 rm -rf node2 rm -rf node3 rm log_node2 log_node1 log_node3 # Initialize primary initdb -D node1 echo "wal_level = logical" >> node1/postgresql.conf echo "max_replication_slots=3" >> node1/postgresql.conf pg_ctl -D node1 -l log_node1 start psql -d postgres -c "CREATE DATABASE db1"; psql -d db1 -c "CHECKPOINT"; sleep 3 # Initialize standby pg_basebackup -v -R -D node2 echo "Port=9000" >> node2/postgresql.conf pg_ctl -D node2 -l log_node2 start # Initialize another standby pg_basebackup -p 9000 -v -R -D node3 echo "Port=9001" >> node3/postgresql.conf pg_ctl -D node3 -l log_node3 start # Insert a tuple to primary psql -d db1 -c "CREATE TABLE c1(a int)"; psql -d db1 -c "Insert into c1 Values(2)"; sleep 3 # And verify it can be seen on another standby psql -d db1 -p 9001 -c "Select * from c1"; pg_createsubscriber -D node2/ -S "host=localhost port=9000 dbname=postgres" -d postgres -d db1 -r -v pg_ctl -D node2 -l log_node2 start
