Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.

On Wed, 19 Mar 2025 at 10:39, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
> > > <david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> > > >
> > >
> > > > It would be good if we could get this to play nicely with —dry-run; maybe connecting to the source for the queries instead 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.

Regards,
Vignesh