| From: | "Euler Taveira" <euler(at)eulerto(dot)com> |
|---|---|
| To: | "Peter Smith" <smithpb2250(at)gmail(dot)com>, "vignesh C" <vignesh21(at)gmail(dot)com> |
| Cc: | tianbing <tian_bing_0531(at)163(dot)com>, "'Shubham Khanna'" <khannashubham1197(at)gmail(dot)com>, "Chao Li" <li(dot)evan(dot)chao(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, "PostgreSQL Hackers" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Add support for specifying tables in pg_createsubscriber. |
| Date: | 2025-12-09 23:31:56 |
| Message-ID: | 2d113bb7-c7e8-42c6-8d6e-8dd8667c7654@app.fastmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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/
| Attachment | Content-Type | Size |
|---|---|---|
| v24-0001-Support-existing-publications-in-pg_createsubscr.patch | text/x-patch | 14.5 KB |
| v24-0002-fixup-Support-existing-publications-in-pg_create.patch | text/x-patch | 14.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Matthias van de Meent | 2025-12-09 23:33:23 | Small bugs regarding resowner handling in aio.c, catcache.c |
| Previous Message | Tomas Vondra | 2025-12-09 23:30:47 | Re: Add a greedy join search algorithm to handle large join problems |