Re: Add support for specifying tables in pg_createsubscriber.

On Tue, Sep 16, 2025 at 7:20 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some v7 review comments.
>
> ======
> Commit message
>
> 1.
> This change eliminates the need to know in advance which publications exist
> on the publisher, making the tool more user-friendly. Users can specify
> publication names and the tool will handle both existing and new publications
> appropriately.
>
> ~
>
> I disagree with the "eliminates the need to know" part. This change
> doesn't remove that responsibility from the user. They still need to
> be aware of what the existing publications are so they don't end up
> reusing a publication they did not intend to reuse.
>
> ~~~
>
> 2. <general>
> It would be better if the commit message wording was consistent with
> the wording in the docs.
>

Updated the commit message as suggested.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 3.
> + <para>
> + When reusing existing publications, you should understand their current
> + configuration. Existing publications are used exactly as configured,
> + which may replicate different tables than expected.
> + New publications created with <literal>FOR ALL TABLES</literal> will
> + replicate all tables in the database, which may be more than intended.
> + </para>
>
> Is that paragraph needed? What does it say that was not already said
> by the previous paragraph?
>

Removed.

> ~~~
>
> 4.
> + <para>
> + Use <option>--dry-run</option> to see which publications will be reused
> + and which will be created before running the actual command.
> + When publications are reused, they will not be dropped during cleanup
> + operations, ensuring they remain available for other uses.
> + Only publications created by
> + <application>pg_createsubscriber</application> will be cleaned up.
> + </para>
>
> There also needs to be more clarity about the location of the
> publications that are getting "cleaned up". AFAIK the function
> check_and_drop_publications() only cleans up for the target server,
> but that does not seem at all obvious here.
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> setup_publisher:
>
> 5.
> - if (num_pubs == 0 || num_subs == 0 || num_replslots == 0)
> + if (dbinfo[i].pubname == NULL || dbinfo[i].subname == NULL ||
> dbinfo[i].replslotname == NULL)
> genname = generate_object_name(conn);
> - if (num_pubs == 0)
> + if (dbinfo[i].pubname == NULL)
> dbinfo[i].pubname = pg_strdup(genname);
> - if (num_subs == 0)
> + if (dbinfo[i].subname == NULL)
> dbinfo[i].subname = pg_strdup(genname);
> - if (num_replslots == 0)
> + if (dbinfo[i].replslotname == NULL)
> dbinfo[i].replslotname = pg_strdup(dbinfo[i].subname);
> ~
>
> Are these changes related to the --publication change, or are these
> some other issue?
>

No, these changes are not related to the --publication modification.
They were unrelated adjustments, and I have now reverted them back to
match the behavior in HEAD.

> ~~~
>
> 6.
> * consistent LSN and the new publication rows (such transactions
> * wouldn't see the new publication rows resulting in an error).
> */
> - create_publication(conn, &dbinfo[i]);
> + if (check_publication_exists(conn, dbinfo[i].pubname, dbinfo[i].dbname))
>
> The comment preceding this if/else is only talking about "Create the
> publications..", but it should be more like "Reuse existing or create
> new publications...". Alternatively, move the comments within the if
> and else.
>

Fixed.

> ~~~
>
> check_and_drop_publications:
>
> 7.
> if (!drop_all_pubs || dry_run)
> - drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
> - &dbinfo->made_publication);
> + {
> + if (dbinfo->made_publication)
> + drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
> + &dbinfo->made_publication);
> + }
>
> I find this function logic confusing. In particular, why is the
> "made_publication" flag only checked for dry_run?
>
> This function comment says it will "drop all pre-existing
> publications." but doesn't that contradict your commit message and
> docs that said statements like "When publications are reused, they are
> never dropped during cleanup operations"?
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 8.
> IMO one of the most important things for the user is that they must be
> able to know exactly which publications will be reused, and which
> publications will be created as FOR ALL TABLES. So, there should be a
> test to verify that the --dry_run option emits all the necessary
> logging so the user can tell that.
>

Fixed.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.