Refactor "mutually exclusive options" error reporting code in parse_subscription_options

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]/messages/by-id/CAA4eK1Jz64rwLyB6H7Z_SmEDouJ41KN42=VkVFp6JTpafJFG8Q@mail.gmail.com. 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]: /messages/by-id/CAA4eK1Jz64rwLyB6H7Z_SmEDouJ41KN42=VkVFp6JTpafJFG8Q@mail.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 ? &copy_data : NULL;

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

if (isadd)
opts.copy_data = &copy_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 ? &copy_data : NULL;

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

if (isadd)
opts.copy_data = &copy_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]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;. 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 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]/messages/by-id/14dde730-1d34-260e-fa9d-7664df2d6313@enterprisedb.com 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]: /messages/by-id/14dde730-1d34-260e-fa9d-7664df2d6313@enterprisedb.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.