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

On Thu, Sep 25, 2025 at 6:36 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Sep 25, 2025, at 16:18, Shubham Khanna <khannashubham1197@gmail.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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia