Re: Add support for specifying tables in pg_createsubscriber.

On Fri, Sep 26, 2025 at 6:06 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Thu, Sep 25, 2025 at 6:36 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Sep 25, 2025, at 16:18, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> >
> >
> >
> > made_publication will always be set regardless of dry_run.
> >
> >
> > You’re right — I made a mistake in my earlier explanation.
> > made_publication is always set in create_publication(), regardless of
> > dry-run. I have double-checked the dry-run output across the cases,
> > and from what I can see the messages are being logged correctly.
> >
> > If you think there’s a specific combination where the dry-run logging
> > isn’t behaving as expected, could you point me to it? From my testing
> > it looks fine, but I want to be sure I’m not missing a corner case.
> >
> >
> > I think, here you code has a logic difference from the old code:
> >
> > * With the old code, even if drop_all_pubs, as long as dry_run, it will still run drop_publication().
> > * With your code, if drop_all_pubs, then never run drop_publication(), because you moved the logic into “else”.
> >
> > To be honest, I am not 100% sure which is correct, I am just pointing out the difference.
> >
>
> Hi Shubham.
>
> Chao is correct - that logic has changed slightly now. That stems from
> my suggestion a couple of versions back to rewrite this as an if/else.
> At that time, I thought there was a risk of a
> double-drop/double-logging happening for the scenario of
> 'drop_all_pubs' in a 'dry_run'. In hindsight, it looks like that was
> not possible because the 'drop_all_pubs' code can only drop
> publications that it *finds*, but for 'dry_run', it's not going to
> find the newly "made" publications because although
> create_publication() was called, they were not actually created. I did
> not recognise that was the intended meaning of the original code
> comment.
>
> ~~
>
> Even if the patch reverts to the original condition, there still seem
> to be some quirks to be worked out:
>
> * The original explanatory comment could have been better:
> BEFORE
> In dry-run mode, we don't create publications, but we still try to
> drop those to provide necessary information to the user.
> AFTER
> In dry-run mode, create_publication() and drop_publication() do not
> actually create or drop anything -- they only do logging. So, we still
> need to call drop_publication() to log information to the user.
>
> * I'm not sure if that "preserve existing" logging should be there or
> not? What exactly is it for? If you reinstate the original
> "(!drop_all_pubs || dry_run)" condition, then it seems possible to log
> "preserve existing" also for 'drop_all_pubs', but that is contrary to
> the docs. Is this just a leftover from a few versions back, when
> 'drop_all_pubs' would not drop everything?
>
> * It is a bit concerning that although this function appears slightly
> broken (e.g. causing wrong logging), the tests cannot detect it.
>
> ~~
>
> The bottom line is, I think you'll need to make a matrix of every
> possible combination of made=Y/N; drop_all_pub=Y/N; dry_run=Y/N; etc
> and then decide exactly what logging you want for each. I don't know
> if it is possible to automate such testing -- it might be overkill --
> but at least the expected logging can be posted in this thread, so the
> code can be reviewed properly.
>

Hi Peter, Chao,

Thanks for the detailed observations. I went back and prepared a
logging matrix for the different combinations of made_publication,
drop_all_pubs, and dry_run. This highlights where the behavior
diverges from the old code.

Summary of findings:
- When --clean=publications is used together with --dry-run, the code
correctly logs "dropping all existing publications", but it fails to
log the individual drop_publication() messages (e.g., "dropping
publication pubX").
- This affects both user-created (--publication=...) and auto-created
publications.
- Most other cases (new pub only, existing pub only, auto pub only)
behave as expected.
- New bug discovered: In the existing pub + clean case, the logs show
both "dropping publication pub1" and "preserve existing publication
pub1". This is contradictory and comes from the original code path
falling through to the “preserve” branch even when drop_all_pubs=true.

The restructuring into if/else caused the missing individual
drop_publication() logs in dry-run mode when drop_all_pubs=true. The
original condition also had a flaw: in the existing pub + clean case,
it could log both drop and preserve for the same publication.
I have updated the conditions so that:
- drop_publication() is always invoked in dry-run for correct logging.
- The “preserve existing” log is suppressed when drop_all_pubs=true,
eliminating the contradictory messages.

The attached patch contains the changes, and the attached image shows
the complete logging matrix for reference.

Thanks and regards,
Shubham Khanna.