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
v2-0001-Refactor-parse_publication_options.patch
Description: Binary data
