On Wed, Oct 8, 2025 at 11:10 AM Peter Smith <[email protected]> wrote: > > Hi Shubham, > > Here are some v14 review comments. > > ====== > src/bin/pg_basebackup/pg_createsubscriber.c > > check_and_drop_publications: > > 1. > + * Publications copied during physical replication remain on the subscriber > + * after promotion. If --clean=publications is specified, drop all existing > + * publications in the subscriber database. Otherwise, only drop publications > + * that were created by pg_createsubscriber during this operation. > + * > + * In dry-run mode, create_publication() and drop_publication() only > log actions > + * without modifying the database. Importantly, since no publication > is actually > + * created in dry-run, the query for existing publications won't include the > + * "would-be" made publication. Thus, we must call drop_publication() for it > + * regardless of drop_all_pubs to ensure the user sees the intended > log message. > */ > > I think the "In dry-mode ..." paragraph is a good comment, but it > doesn't need to be at the function level. IMO, this can all be > cut/paste to later (see the next review comment). >
Updated as suggested — the detailed explanation about dry-run handling
has been moved to the appropriate block-level comment.
> ~~~
>
> 2.
> /*
> - * In dry-run mode, we don't create publications, but we still try to drop
> - * those to provide necessary information to the user.
> + * Handle the publication created (or would-be created) by
> + * pg_createsubscriber. In dry-run mode, enter this block regardless of
> + * drop_all_pubs to log the drop action for the made publication (which
> + * isn't actually present).
> */
>
> The wording in the function header seemed better. IMO, this entire
> comment can be replaced like:
>
> /*
> * Handle publications created by pg_createsubscriber.
> *
> * <here cut/paste all the "In dry-mode..." wording from the function
> header comment>
> */
> ~~~
>
> I provided a top-up diff patch to illustrate my point.
>
> ////////////////////
>
Fixed. The comments have been restructured to improve clarity and
placement per your suggestion.
> I also had a look at the 'results' logging from your script.
>
> ~~~
>
> For test case 1. (new pub pub2 + clean):
>
> I hoped to see some evidence that --clean is dropping all other
> existing pubs; e.g. pub1,pub2,pub3, whatever...
>
> ~~~
>
> For test case 3. (Existing pub pub1 + clean):
>
> I hoped to see some evidence that --clean is dropping existing pubs;
> e.g. pub1,pub2,pub3, whatever...
>
> ~~~
>
> For test case 5. (Auto pub + clean):
>
> I hoped to see some evidence that --clean is dropping existing pubs;
> e.g. pub1,pub2,pub3, whatever...
>
For these cases, I manually created multiple publications in the
postgres, db1, and db2 databases to make the --clean behavior clearly
visible in the logs.
I’ve attached the updated test script, captured logs, and the expected
log matrix for all cases to demonstrate the behavior.
Additionally, following your note in [1], I have removed the redundant
pg_log_info("create publication..."); statement since the same message
was already being logged inside create_publication().
The attached patch incorporates the revised comments and test-related updates.
[1] -
https://www.postgresql.org/message-id/CAHut%2BPsYKXKmMSim5FcFo177gosY7a5xgMH_p4AY1_25HMVL7Q%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
pkill -9 postgres
rm -rf data
rm -rf standby
rm standby.log
rm logfile
rm Results.log
LOGFILE="Results.log"
./initdb -D data -c "wal_level=logical" -c "max_prepared_transactions = 10" -c "listen_addresses=*" -c "max_slot_wal_keep_size=300"
cp pg_hba_primary.conf data/pg_hba.conf
./pg_ctl -D data -l logfile -c start
./psql -d postgres -c "CREATE ROLE replication WITH REPLICATION PASSWORD 'password' LOGIN;"
./psql -d postgres -c "SELECT * FROM pg_create_physical_replication_slot('replica_1', true);"
./pg_ctl -D data -l logfile -c restart
./psql -d postgres -c "create database db1;"
./psql -d postgres -c "create database db2;"
./psql -d postgres -c "CREATE TABLE public.t1 (id int, val text);"
./psql -d db1 -c "CREATE TABLE public.t2 (id int, val text);"
./psql -d db2 -c "CREATE TABLE public.t3 (id int, val text, extra int);"
./psql -d postgres -c "CREATE PUBLICATION pub_existsA FOR table public.t1;"
./psql -d postgres -c "CREATE PUBLICATION pub_existsB FOR table public.t1;"
./psql -d postgres -c "CREATE PUBLICATION pub_existsC FOR table public.t1;"
./psql -d postgres -c "CREATE PUBLICATION pub_existsD FOR table public.t1;"
./psql -d db1 -c "CREATE PUBLICATION pub_existsa FOR table public.t2;"
./psql -d db1 -c "CREATE PUBLICATION pub_existsb FOR table public.t2;"
./psql -d db1 -c "CREATE PUBLICATION pub_existsc FOR table public.t2;"
./psql -d db1 -c "CREATE PUBLICATION pub_existsd FOR table public.t2;"
./psql -d db2 -c "CREATE PUBLICATION pub_exists1 FOR table public.t3;"
./psql -d db2 -c "CREATE PUBLICATION pub_exists2 FOR table public.t3;"
./psql -d db2 -c "CREATE PUBLICATION pub_exists3 FOR table public.t3;"
./psql -d db2 -c "CREATE PUBLICATION pub_exists4 FOR table public.t3;"
./pg_basebackup -h 127.0.0.1 -D /home/shubham/Project/Git/postgres/inst/bin/standby -P -U replication -R
cp standby.signal standby/
echo "wal_level=logical
\n listen_addresses='*'
\n Port=9999
\n max_logical_replication_workers=5
\n max_sync_workers_per_subscription=1" >> standby/postgresql.conf
sleep 3
./pg_ctl -D standby -l standby.log -o "-p 9999" -c start
sleep 2
./psql -d postgres -c "ALTER SYSTEM SET wal_level = 'logical';"
./psql -d postgres -c "ALTER SYSTEM SET listen_addresses = '*';"
./psql -d postgres -c "ALTER SYSTEM SET port = 9999;"
./psql -d postgres -c "ALTER SYSTEM SET max_logical_replication_workers = 5;"
./psql -d postgres -c "ALTER SYSTEM SET max_sync_workers_per_subscription = 1;"
./pg_ctl -D standby -l standby.log -c stop
# ---------------------------------------------------------------------
# Test Cases
# ---------------------------------------------------------------------
run_case() {
local case_no=$1
shift
echo "==================== TEST CASE $case_no ====================" | tee -a "$LOGFILE"
echo "Command: $*" | tee -a "$LOGFILE"
eval "$@" >> "$LOGFILE" 2>&1
echo >> "$LOGFILE"
}
run_case 1 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" -d db2 --publication pub_new2 --clean=publications --dry-run --verbose"
run_case 2 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" -d db2 --publication pub_new2 --dry-run --verbose"
run_case 3 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" -d db1 --publication pub_exists1 --clean=publications --dry-run --verbose"
run_case 4 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" -d db1 --publication pub_exists1 --dry-run --verbose"
run_case 5 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" --clean=publications --dry-run --verbose"
run_case 6 "./pg_createsubscriber -D standby/ -P \"host=localhost port=5432 dbname=postgres\" --dry-run --verbose"
echo "========== All test cases completed ==========" | tee -a "$LOGFILE"
v15-0001-Support-existing-publications-in-pg_createsubscr.patch
Description: Binary data
Expected-log-matrix.xlsx
Description: MS-Excel 2007 spreadsheet
Results.log
Description: Binary data
