Re: [PATCH] Refactor parse_publication_options

Hi Peter, thanks for your review.

> IIRC the introduction of SubOpt had become a necessity because there
> was an excessive number of parameters to parse_subscription_options.
> OTOH, publications have a lot fewer options than subscriptions, so
> this patch to introduce PubOpts seems more a matter of code
> consistency than necessity.
>
> Anyway, +1 from me for attempting this, but I expect it could receive
> pushback from the "if-it-aint-broke-dont-fix-it" people.

I understand that some people might not want to change working
code, but IMO it's worth the change for the sake of consistency.

> ~~~
>
> Meanwhile, here are some other review comments for patch 0001.
>
> ======
>
> 1.
> +typedef struct PubOpts
> +{
> + bool publish_given;
> + PublicationActions pubactions;
> + bool publish_via_partition_root_given;
> + bool publish_via_partition_root;
> + bool publish_generated_columns_given;
> + char publish_generated_columns;
> +} PubOpts;
>
> To make this more similar to SubOpts, then you should also remove
> those `xxx_given` members and instead use a `specified_opts` bitmap
> member and PUBOPT_xxx macros, exactly the same as SubOpts does.
>
> ~~~
>
> parse_publication_options:
>
> 2.
> + /* Start out with cleared opts. */
> + memset(opts, 0, sizeof(PubOpts));
> +
> + /* Set default values */
> + opts->publish_given = false;
> + opts->publish_via_partition_root_given = false;
> + opts->publish_generated_columns_given = false;
>
> IMO it's unnecessary to assign these `xxx_given` members to false,
> since you've only just done the memset 0.

You're right. I made the v2 version more similar to SubOpts.

One thing I'm not so sure about is `IsSet` macro. I copied it from the 'subscriptionscmds.c',
but maybe it should be extracted to a header file?

Kind regards,
Miłosz Bieniek