On Mon, Aug 24, 2026 at 10:16 PM Miłosz Bieniek <[email protected]> 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
