Re: Re: Add support for specifying tables in pg_createsubscriber. - Mailing list pgsql-hackers

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