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

On Tue, Sep 16, 2025 at 2:17 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham,
>
> Here are some v8 review comments.
>
> ======
> Commit message
>
> 1.
> Previously, pg_createsubscriber would fail if any specified publication already
> existed. Now, existing publications are reused as-is with their current
> configuration, and non-existing publications are createdcautomatically with
> FOR ALL TABLES.
>
> ~
>
> typo: "createdcautomatically"
>

Fixed.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> +      <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> on the target server will
> +       be cleaned up if the operation fails. Publications on the publisher
> +       server are never modified or dropped by cleanup operations.
> +      </para>
>
> I still find all this confusing, for the following reasons:
>
> * The "dry-run" advice is OK, but the "cleanup of existing pubs" seems
> like a totally different topic which has nothing much to do with the
> --publication option. I'm wondering if you need to talk about cleaning
> existing pubs, maybe that info belongs in the "Notes" part of this
> documentation.
>
> * I don't understand how does saying "existing pubs will not be
> dropped" reconcile with the --clean=publication option which says it
> will drop all publications? They seem contradictory.
>

I have now moved the paragraph about publications being preserved or
dropped under the description of the --clean option instead, where it
fits more naturally. This way, the --publication section focuses only
on creation/reuse semantics, while the --clean section clearly
documents the cleanup behavior.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> check_and_drop_publications:
>
> 3.
>   /*
> - * In dry-run mode, we don't create publications, but we still try to drop
> - * those to provide necessary information to the user.
> + * Only drop publications that were created by pg_createsubscriber during
> + * this operation. Pre-existing publications are preserved.
>   */
> - if (!drop_all_pubs || dry_run)
> + if (dbinfo->made_publication)
>   drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
>   &dbinfo->made_publication);
>
> 3a.
> Sorry, but I still have the same question that I had in my previous v7
> review. This function logic will already remove all pre-existing
> publications when the 'drop_all_pubs' variable is true. It seems
> contrary to the commit message that says "When publications are
> reused, they are never dropped during cleanup operations, ensuring
> pre-existing publications remain available for other uses."
>

Fixed. The logic has been updated so that pre-existing publications
are not dropped, consistent with the commit message.

> ~
>
> 3b.
> Kind of similar to the previous review -- If the 'drop_all_pubs'
> variable is true, then AFAICT this code attempts to drop again one of
> the publications that was already dropped in the earlier part of this
> function?
>

Fixed. The redundant drop call has been removed.

> ~
>
> 3c.
> If this drop logic is broken wrt the intended cleanup rules then it
> also means more/better --clean option tests are needed, otherwise how
> was this passing the tests?
>

I have added a dedicated test case for the --clean option to cover
these scenarios and verify correct cleanup behavior.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.