Re: Re: Add support for specifying tables in pg_createsubscriber.

On Thu, Dec 4, 2025 at 1:56 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 4 Dec 2025 at 07:47, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Wed, Dec 3, 2025 at 4:47 PM tianbing <tian_bing_0531(at)163(dot)com> 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