Re: [PATCH] Refactor parse_publication_options

On Mon, Aug 24, 2026 at 10:16 PM Miłosz Bieniek <milosz(at)bieniek(dot)cc> wrote:
>
> Hi,
> While reading the code related to creating subscriptions and publications for logical replication I found a difference between passing options to parse_subscription_options and parse_publication_options.
> To parse_publication_options function we pass all options separately, while in parse_subscription_options we use SubOpts.
> IMO the usage of a struct instead of each option separately seems cleaner.
> I attached a patch that creates a PubOpts struct and uses it in all parse_publication_options occurrences.
>

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.

~~~

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.

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