Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jul 8, 2021 at 6:24 AM Peter Smith 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jul 7, 2021 at 1:35 PM Bharath Rupireddy wrote: > > On Wed, Jul 7, 2021 at 5:33 AM Peter Smith 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jul 7, 2021 at 5:33 AM Peter Smith 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jul 7, 2021 at 7:36 AM Alvaro Herrera 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jul 6, 2021 at 9:24 PM Alvaro Herrera 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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/
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera
> > 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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/
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jul 6, 2021 at 1:51 PM Amit Kapila 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. v10-0001-Remove-similar-ereport-calls-in-parse_subscripti.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila wrote: > > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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/
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jul 1, 2021 at 8:00 PM Alvaro Herrera 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jul 1, 2021 at 6:32 PM Amit Kapila 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. v11-0001-Refactor-function-parse_subscription_options.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index edfef9f14f..eb88d877a5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -50,16 +50,15 @@
* Options that can be specified by the user in CREATE/ALTER SUBSCRIPTION
* command.
*/
-#define SUBOPT_NONE 0x
-#define SUBOPT_CONNECT 0x0001
-#define SUBOPT_ENABLED 0x0002
-#define SUBOPT_CREATE_SLOT 0x0004
-#define SUBOPT_SLOT_NAME 0x0008
-#define SUBOPT_COPY_DATA 0x0010
-#define SUBOPT_SYNCHRONOUS_COMMIT 0x0020
-#define SUBOPT_REFRESH 0x0040
-#define SUBOPT_BINARY 0x0080
-#define SUBOPT_STREAMING 0x0100
+#define SUBOPT_CONNECT0x0001
+#define SUBOPT_ENABLED0x0002
+#define SUBOPT_CREATE_SLOT 0x0004
+#define SUBOPT_SLOT_NAME 0x0008
+#define SUBOPT_COPY_DATA 0x0010
+#define SUBOPT_SYNCHRONOUS_COMMIT 0x0020
+#define SUBOPT_REFRESH0x0040
+#define SUBOPT_BINARY0x0080
+#define SUBOPT_STREAMING 0x0100
/* check if the 'val' has 'bits' set */
#define IsSet(val, bits) (((val) & (bits)) == (bits))
@@ -93,48 +92,35 @@ static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname,
*
* Since not all options can be specified in both commands, this function
* will report an error if mutually exclusive options are specified.
+ *
+ * Caller is expected to have cleared 'opts'.
*/
static void
parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *opts)
{
ListCell *lc;
- bits32 specified_opts;
-
- specified_opts = SUBOPT_NONE;
/* caller must expect some option */
- Assert(supported_opts != SUBOPT_NONE);
+ Assert(supported_opts != 0);
- /* If connect option is supported, the others also need to be. */
+ /* If connect option is supported, these others also need to be. */
Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
IsSet(supported_opts, SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
SUBOPT_COPY_DATA));
- /* Set default values for the supported options. */
+ /* Set default values for the boolean supported options. */
if (IsSet(supported_opts, SUBOPT_CONNECT))
opts->connect = true;
-
if (IsSet(supported_opts, SUBOPT_ENABLED))
opts->enabled = true;
-
if (IsSet(supported_opts, SUBOPT_CREATE_SLOT))
opts->create_slot = true;
-
- if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
- opts->slot_name = NULL;
-
if (IsSet(supported_opts, SUBOPT_COPY_DATA))
opts->copy_data = true;
-
- if (IsSet(supported_opts, SUBOPT_SYNCHRONOUS_COMMIT))
- opts->synchronous_commit = NULL;
-
if (IsSet(supported_opts, SUBOPT_REFRESH))
opts->refresh = true;
-
if (IsSet(supported_opts, SUBOPT_BINARY))
opts->binary = false;
-
if (IsSet(supported_opts, SUBOPT_STREAMING))
opts->streaming = false;
@@ -146,45 +132,45 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
if (IsSet(supported_opts, SUBOPT_CONNECT) &&
strcmp(defel->defname, "connect") == 0)
{
- if (IsSet(specified_opts, SUBOPT_CONNECT))
+ if (IsSet(opts->specified_opts, SUBOPT_CONNECT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_CONNECT;
+ opts->specified_opts |= SUBOPT_CONNECT;
opts->connect = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_ENABLED) &&
strcmp(defel->defname, "enabled") == 0)
{
- if (IsSet(specified_opts, SUBOPT_ENABLED))
+ if (IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_ENABLED;
+ opts->specified_opts |= SUBOPT_ENABLED;
opts->enabled = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
strcmp(defel->defname, "create_slot") == 0)
{
- if (IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+ if (IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_CREATE_SLOT;
+ opts->specified_opts |=
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jul 1, 2021 at 5:37 PM Bharath Rupireddy wrote: > > On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila wrote: > > > > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy > > 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jul 1, 2021 at 4:38 PM Amit Kapila wrote: > > On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy > 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 30, 2021 at 7:38 PM Bharath Rupireddy 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. v10-0001-Refactor-function-parse_subscription_options.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 30, 2021 at 11:11 AM Amit Kapila wrote:
>
> On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
> wrote:
> >
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 30, 2021 at 10:52 AM Amit Kapila wrote:
>
> On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera
> wrote:
> >
> > On 2021-Jun-29, Bharath Rupireddy wrote:
> >
> > > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila
> > > 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.
v9-0001-Refactor-parse_subscription_options.patch
Description: Binary data
v9-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch
Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
wrote:
>
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera wrote:
>
> On 2021-Jun-29, Bharath Rupireddy wrote:
>
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On 2021-Jun-29, Bharath Rupireddy wrote:
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila 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)
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila 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.
From 6dfaef3d3425278304190e82400bc3b379ffccc5 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Tue, 29 Jun 2021 15:20:19 +
Subject: [PATCH v8] Refactor parse_subscription_options
Currently parse_subscription_options function receives a lot of
input parameters which makes it inextensible to add the new
parameters. So, better wrap all the input parameters within a
structure to which new parameters can be added easily. Also, use
bitmaps to pass the supported and specified options much like the
way it is done in the commit a3dc926.
---
src/backend/commands/subscriptioncmds.c | 446
src/tools/pgindent/typedefs.list| 1 +
2 files changed, 216 insertions(+), 231 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index b862e59f1d..bf73fd6702 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -46,6 +46,38 @@
#include "utils/memutils.h"
#include "utils/syscache.h"
+#define SUBOPT_NONE 0x
+#define SUBOPT_CONNECT 0x0001
+#define SUBOPT_ENABLED 0x0002
+#define SUBOPT_CREATE_SLOT 0x0004
+#define SUBOPT_SLOT_NAME 0x0008
+#define SUBOPT_COPY_DATA 0x0010
+#define SUBOPT_SYNCHRONOUS_COMMIT 0x0020
+#define SUBOPT_REFRESH 0x0040
+#define SUBOPT_BINARY 0x0080
+#define SUBOPT_STREAMING 0x0100
+
+#define IsSet(val, bits) ((val & (bits)) == (bits))
+
+/*
+ * Structure to hold the bitmaps and values of all the options for
+ * CREATE/ALTER SUBSCRIPTION commands.
+ */
+typedef struct SubOpts
+{
+ bits32 supported_opts; /* bitmap of supported SUBOPT_* */
+ bits32 specified_opts; /* bitmap of user specified SUBOPT_* */
+ char *slot_name;
+ char *synchronous_commit;
+ bool connect;
+ bool enabled;
+ bool create_slot;
+ bool copy_data;
+ bool refresh;
+ bool binary;
+ bool streaming;
+} SubOpts;
+
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
static void check_duplicates_in_publist(List *publist, Datum *datums);
static List *merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname);
@@ -56,164 +88,160 @@ static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname,
* Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
*
* 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.
+ * will report an error if mutually exclusive options are specified.
*/
static void
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Mon, Jun 28, 2021 at 3:24 PM Bharath Rupireddy
wrote:
>
> On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy
> 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Fri, Jun 18, 2021 at 6:35 PM Bharath Rupireddy 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. v7-0001-Refactor-parse_subscription_options.patch Description: Binary data v7-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jun 10, 2021 at 6:36 PM Bharath Rupireddy wrote: > > On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy > 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. v6-0001-Refactor-parse_subscription_options.patch Description: Binary data v6-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
At Fri, 11 Jun 2021 16:29:10 -0400, Robert Haas wrote in > On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera > 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, May 25, 2021 at 9:38 AM Alvaro Herrera 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jun 10, 2021 at 9:17 AM Bharath Rupireddy 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. v5-0001-Refactor-parse_subscription_options.patch Description: Binary data v5-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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 signature.asc Description: PGP signature
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jun 10, 2021 at 8:55 AM Peter Smith 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy wrote: > > On Wed, Jun 9, 2021 at 10:37 AM Peter Smith 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 9, 2021 at 10:37 AM Peter Smith wrote:
>
> On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:43 AM Peter Smith 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" */
> -_given, ,
> -NULL, /* no "copy_data" */
> -_commit,
> -NULL, /* no "refresh" */
> -_given, ,
> -_given, );
> -
> - 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.
From 57e8189a43bffa3b3464e8b878ed18b6c354a4a8 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Wed, 9 Jun 2021 07:37:41 -0700
Subject: [PATCH v4] Refactor
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith 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" */
-_given, ,
-NULL, /* no "copy_data" */
-_commit,
-NULL, /* no "refresh" */
-_given, ,
-_given, );
-
- 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 6:11 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, Jun 2, 2021 at 11:43 AM Peter Smith 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. Added it to the commitfeset - https://commitfest.postgresql.org/33/3151/ With Regards, Bharath Rupireddy.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 11:43 AM Peter Smith 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. v3-0001-Refactor-parse_subscription_options.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 9:07 AM Peter Smith wrote:
>
> On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
> wrote:
> >
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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
> +{
> + bits32supported_opts; /* bitmask of supported SUBOPT_* */
> + bits32specified_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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
wrote:
>
> On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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
+{
+ bits32supported_opts; /* bitmask of supported SUBOPT_* */
+ bits32specified_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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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. 0001-WIP-refactor-parse_subscription_options.patch Description: Binary data
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, May 25, 2021 at 7:08 PM Alvaro Herrera 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On 2021-May-25, Bharath Rupireddy wrote:
> On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera
> 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Tue, May 25, 2021 at 11:04 AM Michael Paquier 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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 signature.asc Description: PGP signature
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Mon, May 24, 2021 at 11:37 PM Alvaro Herrera wrote: > > On 2021-May-24, Bharath Rupireddy wrote: > > > On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On 2021-May-24, Bharath Rupireddy wrote: > On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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 Herrera39°49'30"S 73°17'W
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Mon, May 24, 2021 at 7:04 AM Michael Paquier 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
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
signature.asc
Description: PGP signature
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Sat, May 22, 2021 at 6:32 AM Peter Smith 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, );
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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Fri, May 21, 2021 at 9:21 PM Peter Smith wrote:
>
> On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> > 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 =
>
>
> 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(, 0, sizeof(opts));
> opts.stmt_options = stmt->options;
> opts.connect =
> opts.enabled_given = _given;
> opts.enabled =
> opts.create_slot = _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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
wrote:
>
> On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> 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 =
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(, 0, sizeof(opts));
opts.stmt_options = stmt->options;
opts.connect =
opts.enabled_given = _given;
opts.enabled =
opts.create_slot = _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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
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
From b72336b7dc803c4ebd49d25850c0b15d8fbdbbed Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Thu, 20 May 2021 09:03:37 +0530
Subject: [PATCH v2] Refactor parse_subscription_options
Currently parse_subscription_options function receives a lot(14) of input
parameters which makes it inextensible to add the new parameters. So,
better wrap all the input parameters within a ParseSubOptions structure to
which new parameters can be added easily.
Also, remove similar code (with the only difference in the option) when
throwing errors for mutually exclusive options. Have a variable for the
option and use it in the error message. It saves some LOC and makes the
code look better with lesser ereport(ERROR statements.
---
src/backend/commands/subscriptioncmds.c | 348 +---
1 file changed, 187 insertions(+), 161 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..feb9074436 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -51,6 +51,26 @@ static void check_duplicates_in_publist(List *publist, Datum *datums);
static List *merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
+/*
+ * 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;
/*
* Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
@@ -60,16 +80,7 @@ static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname,
* accommodate that.
*/
static void
-parse_subscription_options(List *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)
+parse_subscription_options(ParseSubOptions *opts)
{
ListCell *lc;
bool connect_given = false;
@@ -78,45 +89,46 @@ parse_subscription_options(List *options,
bool refresh_given = false;
/* If connect is specified, the others also need to be. */
- Assert(!connect || (enabled && create_slot && copy_data));
+ Assert(!opts->connect || (opts->enabled && opts->create_slot &&
+ opts->copy_data));
- if (connect)
- *connect = true;
- if (enabled)
+ if (opts->connect)
+ *opts->connect = true;
+ if (opts->enabled)
{
- *enabled_given = false;
- *enabled = true;
+ *opts->enabled_given = false;
+ *opts->enabled = true;
}
- if (create_slot)
- *create_slot = true;
- if (slot_name)
+ if (opts->create_slot)
+ *opts->create_slot = true;
+ if (opts->slot_name)
{
- *slot_name_given = false;
- *slot_name = NULL;
+ *opts->slot_name_given = false;
+ *opts->slot_name = NULL;
}
- if (copy_data)
- *copy_data = true;
- if (synchronous_commit)
- *synchronous_commit = NULL;
- if (refresh)
- *refresh = true;
- if (binary)
+ if (opts->copy_data)
+ *opts->copy_data = true;
+ if (opts->synchronous_commit)
+ *opts->synchronous_commit = NULL;
+ if (opts->refresh)
+ *opts->refresh = true;
+ if (opts->binary)
{
- *binary_given = false;
- *binary = false;
+ *opts->binary_given = false;
+ *opts->binary = false;
}
- if (streaming)
+ if (opts->streaming)
{
- *streaming_given = false;
- *streaming = false;
+ *opts->streaming_given = false;
+ *opts->streaming = false;
}
/* Parse options */
- foreach(lc, options)
+ foreach(lc, opts->stmt_options)
{
DefElem*defel = (DefElem *) lfirst(lc);
- if (strcmp(defel->defname, "connect") == 0 && connect)
+ if (strcmp(defel->defname, "connect") == 0 && opts->connect)
{
if (connect_given)
ereport(ERROR,
@@ -124,19 +136,19 @@ parse_subscription_options(List *options,
errmsg("conflicting or redundant options")));
connect_given = true;
- *connect = defGetBoolean(defel);
+ *opts->connect = defGetBoolean(defel);
}
- else if (strcmp(defel->defname, "enabled") == 0 && enabled)
+ else if (strcmp(defel->defname, "enabled") == 0 && opts->enabled)
{
- if (*enabled_given)
+ if (*opts->enabled_given)
ereport(ERROR,
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 5:56 PM Amit Kapila wrote: > > On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy > wrote: > > > > On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote: > > > > > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy > > > wrote: > > > > > > > > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote: > > > > > > > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > > > > > 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy wrote: > > On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote: > > > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy > > wrote: > > > > > > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote: > > > > > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > > > > 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 4:10 PM Amit Kapila wrote: > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy > wrote: > > > > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote: > > > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > > > 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy wrote: > > On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote: > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > > 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.
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 2:33 PM Amul Sul wrote: > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > 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
Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy 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
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
From 6e4824653d2849631cba3cfe1fe562e1b2823c4a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Sat, 1 May 2021 20:26:30 +0530
Subject: [PATCH v1] Refactoring of error code in parse_subscription_options
---
src/backend/commands/subscriptioncmds.c | 50 -
1 file changed, 23 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 517c8edd3b..c4c9b4bd8c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -227,25 +227,21 @@ parse_subscription_options(List *options,
*/
if (connect && !*connect)
{
+ char *option = NULL;
+
/* Check for incompatible options from the user. */
if (enabled && *enabled_given && *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)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s and %s are mutually exclusive options",
- "connect = false", "create_slot = true")));
+ option = "enabled = true";
+ else if (create_slot && create_slot_given && *create_slot)
+ option = "create_slot = true";
+ else if (copy_data && copy_data_given && *copy_data)
+ option = "copy_data = true";
- if (copy_data && copy_data_given && *copy_data)
+ if (option)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
- "connect = false", "copy_data = true")));
+ "connect = false", option)));
/* Change the defaults of other options. */
*enabled = false;
@@ -259,31 +255,31 @@ parse_subscription_options(List *options,
*/
if (slot_name && *slot_name_given && !*slot_name)
{
+ char *option = NULL;
+
if (enabled && *enabled_given && *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")));
+ option = "enabled = true";
+ else if (create_slot && create_slot_given && *create_slot)
+ option = "create_slot = true";
- if (create_slot && create_slot_given && *create_slot)
+ if (option)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
- "slot_name = NONE", "create_slot = true")));
+ "slot_name = NONE", option)));
+
+ 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")));
+ option = "enabled = false";
+ else if (create_slot && !create_slot_given && *create_slot)
+ option = "create_slot = false";
- if (create_slot && !create_slot_given && *create_slot)
+ if (option)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("subscription with %s must also set %s",
- "slot_name = NONE", "create_slot = false")));
+ "slot_name = NONE", option)));
}
}
--
2.25.1
