On Sun, Dec 7, 2025, at 11:52 PM, Peter Smith wrote:
>
> OK. Done as suggested in v23.
>
I took another look at this patch. I noticed that it increased the test time in
3 or 4s. I started writing the suggestions to this email but since it increases
a lot, it is better to provide a patch and explain the main points. Regarding
the tests, there are 2 new nodes (one used to test this feature -- node_s2 --
and another one with no real function -- node_s3). There is no need to add new
nodes, just use the created publication into one of the existing tests. That's
what I did (see test_pub3). I also added a new table to ensure the publication
only contains tbl1 and not not_replicated table. I removed the dry run part
because it only increases the total time and doesn't improve coverage. Finally,
coverage stays the same and a small increment in the execution time (less than
1s).
$ tail -n 8 src/bin/pg_basebackup/pg_createsubscriber.c.gcov.out
File 'pg_createsubscriber.c'
Lines executed:83.81% of 951
Branches executed:93.15% of 467
Taken at least once:74.73% of 467
Calls executed:73.48% of 675
Creating 'pg_createsubscriber.c.gcov'
Lines executed:83.81% of 951
I also changed the code a bit. In particular, I don't like
check_publication_exists and find_publication seems cleaner to me so I renamed
it. I replaced the psprintf because the pattern is to use PQExpBuffer functions
for queries. Added pg_catalog to the query. I changed pg_fatal with pg_log_error
plus disconnect_database -- see previous discussion about it. I added a comment
explaining why made_publication = false. Finally, I refactored the logic that
drops publications; the if (!drop_all_pubs || dry_run) is rather confusing.
Documentation was also changed a bit. One paragraph instead of two because it is
talking about the same topic.
Here is your v23 plus the fixes that I described above. IMO it is good to go so
I set it to Ready for Committer. Additional comments are always welcome.
--
Euler Taveira
EDB https://www.enterprisedb.com/
