On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few comments:
> ===============
> 1.
> +typedef struct SubOpts
> +{
> + bits32 supported_opts; /* bitmap of supported SUBOPT_* */
> + bits32 specified_opts; /* bitmap of user specified SUBOPT_* */
>
> I think it will be better to not keep these as part of this structure.
> Is there a reason for doing so?
I wanted to pack all the parsing related params passed to
parse_subscription_options into a single structure since this is one
of the main points (reducing the number of function params) on which
the patch is coded.
> 2.
> +parse_subscription_options(List *stmt_options, SubOpts *opts)
> {
> ListCell *lc;
> - bool connect_given = false;
> - bool create_slot_given = false;
> - bool copy_data_given = false;
> - bool refresh_given = false;
> + bits32 supported_opts;
> + bits32 specified_opts;
>
> - /* If connect is specified, the others also need to be. */
> - Assert(!connect || (enabled && create_slot && copy_data));
>
> I am not sure whether removing this assertion will bring back the
> coverity error for which it was added but I see that the reason for
> which it was added still holds true. The same is explained by Michael
> as well in his email [1]. I think it is better to keep an equivalent
> assert.
The coverity was complaining FORWARD_NULL which, I think, can occur
with the pointers. In the patch, we don't deal with the pointers for
the options but with the bitmaps. So, I don't think we need that
assertion. However, we can look for the coverity warnings in the
buildfarm after this patch gets in and fix if found any warnings.
> 3.
> * Since not all options can be specified in both commands, this function
> * will report an error on options if the target output pointer is NULL to
> * accommodate that.
> */
> static void
> parse_subscription_options(List *stmt_options, SubOpts *opts)
>
> The comment above this function doesn't seem to match with the new
> code. I think here it is referring to the mutually exclusive errors in
> the function. If you agree with that, can we change the comment to
> something like: "Since not all options can be specified in both
> commands, this function will report an error if mutually exclusive
> options are specified."
Yes. Modified.
Thanks for taking a look at this. PFA v8 patch set for further review.
With Regards,
Bharath Rupireddy.