Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Wed, Mar 19, 2025 at 12:09 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 19 Mar 2025 at 10:39, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> > > <khannashubham1197@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
> > > > <david.g.johnston@gmail.com> wrote:
> > > > >
> > > >
> > > > > It would be good if we could get this to play nicely with —dry-run; maybe connecting to the source for the
queriesinstead of the target.  That would help alleviate my issue with the current auto-drop behavior. 
> > > > >
> > > >
> > > ...
> > > > IIUC, for testing with --dry-run, we can directly check the relevant
> > > > stdout logs (e.g., "dropping publication 'test_pub1' ...") to verify
> > > > the call without actually dropping the publications.
> > > > However, IMO, using --dry-run alone would miss code coverage for the
> > > > actual drop publication execution part.
> > > >
> > >
> > > I don't understand --dry-run part of conversation here. As per
> > > existing code (or with the patch), we seem to be already printing the
> > > publications to be dropped in dry-run mode.
> > >
> > > *
> > > - drop_publication(conn, &dbinfo[i]);
> > > + if (dbinfos.remove_objects & OBJECT_PUBLICATIONS)
> > > + drop_all_publications(conn, &dbinfo[i]);
> > > + else
> > > + drop_publication(conn, &dbinfo[i], dbinfo[i].pubname);
> > >
> > > I was expecting a call like the existing function
> > > check_and_drop_existing_subscriptions(). The new function should check
> > > if the user requested to remove the publication then it should query
> > > the publisher, otherwise, just remove the one specified by dbinfo.
> > > Also, the core drop_publication() function should take the required
> > > parameters instead of dbinfo after this patch. That would simplify the
> > > code.
> > >
> > > --
> >
> > I have updated the function drop_all_publications() to
> > check_and_drop_publications(), which now checks if the user requested
> > to remove the publication; if so, it queries the publisher. Otherwise,
> > it removes the one specified by dbinfo. Additionally, the
> > drop_publication() function now takes the required parameters directly
> > instead of dbinfo, simplifying the code.
> >
> > The attached patch contains the suggested changes.
>
> Few comments
> 1)How about we change the below:
>  #define        DEFAULT_SUB_PORT        "50432"
> +#define        OBJECTTYPE_PUBLICATIONS  0x1
>
> to
> #define        OBJECTTYPE_PUBLICATIONS  0x0001
>
> That way it will not require to be changed when we add more remove
> options in the future.
>
> 2) Since this is a global variable, I felt this initialization is not required:
> +       /* Verify the object types specified for removal from the subscriber */
> +       dbinfos.objecttypes_to_remove = 0x0;
> +       if (opt.objecttypes_to_remove.head != NULL)
>
> 3) Since this is a single statement, braces is not required:
> +                       if (pg_strcasecmp(cell->val, "publications") == 0)
> +                       {
> +                               dbinfos.objecttypes_to_remove |=
> OBJECTTYPE_PUBLICATIONS;
> +                       }
>
> 4) Similarly here too:
> +                               {
> +
> simple_string_list_append(&opt.objecttypes_to_remove, optarg);
> +                               }
>
> I have attached the changes for the same, if you are ok with the
> changes kindly merge it.
>

Thank you for the suggestions. I agree with the changes you have
attached — defining OBJECTTYPE_PUBLICATIONS as 0x0001 makes future
extensions easier, and removing unnecessary braces and redundant
initialization simplifies the code.

I have merged the changes and prepared the latest patch. The attached
patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.