On Wed, Apr 20, 2022 at 7:26 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Below are my review comments for the v9-0002 patch (except I did not
> yet look at the TAP tests).
>
> ~~~
>
> 1. General comment - describe.c
>
> I wondered why the copy_data enum value is not displayed by the psql
> \drs+ command. Should it be?
>
I noticed that we generally don't display the values for the options.
I did not add it to keep it consistent with other options.
> ~~~
>
> 2. General comment - SUBOPT_LOCAL_ONLY
>
> @@ -1134,7 +1204,7 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
>
> case ALTER_SUBSCRIPTION_SET_PUBLICATION:
> {
> - supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
> + supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH | SUBOPT_LOCAL_ONLY;
> parse_subscription_options(pstate, stmt->options,
> supported_opts, &opts);
>
> @@ -1236,7 +1308,8 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
> errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
> subscriptions")));
>
> parse_subscription_options(pstate, stmt->options,
> - SUBOPT_COPY_DATA, &opts);
> + SUBOPT_COPY_DATA | SUBOPT_LOCAL_ONLY,
> + &opts);
>
> I noticed there is some new code that appears to be setting the
> SUBOT_LOCAL_ONLY as a supported option. Shouldn't those changes belong
> in the patch 0001 instead of in this patch 0002?
Modified
> ~~~
>
> 3. Commit message - wording
>
> CURRENT
> a) Node1 - Publication publishing employee table.
> b) Node2 - Subscription subscribing from publication pub1 with
> local_only.
> c) Node2 - Publication publishing employee table.
> d) Node1 - Subscription subscribing from publication pub2 with
> local_only.
>
> SUGGESTION (I think below has the same meaning but is simpler)
> a) Node1 - PUBLICATION pub1 for the employee table
> b) Node2 - SUBSCRIPTION from pub1 with local_only=true
> c) Node2 - PUBLICATION pub2 for the employee table
> d) Node1 - SUBSCRIPTION from pub2 with local_only=true
Modified
> ~~~
>
> 4. Commit message - meaning?
>
> CURRENT
> Now when user is trying to add another node Node3 to the
> above Multi master logical replication setup:
> a) user will have to create one subscription subscribing from Node1 to
> Node3
> b) user wil have to create another subscription subscribing from
> Node2 to Node3 using local_only option and copy_data as true.
>
> While the subscription is created, server will identify that Node2 is
> subscribing from Node1 and Node1 is subscribing from Node2 and throw an
> error so that user can handle the initial copy data.
>
> ~
>
> The wording above confused me. Can you clarify it?
> e.g.
> a) What exactly was the user hoping to achieve here?
> b) Are the user steps doing something deliberately wrong just so you
> can describe later that an error gets thrown?
Modified
> ~~~
>
> 5. doc/src/sgml/logical-replication.sgml - how to get here?
>
> I didn’t see any easy way to get to this page. (No cross refs from anywhere?)
I have added a link from create subscription page
> ~~~
>
> 6. doc/src/sgml/logical-replication.sgml - section levels
>
> I think the section levels may be a bit messed up. e.g. The HTML
> rendering of sections looks a bit confused. Maybe this is same as my
> review comment #13.
Modified
> ~~
>
> 7. doc/src/sgml/logical-replication.sgml - headings
>
> <title>Setting Bidirection logical replication between two nodes:</title>
>
> 7a. Maybe better just to have a simpler main heading like
> "Bidirectional logical replication".
Modified
> 7b. Don't put ":" in the title.
Modified
> ~~~
>
> 8. doc/src/sgml/logical-replication.sgml - missing intro
>
> IMO this page needs some sort of introduction/blurb instead of leaping
> straight into examples without any preamble text to give context.
Modified
> ~~~
>
> 9. doc/src/sgml/logical-replication.sgml - bullets
>
> Suggest removing all the bullets from the example steps. (I had
> something similar a while ago for the "Row Filter" page but
> eventually, they all had to be removed).
Modified
> ~~~
>
> 10. doc/src/sgml/logical-replication.sgml - SQL too long
>
> Many of the example commands are much too long, and for me, they are
> giving scroll bars when rendered. It would be better if they can be
> wrapped in appropriate places so easier to read (and no resulting
> scroll bars).
Modified
> ~~~
>
> 11. doc/src/sgml/logical-replication.sgml - add the psql prompt
>
> IMO it would also be easier to understand the examples if you show the
> psql prompt. Then you can easily know the node context without having
> to describe it in the text so often.
>
> e.g.
>
> + <para>
> + Create the subscription in node2 to subscribe the changes from node1:
> +<programlisting>
> +CREATE SUBSCRIPTION sub_node1_node2 ...
>
> SUGGGESTION
> + <para>
> + Create the subscription in node2 to subscribe the changes from node1
> +<programlisting>
> +node_2=# CREATE SUBSCRIPTION sub_node1_node2 ...
Modified
> ~~~
>
> 12. doc/src/sgml/logical-replication.sgml - typo
>
> + <para>
> + Now the BiDirectional logical replication setup is complete between node1
>
> typo "BiDirectional"
Modified to bidirectional
> ~~~
>
> 13. doc/src/sgml/logical-replication.sgml - deep levels
>
> The section levels are very deep, but > 3 will not appear in the table
> of contents when rendered. Maybe you can rearrange to raise them all
> up one level, then IMO the TOC will work better and the whole page
> will be easier to read.
Modified
> ~~~
>
> 14. doc/src/sgml/logical-replication.sgml - unnecessarily complex?
>
> +<programlisting>
> +# Truncate the table data but do not replicate the truncate.
> +ALTER PUBLICATION pub_node3 SET (publish='insert,update,delete');
> +TRUNCATE t1;
> +
> +CREATE SUBSCRIPTION sub_node3_node1 CONNECTION 'dbname=foo host=node1
> user=repuser' PUBLICATION pub_node1 WITH (copy_data = force,
> local_only = on);
> +
> +CREATE SUBSCRIPTION sub_node3_node2 CONNECTION 'dbname=foo host=node2
> user=repuser' PUBLICATION pub_node2 WITH (copy_data = off, local_only
> = on);
> +
> +# Include truncate operations from now
> +ALTER PUBLICATION pub_node3 SET (publish='insert,update,delete,truncate');
> +</programlisting>
>
> Is it really necessary for those CREATE SUBSCRIPTION to be placed
> where they are? I felt it would be less confusing if you just do the
> TRUNCATE between the 2x ALTER PUBLICATION... and then do those CREATE
> SUBSCRIPTION separately later.
Modified
> ~~~
>
> 14. doc/src/sgml/logical-replication.sgml - force?
>
> There seems no documentation anywhere that says what is the
> purpose/meaning of the copy_data enum "force".
This is added in create subscription notes
> ~~~
>
> 15. doc/src/sgml/ref/alter_subscription.sgml - force?
>
> Meanings of copy_data true/false are self-evident but should something
> here be explaining what is the meaning of "force"? Or a reference to
> some place that explains it?
This is mentioned in create subscription notes, added a reference to
create subscription notes.
> ~~~
>
> 16. doc/src/sgml/ref/create_subscription.sgml - local_only missing notes?
>
> @@ -161,6 +161,11 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
> publisher node changes regardless of their origin. The default is
> <literal>false</literal>.
> </para>
> + <para>
> + There is some interation between the "local_only" option and
> + "copy_data" option. Refer to the
> + <xref linkend="sql-createsubscription-notes" /> for details.
> + </para>
> </listitem>
> </varlistentry>
>
> 16a. Typo "interation"
Modified
> 16b. But where in the referenced notes does it actually say anything about
> this?
Modified
> ~~~
>
> 17. doc/src/sgml/ref/create_subscription.sgml - force?
>
> The notes about the copy_data do not say anything about what the
> values mean. Specifically, there seems nothing that describes what is
> the meaning of "force".
I have mentioned this in the notes section.
> ~~~
>
> 18. doc/src/sgml/ref/create_subscription.sgml - copy_data missing notes?
>
> +
> + <para>
> + There is some interation between the "local_only" option and
> + "copy_data" option. Refer to the
> + <xref linkend="sql-createsubscription-notes" /> for details.
> + </para>
>
> 18a. Typo "interation"
Modified
> 18b. But where in the referenced notes does it actually say anything about
> this?
Added
> ~~~
>
> 19. doc/src/sgml/ref/create_subscription.sgml - xref to the new page
>
> Shouldn't there be an xref on this page somewhere to your other new
> "Bidirectional" docs page?
Added reference
> ~~~
>
> 20. src/backend/commands/subscriptioncmds.c - IS_COPY_DATA_ON_OR_FORCE
>
> @@ -69,6 +69,18 @@
> /* check if the 'val' has 'bits' set */
> #define IsSet(val, bits) (((val) & (bits)) == (bits))
>
> +#define IS_COPY_DATA_ON_OR_FORCE(copy_data) (copy_data != COPY_DATA_OFF)
>
> Maybe this would be better as a static inline function?
What is the advantage of doing this change? I have not changed this
as the macro usage is fine.
Thoughts?
> ~~~
>
> 21. src/backend/commands/subscriptioncmds.c - comment
>
> +/*
> + * Represents whether copy_data option is specified with on, off or force.
> + */
> +typedef enum CopyData
> +{
> + COPY_DATA_OFF,
> + COPY_DATA_ON,
> + COPY_DATA_FORCE
> +} CopyData;
>
> I felt it might be better if the comment described these enums in the
> same order they are defined.
>
> E.g. "Represents whether copy_data option is specified as off/on, or force."
Modified
> ~~~
>
> 22. src/backend/commands/subscriptioncmds.c - CreateSubscription bug?
>
> @@ -720,7 +788,8 @@ CreateSubscription(ParseState *pstate,
> CreateSubscriptionStmt *stmt,
> * PENDING, to allow ALTER SUBSCRIPTION ... REFRESH
> * PUBLICATION to work.
> */
> - if (opts.twophase && !opts.copy_data && tables != NIL)
> + if (opts.twophase && IS_COPY_DATA_ON_OR_FORCE(opts.copy_data)
> + && tables != NIL)
> twophase_enabled = true;
> Is that a bug? It does not seem to match the previous code. Should
> that IS_COPY_DATA_ON_OR_FORCE be "not" ?
Modified
> ~~~
>
> 23. src/backend/commands/subscriptioncmds.c - long errmsg
>
> + if (copydata == COPY_DATA_ON && local_only && !slot_attisnull(slot, 3))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("CREATE/ALTER SUBSCRIPTION with local_only and copy_data as
> true is not allowed when the publisher might have replicated data,
> table:%s.%s might have replicated data in the publisher",
> + nspname, relname),
> + errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off or force"));
> +
>
> The errmsg seems way too long for the source code. Can you use string
> concatenation or continuation chars to wrap the message over multiple
> lines?
I had seen that the long error message elsewhere also is in a single
line. I think we should keep it as it is to maintain the coding
standard.
Thoughts?
> ~~~
>
> 24. src/test/regress/sql/subscription.sql - typo
>
> @@ -66,6 +69,15 @@ ALTER SUBSCRIPTION regress_testsub4 SET (local_only =
> false);
> DROP SUBSCRIPTION regress_testsub3;
> DROP SUBSCRIPTION regress_testsub4;
>
> +-- ok - valid coy_data options
>
> Typo "coy_data". (it looks like this typo is not caused by this patch,
> but I think this patch should fix it anyhow).
Modified
> ~~~
>
> 25. src/test/regress/sql/subscription.sql - test order
>
> The new tests are OK but IMO they could be re-ordered so then they
> will be more consistent for the positive and negative tests.
>
> CURRENT
> "true/force/on/1" and "off/false/0"
>
> SUGGEST
> "true/on/1/force" and "false/off/0"
Modified
Thanks for the comments, the v10 patch attached at [1] has the changes
for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm0PmOz71O6ofhZkB0rts5Ak2HUhMuuMQoViH_LAXTBeBw%40mail.gmail.com
Regards,
Vignesh
