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

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

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

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

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

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

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

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

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

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

/* Similarly get the other options. */

bms_free(supported);
bms_free(specified);

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

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

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

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

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

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