On Friday, November 12, 2021 1:09 PM vignesh C <vignesh21@gmail.com> wrote:
> On Thu, Nov 11, 2021 at 2:50 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> Thanks for the updated patch, Few comments:
> 1) tab completion should be added for disable_on_error:
> /* Complete "CREATE SUBSCRIPTION <name> ... WITH ( <opt>" */ else if
> (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
> COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
> "enabled", "slot_name", "streaming",
> "synchronous_commit", "two_phase");
Fixed.
> 2) disable_on_error is supported by alter subscription, the same should be
> documented:
> @ -871,11 +886,19 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
> {
> supported_opts = (SUBOPT_SLOT_NAME |
>
> SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
> -
> SUBOPT_STREAMING);
> +
> SUBOPT_STREAMING | SUBOPT_DISABLE_ON_ERR);
>
> parse_subscription_options(pstate,
> stmt->options,
>
> supported_opts, &opts);
>
> + if (IsSet(opts.specified_opts,
> SUBOPT_DISABLE_ON_ERR))
> + {
> +
> values[Anum_pg_subscription_subdisableonerr - 1]
> + =
> BoolGetDatum(opts.disableonerr);
> +
> replaces[Anum_pg_subscription_subdisableonerr - 1]
> + = true;
> + }
> +
Fixed the documentation. Also, add one test for alter subscription.
> 3) Describe subscriptions (dRs+) should include displaying of disableonerr:
> \dRs+ sub1
> List of subscriptions
> Name | Owner | Enabled | Publication | Binary | Streaming | Two
> phase commit | Synchronous commit | Conninfo
> ------+---------+---------+-------------+--------+-----------+----------
> --------+--------------------+---------------------------
> sub1 | vignesh | t | {pub1} | f | f | d
> | off | dbname=postgres port=5432
> (1 row)
Fixed.
> 4) I felt transicent should be transient, might be a typo:
> + Specifies whether the subscription should be automatically
> disabled
> + if replicating data from the publisher triggers non-transicent errors
> + such as referential integrity or permissions errors. The default is
> + <literal>false</literal>.
Fixed.
> 5) The commented use PostgresNode and use TestLib can be removed:
> +# Test of logical replication subscription self-disabling feature use
> +strict; use warnings; # use PostgresNode; # use TestLib; use
> +PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More
> +tests => 10;
Removed.
Also, my colleague Greg provided an offlist patch to me and
I've incorporated his suggested modifications into this version.
So, I noted his name as a coauthor.
C codes are checked by pgindent again.
This v5 depends on v23 skip xid in [1].
[1] - https://www.postgresql.org/message-id/CAD21AoA5jupM6O%3DpYsyfaxQ1aMX-en8%3DQNgpW6KfXsg7_CS0CQ%40mail.gmail.com
Best Regards,
Takamichi Osumi
