From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Amul Sul <sulamul(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options |
Date: | 2021-06-29 15:26:40 |
Message-ID: | CALj2ACWYzWG+Hk6AbKd7pg-Htsii+GxeG5Vj7gMaPXBMq-ALAQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
Attachment | Content-Type | Size |
---|---|---|
v8-0001-Refactor-parse_subscription_options.patch | application/x-patch | 26.9 KB |
v8-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch | application/x-patch | 4.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2021-06-29 16:11:44 | Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options |
Previous Message | Julien Rouhaud | 2021-06-29 15:12:36 | Re: track_planning causing performance regression |