Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options - Mailing list pgsql-hackers

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

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

- The patch applies OK.

- The code builds OK.

- The make check and TAP subscription tests are OK


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

COMMENTS
==========

parse_subscription_options:

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

----------

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

----------

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

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

----------

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

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


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

==========

Create Subscription:

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


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

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

----------

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

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

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

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

==========

AlterSubscription

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

----------

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

----------

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

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

if (isadd)
opts.copy_data = ©_data;

==========

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

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