On Thu, Dec 4, 2025 at 1:56 PM vignesh C <[email protected]> wrote: > > On Thu, 4 Dec 2025 at 07:47, Peter Smith <[email protected]> wrote: > > > > On Wed, Dec 3, 2025 at 4:47 PM tianbing <[email protected]> wrote: > > > > > > Hi， Peter， > > > I have reviewed the v21 patch and noticed that there seems to be a memory > > > leak. > > > > > > +static bool > > > +check_publication_exists(PGconn *conn, const char *pubname, const char > > > *dbname) > > > +{ > > > + PGresult *res; > > > + bool exists; > > > + char *query; > > > + > > > + query = psprintf("SELECT 1 FROM pg_publication WHERE pubname = %s", > > > + PQescapeLiteral(conn, pubname, strlen(pubname))); > > > + res = PQexec(conn, query); > > > + > > > + if (PQresultStatus(res) != PGRES_TUPLES_OK) > > > + pg_fatal("could not check for publication \"%s\" in database \"%s\": > > > %s", > > > + pubname, dbname, PQerrorMessage(conn)); > > > + > > > + exists = (PQntuples(res) == 1); > > > + > > > + PQclear(res); > > > + pg_free(query); > > > + return exists; > > > +} > > > > > > The PQescapeLiteral() function through malloc to allocate memmory，and > > > should be free by PQfreemem。 > > > > > > I suggest making the following modifications： > > > > > > + char *pub = PQescapeLiteral(conn, pubname, strlen(pubname); > > > + query = psprintf("SELECT 1 FROM pg_publication WHERE pubname = %s", > > > pub); > > > ...... > > > + PQfreemem(pub); > > > > > > > Fixed in v22. > > I felt that instead of adding a new test case, let's try to combine > with the existing test case, something like below: > diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > index e2a78f28c72..981da668507 100644 > --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > @@ -443,10 +443,14 @@ is(scalar(() = $stderr =~ /would create the > replication slot/g), > is(scalar(() = $stderr =~ /would create subscription/g), > 3, "verify subscriptions are created for all databases"); > > +# Create user-defined publications. > +$node_p->safe_psql($db2, > + "CREATE PUBLICATION test_pub_existing FOR TABLE tbl2"); > + > # Run pg_createsubscriber on node S. --verbose is used twice > # to show more information. > -# In passing, also test the --enable-two-phase option and > -# --clean option > +# In passing, also test the --enable-two-phase option, > +# --clean option and specifying an existing publication test_pub_existing. > command_ok( > [ > 'pg_createsubscriber', > @@ -457,7 +461,7 @@ command_ok( > '--socketdir' => $node_s->host, > '--subscriber-port' => $node_s->port, > '--publication' => 'pub1', > - '--publication' => 'pub2', > + '--publication' => 'test_pub_existing', > '--replication-slot' => 'replslot1', > '--replication-slot' => 'replslot2', > '--database' => $db1, > @@ -502,6 +506,17 @@ $result = $node_s->safe_psql( > )); > is($result, qq(0), 'pre-existing subscription was dropped'); > > +# Get subscription names and publications > +$result = $node_s->safe_psql( > + 'postgres', qq( > + SELECT subname, subpublications FROM pg_subscription WHERE > subname ~ '^pg_createsubscriber_' > +)); > +like( > + $result, > + qr/^pg_createsubscriber_\d+_[0-9a-f]+ \|\{pub1\}\n > + pg_createsubscriber_\d+_[0-9a-f]+ \|\{test_pub_existing\}$/x, > + "Subscription and publication name are ok"); > + > # Get subscription names > $result = $node_s->safe_psql( > 'postgres', qq( > > Thoughts? >
Hi Vignesh.
OK. Done as suggested in v23.
Normally, I prefer to keep test cases more focused instead of mushing
lots of different test scenarios together. But in the interest of
reducing total the number of tests, I have removed that last --clean
test and combined it with the earlier ("in passing") --clean test as
suggested.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
v23-0001-Support-existing-publications-in-pg_createsubscr.patch
Description: Binary data
