Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

On Mon, Jun 28, 2021 at 3:24 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)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.