Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
Date
Msg-id CAA4eK1LU+E_vh=9kaq7sAh81bH2gRj_TFDhn8eeakp+FyvfGrQ@mail.gmail.com
Whole thread Raw
In response to Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
List pgsql-hackers
On Mon, Jun 28, 2021 at 3:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > PSA v5 patch set.
> >
> > PSA v6 patch set rebased onto the latest master.
>
> PSA v7 patch set rebased onto the latest master.
>

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?

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.

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."


[1] - https://www.postgresql.org/message-id/YMGSbdV1tMTJroA6%40paquier.xyz

-- 
With Regards,
Amit Kapila.



pgsql-hackers by date:

Previous
From: Dean Rasheed
Date:
Subject: Re: Numeric multiplication overflow errors
Next
From: Dean Rasheed
Date:
Subject: Numeric x^y for negative x