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