On Tuesday, March 15, 2022 8:04 AM Nathan Bossart
wrote:
> My compiler is worried that syncslotname may be used uninitialized in
> start_table_sync(). The attached patch seems to silence this warning.
Thank you for your reporting !
Your fix looks good to me.
Best Regards,
Takamichi
My compiler is worried that syncslotname may be used uninitialized in
start_table_sync(). The attached patch seems to silence this warning.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 05e4e03af5afa1658ede8d78b31c1c999b5c7deb Mon Sep 17 00:00:00 2001
From: Nathan Bossart
On Monday, March 14, 2022 7:49 PM Amit Kapila wrote:
> On Thu, Mar 10, 2022 at 12:04 PM Amit Kapila
> wrote:
> >
> > On Wed, Mar 9, 2022 at 7:57 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > Hi, attached v32 removed my additional code for
> maybe_reread_subscription.
> > >
> >
> >
On Thu, Mar 10, 2022 at 12:04 PM Amit Kapila wrote:
>
> On Wed, Mar 9, 2022 at 7:57 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Hi, attached v32 removed my additional code for maybe_reread_subscription.
> >
>
> Thanks, the patch looks good to me. I have made minor edits in the
> attached. I
On Wed, Mar 9, 2022 at 7:57 PM osumi.takami...@fujitsu.com
wrote:
>
> Hi, attached v32 removed my additional code for maybe_reread_subscription.
>
Thanks, the patch looks good to me. I have made minor edits in the
attached. I am planning to commit this early next week (Monday) unless
there are
On Wednesday, March 9, 2022 8:22 PM Amit Kapila wrote:
> On Wed, Mar 9, 2022 at 2:21 PM Masahiko Sawada
> wrote:
> >
> > On Wed, Mar 9, 2022 at 4:33 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, March 8, 2022 10:23 PM Amit Kapila
> wrote:
> > > > On Tue, Mar 8, 2022 at 1:37
On Wed, Mar 9, 2022 at 2:21 PM Masahiko Sawada wrote:
>
> On Wed, Mar 9, 2022 at 4:33 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Tuesday, March 8, 2022 10:23 PM Amit Kapila
> > wrote:
> > > On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> > > wrote:
> > > >
> >
> >
> > >
On Wed, Mar 9, 2022 at 4:33 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, March 8, 2022 10:23 PM Amit Kapila
> wrote:
> > On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
>
>
> > 2. Is there a reason the patch doesn't allow workers to restart via
> >
On Tuesday, March 8, 2022 10:23 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at v30.
> >
>
> Review comments:
Thank you for checking !
> ===
> 1.
> + ereport(LOG,
> + errmsg("logical replication subscription
On Wednesday, March 9, 2022 9:58 AM Masahiko Sawada
wrote:
> On Tue, Mar 8, 2022 at 5:07 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at v30.
>
> Thank you for updating the patch. Here are some comments:
Hi, thank you for your review !
> + /*
> +* Allocate the
On Wednesday, March 9, 2022 1:29 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 6:53 PM Amit Kapila wrote:
> >
> > On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > Kindly have a look at v30.
> > >
> >
> > Review comments:
> > ===
Thank you for
On Wednesday, March 9, 2022 3:02 PM Amit Kapila wrote:
> On Wed, Mar 9, 2022 at 11:22 AM Masahiko Sawada
> wrote:
> >
> > On Wed, Mar 9, 2022 at 12:37 PM Amit Kapila
> wrote:
> > >
> > > On Wed, Mar 9, 2022 at 6:29 AM Masahiko Sawada
> wrote:
> > > >
> > > > ---
> > > > It might have already
On Wed, Mar 9, 2022 at 11:22 AM Masahiko Sawada wrote:
>
> On Wed, Mar 9, 2022 at 12:37 PM Amit Kapila wrote:
> >
> > On Wed, Mar 9, 2022 at 6:29 AM Masahiko Sawada
> > wrote:
> > >
> > > ---
> > > It might have already been discussed but the worker disables the
> > > subscription on an error
On Wed, Mar 9, 2022 at 12:37 PM Amit Kapila wrote:
>
> On Wed, Mar 9, 2022 at 6:29 AM Masahiko Sawada wrote:
> >
> > ---
> > It might have already been discussed but the worker disables the
> > subscription on an error but doesn't work for a fatal. Is that
> > expected or should we handle that
On Tue, Mar 8, 2022 at 6:53 PM Amit Kapila wrote:
>
> On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at v30.
> >
>
> Review comments:
> ===
>
Few comments on test script:
===
1.
+# This tests the uniqueness violation
On Wed, Mar 9, 2022 at 6:29 AM Masahiko Sawada wrote:
>
> ---
> It might have already been discussed but the worker disables the
> subscription on an error but doesn't work for a fatal. Is that
> expected or should we handle that too?
>
I am not too sure about handling FATALs with this feature
On Tue, Mar 8, 2022 at 5:07 PM osumi.takami...@fujitsu.com
wrote:
>
> Kindly have a look at v30.
Thank you for updating the patch. Here are some comments:
+ /*
+* Allocate the origin name in long-lived context for error context
+* message.
+*/
+
On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
wrote:
>
> Kindly have a look at v30.
>
Review comments:
===
1.
+ ereport(LOG,
+ errmsg("logical replication subscription \"%s\" has been be disabled
due to an error",
Typo.
/been be/been
2. Is there a reason the patch
On Tuesday, March 8, 2022 1:07 PM Peter Smith wrote:
> Please find below some review comments for v29.
Thank you for your comments !
> ==
>
> 1. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> +/*
> + * Abort and cleanup the current transaction, then do
On Tuesday, March 8, 2022 2:52 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 9:37 AM Peter Smith wrote:
> >
> > Please find below some review comments for v29.
> >
> > ==
> >
> > 1. src/backend/replication/logical/worker.c -
> > worker_post_error_processing
> >
> > +/*
> > + * Abort and
On Tue, Mar 8, 2022 at 9:37 AM Peter Smith wrote:
>
> Please find below some review comments for v29.
>
> ==
>
> 1. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> +/*
> + * Abort and cleanup the current transaction, then do post-error processing.
> + * This
Please find below some review comments for v29.
==
1. src/backend/replication/logical/worker.c - worker_post_error_processing
+/*
+ * Abort and cleanup the current transaction, then do post-error processing.
+ * This function must be called in a PG_CATCH() block.
+ */
+static void
On Monday, March 7, 2022 5:45 PM Amit Kaila wrote:
> On Mon, Mar 7, 2022 at 4:55 AM Peter Smith
> wrote:
> >
> > On Fri, Mar 4, 2022 at 5:55 PM Masahiko Sawada
> wrote:
> > >
> > > ---
> > > +/*
> > > + * First, ensure that we log the error message so
> > > that
On Mon, Mar 7, 2022 at 4:55 AM Peter Smith wrote:
>
> On Fri, Mar 4, 2022 at 5:55 PM Masahiko Sawada wrote:
> >
> > ---
> > +/*
> > + * First, ensure that we log the error message so
> > that it won't be
> > + * lost if some other internal error
On Monday, March 7, 2022 12:01 PM Shi, Yu/侍 雨 wrote:
> On Wed, Mar 2, 2022 5:39 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Attached an updated patch v26.
> >
>
> Thanks for your patch. A comment on the document.
Hi, thank you for checking my patch !
> @@ -7771,6 +7771,16 @@
On Friday, March 4, 2022 3:55 PM Masahiko Sawada wrote:
> Thank you for updating the patch.
>
> Here are some comments on v26 patch:
Thank you for your review !
> +/*
> + * Disable the current subscription.
> + */
> +static void
> +DisableSubscriptionOnError(void)
>
> This function now just
On Wed, Mar 2, 2022 5:39 PM osumi.takami...@fujitsu.com
wrote:
>
> Attached an updated patch v26.
>
Thanks for your patch. A comment on the document.
@@ -7771,6 +7771,16 @@ SCRAM-SHA-256$iteration
count:
+ subdisableonerr bool
+
+
+ If true, the
On Fri, Mar 4, 2022 at 5:55 PM Masahiko Sawada wrote:
>
> On Wed, Mar 2, 2022 at 6:38 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Wednesday, March 2, 2022 12:47 PM Masahiko Sawada
> > wrote:
> > > After more thoughts, should we do both AbortOutOfAnyTransaction() and
> > > error
> > >
On Wed, Mar 2, 2022 at 6:38 PM osumi.takami...@fujitsu.com
wrote:
>
> On Wednesday, March 2, 2022 12:47 PM Masahiko Sawada
> wrote:
> > After more thoughts, should we do both AbortOutOfAnyTransaction() and error
> > message handling while holding interrupts? That is,
> >
> > HOLD_INTERRUPTS();
On Wednesday, March 2, 2022 12:47 PM Masahiko Sawada
wrote:
> After more thoughts, should we do both AbortOutOfAnyTransaction() and error
> message handling while holding interrupts? That is,
>
> HOLD_INTERRUPTS();
> EmitErrorReport();
> FlushErrorState();
> AbortOutOfAny Transaction();
>
On Wed, Mar 2, 2022 at 9:34 AM Peter Smith wrote:
>
> Please see below my review comments for v24.
>
> ==
>
> 1. src/backend/replication/logical/worker.c - start_table_sync
>
> + /* Report the worker failed during table synchronization */
> +
On Wednesday, March 2, 2022 9:34 AM Peter Smith wrote:
> Please see below my review comments for v24.
Thank you for checking my patch !
> ==
>
> 1. src/backend/replication/logical/worker.c - start_table_sync
>
> + /* Report the worker failed during table synchronization */
> +
Please see below my review comments for v24.
==
1. src/backend/replication/logical/worker.c - start_table_sync
+ /* Report the worker failed during table synchronization */
+ pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());
(This review comment is just FYI in
On Tuesday, March 1, 2022 9:49 AM Peter Smith wrote:
> Please see below my review comments for v22.
>
> ==
>
> 1. Commit message
>
> "table sync worker" -> "tablesync worker"
Fixed.
> ~~~
>
> 2. doc/src/sgml/catalogs.sgml
>
> +
> + If true, the subscription will be disabled
On Friday, February 25, 2022 9:45 PM osumi.takami...@fujitsu.com
wrote:
> Kindly have a look at attached the v22.
> It has incorporated other improvements of TAP test, refinement of the
> DisableSubscriptionOnError function and so on.
The recent commit(7a85073) has changed the subscription
Please see below my review comments for v22.
==
1. Commit message
"table sync worker" -> "tablesync worker"
~~~
2. doc/src/sgml/catalogs.sgml
+
+ If true, the subscription will be disabled when subscription
+ workers detect any errors
+
It felt a bit strange to
On Tuesday, February 22, 2022 3:03 PM Peter Smith wrote:
> Here are a couple more review comments for v21.
>
> ~~~
>
> 1. worker.c - comment
>
> + subform = (Form_pg_subscription) GETSTRUCT(tup);
> +
> + /*
> + * We would not be here unless this subscription's disableonerr field
> + was
> + *
On Wednesday, February 23, 2022 6:52 PM Tang, Haiying/唐 海英
wrote:
> I have a comment on v21 patch.
>
> I wonder if we really need subscription s2 in 028_disable_on_error.pl. I
> think for
> subscription s2, we only tested some normal cases(which could be tested with
> s1), and didn't test any
On Thursday, February 24, 2022 4:50 PM Masahiko Sawada
wrote:
> On Tue, Feb 22, 2022 at 3:03 PM Peter Smith
> wrote:
> >
> > ~~~
> >
> > 1. worker.c - comment
> >
> > + subform = (Form_pg_subscription) GETSTRUCT(tup);
> > +
> > + /*
> > + * We would not be here unless this subscription's
On Thursday, February 24, 2022 8:09 PM Amit Kapila
> On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada
> wrote:
> > + /*
> > +* Log the error that caused DisableSubscriptionOnError to be
> called. We
> > +* do this immediately so that it won't be lost if some other
> >
On Friday, February 25, 2022 12:57 PM Amit Kapila
wrote:
> On Thu, Feb 24, 2022 at 6:30 PM Masahiko Sawada
> wrote:
> >
> > On Thu, Feb 24, 2022 at 8:08 PM Amit Kapila
> wrote:
> > >
> > > On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada
> wrote:
> > > >
> > > > Here are some comments:
> > > >
On Thu, Feb 24, 2022 at 6:30 PM Masahiko Sawada wrote:
>
> On Thu, Feb 24, 2022 at 8:08 PM Amit Kapila wrote:
> >
> > On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada
> > wrote:
> > >
> > > Here are some comments:
> > >
> > > Why do we need SyncTableStartWrapper() and ApplyLoopWrapper()?
> > >
On Thu, Feb 24, 2022 at 8:08 PM Amit Kapila wrote:
>
> On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada wrote:
> >
> > Here are some comments:
> >
> > Why do we need SyncTableStartWrapper() and ApplyLoopWrapper()?
> >
>
> I have given this comment to move the related code to separate
> functions
On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada wrote:
>
> Here are some comments:
>
> Why do we need SyncTableStartWrapper() and ApplyLoopWrapper()?
>
I have given this comment to move the related code to separate
functions to slightly simplify ApplyWorkerMain() code but if you don't
like we
On Tue, Feb 22, 2022 at 3:03 PM Peter Smith wrote:
>
> ~~~
>
> 1. worker.c - comment
>
> + subform = (Form_pg_subscription) GETSTRUCT(tup);
> +
> + /*
> + * We would not be here unless this subscription's disableonerr field was
> + * true, but check whether that field has changed in the interim.
On Wednesday, February 23, 2022 6:52 PM Tang, Haiying/唐 海英
wrote:
> I have a comment on v21 patch.
>
> I wonder if we really need subscription s2 in 028_disable_on_error.pl. I
> think for
> subscription s2, we only tested some normal cases(which could be tested with
> s1), and didn't test any
Hi Osumi-san,
I have a comment on v21 patch.
I wonder if we really need subscription s2 in 028_disable_on_error.pl. I think
for subscription s2, we only tested some normal cases(which could be tested
with s1),
and didn't test any error case, which means it wouldn't be automatically
disabled.
On Tue, Feb 22, 2022 at 3:11 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, February 22, 2022 7:53 AM Peter Smith
> wrote:
> > On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Monday, February 21, 2022 2:56 PM Peter Smith
> > wrote:
> > > > Thanks
On Tuesday, February 22, 2022 7:53 AM Peter Smith wrote:
> On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Monday, February 21, 2022 2:56 PM Peter Smith
> wrote:
> > > Thanks for addressing my previous comments. Now I have looked at v19.
> > >
> > > On Mon, Feb
On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
wrote:
>
> On Monday, February 21, 2022 2:56 PM Peter Smith
> wrote:
> > Thanks for addressing my previous comments. Now I have looked at v19.
> >
> > On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
> > wrote:
> > >
> >
On Monday, February 21, 2022 2:56 PM Peter Smith wrote:
> Thanks for addressing my previous comments. Now I have looked at v19.
>
> On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Friday, February 18, 2022 3:27 PM Peter Smith
> wrote:
> > > Hi. Below are my
Thanks for addressing my previous comments. Now I have looked at v19.
On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
wrote:
>
> On Friday, February 18, 2022 3:27 PM Peter Smith
> wrote:
> > Hi. Below are my code review comments for v18.
> Thank you for your review !
...
> > 5.
On Friday, February 18, 2022 3:27 PM Peter Smith wrote:
> Hi. Below are my code review comments for v18.
Thank you for your review !
> ==
>
> 1. Commit Message - wording
>
> BEFORE
> To partially remedy the situation, adding a new subscription_parameter named
> 'disable_on_error'.
>
>
On Tuesday, February 15, 2022 2:19 PM I wrote
> On Monday, February 14, 2022 8:58 PM Amit Kapila
> > 2. Can we move the code related to tablesync worker and its error
> > handing (the code insider if (am_tablesync_worker())) to a separate
> > function say
> > LogicalRepHandleTableSync() or
On Monday, February 14, 2022 8:58 PM Amit Kapila
wrote:
> On Thu, Jan 6, 2022 at 11:23 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at the attached v16.
> >
>
> Few comments:
Hi, thank you for checking the patch !
> =
> 1.
> @@ -3594,13 +3698,29 @@
On Thu, Jan 6, 2022 at 11:23 AM osumi.takami...@fujitsu.com
wrote:
>
> Kindly have a look at the attached v16.
>
Few comments:
=
1.
@@ -3594,13 +3698,29 @@ ApplyWorkerMain(Datum main_arg)
apply_error_callback_arg.command,
apply_error_callback_arg.remote_xid,
On Thursday, January 6, 2022 12:17 PM Tang, Haiying/唐 海英
wrote:
> Thanks for updating the patch. Here are some comments:
Thank you for your review !
> 1)
> + /*
> + * We would not be here unless this subscription's disableonerr field
> was
> + * true when our worker began applying
On Wednesday, January 5, 2022 8:53 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威
> wrote:
> > On Thursday, December 16, 2021 8:51 PM osumi.takami...@fujitsu.com
> > wrote:
> > > Attached the updated patch v14.
> >
> > A comment to the timing of
On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威
wrote:
> On Thursday, December 16, 2021 8:51 PM osumi.takami...@fujitsu.com
> wrote:
> > Attached the updated patch v14.
>
> A comment to the timing of printing a log:
Thank you for your review !
> After the log[1] was printed, I altered
On Thursday, December 16, 2021 8:51 PM osumi.takami...@fujitsu.com
wrote:
> Attached the updated patch v14.
A comment to the timing of printing a log:
After the log[1] was printed, I altered subscription's option
(DISABLE_ON_ERROR) from true to false before invoking DisableSubscriptionOnError
On Tuesday, December 21, 2021 11:18 PM I wrote:
> On Thursday, December 16, 2021 9:51 PM I wrote:
> > Attached the updated patch v14.
> FYI, I've conducted a test of disable_on_error flag using pg_upgrade. I
> prepared PG14 and HEAD applied with disable_on_error patch.
> Then, I setup a logical
On Thursday, December 16, 2021 9:51 PM I wrote:
> Attached the updated patch v14.
FYI, I've conducted a test of disable_on_error flag using
pg_upgrade. I prepared PG14 and HEAD applied with disable_on_error patch.
Then, I setup a logical replication pair of the publisher and the subscriber by
14
On Thursday, December 16, 2021 2:32 PM Greg Nancarrow
wrote:
> On Tue, Dec 14, 2021 at 4:34 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Besides all of those changes, I've removed the obsolete comment of
> > DisableSubscriptionOnError in v12.
> >
>
> I have a few minor comments, otherwise
On Tue, Dec 14, 2021 at 4:34 PM osumi.takami...@fujitsu.com
wrote:
>
> Besides all of those changes, I've removed the obsolete
> comment of DisableSubscriptionOnError in v12.
>
I have a few minor comments, otherwise the patch LGTM at this point:
doc/src/sgml/catalogs.sgml
(1)
Current comment
On Monday, December 13, 2021 6:57 PM vignesh C wrote:
> On Mon, Dec 6, 2021 at 4:22 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > I've attached the new version v12.
I appreciate your review.
> Thanks for the updated patch, few comments:
> 1) This is not required as it is not used in the
On Mon, Dec 6, 2021 at 4:22 PM osumi.takami...@fujitsu.com
wrote:
>
> On Monday, December 6, 2021 1:16 PM Greg Nancarrow
> wrote:
> > On Sat, Dec 4, 2021 at 12:20 AM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > Hi, I've made a new patch v11 that incorporated suggestions described
> >
> On Dec 8, 2021, at 8:09 PM, Amit Kapila wrote:
>
> So, do you agree that we can disable the subscription on any error if
> this parameter is set?
Yes, I think that is fine. We can commit it that way, and revisit the issue
for v16 if it becomes a problem in practice.
—
Mark Dilger
On Wed, Dec 8, 2021 at 9:22 PM Mark Dilger wrote:
>
>
> > On Dec 8, 2021, at 5:10 AM, Amit Kapila wrote:
> >
> > I think if we are really worried about transient errors then probably
> > the idea "disable only if the same error has occurred more than X
> > times" seems preferable as compared to
> On Dec 8, 2021, at 5:10 AM, Amit Kapila wrote:
>
> I think if we are really worried about transient errors then probably
> the idea "disable only if the same error has occurred more than X
> times" seems preferable as compared to taking a decision on which
> error_codes fall in the
On Tue, Dec 7, 2021 at 5:52 AM Greg Nancarrow wrote:
>
> On Tue, Dec 7, 2021 at 3:06 AM Mark Dilger
> wrote:
> >
> > My concern about disabling a subscription in response to *any* error is
> > that people may find the feature does more harm than good. Disabling the
> > subscription in
On Tue, Dec 7, 2021 at 3:06 AM Mark Dilger wrote:
>
> My concern about disabling a subscription in response to *any* error is that
> people may find the feature does more harm than good. Disabling the
> subscription in response to an occasional deadlock against other database
> users, or
> On Dec 5, 2021, at 10:56 PM, osumi.takami...@fujitsu.com wrote:
>
> In my humble opinion, I felt the original purpose of the patch was to
> partially remedy
> the situation that during the failure of apply, the apply process keeps going
> into the infinite error loop.
I agree.
> I'd say
On Mon, Dec 6, 2021 at 10:07 AM Mark Dilger
wrote:
>
> > On Dec 1, 2021, at 8:48 PM, Amit Kapila wrote:
> >
> > The patch disables the subscription for non-transient errors. I am not
> > sure if we can easily make the call to decide whether any particular
> > error is transient or not. For
On Monday, December 6, 2021 1:16 PM Greg Nancarrow wrote:
> On Sat, Dec 4, 2021 at 12:20 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > Hi, I've made a new patch v11 that incorporated suggestions described
> above.
> >
>
> Some review comments for the v11 patch:
Thank you for your reviews !
On Monday, December 6, 2021 1:38 PM Mark Dilger
wrote:
> > On Dec 1, 2021, at 8:48 PM, Amit Kapila wrote:
> >
> > The patch disables the subscription for non-transient errors. I am not
> > sure if we can easily make the call to decide whether any particular
> > error is transient or not. For
> On Dec 1, 2021, at 8:48 PM, Amit Kapila wrote:
>
> The patch disables the subscription for non-transient errors. I am not
> sure if we can easily make the call to decide whether any particular
> error is transient or not. For example, DISK_FULL or OUT_OF_MEMORY
> might not rectify itself.
On Sat, Dec 4, 2021 at 12:20 AM osumi.takami...@fujitsu.com
wrote:
>
> Hi, I've made a new patch v11 that incorporated suggestions described above.
>
Some review comments for the v11 patch:
doc/src/sgml/ref/create_subscription.sgml
(1) Possible wording improvement?
BEFORE:
+ Specifies whether
Thursday, December 2, 2021 4:41 PM I wrote:
> On Thursday, December 2, 2021 1:49 PM Amit Kapila
> wrote:
> > On Thu, Dec 2, 2021 at 6:35 AM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Wednesday, December 1, 2021 10:16 PM Amit Kapila
> > wrote:
> > > Updated the patch to include the
On Thursday, December 2, 2021 1:49 PM Amit Kapila
wrote:
> On Thu, Dec 2, 2021 at 6:35 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Wednesday, December 1, 2021 10:16 PM Amit Kapila
> wrote:
> > Updated the patch to include the notification.
> >
> The patch disables the subscription for
On Thu, Dec 2, 2021 at 6:35 AM osumi.takami...@fujitsu.com
wrote:
>
> On Wednesday, December 1, 2021 10:16 PM Amit Kapila
> wrote:
> Updated the patch to include the notification.
>
The patch disables the subscription for non-transient errors. I am not
sure if we can easily make the call to
On Thu, Dec 2, 2021 at 12:05 PM osumi.takami...@fujitsu.com
wrote:
>
> Updated the patch to include the notification.
>
For the catalogs.sgml update, I was thinking that the following
wording might sound a bit better:
+ If true, the subscription will be disabled when a subscription
+
On Wednesday, December 1, 2021 10:16 PM Amit Kapila
wrote:
> On Wed, Dec 1, 2021 at 5:55 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Wednesday, December 1, 2021 3:02 PM vignesh C
> wrote:
> > > On Tue, Nov 30, 2021 at 5:34 PM osumi.takami...@fujitsu.com
> > > wrote:
> >
> > > 3) Can
On Wed, Dec 1, 2021 at 5:55 PM osumi.takami...@fujitsu.com
wrote:
>
> On Wednesday, December 1, 2021 3:02 PM vignesh C wrote:
> > On Tue, Nov 30, 2021 at 5:34 PM osumi.takami...@fujitsu.com
> > wrote:
>
> > 3) Can add a line in the commit message saying "Bump catalog version."
> > as the patch
On Wednesday, December 1, 2021 3:02 PM vignesh C wrote:
> On Tue, Nov 30, 2021 at 5:34 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow
> wrote:
> > > On Sat, Nov 27, 2021 at 1:36 AM osumi.takami...@fujitsu.com
> > > wrote:
> > > >
> > > >
On Tue, Nov 30, 2021 at 5:34 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow
> wrote:
> > On Sat, Nov 27, 2021 at 1:36 AM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > This v7 uses v26 of skip xid patch [1]
> > This patch no longer applies on
On Monday, November 29, 2021 2:38 PM vignesh C
> Thanks for the updated patch, Few comments:
Thank you for your review !
> 1) Since this function is used only from 027_disable_on_error and not used by
> others, this can be moved to 027_disable_on_error:
> +sub wait_for_subscriptions
> +{
> +
On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow
wrote:
> On Sat, Nov 27, 2021 at 1:36 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > This v7 uses v26 of skip xid patch [1]
> This patch no longer applies on the latest source.
> Also, the patch is missing an update to
On Sat, Nov 27, 2021 at 1:36 AM osumi.takami...@fujitsu.com
wrote:
>
> This v7 uses v26 of skip xid patch [1]
>
This patch no longer applies on the latest source.
Also, the patch is missing an update to doc/src/sgml/catalogs.sgml,
for the new "subdisableonerr" column of pg_subscription.
On Fri, Nov 26, 2021 at 8:06 PM osumi.takami...@fujitsu.com
wrote:
>
> On Monday, November 22, 2021 3:53 PM vignesh C wrote:
> > Few comments:
> Thank you so much for your review !
>
> > 1) Changes to handle pg_dump are missing. It should be done in
> > dumpSubscription and getSubscriptions
>
On Monday, November 22, 2021 3:53 PM vignesh C wrote:
> Few comments:
Thank you so much for your review !
> 1) Changes to handle pg_dump are missing. It should be done in
> dumpSubscription and getSubscriptions
Fixed.
> 2) "And" is missing
> --- a/doc/src/sgml/ref/alter_subscription.sgml
> +++
On Thu, Nov 18, 2021 at 12:52 PM osumi.takami...@fujitsu.com
wrote:
>
> On Thursday, November 18, 2021 2:08 PM Greg Nancarrow
> wrote:
> > A minor comment:
> Thanks for your comments !
>
> > doc/src/sgml/ref/alter_subscription.sgml
> > (1) disable_on_err?
> >
> > + disable_on_err.
> >
> >
On Thursday, November 18, 2021 2:08 PM Greg Nancarrow
wrote:
> A minor comment:
Thanks for your comments !
> doc/src/sgml/ref/alter_subscription.sgml
> (1) disable_on_err?
>
> + disable_on_err.
>
> This doc update names the new parameter as "disable_on_err" instead of
>
On Tue, Nov 16, 2021 at 6:53 PM osumi.takami...@fujitsu.com
wrote:
>
> This v5 depends on v23 skip xid in [1].
>
A minor comment:
doc/src/sgml/ref/alter_subscription.sgml
(1) disable_on_err?
+ disable_on_err.
This doc update names the new parameter as "disable_on_err" instead of
Thank you for checking the patch !
On Friday, November 12, 2021 1:49 PM Greg Nancarrow wrote:
> On Thu, Nov 11, 2021 at 8:20 PM osumi.takami...@fujitsu.com
> wrote:
> Some comments on the v4 patch:
>
> (1) Patch subject
> I think the patch subject should say "disable" instead of "disabling":
>
On Friday, November 12, 2021 1:09 PM vignesh C wrote:
> On Thu, Nov 11, 2021 at 2:50 PM osumi.takami...@fujitsu.com
> wrote:
> Thanks for the updated patch, Few comments:
> 1) tab completion should be added for disable_on_error:
> /* Complete "CREATE SUBSCRIPTION ... WITH ( " */ else if
>
On Thu, Nov 11, 2021 at 8:20 PM osumi.takami...@fujitsu.com
wrote:
>
> C codes are checked by pgindent.
>
> Note that this depends on the v20 skip xide patch in [1]
>
Some comments on the v4 patch:
(1) Patch subject
I think the patch subject should say "disable" instead of "disabling":
On Thu, Nov 11, 2021 at 2:50 PM osumi.takami...@fujitsu.com
wrote:
>
> On Wednesday, November 10, 2021 1:23 PM Greg Nancarrow
> wrote:
> > On Wed, Nov 10, 2021 at 12:26 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Monday, November 8, 2021 10:15 PM vignesh C
> > wrote:
> > > >
On Wednesday, November 10, 2021 1:23 PM Greg Nancarrow
wrote:
> On Wed, Nov 10, 2021 at 12:26 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Monday, November 8, 2021 10:15 PM vignesh C
> wrote:
> > > Thanks for the updated patch. Please create a Commitfest entry for
> > > this. It will
On Wed, Nov 10, 2021 at 3:22 PM Greg Nancarrow wrote:
>
> I had a look at this patch and have a couple of initial review
> comments for some issues I spotted:
>
Incidentally, I found that the v3 patch only applies after the skip xid v20
patch [1] has been applied.
[2] -
On Wed, Nov 10, 2021 at 12:26 PM osumi.takami...@fujitsu.com
wrote:
>
> On Monday, November 8, 2021 10:15 PM vignesh C wrote:
> > Thanks for the updated patch. Please create a Commitfest entry for this. It
> > will
> > help to have a look at CFBot results for the patch, also if required rebase
1 - 100 of 132 matches
Mail list logo
