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. -- With Regards, Amit Kapila.
