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

Hi,

parse_subscription_options function has some similar code when
throwing errors [with the only difference in the option]. I feel we
could just use a variable for the option and use it in the error.
While this has no benefit at all, it saves some LOC and makes the code
look better with lesser ereport(ERROR statements. PSA patch.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> parse_subscription_options function has some similar code when
> throwing errors [with the only difference in the option]. I feel we
> could just use a variable for the option and use it in the error.
> While this has no benefit at all, it saves some LOC and makes the code
> look better with lesser ereport(ERROR statements. PSA patch.
>
> Thoughts?

I don't have a strong opinion on this, but the patch should add
__translator__ help comment for the error msg.

Regards,
Amul

On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > parse_subscription_options function has some similar code when
> > throwing errors [with the only difference in the option]. I feel we
> > could just use a variable for the option and use it in the error.
> > While this has no benefit at all, it saves some LOC and makes the code
> > look better with lesser ereport(ERROR statements. PSA patch.
> >
> > Thoughts?
>
> I don't have a strong opinion on this, but the patch should add
> __translator__ help comment for the error msg.

Is the "/*- translator:" help comment something visible to the user or
some other tool? If not, I don't think that's necessary as the meaning
of the error message is evident by looking at the error message
itself. IMO, anyone who looks at that part of the code can understand
it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > parse_subscription_options function has some similar code when
> > > throwing errors [with the only difference in the option]. I feel we
> > > could just use a variable for the option and use it in the error.

I am not sure how much it helps to just refactor this part of the code
alone unless we need to add/change it more. Having said that, this
function is being modified by one of the proposed patches for logical
decoding of 2PC and I noticed that the proposed patch is adding more
parameters to this function which already takes 14 input parameters,
so I suggested refactoring it. See comment 11 in email[1]. See, if
that makes sense to you then we can refactor this function such that
it can be enhanced easily by future patches.

> > > While this has no benefit at all, it saves some LOC and makes the code
> > > look better with lesser ereport(ERROR statements. PSA patch.
> > >
> > > Thoughts?
> >
> > I don't have a strong opinion on this, but the patch should add
> > __translator__ help comment for the error msg.
>
> Is the "/*- translator:" help comment something visible to the user or
> some other tool?
>

We use similar comments at other places. So, it makes sense to retain
the comment as it might help translation tools.

[1] - https://www.postgresql.org/message-id/CAA4eK1Jz64rwLyB6H7Z_SmEDouJ41KN42%3DVkVFp6JTpafJFG8Q%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

On Wed, May 19, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> > >
> > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > parse_subscription_options function has some similar code when
> > > > throwing errors [with the only difference in the option]. I feel we
> > > > could just use a variable for the option and use it in the error.
>
> I am not sure how much it helps to just refactor this part of the code
> alone unless we need to add/change it more. Having said that, this
> function is being modified by one of the proposed patches for logical
> decoding of 2PC and I noticed that the proposed patch is adding more
> parameters to this function which already takes 14 input parameters,
> so I suggested refactoring it. See comment 11 in email[1]. See, if
> that makes sense to you then we can refactor this function such that
> it can be enhanced easily by future patches.

Thanks Amit for the comments. I agree to move the parse options to a
new structure ParseSubOptions as suggested. Then the function can just
be parse_subscription_options(ParseSubOptions opts); I wonder if we
should also have a structure for parse_publication_options as we might
add new options there in the future?

If okay, I can work on these changes and attach it along with these
error message changes. Thoughts?

> > > > While this has no benefit at all, it saves some LOC and makes the code
> > > > look better with lesser ereport(ERROR statements. PSA patch.
> > > >
> > > > Thoughts?
> > >
> > > I don't have a strong opinion on this, but the patch should add
> > > __translator__ help comment for the error msg.
> >
> > Is the "/*- translator:" help comment something visible to the user or
> > some other tool?
> >
>
> We use similar comments at other places. So, it makes sense to retain
> the comment as it might help translation tools.

I will retail it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > parse_subscription_options function has some similar code when
> > > > > throwing errors [with the only difference in the option]. I feel we
> > > > > could just use a variable for the option and use it in the error.
> >
> > I am not sure how much it helps to just refactor this part of the code
> > alone unless we need to add/change it more. Having said that, this
> > function is being modified by one of the proposed patches for logical
> > decoding of 2PC and I noticed that the proposed patch is adding more
> > parameters to this function which already takes 14 input parameters,
> > so I suggested refactoring it. See comment 11 in email[1]. See, if
> > that makes sense to you then we can refactor this function such that
> > it can be enhanced easily by future patches.
>
> Thanks Amit for the comments. I agree to move the parse options to a
> new structure ParseSubOptions as suggested. Then the function can just
> be parse_subscription_options(ParseSubOptions opts); I wonder if we
> should also have a structure for parse_publication_options as we might
> add new options there in the future?
>

That function has just 5 parameters so not sure if that needs the same
treatment. Let's leave it for now.

-- 
With Regards,
Amit Kapila.

On Wed, May 19, 2021 at 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> > > > >
> > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > parse_subscription_options function has some similar code when
> > > > > > throwing errors [with the only difference in the option]. I feel we
> > > > > > could just use a variable for the option and use it in the error.
> > >
> > > I am not sure how much it helps to just refactor this part of the code
> > > alone unless we need to add/change it more. Having said that, this
> > > function is being modified by one of the proposed patches for logical
> > > decoding of 2PC and I noticed that the proposed patch is adding more
> > > parameters to this function which already takes 14 input parameters,
> > > so I suggested refactoring it. See comment 11 in email[1]. See, if
> > > that makes sense to you then we can refactor this function such that
> > > it can be enhanced easily by future patches.
> >
> > Thanks Amit for the comments. I agree to move the parse options to a
> > new structure ParseSubOptions as suggested. Then the function can just
> > be parse_subscription_options(ParseSubOptions opts); I wonder if we
> > should also have a structure for parse_publication_options as we might
> > add new options there in the future?
> >
>
> That function has just 5 parameters so not sure if that needs the same
> treatment. Let's leave it for now.

Thanks. I will work on the new structure ParseSubOption only for
subscription options.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Thanks. I will work on the new structure ParseSubOption only for
> subscription options.

PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
the error reporting code refactoring.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > Thanks. I will work on the new structure ParseSubOption only for
> > subscription options.
>
> PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
> the error reporting code refactoring.
>

I have applied the v2 patch and done some review of the code.

- The patch applies OK.

- The code builds OK.

- The make check and TAP subscription tests are OK


I am not really a big fan of this patch - it claims to make things
easier for future options, but IMO the changes sometimes seem at the
expense of readability of the *current* code. The following comments
are only posted here, not as endorsement, but because I already
reviewed the code so they may be of some use in case the patch goes
ahead...

COMMENTS
==========

parse_subscription_options:

1.
I felt the function implementation is less readable now than
previously due to the plethora of "opts->" introduced everywhere.
Maybe it would be worthwhile to assign all those opts back to local
vars (of the same name as the original previous 14 args), just for the
sake of getting rid of all those "opts->"?

----------

2.
(not the fault of this patch) Inside the parse_subscription_options
function, there seem many unstated assertions that if a particular
option member opts->XXX is passed, then the opts->XXX_given is also
present (although that is never checked). Perhaps the code should
explicitly Assert those XXX_given vars?

----------

3.
@@ -225,65 +238,63 @@ parse_subscription_options(List *options,
  * We've been explicitly asked to not connect, that requires some
  * additional processing.
  */
- if (connect && !*connect)
+ if (opts->connect && !*opts->connect)
  {
+ char *option = NULL;

"option" seems too generic. Maybe "incompatible_option" would be a
better name for that variable?

----------

4.
- errmsg("%s and %s are mutually exclusive options",
- "slot_name = NONE", "create_slot = true")));
+ option = NULL;

- if (enabled && !*enabled_given && *enabled)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- /*- translator: both %s are strings of the form "option = value" */
- errmsg("subscription with %s must also set %s",
- "slot_name = NONE", "enabled = false")));
+ if (opts->enabled && !*opts->enabled_given && *opts->enabled)
+ option = "enabled = false";
+ else if (opts->create_slot && !create_slot_given && *opts->create_slot)
+ option = "create_slot = false";


In the above code you don't need to set option = NULL, because it must
already be NULL. But for this 2nd chunk of code I think it would be
better to introduce another variable called something like
"required_option".

==========

Create Subscription:

5.
@@ -346,22 +357,32 @@ CreateSubscription(CreateSubscriptionStmt *stmt,
bool isTopLevel)
  char originname[NAMEDATALEN];
  bool create_slot;
  List    *publications;
+ ParseSubOptions *opts;
+
+ opts = (ParseSubOptions *) palloc0(sizeof(ParseSubOptions));
+
+ /* Fill only the options that are of interest here. */
+ opts->stmt_options = stmt->options;
+ opts->connect = &connect;


I feel that this code ought to be using a stack variable instead of
allocating on the heap because - less code, easier to read, no free
required. etc.

Just memset it to fill all 0s before assigning the values.

----------

6.
+ /* Fill only the options that are of interest here. */

The comment is kind of redundant, and what you are setting are not
really all options either.

Maybe better like this? Or maybe don't have the comment at all?

/* Assign only members of interest. */
MemSet(&opts, 0, sizeof(opts));
opts.stmt_options = stmt->options;
opts.connect = &connect;
opts.enabled_given = &enabled_given;
opts.enabled = &enabled;
opts.create_slot = &create_slot;
...

==========

AlterSubscription

7.
Same review comment as for CreateSubscription.
- Use a stack variable and memset.
- Change or remove the comment.

----------

8.
For AlterSubscriotion you could also declare the "opts" just time only
and memset it at top of the function, instead of the current code
which repeats 5X the same thing.

----------

9.
+ /* For DROP PUBLICATION, copy_data option is not supported. */
+ opts->copy_data = isadd ? ©_data : NULL;

The opts struct is already zapped 0/NULL so this code maybe should be:

if (isadd)
opts.copy_data = ©_data;

==========

10.
Since the new typedef ParseSubOptions was added by this patch
shouldn't the src/tools/pgindent/typedefs.list file be updated also?

----------
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Fri, May 21, 2021 at 9:21 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > Thanks. I will work on the new structure ParseSubOption only for
> > > subscription options.
> >
> > PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
> > the error reporting code refactoring.
> >
>
> I have applied the v2 patch and done some review of the code.
>
> - The patch applies OK.
>
> - The code builds OK.
>
> - The make check and TAP subscription tests are OK
>
>
> I am not really a big fan of this patch - it claims to make things
> easier for future options, but IMO the changes sometimes seem at the
> expense of readability of the *current* code. The following comments
> are only posted here, not as endorsement, but because I already
> reviewed the code so they may be of some use in case the patch goes
> ahead...
>
> COMMENTS
> ==========
>
> parse_subscription_options:
>
> 1.
> I felt the function implementation is less readable now than
> previously due to the plethora of "opts->" introduced everywhere.
> Maybe it would be worthwhile to assign all those opts back to local
> vars (of the same name as the original previous 14 args), just for the
> sake of getting rid of all those "opts->"?
>
> ----------
>
> 2.
> (not the fault of this patch) Inside the parse_subscription_options
> function, there seem many unstated assertions that if a particular
> option member opts->XXX is passed, then the opts->XXX_given is also
> present (although that is never checked). Perhaps the code should
> explicitly Assert those XXX_given vars?
>
> ----------
>
> 3.
> @@ -225,65 +238,63 @@ parse_subscription_options(List *options,
>   * We've been explicitly asked to not connect, that requires some
>   * additional processing.
>   */
> - if (connect && !*connect)
> + if (opts->connect && !*opts->connect)
>   {
> + char *option = NULL;
>
> "option" seems too generic. Maybe "incompatible_option" would be a
> better name for that variable?
>
> ----------
>
> 4.
> - errmsg("%s and %s are mutually exclusive options",
> - "slot_name = NONE", "create_slot = true")));
> + option = NULL;
>
> - if (enabled && !*enabled_given && *enabled)
> - ereport(ERROR,
> - (errcode(ERRCODE_SYNTAX_ERROR),
> - /*- translator: both %s are strings of the form "option = value" */
> - errmsg("subscription with %s must also set %s",
> - "slot_name = NONE", "enabled = false")));
> + if (opts->enabled && !*opts->enabled_given && *opts->enabled)
> + option = "enabled = false";
> + else if (opts->create_slot && !create_slot_given && *opts->create_slot)
> + option = "create_slot = false";
>
>
> In the above code you don't need to set option = NULL, because it must
> already be NULL. But for this 2nd chunk of code I think it would be
> better to introduce another variable called something like
> "required_option".
>
> ==========
>
> Create Subscription:
>
> 5.
> @@ -346,22 +357,32 @@ CreateSubscription(CreateSubscriptionStmt *stmt,
> bool isTopLevel)
>   char originname[NAMEDATALEN];
>   bool create_slot;
>   List    *publications;
> + ParseSubOptions *opts;
> +
> + opts = (ParseSubOptions *) palloc0(sizeof(ParseSubOptions));
> +
> + /* Fill only the options that are of interest here. */
> + opts->stmt_options = stmt->options;
> + opts->connect = &connect;
>
>
> I feel that this code ought to be using a stack variable instead of
> allocating on the heap because - less code, easier to read, no free
> required. etc.
>
> Just memset it to fill all 0s before assigning the values.
>
> ----------
>
> 6.
> + /* Fill only the options that are of interest here. */
>
> The comment is kind of redundant, and what you are setting are not
> really all options either.
>
> Maybe better like this? Or maybe don't have the comment at all?
>
> /* Assign only members of interest. */
> MemSet(&opts, 0, sizeof(opts));
> opts.stmt_options = stmt->options;
> opts.connect = &connect;
> opts.enabled_given = &enabled_given;
> opts.enabled = &enabled;
> opts.create_slot = &create_slot;
> ...
>
> ==========
>
> AlterSubscription
>
> 7.
> Same review comment as for CreateSubscription.
> - Use a stack variable and memset.
> - Change or remove the comment.
>
> ----------
>
> 8.
> For AlterSubscriotion you could also declare the "opts" just time only
> and memset it at top of the function, instead of the current code
> which repeats 5X the same thing.
>
> ----------
>
> 9.
> + /* For DROP PUBLICATION, copy_data option is not supported. */
> + opts->copy_data = isadd ? ©_data : NULL;
>
> The opts struct is already zapped 0/NULL so this code maybe should be:
>
> if (isadd)
> opts.copy_data = ©_data;
>
> ==========
>
> 10.
> Since the new typedef ParseSubOptions was added by this patch
> shouldn't the src/tools/pgindent/typedefs.list file be updated also?
>

Thinking about this some more, a few other things occurred to me which
might help simplify the code.

==========

11.

+/*
+ * Structure to hold subscription options for parsing
+ */
+typedef struct ParseSubOptions
+{
+ List *stmt_options;
+ bool *connect;
+ bool *enabled_given;
+ bool *enabled;
+ bool *create_slot;
+ bool *slot_name_given;
+ char **slot_name;
+ bool *copy_data;
+ char **synchronous_commit;
+ bool *refresh;
+ bool *binary_given;
+ bool *binary;
+ bool *streaming_given;
+ bool *streaming;
+} ParseSubOptions;

Maybe I am mistaken, but I am wondering why all this indirection is
even necessary anymore?

IIUC previously args were declared like bool * so the information
could be returned to the caller. But now you have ParseSubOption * to
do that, so can't you simply declare all those "given" members as bool
instead of bool *; And when you do that you can remove all the
unnecessary storage vars in the calling code as well.

--------

12.
The original code seems to have a way to "register interest in the
option" by passing the storage var in which to return the result. (eg.
pass "enabled" not NULL).

IIUC the purpose of this is what it says in the function comment
function comment:

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

But now that you have your new struct, I wonder if there is another
easy way to achieve the same. e.g. you now could add some more members
instead of the non-NULL pointer to register interest in a particular
option. Then those other members (like "enabled") also only need to be
bool instead of bool *;

Something like this?

BEFORE:
else if (strcmp(defel->defname, "enabled") == 0 && opts->enabled)
{
if (*opts->enabled_given)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));

*opts->enabled_given = true;
*opts->enabled = defGetBoolean(defel);
}
AFTER:
if (opts->enabled_is_allowed && strcmp(defel->defname, "enabled") == 0)
{
if (opts->enabled_given)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));

opts->enabled_given = true;
opts->enabled = defGetBoolean(defel);
}

I am unsure if this will lead to better code or not; Anyway, it is
something to consider - maybe you can experiment with it to see.

----------

13.
Regardless of review comment #12, I think all those strcmp conditions
ought to be reversed for better efficiency.

e.g.
BEFORE:
else if (strcmp(defel->defname, "binary") == 0 && opts->binary)
AFTER:
else if (opts->binary && strcmp(defel->defname, "binary") == 0)

----------
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Sat, May 22, 2021 at 6:32 AM Peter Smith <smithpb2250@gmail.com> wrote:
> I am unsure if this will lead to better code or not; Anyway, it is
> something to consider - maybe you can experiment with it to see.

Thanks. I think using bitmaps would help us have clean code. This is
also more extensible. See pseudo code at [1]. One disadvantage is that
we might have bms_XXXfunction calls, but that's okay and it shouldn't
add too much to the performance. Thoughts?

[1]
typedef enum SubOpts_enum
{
SUB_OPT_NONE = 0,
SUB_OPT_CONNECT,
SUB_OPT_ENABLED,
SUB_OPT_CREATE_SLOT,
SUB_OPT_SLOT_NAME,
SUB_OPT_COPY_DATA,
SUB_OPT_SYNCHRONOUS_COMMIT,
SUB_OPT_REFRESH,
SUB_OPT_BINARY,
SUB_OPT_STREAMING
} SubOpts_enum;

typedef struct SubOptsVals
{
bool connect;
bool enabled;
bool create_slot;
char *slot_name;
bool copy_data;
char *synchronous_commit;
bool refresh;
bool binary;
bool streaming;
} SubOptsVals;

Bitmapset  *supported = NULL;
Bitmapset  *specified = NULL;
ParsedSubOpts opts;

MemSet(opts, 0, sizeof(ParsedSubOpts));
/* Fill in all the supported options, we could use bms_add_member as
well if there are less number of supported options.*/
supported = bms_add_range(NULL, SUB_OPT_CONNECT, SUB_OPT_STREAMING);
supported = bms_del_member(supported, SUB_OPT_REFRESH);

parse_subscription_options(stmt_options, supported, specified, &opts);

if (bms_is_member(SUB_OPT_SLOT_NAME, specified))
{
    /* get slot name with opts.slot_name */
}

if (bms_is_member(SUB_OPT_SYNCHRONOUS_COMMIT, specified))
{
     /* get slot name with opts.synchronous_commit */
}

/* Similarly get the other options. */

bms_free(supported);
bms_free(specified);

static void
parse_subscription_options(List *stmt_options,
                Bitmapset *supported,
                Bimapset *specified,
                SubOptsVals *opts)
{

    foreach(lc, stmt_options)
    {
        DefElem    *defel = (DefElem *) lfirst(lc);

        if (bms_is_member(SUB_OPT_CONNECT, supported) &&
            strcmp(defel->defname, "connect") == 0)
        {
            if (bms_is_member(SUB_OPT_CONNECT, specified))
                ereport(ERROR,
                        (errcode(ERRCODE_SYNTAX_ERROR),
                         errmsg("conflicting or redundant options")));

            specified = bms_add_member(specified, SUB_OPT_CONNECT);
            opts->connect = defGetBoolean(defel);
        }

        /* Similarly do the same for the other options. */
    }
}

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 22, 2021 at 01:47:24PM +0530, Bharath Rupireddy wrote:
> Thanks. I think using bitmaps would help us have clean code. This is
> also more extensible. See pseudo code at [1]. One disadvantage is that
> we might have bms_XXXfunction calls, but that's okay and it shouldn't
> add too much to the performance. Thoughts?
>
> [1]
> typedef enum SubOpts_enum
> {
> SUB_OPT_NONE = 0,
> SUB_OPT_CONNECT,
> SUB_OPT_ENABLED,
> SUB_OPT_CREATE_SLOT,
> SUB_OPT_SLOT_NAME,
> SUB_OPT_COPY_DATA,
> SUB_OPT_SYNCHRONOUS_COMMIT,
> SUB_OPT_REFRESH,
> SUB_OPT_BINARY,
> SUB_OPT_STREAMING
> } SubOpts_enum;

What you are writing here and your comment two paragraphs above are
inconsistent as you are using an enum here.  Please see a3dc926 and
the surrounding discussion for reasons why we've been using bitmaps
for option parsing lately.
--
Michael

On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> What you are writing here and your comment two paragraphs above are
> inconsistent as you are using an enum here.  Please see a3dc926 and
> the surrounding discussion for reasons why we've been using bitmaps
> for option parsing lately.

Thanks! I'm okay to do something similar to what the commit a3dc926
did using bits32. But I wonder if we will ever cross the 32 options
limit (imposed by bits32) for CREATE/ALTER SUBSCRIPTION command.
Having said that, for now, we can have an error similar to
last_assigned_kind in add_reloption_kind() if the limit is crossed.

I would like to hear opinions before proceeding with the implementation.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-May-24, Bharath Rupireddy wrote:

> On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> > What you are writing here and your comment two paragraphs above are
> > inconsistent as you are using an enum here.  Please see a3dc926 and
> > the surrounding discussion for reasons why we've been using bitmaps
> > for option parsing lately.
> 
> Thanks! I'm okay to do something similar to what the commit a3dc926
> did using bits32. But I wonder if we will ever cross the 32 options
> limit (imposed by bits32) for CREATE/ALTER SUBSCRIPTION command.
> Having said that, for now, we can have an error similar to
> last_assigned_kind in add_reloption_kind() if the limit is crossed.

There's no API limitation here, since that stuff is not user-visible, so
it doesn't matter.  If we ever need a 33rd option, we can change the
datatype to bits64.  Any extensions using it will have to be recompiled
across a major version jump anyway.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-24, Bharath Rupireddy wrote:
>
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > What you are writing here and your comment two paragraphs above are
> > > inconsistent as you are using an enum here.  Please see a3dc926 and
> > > the surrounding discussion for reasons why we've been using bitmaps
> > > for option parsing lately.
> >
> > Thanks! I'm okay to do something similar to what the commit a3dc926
> > did using bits32. But I wonder if we will ever cross the 32 options
> > limit (imposed by bits32) for CREATE/ALTER SUBSCRIPTION command.
> > Having said that, for now, we can have an error similar to
> > last_assigned_kind in add_reloption_kind() if the limit is crossed.
>
> There's no API limitation here, since that stuff is not user-visible, so
> it doesn't matter.  If we ever need a 33rd option, we can change the
> datatype to bits64.  Any extensions using it will have to be recompiled
> across a major version jump anyway.

Thanks. I think there's no bits64 data type currently, I'm sure you
meant we will define (when requirement arises) something like typedef
uint64 bits64; Am I correct?

I see that the commit a3dc926 and discussion at [1] say below respectively:
"All the options of those commands are changed to use hex values
rather than enums to reduce the risk of compatibility bugs when
introducing new options."
"My reasoning is that if you look at an enum value of this type,
either say in a switch statement or a debugger, the enum value might
not be any of the defined symbols. So that way you lose all the type
checking that an enum might give you."

I'm not able to grasp what are the incompatibilities we can have if
the enums are used as bit masks. It will be great if anyone throws
some light on this?

[1] - https://www.postgresql.org/message-id/flat/14dde730-1d34-260e-fa9d-7664df2d6313%40enterprisedb.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 25, 2021 at 10:59:37AM +0530, Bharath Rupireddy wrote:
> I'm not able to grasp what are the incompatibilities we can have if
> the enums are used as bit masks. It will be great if anyone throws
> some light on this?

0176753 is one example.
--
Michael

On Tue, May 25, 2021 at 11:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, May 25, 2021 at 10:59:37AM +0530, Bharath Rupireddy wrote:
> > I'm not able to grasp what are the incompatibilities we can have if
> > the enums are used as bit masks. It will be great if anyone throws
> > some light on this?
>
> 0176753 is one example.

Hm. I get it, it is the coding style incompatibilities. Thanks.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-May-25, Bharath Rupireddy wrote:

> On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > There's no API limitation here, since that stuff is not user-visible, so
> > it doesn't matter.  If we ever need a 33rd option, we can change the
> > datatype to bits64.  Any extensions using it will have to be recompiled
> > across a major version jump anyway.
> 
> Thanks. I think there's no bits64 data type currently, I'm sure you
> meant we will define (when requirement arises) something like typedef
> uint64 bits64; Am I correct?

Right.

> I see that the commit a3dc926 and discussion at [1] say below respectively:
> "All the options of those commands are changed to use hex values
> rather than enums to reduce the risk of compatibility bugs when
> introducing new options."
> "My reasoning is that if you look at an enum value of this type,
> either say in a switch statement or a debugger, the enum value might
> not be any of the defined symbols. So that way you lose all the type
> checking that an enum might give you."
> 
> I'm not able to grasp what are the incompatibilities we can have if
> the enums are used as bit masks. It will be great if anyone throws
> some light on this?

The problem is that enum members have consecutive integers assigned by
the compiler.  Say you have an enum with three values for options.  They
get assigned 0, 1, and 2.  You can test for each option with "opt &
VAL_ONE" and "opt & VAL_TWO" and everything works -- each test returns
true when that specific option is set, and all is well.  Now if somebody
later adds a fourth option, it gets value 3.  When that option is set,
"opt & VAL_ONE" magically returns true, even though you did not set that
bit in your code.  So that becomes a bug.

Using hex values or bitshifting (rather than letting the compiler decide
its value in the enum) is a more robust way to ensure that the options
will not collide in that way.

So why not define the enum as a list, and give each option an exclusive
bit by bitshifting? For example,

enum options {
  OPT_ZERO  = 0,
  OPT_ONE   = 1 << 1,
  OPT_TWO   = 1 << 2,
  OPT_THREE = 1 << 3,
};

This should be okay, right?  Well, almost. The problem here is if you
want to have a variable where you set more than one option, you have to
use bit-and of the enum values ... and the resulting value is no longer
part of the enum.  A compiler would be understandably upset if you try
to pass that value in a variable of the enum datatype.

-- 
Álvaro Herrera       Valdivia, Chile
"Ed is the standard text editor."
      http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3

On Tue, May 25, 2021 at 7:08 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > I see that the commit a3dc926 and discussion at [1] say below respectively:
> > "All the options of those commands are changed to use hex values
> > rather than enums to reduce the risk of compatibility bugs when
> > introducing new options."
> > "My reasoning is that if you look at an enum value of this type,
> > either say in a switch statement or a debugger, the enum value might
> > not be any of the defined symbols. So that way you lose all the type
> > checking that an enum might give you."
> >
> > I'm not able to grasp what are the incompatibilities we can have if
> > the enums are used as bit masks. It will be great if anyone throws
> > some light on this?
>
> The problem is that enum members have consecutive integers assigned by
> the compiler.  Say you have an enum with three values for options.  They
> get assigned 0, 1, and 2.  You can test for each option with "opt &
> VAL_ONE" and "opt & VAL_TWO" and everything works -- each test returns
> true when that specific option is set, and all is well.  Now if somebody
> later adds a fourth option, it gets value 3.  When that option is set,
> "opt & VAL_ONE" magically returns true, even though you did not set that
> bit in your code.  So that becomes a bug.
>
> Using hex values or bitshifting (rather than letting the compiler decide
> its value in the enum) is a more robust way to ensure that the options
> will not collide in that way.
>
> So why not define the enum as a list, and give each option an exclusive
> bit by bitshifting? For example,
>
> enum options {
>   OPT_ZERO  = 0,
>   OPT_ONE   = 1 << 1,
>   OPT_TWO   = 1 << 2,
>   OPT_THREE = 1 << 3,
> };
>
> This should be okay, right?  Well, almost. The problem here is if you
> want to have a variable where you set more than one option, you have to
> use bit-and of the enum values ... and the resulting value is no longer
> part of the enum.  A compiler would be understandably upset if you try
> to pass that value in a variable of the enum datatype.

Thanks a lot for the detailed explanation.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>  Please see a3dc926 and the surrounding discussion for reasons why we've
>  been using bitmaps for option parsing lately.

Thanks for the suggestion. Here's a WIP patch implementing the
subscription command options as bitmaps similar to what commit a3dc926
did. Thoughts?

If the attached WIP patch seems reasonable, I would also like to
implement a similar idea for the parse_publication_options although
there are only two options right now. Thoughts?

With Regards,
Bharath Rupireddy.

On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> >  Please see a3dc926 and the surrounding discussion for reasons why we've
> >  been using bitmaps for option parsing lately.
>
> Thanks for the suggestion. Here's a WIP patch implementing the
> subscription command options as bitmaps similar to what commit a3dc926
> did. Thoughts?

I took a look at this latest WIP patch.

The patch applied cleanly.
The code builds OK.
The make check result is OK.
The TAP subscription make check result is OK.

Below are some minor review comments:

------

+typedef struct SubOptVals
+{
+ bool connect;
+ bool enabled;
+ bool create_slot;
+ char *slot_name;
+ bool copy_data;
+ char *synchronous_commit;
+ bool refresh;
+ bool binary;
+ bool streaming;
+} SubOptVals;
+
+/* options for CREATE/ALTER SUBSCRIPTION  */
+typedef struct SubOpts
+{
+ bits32    supported_opts; /* bitmask of supported SUBOPT_* */
+ bits32    specified_opts; /* bitmask of user specified SUBOPT_* */
+ SubOptVals vals;
+} SubOpts;
+

1. These seem only used by the subscriptioncmds.c file, so should they
be declared in there also instead of in the .h?

2. I don't see what was gained by having the SubOptVals as a separate
struct; OTOH the code accessing the vals is more verbose because of
it. Maybe consider combining everything into SubOpts and then can just
access "opts.copy_data" (etc) instead of "opts.vals.copy_data";

------

+ /* If connect option is supported, the others also need to be. */
+ Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
+    ((supported_opts & SUBOPT_ENABLED) != 0 &&
+ (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
+ (supported_opts & SUBOPT_COPY_DATA) != 0));
+
+ /* Set default values for the supported options. */
+ if ((supported_opts & SUBOPT_CONNECT) != 0)
+ vals->connect = true;
+
+ if ((supported_opts & SUBOPT_ENABLED) != 0)
+ vals->enabled = true;
+
+ if ((supported_opts & SUBOPT_CREATE_SLOT) != 0)
+ vals->create_slot = true;
+
+ if ((supported_opts & SUBOPT_SLOT_NAME) != 0)
+ vals->slot_name = NULL;
+
+ if ((supported_opts & SUBOPT_COPY_DATA) != 0)
+ vals->copy_data = true;

3. Are all those "!= 0" really necessary when checking the
supported_opts against the bit masks? Maybe it is just a style thing,
but since there are so many of them I felt it contributed to clutter
and made the code less readable. This pattern was in many places, not
just the example above.

------

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jun 2, 2021 at 9:07 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >  Please see a3dc926 and the surrounding discussion for reasons why we've
> > >  been using bitmaps for option parsing lately.
> >
> > Thanks for the suggestion. Here's a WIP patch implementing the
> > subscription command options as bitmaps similar to what commit a3dc926
> > did. Thoughts?
>
> I took a look at this latest WIP patch.

Thanks.

> The patch applied cleanly.
> The code builds OK.
> The make check result is OK.
> The TAP subscription make check result is OK.

Thanks for testing.

> Below are some minor review comments:
>
> ------
>
> +typedef struct SubOptVals
> +{
> + bool connect;
> + bool enabled;
> + bool create_slot;
> + char *slot_name;
> + bool copy_data;
> + char *synchronous_commit;
> + bool refresh;
> + bool binary;
> + bool streaming;
> +} SubOptVals;
> +
> +/* options for CREATE/ALTER SUBSCRIPTION  */
> +typedef struct SubOpts
> +{
> + bits32    supported_opts; /* bitmask of supported SUBOPT_* */
> + bits32    specified_opts; /* bitmask of user specified SUBOPT_* */
> + SubOptVals vals;
> +} SubOpts;
> +
>
> 1. These seem only used by the subscriptioncmds.c file, so should they
> be declared in there also instead of in the .h?

Agreed.

> 2. I don't see what was gained by having the SubOptVals as a separate
> struct; OTOH the code accessing the vals is more verbose because of
> it. Maybe consider combining everything into SubOpts and then can just
> access "opts.copy_data" (etc) instead of "opts.vals.copy_data";

Agreed.

> + /* If connect option is supported, the others also need to be. */
> + Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
> +    ((supported_opts & SUBOPT_ENABLED) != 0 &&
> + (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
> + (supported_opts & SUBOPT_COPY_DATA) != 0));
> +
> + /* Set default values for the supported options. */
> + if ((supported_opts & SUBOPT_CONNECT) != 0)
> + vals->connect = true;
> +
> + if ((supported_opts & SUBOPT_ENABLED) != 0)
> + vals->enabled = true;
> +
> + if ((supported_opts & SUBOPT_CREATE_SLOT) != 0)
> + vals->create_slot = true;
> +
> + if ((supported_opts & SUBOPT_SLOT_NAME) != 0)
> + vals->slot_name = NULL;
> +
> + if ((supported_opts & SUBOPT_COPY_DATA) != 0)
> + vals->copy_data = true;
>
> 3. Are all those "!= 0" really necessary when checking the
> supported_opts against the bit masks? Maybe it is just a style thing,
> but since there are so many of them I felt it contributed to clutter
> and made the code less readable. This pattern was in many places, not
> just the example above.

Yeah these are necessary to know whether a particular option's bit is
set in the bitmask. How about having a macro like below:
#define IsSet(val, option)  ((val & option) != 0)
The if statements can become like below:
if (IsSet(supported_opts, SUBOPT_CONNECT))
if (IsSet(supported_opts, SUBOPT_ENABLED))
if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
if (IsSet(supported_opts, SUBOPT_COPY_DATA))

The above looks better to me. Thoughts?

Can we implement a similar idea for the parse_publication_options
although there are only two options right now. Option parsing code
will be consistent for logical replication DDLs and is extensible.
Thoughts?

With Regards,
Bharath Rupireddy.

On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> > + /* If connect option is supported, the others also need to be. */
> > + Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
> > +    ((supported_opts & SUBOPT_ENABLED) != 0 &&
> > + (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
> > + (supported_opts & SUBOPT_COPY_DATA) != 0));
> > +
> > + /* Set default values for the supported options. */
> > + if ((supported_opts & SUBOPT_CONNECT) != 0)
> > + vals->connect = true;
> > +
> > + if ((supported_opts & SUBOPT_ENABLED) != 0)
> > + vals->enabled = true;
> > +
> > + if ((supported_opts & SUBOPT_CREATE_SLOT) != 0)
> > + vals->create_slot = true;
> > +
> > + if ((supported_opts & SUBOPT_SLOT_NAME) != 0)
> > + vals->slot_name = NULL;
> > +
> > + if ((supported_opts & SUBOPT_COPY_DATA) != 0)
> > + vals->copy_data = true;
> >
> > 3. Are all those "!= 0" really necessary when checking the
> > supported_opts against the bit masks? Maybe it is just a style thing,
> > but since there are so many of them I felt it contributed to clutter
> > and made the code less readable. This pattern was in many places, not
> > just the example above.
>
> Yeah these are necessary to know whether a particular option's bit is
> set in the bitmask.

Hmmm. Maybe I did not ask the question properly. See below.

> How about having a macro like below:
> #define IsSet(val, option)  ((val & option) != 0)
> The if statements can become like below:
> if (IsSet(supported_opts, SUBOPT_CONNECT))
> if (IsSet(supported_opts, SUBOPT_ENABLED))
> if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
> if (IsSet(supported_opts, SUBOPT_COPY_DATA))
>
> The above looks better to me. Thoughts?

Yes, it looks better, but (since the masks are all 1 bit) I was only
asking why not do like:

if (supported_opts & SUBOPT_CONNECT)
if (supported_opts & SUBOPT_ENABLED)
if (supported_opts & SUBOPT_SLOT_NAME)
if (supported_opts & SUBOPT_COPY_DATA)

>
> Can we implement a similar idea for the parse_publication_options
> although there are only two options right now. Option parsing code
> will be consistent for logical replication DDLs and is extensible.
> Thoughts?

I have no strong opinion about it. It seems a trade off between having
a goal of "code consistency", versus "if it aint broke don't fix it".

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jun 2, 2021 at 11:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
> Yes, it looks better, but (since the masks are all 1 bit) I was only
> asking why not do like:
>
> if (supported_opts & SUBOPT_CONNECT)
> if (supported_opts & SUBOPT_ENABLED)
> if (supported_opts & SUBOPT_SLOT_NAME)
> if (supported_opts & SUBOPT_COPY_DATA)

Please review the attached v3 patch further.

With Regards,
Bharath Rupireddy.

On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > asking why not do like:
> >
> > if (supported_opts & SUBOPT_CONNECT)
> > if (supported_opts & SUBOPT_ENABLED)
> > if (supported_opts & SUBOPT_SLOT_NAME)
> > if (supported_opts & SUBOPT_COPY_DATA)
>
> Please review the attached v3 patch further.

OK. I have applied the v3 patch and reviewed it again:

- It applies OK.
- The code builds OK.
- The make check and TAP subscription tests are OK

========

1.
+/*
+ * Structure to hold the bitmaps and values of all the options for
+ * CREATE/ALTER SUBSCRIPTION  commands.
+ */

There seems to be an extra space before "commands."

------

2.
+ /* If connect option is supported, the others also need to be. */
+ Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
+    (IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA)));

This comment about "the others" doesn’t make sense to me.

e.g. Why only these 3 options? What about all those other SUBOPT_* options?

------

3.
I feel that this patch should be split into 2 parts
a) the SubOpts changes, and
b) the mutually exclusive options change.

I agree that the new SubOpts struct etc. is an improvement over existing code.

But, for the mutually exclusive options part I don't see what is
gained by the new patch code. I preferred the old code with its
multiple ereports. Although it was a bit repetitive IMO it was easier
to read that way, and length-wise there is almost no difference. So if
it is less readable and not a lot shorter then what is the benefit of
the change?

------

4.
- char    *slotname;
- bool slotname_given;
- char    *synchronous_commit;
- bool binary_given;
- bool binary;
- bool streaming_given;
- bool streaming;
-
- parse_subscription_options(stmt->options,
-    NULL, /* no "connect" */
-    NULL, NULL, /* no "enabled" */
-    NULL, /* no "create_slot" */
-    &slotname_given, &slotname,
-    NULL, /* no "copy_data" */
-    &synchronous_commit,
-    NULL, /* no "refresh" */
-    &binary_given, &binary,
-    &streaming_given, &streaming);
-
- if (slotname_given)
+ SubOpts opts = {0};

I feel it would be simpler to declare/init this "opts" variable just 1
time at top of the function AlterSubscription, instead of the 6
separate declarations in this v3 patch. Doing that can allow other
code simplifications too. (see #5)

------

5.
  case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
  {
  bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
- bool copy_data;
- bool refresh;
  List    *publist;
+ SubOpts opts = {0};
+
+ opts.supported_opts |= SUBOPT_REFRESH;
+
+ if (isadd)
+ opts.supported_opts |= SUBOPT_COPY_DATA;

I think having a separate "isadd" variable is made moot now since
adding the SubOpts struct.

Instead you can do this:
+ if (stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION)
+ opts.supported_opts |= SUBOPT_COPY_DATA;

OR (after #4) you could do this:

case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
        opts.supported_opts |= SUBOPT_COPY_DATA;
        /* fall thru. */
case ALTER_SUBSCRIPTION_DROP_PUBLICATION:

------

6.
+
+#define IsSet(val, option)  ((val & option) != 0)
+

Your IsSet macro might be better if changed to test *multiple* bits are all set.

Like this:
#define IsSet(val, bits)  ((val & (bits)) == (bits))

~

Most of the code remains the same, but some can be simplified.
e.g.
+ /* If connect option is supported, the others also need to be. */
+ Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
+    (IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA)));

Becomes:
Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
   IsSet(supported_opts, SUBOPT_ENABLED|SUBOPT_CREATE_SLOT|SUBOPT_COPY_DATA));

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jun 9, 2021 at 10:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > > asking why not do like:
> > >
> > > if (supported_opts & SUBOPT_CONNECT)
> > > if (supported_opts & SUBOPT_ENABLED)
> > > if (supported_opts & SUBOPT_SLOT_NAME)
> > > if (supported_opts & SUBOPT_COPY_DATA)
> >
> > Please review the attached v3 patch further.
>
> OK. I have applied the v3 patch and reviewed it again:
>
> - It applies OK.
> - The code builds OK.
> - The make check and TAP subscription tests are OK

Thanks.

> 1.
> +/*
> + * Structure to hold the bitmaps and values of all the options for
> + * CREATE/ALTER SUBSCRIPTION  commands.
> + */
>
> There seems to be an extra space before "commands."

Removed.

> 2.
> + /* If connect option is supported, the others also need to be. */
> + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> +    (IsSet(supported_opts, SUBOPT_ENABLED) &&
> + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> + IsSet(supported_opts, SUBOPT_COPY_DATA)));
>
> This comment about "the others" doesn’t make sense to me.
>
> e.g. Why only these 3 options? What about all those other SUBOPT_* options?

It is an existing Assert and comment for ensuring somebody doesn't
call parse_subscription_options with SUBOPT_CONNECT, without
SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
words, when SUBOPT_CONNECT is passed in, the other three options
should also be passed. " the others" there in the comment makes sense
just by looking at the Assert statement.

> 3.
> I feel that this patch should be split into 2 parts
> a) the SubOpts changes, and
> b) the mutually exclusive options change.

Divided the patch into two.

> I agree that the new SubOpts struct etc. is an improvement over existing code.
>
> But, for the mutually exclusive options part I don't see what is
> gained by the new patch code. I preferred the old code with its
> multiple ereports. Although it was a bit repetitive IMO it was easier
> to read that way, and length-wise there is almost no difference. So if
> it is less readable and not a lot shorter then what is the benefit of
> the change?

I personally don't like the repeated code when there's a chance of
doing it better. It might not reduce the loc, but it removes the many
similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
can take a call on it.

> 4.
> - char    *slotname;
> - bool slotname_given;
> - char    *synchronous_commit;
> - bool binary_given;
> - bool binary;
> - bool streaming_given;
> - bool streaming;
> -
> - parse_subscription_options(stmt->options,
> -    NULL, /* no "connect" */
> -    NULL, NULL, /* no "enabled" */
> -    NULL, /* no "create_slot" */
> -    &slotname_given, &slotname,
> -    NULL, /* no "copy_data" */
> -    &synchronous_commit,
> -    NULL, /* no "refresh" */
> -    &binary_given, &binary,
> -    &streaming_given, &streaming);
> -
> - if (slotname_given)
> + SubOpts opts = {0};
>
> I feel it would be simpler to declare/init this "opts" variable just 1
> time at top of the function AlterSubscription, instead of the 6
> separate declarations in this v3 patch. Doing that can allow other
> code simplifications too. (see #5)

Done.

> 5.
>   case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
>   {
>   bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
> - bool copy_data;
> - bool refresh;
>   List    *publist;
> + SubOpts opts = {0};
> +
> + opts.supported_opts |= SUBOPT_REFRESH;
> +
> + if (isadd)
> + opts.supported_opts |= SUBOPT_COPY_DATA;
>
> I think having a separate "isadd" variable is made moot now since
> adding the SubOpts struct.
>
> Instead you can do this:
> + if (stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION)
> + opts.supported_opts |= SUBOPT_COPY_DATA;
>
> OR (after #4) you could do this:
>
> case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
>         opts.supported_opts |= SUBOPT_COPY_DATA;
>         /* fall thru. */
> case ALTER_SUBSCRIPTION_DROP_PUBLICATION:

Done.

> 6.
> +
> +#define IsSet(val, option)  ((val & option) != 0)
> +
>
> Your IsSet macro might be better if changed to test *multiple* bits are all set.
>
> Like this:
> #define IsSet(val, bits)  ((val & (bits)) == (bits))
>
> ~
>
> Most of the code remains the same, but some can be simplified.
> e.g.
> + /* If connect option is supported, the others also need to be. */
> + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> +    (IsSet(supported_opts, SUBOPT_ENABLED) &&
> + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> + IsSet(supported_opts, SUBOPT_COPY_DATA)));
>
> Becomes:
> Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
>    IsSet(supported_opts, SUBOPT_ENABLED|SUBOPT_CREATE_SLOT|SUBOPT_COPY_DATA));

Changed.

PSA v4 patch set.

With Regards,
Bharath Rupireddy.

On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jun 9, 2021 at 10:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
[...]

I checked the v4* patches.
Everything applies and builds and tests OK for me.

> > 2.
> > + /* If connect option is supported, the others also need to be. */
> > + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> > +    (IsSet(supported_opts, SUBOPT_ENABLED) &&
> > + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> > + IsSet(supported_opts, SUBOPT_COPY_DATA)));
> >
> > This comment about "the others" doesn’t make sense to me.
> >
> > e.g. Why only these 3 options? What about all those other SUBOPT_* options?
>
> It is an existing Assert and comment for ensuring somebody doesn't
> call parse_subscription_options with SUBOPT_CONNECT, without
> SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
> words, when SUBOPT_CONNECT is passed in, the other three options
> should also be passed. " the others" there in the comment makes sense
> just by looking at the Assert statement.
>

This misses the point of my question. And deducing the meaning of the
"the others" from the code is completely backwards! The comment
describes the code. The code doesn't describe the comment.

Again, I was asking why “the others” are only these 3 options?. What
about binary? What about streaming? What about refresh?
In other words - what was the *intent* of that comment, and does the
new code still meet the requirements of that intent? I think it does
not.

If you see github [1] when that code was first  implemented you can
see that “the others” referred to every option other than the
“connect”. At that time, the only others were those 3 - enabled,
create_slot, copy_data. But now there are lots more options so
something definitely needs to change.
E.g.
- Maybe the Assert now needs to include all the new options as well?
- Maybe the entire reason for the Assert has become redundant now due
to the introduction of SubOpts. It looks like it was not functional
code - just something to quieten a static analysis tool.
- Certainly “the others” is too vague and no longer has the original
meaning anymore

I don't know the answer; my guess is that all this has become obsolete
and the whole Assert and the dodgy comment can just be deleted.

> > 3.
> > I feel that this patch should be split into 2 parts
> > a) the SubOpts changes, and
> > b) the mutually exclusive options change.
>
> Divided the patch into two.
>
> > I agree that the new SubOpts struct etc. is an improvement over existing code.
> >
> > But, for the mutually exclusive options part I don't see what is
> > gained by the new patch code. I preferred the old code with its
> > multiple ereports. Although it was a bit repetitive IMO it was easier
> > to read that way, and length-wise there is almost no difference. So if
> > it is less readable and not a lot shorter then what is the benefit of
> > the change?
>
> I personally don't like the repeated code when there's a chance of
> doing it better. It might not reduce the loc, but it removes the many
> similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
> can take a call on it.
>

Thanks for splitting them. My votes are +1 for patch 0001 and  -1 for
patch 0002. As you say, someone else can decide.

------
[1] https://github.com/postgres/postgres/commit/b1ff33fd9bb82937f4719f264972e6a3c83cdb89#

Kind Regards,
Peter Smith
Fujitsu Australia.

On Thu, Jun 10, 2021 at 8:55 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > 2.
> > > + /* If connect option is supported, the others also need to be. */
> > > + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> > > +    (IsSet(supported_opts, SUBOPT_ENABLED) &&
> > > + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> > > + IsSet(supported_opts, SUBOPT_COPY_DATA)));
> > >
> > > This comment about "the others" doesn’t make sense to me.
> > >
> > > e.g. Why only these 3 options? What about all those other SUBOPT_* options?
> >
> > It is an existing Assert and comment for ensuring somebody doesn't
> > call parse_subscription_options with SUBOPT_CONNECT, without
> > SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
> > words, when SUBOPT_CONNECT is passed in, the other three options
> > should also be passed. " the others" there in the comment makes sense
> > just by looking at the Assert statement.
> >
>
> This misses the point of my question. And deducing the meaning of the
> "the others" from the code is completely backwards! The comment
> describes the code. The code doesn't describe the comment.
>
> Again, I was asking why “the others” are only these 3 options?. What
> about binary? What about streaming? What about refresh?
> In other words - what was the *intent* of that comment, and does the
> new code still meet the requirements of that intent? I think it does
> not.
>
> If you see github [1] when that code was first  implemented you can
> see that “the others” referred to every option other than the
> “connect”. At that time, the only others were those 3 - enabled,
> create_slot, copy_data. But now there are lots more options so
> something definitely needs to change.
> E.g.
> - Maybe the Assert now needs to include all the new options as well?
> - Maybe the entire reason for the Assert has become redundant now due
> to the introduction of SubOpts. It looks like it was not functional
> code - just something to quieten a static analysis tool.
> - Certainly “the others” is too vague and no longer has the original
> meaning anymore
>
> I don't know the answer; my guess is that all this has become obsolete
> and the whole Assert and the dodgy comment can just be deleted.

Hm. I get it. Unfortunately the commit b1ff33f is missing information
on what the coverity tool was complaining of and it has no related
discussion at all.

I agree to remove that assertion entirely. I will post a new patch set soon.

> > > 3.
> > > I feel that this patch should be split into 2 parts
> > > a) the SubOpts changes, and
> > > b) the mutually exclusive options change.
> >
> > Divided the patch into two.
> >
> > > I agree that the new SubOpts struct etc. is an improvement over existing code.
> > >
> > > But, for the mutually exclusive options part I don't see what is
> > > gained by the new patch code. I preferred the old code with its
> > > multiple ereports. Although it was a bit repetitive IMO it was easier
> > > to read that way, and length-wise there is almost no difference. So if
> > > it is less readable and not a lot shorter then what is the benefit of
> > > the change?
> >
> > I personally don't like the repeated code when there's a chance of
> > doing it better. It might not reduce the loc, but it removes the many
> > similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
> > can take a call on it.
> >
>
> Thanks for splitting them. My votes are +1 for patch 0001 and  -1 for
> patch 0002. As you say, someone else can decide.

Let's see how it goes further.

With Regards,
Bharath Rupireddy.

On Thu, Jun 10, 2021 at 09:17:55AM +0530, Bharath Rupireddy wrote:
> Hm. I get it. Unfortunately the commit b1ff33f is missing information
> on what the coverity tool was complaining of and it has no related
> discussion at all.

This came from a FORWARD_NULL complain, due to the fact that
parse_subscription_options() has checks for all three options if
connect is non-NULL a bit down after being done with the value
assignments with the DefElems.  So coverity was warning that we'd
better be careful to always have all three pointers set if a
connection is wanted by the caller.
--
Michael

On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> > I don't know the answer; my guess is that all this has become obsolete
> > and the whole Assert and the dodgy comment can just be deleted.
>
> Hm. I get it. Unfortunately the commit b1ff33f is missing information
> on what the coverity tool was complaining of and it has no related
> discussion at all.
>
> I agree to remove that assertion entirely. I will post a new patch set soon.

PSA v5 patch set.

With Regards,
Bharath Rupireddy.

On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> This should be okay, right?  Well, almost. The problem here is if you
> want to have a variable where you set more than one option, you have to
> use bit-and of the enum values ... and the resulting value is no longer
> part of the enum.  A compiler would be understandably upset if you try
> to pass that value in a variable of the enum datatype.

Yes. I dislike this style for precisely this reason.

I may, however, be in the minority.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

At Fri, 11 Jun 2021 16:29:10 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > This should be okay, right?  Well, almost. The problem here is if you
> > want to have a variable where you set more than one option, you have to
> > use bit-and of the enum values ... and the resulting value is no longer
> > part of the enum.  A compiler would be understandably upset if you try
> > to pass that value in a variable of the enum datatype.
> 
> Yes. I dislike this style for precisely this reason.
> 
> I may, however, be in the minority.

I personaly don't hate that so much, but generally an "enumeration"
type is considered to be non-numbers. That is, no arithmetics are
defined between two enum values. I think that C being able to perform
arithmetics on enums is just for implement reasons. I think that
arithmetics (logical operations are not arithmetics?) between boolean
values are for the same reasons.  Actually Java refuses arithmetics on
enum values.

> hoge.java:27: error: bad operand types for binary operator '+'
>         int x = theenum.x + theenum.z;
>                           ^
>   first type:  theenum
>   second type: theenum

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jun 10, 2021 at 6:36 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > I don't know the answer; my guess is that all this has become obsolete
> > > and the whole Assert and the dodgy comment can just be deleted.
> >
> > Hm. I get it. Unfortunately the commit b1ff33f is missing information
> > on what the coverity tool was complaining of and it has no related
> > discussion at all.
> >
> > I agree to remove that assertion entirely. I will post a new patch set soon.
>
> PSA v5 patch set.

PSA v6 patch set rebased onto the latest master.

With Regards,
Bharath Rupireddy.

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.

With Regards,
Bharath Rupireddy.

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.

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.

On 2021-Jun-29, Bharath Rupireddy wrote:

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

Yeah I was looking at the struct too and this bit didn't seem great. I
think it'd be better to have the struct be output only; so
"specified_opts" would be part of the struct (not necessarily with that
name), but "supported opts" (which is input data) would be passed as a
separate argument.  That seems cleaner to *me*, at least.

-- 
Álvaro Herrera       Valdivia, Chile
"Right now the sectors on the hard disk run clockwise, but I heard a rumor that
you can squeeze 0.2% more throughput by running them counterclockwise.
It's worth the effort. Recommended."  (Gerry Pourwelle)

On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jun-29, Bharath Rupireddy wrote:
>
> > 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.
>
> Yeah I was looking at the struct too and this bit didn't seem great. I
> think it'd be better to have the struct be output only; so
> "specified_opts" would be part of the struct (not necessarily with that
> name), but "supported opts" (which is input data) would be passed as a
> separate argument.  That seems cleaner to *me*, at least.
>

Yeah, that sounds better than what we have in the patch. Also, I am
not sure if it is a good idea to use "supported_opts" for input data
as that sounds more like what is output from the function, how about
required_opts or input_opts? Also, we can name the output structure as
SpecifiedSubOpts and "specified_opts" as either "opts" or "out_opts".
I think naming these things is a bit matter of personal preference so
I am fine if both you and Bharath find current naming more meaningful.

+#define IsSet(val, bits)  ((val & (bits)) == (bits))
Also, do you have any opinion on this define? I see at other places we
use in-place checks but as in this patch there are multiple instances
of such check so probably such a define should be acceptable.

-- 
With Regards,
Amit Kapila.

On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Few comments:
> > ===============
>
> > 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.
>

I think irrespective of whether coverity reports or not, the assert
appears useful to me because we are still doing the check for the
other three options only if connect is supported.

-- 
With Regards,
Amit Kapila.

On Wed, Jun 30, 2021 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Jun-29, Bharath Rupireddy wrote:
> >
> > > 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.
> >
> > Yeah I was looking at the struct too and this bit didn't seem great. I
> > think it'd be better to have the struct be output only; so
> > "specified_opts" would be part of the struct (not necessarily with that
> > name), but "supported opts" (which is input data) would be passed as a
> > separate argument.  That seems cleaner to *me*, at least.
> >
>
> Yeah, that sounds better than what we have in the patch. Also, I am
> not sure if it is a good idea to use "supported_opts" for input data
> as that sounds more like what is output from the function, how about
> required_opts or input_opts? Also, we can name the output structure as
> SpecifiedSubOpts and "specified_opts" as either "opts" or "out_opts".

IMO, SubOpts looks okay. Also, I retained the specified_opts but moved
supported_opts out of the structure.

> I think naming these things is a bit matter of personal preference so
> I am fine if both you and Bharath find current naming more meaningful.

Please let me know if any of the names look odd.

> +#define IsSet(val, bits)  ((val & (bits)) == (bits))
> Also, do you have any opinion on this define? I see at other places we
> use in-place checks but as in this patch there are multiple instances
> of such check so probably such a define should be acceptable.

Yeah. I'm retaining this macro as it makes code readable.

PFA v9 patch set for further review.

With Regards,
Bharath Rupireddy.

On Wed, Jun 30, 2021 at 11:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Few comments:
> > > ===============
> >
> > > 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.
> >
>
> I think irrespective of whether coverity reports or not, the assert
> appears useful to me because we are still doing the check for the
> other three options only if connect is supported.

Added the assert back. PSA v9 patch set posted upthread.

With Regards,
Bharath Rupireddy.

On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> PFA v9 patch set for further review.
>

The first patch looks mostly good to me. I have made some minor
modifications to the 0001 patch: (a) added/edited few comments, (b)
there is no need to initialize supported_opts variable in
CreateSubscription, (c) used extra bracket in macro, (d) ran pgindent.

Kindly check and let me know what you think of the attached? I am not
sure whether second patch is an improvement over what we have
currently but if you and others feel that is a good idea then you can
submit the same after the main patch gets committed.

-- 
With Regards,
Amit Kapila.

On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > PFA v9 patch set for further review.
> >
>
> The first patch looks mostly good to me. I have made some minor
> modifications to the 0001 patch: (a) added/edited few comments, (b)
> there is no need to initialize supported_opts variable in
> CreateSubscription, (c) used extra bracket in macro, (d) ran pgindent.

Thanks a lot Amit.

> Kindly check and let me know what you think of the attachment?
1) Isn't good to mention in the commit message a note about the
limitation of the maximum number of SUBOPT_*? Currently it is 32
because of bits32 data type. If required, then we might have to
introduce bits64 (typedef to uint64).
2) How about just saying "Refactor function
parse_subscription_options." instead of "Refactor function
parse_subscription_options()." in the commit message? This is similar
to the commit 531737d "Refactor function parse_output_parameters."
3) There's an whitespace introduced making the SUBOPT_SLOT_NAME,
SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with
the SUBOPT_CONNECT
+ /* Options that can be specified by CREATE SUBSCRIPTION command. */
+ supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
+    SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
+    SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+    SUBOPT_STREAMING);
Shouldn't it be something like below?
+ supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
+   SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
+   SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+   SUBOPT_STREAMING);

The other changes look good to me.

> I am not sure whether the second patch is an improvement over what we have
> currently but if you and others feel that is a good idea then you can
> submit the same after the main patch gets committed.

 Peter Smith was also not happy with that patch. Anyways, I will post
that patch in this thread after 0001 gets in and see if it interests
other hackers.

Regards,
Bharath Rupireddy.

On Thu, Jul 1, 2021 at 5:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > PFA v9 patch set for further review.
> > >
> >
> > The first patch looks mostly good to me. I have made some minor
> > modifications to the 0001 patch: (a) added/edited few comments, (b)
> > there is no need to initialize supported_opts variable in
> > CreateSubscription, (c) used extra bracket in macro, (d) ran pgindent.
>
> Thanks a lot Amit.
>
> > Kindly check and let me know what you think of the attachment?
> 1) Isn't good to mention in the commit message a note about the
> limitation of the maximum number of SUBOPT_*? Currently it is 32
> because of bits32 data type. If required, then we might have to
> introduce bits64 (typedef to uint64).
>

I am not sure if it is required to mention it as this is not an
exposed struct and I think we can't reach that number in near future.

> 2) How about just saying "Refactor function
> parse_subscription_options." instead of "Refactor function
> parse_subscription_options()." in the commit message? This is similar
> to the commit 531737d "Refactor function parse_output_parameters."
>

It hardly matters. We can write either way. I normally use () after
function name.

> 3) There's an whitespace introduced making the SUBOPT_SLOT_NAME,
> SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with
> the SUBOPT_CONNECT
>

okay, will fix it.

> + /* Options that can be specified by CREATE SUBSCRIPTION command. */
> + supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
> +    SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
> +    SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
> +    SUBOPT_STREAMING);
> Shouldn't it be something like below?
> + supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
> +   SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
> +   SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
> +   SUBOPT_STREAMING);
>

Both appear the same to me. Can you please highlight the difference in some way?


-- 
With Regards,
Amit Kapila.

I find the business with OPT_NONE a bit uselessly verbose.  It's like we
haven't completely made up our minds that zero means no options set.
Wouldn't it be simpler to remove that #define and leave the variable
uninitialized until we want to set the options we want, and then use
plain assignment instead of |= ?

I propose the attached cleanup.  Some comments seem a bit too obvious;
the use of a local variable for specified_opts instead of directly
assigning to the one in the struct seemed unnecessary; places that call
parse_subscription_options() with only one bit set don't need a separate
variable for the allowed options; added some whitespace.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On Thu, Jul 1, 2021 at 6:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > 3) There's an whitespace introduced making the SUBOPT_SLOT_NAME,
> > SUBOPT_SYNCHRONOUS_COMMIT and SUBOPT_STREAMING not falling line with
> > the SUBOPT_CONNECT
> >
>
> okay, will fix it.

PSA v11 patch which also has the cleanup patch shared by Alvaro Herrera.

Regards,
Bharath Rupireddy.

On Thu, Jul 1, 2021 at 8:00 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> I find the business with OPT_NONE a bit uselessly verbose.  It's like we
> haven't completely made up our minds that zero means no options set.
> Wouldn't it be simpler to remove that #define and leave the variable
> uninitialized until we want to set the options we want, and then use
> plain assignment instead of |= ?
>

Yeah, that makes sense. I have removed its usage from
CreateSubscription but I think we can get rid of it entirely as well.

The latest patch sent by Bharath looks good to me. Would you like to
commit it or shall I take care of it?

-- 
With Regards,
Amit Kapila.

On 2021-Jul-02, Amit Kapila wrote:

> Yeah, that makes sense. I have removed its usage from
> CreateSubscription but I think we can get rid of it entirely as well.

Nod.

> The latest patch sent by Bharath looks good to me. Would you like to
> commit it or shall I take care of it?

Please, go ahead.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > The latest patch sent by Bharath looks good to me. Would you like to
> > commit it or shall I take care of it?
>
> Please, go ahead.
>

Okay, I'll push it early next week (by Tuesday) unless there are more
comments or suggestions. Thanks!


-- 
With Regards,
Amit Kapila.

On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > > The latest patch sent by Bharath looks good to me. Would you like to
> > > commit it or shall I take care of it?
> >
> > Please, go ahead.
> >
>
> Okay, I'll push it early next week (by Tuesday) unless there are more
> comments or suggestions. Thanks!
>

Pushed!

-- 
With Regards,
Amit Kapila.

On Tue, Jul 6, 2021 at 1:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Okay, I'll push it early next week (by Tuesday) unless there are more
> > comments or suggestions. Thanks!
> >
>
> Pushed!

Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
calls using local variables. Please review it.

Regards,
Bharath Rupireddy.

On 2021-Jul-06, Bharath Rupireddy wrote:

> Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
> calls using local variables. Please review it.

I looked at this the other day and I'm not sure I like it very much.
It's not making anything any simpler, it's barely saving two lines of
code.  I think we can do without this change.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > > The latest patch sent by Bharath looks good to me. Would you like to
> > > > commit it or shall I take care of it?
> > >
> > > Please, go ahead.
> > >
> >
> > Okay, I'll push it early next week (by Tuesday) unless there are more
> > comments or suggestions. Thanks!
> >
>
> Pushed!

Yesterday, I needed to refactor a lot of code due to this push [1].

The refactoring exercise caused me to study these v11 changes much more deeply.

IMO there are a few improvements that should be made:

//////

1. Zap 'opts' up-front

+ *
+ * Caller is expected to have cleared 'opts'.

This comment is putting the onus on the caller to "do the right thing".

I think that hopeful expectations about input should be removed - the
function should just be responsible itself just to zap the SubOpts
up-front. It makes the code more readable, and it removes any
potential risk of garbage unintentionally passed in that struct.

    /* Start out with cleared opts. */
    memset(opts, 0, sizeof(SubOpts));


Alternatively, at least there should be an assertion for some sanity check.

Assert(opt->specified_opts == 0);

----

2. Remove redundant conditions

  /* Check for incompatible options from the user. */
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "create_slot = true")));

- if (copy_data && copy_data_given && *copy_data)
+ if (opts->copy_data &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA) &&
+ IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "copy_data = true")));

By definition, this function only allows any option to be
"specified_opts" if that option is also "supported_opts". So, there is
really no need in the above code to re-check again that it is
supported.

It can be simplified like this:

  /* Check for incompatible options from the user. */
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "create_slot = true")));

- if (copy_data && copy_data_given && *copy_data)
+ if (opts->copy_data &&
+ IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("%s and %s are mutually exclusive options",
  "connect = false", "copy_data = true")));

-----

3. Remove redundant conditions

Same as 2. Here are more examples of conditions where the redundant
checking of "supported_opts" can be removed.

  /*
  * Do additional checking for disallowed combination when slot_name = NONE
  * was used.
  */
- if (slot_name && *slot_name_given && !*slot_name)
+ if (!opts->slot_name &&
+ IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
+ IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
  {
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "slot_name = NONE", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "slot_name = NONE", "create_slot = true")));

It can be simplified like this:

  /*
  * Do additional checking for disallowed combination when slot_name = NONE
  * was used.
  */
- if (slot_name && *slot_name_given && !*slot_name)
+ if (!opts->slot_name &&
+ IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
  {
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "slot_name = NONE", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
  errmsg("%s and %s are mutually exclusive options",
  "slot_name = NONE", "create_slot = true")));

------

4. Remove redundant conditions

- if (enabled && !*enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ !IsSet(opts->specified_opts, SUBOPT_ENABLED))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("subscription with %s must also set %s",
  "slot_name = NONE", "enabled = false")));

- if (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ !IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
  errmsg("subscription with %s must also set %s",
  "slot_name = NONE", "create_slot = false")));


This code can be simplified even more than the others mentioned,
because here the "specified_opts" checks were already done in the code
that precedes this.

It can be simplified like this:

- if (enabled && !*enabled_given && *enabled)
+ if (opts->enabled)
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
  /*- translator: both %s are strings of the form "option = value" */
  errmsg("subscription with %s must also set %s",
  "slot_name = NONE", "enabled = false")));

- if (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot)
  ereport(ERROR,
  (errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
  errmsg("subscription with %s must also set %s",
  "slot_name = NONE", "create_slot = false")));

//////

PSA my patch which includes all the fixes mentioned above.

(Make check, and TAP subscription tests are tested and pass OK)

------
[1] https://github.com/postgres/postgres/commit/8aafb02616753f5c6c90bbc567636b73c0cbb9d4

Kind Regards,
Peter Smith.
Fujitsu Australia

On 2021-Jul-07, Peter Smith wrote:

> 1. Zap 'opts' up-front
> 
> + *
> + * Caller is expected to have cleared 'opts'.
> 
> This comment is putting the onus on the caller to "do the right thing".
> 
> I think that hopeful expectations about input should be removed - the
> function should just be responsible itself just to zap the SubOpts
> up-front. It makes the code more readable, and it removes any
> potential risk of garbage unintentionally passed in that struct.
> 
>     /* Start out with cleared opts. */
>     memset(opts, 0, sizeof(SubOpts));

Yeah, I gave the initialization aspect some thought too when I reviewed
0001.  Having an explicit memset() just for sanity check is a waste when
you don't really need it; and we're initializing the struct already at
declaration time by assigning {0} to it, so having to add memset feels
like such a waste.  Another point in the same area is that some of the
struct members are initialized to some default value different from 0,
so I wondered if it would have been better to remove the = {0} and
instead have another function that would set everything up the way we
want; but it seemed a bit excessive, so I ended up not suggesting that.

We have many places in the source tree where the caller is expected to
do the right thing, even when those right things are more complex than
this one.  This one place isn't terribly bad in that regard, given that
it's a pretty well contained static struct anyway (we would certainly
not export a struct of this name in any .h file.)  I don't think it's
all that bad.

> Alternatively, at least there should be an assertion for some sanity check.
> 
> Assert(opt->specified_opts == 0);

No opinion on this.  It doesn't seem valuable enough, but maybe I'm on
the minority on this.

> 2. Remove redundant conditions
> 
>   /* Check for incompatible options from the user. */
> - if (enabled && *enabled_given && *enabled)
> + if (opts->enabled &&
> + IsSet(supported_opts, SUBOPT_ENABLED) &&
> + IsSet(opts->specified_opts, SUBOPT_ENABLED))

(etc)

Yeah, I thought about this too when I saw the 0002 patch in this series.
I agree that the extra rechecks are a bit pointless.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Tue, Jul 6, 2021 at 9:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jul-06, Bharath Rupireddy wrote:
>
> > Thanks, Amit. I'm posting the 0002 patch which removes extra ereport
> > calls using local variables. Please review it.
>
> I looked at this the other day and I'm not sure I like it very much.
> It's not making anything any simpler, it's barely saving two lines of
> code.  I think we can do without this change.

Just for the records. I will withdraw the 0002 patch as no one has
shown interest. Thanks.

Regards,
Bharath Rupireddy.

On Wed, Jul 7, 2021 at 7:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jul-07, Peter Smith wrote:
>
> > 1. Zap 'opts' up-front
> >
> > + *
> > + * Caller is expected to have cleared 'opts'.
> >
> > This comment is putting the onus on the caller to "do the right thing".
> >
> > I think that hopeful expectations about input should be removed - the
> > function should just be responsible itself just to zap the SubOpts
> > up-front. It makes the code more readable, and it removes any
> > potential risk of garbage unintentionally passed in that struct.
> >
> >     /* Start out with cleared opts. */
> >     memset(opts, 0, sizeof(SubOpts));
>
> Yeah, I gave the initialization aspect some thought too when I reviewed
> 0001.  Having an explicit memset() just for sanity check is a waste when
> you don't really need it; and we're initializing the struct already at
> declaration time by assigning {0} to it, so having to add memset feels
> like such a waste.  Another point in the same area is that some of the
> struct members are initialized to some default value different from 0,
> so I wondered if it would have been better to remove the = {0} and
> instead have another function that would set everything up the way we
> want; but it seemed a bit excessive, so I ended up not suggesting that.
>
> We have many places in the source tree where the caller is expected to
> do the right thing, even when those right things are more complex than
> this one.  This one place isn't terribly bad in that regard, given that
> it's a pretty well contained static struct anyway (we would certainly
> not export a struct of this name in any .h file.)  I don't think it's
> all that bad.
>
> > Alternatively, at least there should be an assertion for some sanity check.
> >
> > Assert(opt->specified_opts == 0);
>
> No opinion on this.  It doesn't seem valuable enough, but maybe I'm on
> the minority on this.
>

I am also not sure if such an assertion adds much value.

> > 2. Remove redundant conditions
> >
> >   /* Check for incompatible options from the user. */
> > - if (enabled && *enabled_given && *enabled)
> > + if (opts->enabled &&
> > + IsSet(supported_opts, SUBOPT_ENABLED) &&
> > + IsSet(opts->specified_opts, SUBOPT_ENABLED))
>
> (etc)
>
> Yeah, I thought about this too when I saw the 0002 patch in this series.
> I agree that the extra rechecks are a bit pointless.
>

I don't think the committed patch has made anything worse here or
added any new condition. Now, even if we want to change these
conditions, it is better to discuss them separately. If we see as per
current code these look a bit redundant but OTOH, in the future one
might expect that if the supported option is not passed by the caller
and the user has specified it then it should be an error. For example,
we don't want to support some option via some Alter variant but it is
supported by Create variant.

-- 
With Regards,
Amit Kapila.

On Wed, Jul 7, 2021 at 5:33 AM Peter Smith <smithpb2250@gmail.com> wrote:
> PSA my patch which includes all the fixes mentioned above.

I agree with Amit to start a separate thread to discuss these points.
IMO, we can close this thread. What do you think?

Regards,
Bharath Rupireddy.

On Wed, Jul 7, 2021 at 1:35 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jul 7, 2021 at 5:33 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > PSA my patch which includes all the fixes mentioned above.
>
> I agree with Amit to start a separate thread to discuss these points.
> IMO, we can close this thread. What do you think?
>

OK. I created a new thread called "parse_subscription_options -
suggested improvements" here [1]

------
[1] https://www.postgresql.org/message-id/CAHut%2BPtXHfLgLHDDJ8ZN5f5Be_37mJoxpEsRg8LNmm4XCr06Rw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia.

On Thu, Jul 8, 2021 at 6:24 AM Peter Smith <smithpb2250@gmail.com> wrote:
> OK. I created a new thread called "parse_subscription_options -
> suggested improvements" here [1]

Thanks. I closed the CF entry for this thread.

Regards,
Bharath Rupireddy.