On Thu, Sep 16, 2021 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 15, 2021 at 4:45 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> >
> > On Tue, Sep 14, 2021 at 6:38 PM vignesh C <vignesh21@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
