On Thu, Sep 16, 2021 at 11:24 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Sep 15, 2021 at 4:45 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > On Tue, Sep 14, 2021 at 6:38 PM vignesh C <vignes...@gmail.com> wrote: > > > > > > I have handled this in the patch attached. > > > > > > > Regarding the following function in the v28-0002 patch: > > > > +/* > > + * Check if the relation schema is member of the schema list. > > + */ > > +static void > > +RelSchemaIsMemberOfSchemaList(List *rels, List *schemaidlist, bool > > schemacheck) > > > > I think this function is not well named or commented, and I don't like > > how the "schemacheck" bool parameter determines the type of objects in > > the "rels" list. > > > > I think after fixing the comments in my previous email, the rels list > will become the same for this function but surely the extra parameter > is required for giving object-specific errors. > > > I would suggest you simply split this function into two separate > > functions, corresponding to each of the blocks of the "if-else" within > > the for-loop of the existing RelSchemaIsMemberOfSchemaList function. > > The "Is" part of the existing "RelSchemaIsMemberOfSchemaList" function > > name implies a boolean return value, so seems misleading. > > So I think the names of the two functions that I am suggesting should > > be "CheckXXXXNotAlreadyInPublication" or something similar. > > > > I think if we write individual functions then we need to add new > functions as and when we add new object types like sequences. The > other idea could be to keep a single function like now say > CheckObjSchemaNotAlreadyInPublication and instead of the bool > parameter as the patch has now, we can keep an enum parameter > "add_obj_type" for 'rel', 'schema', 'sequence'. We can either use > exiting enum PublicationObjSpecType or define a new one for the same.
Modified the function name and changed the parameter to PublicationObjSpecType. The changes are present at the v29 patch posted at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1Wb%3D_HGd85wp2WM%2BfLc-8PSJ824TOZEJ6nDz3akWTidw%40mail.gmail.com Regards, Vignesh