On Tue, Jan 10, 2023 at 7:42 PM Takamichi Osumi (Fujitsu) <osumi.takami...@fujitsu.com> wrote: > > On Tuesday, January 3, 2023 4:01 PM vignesh C <vignes...@gmail.com> wrote: > Hi, thanks for your review ! > > > > 1) This global variable can be removed as it is used only in send_feedback > > which > > is called from maybe_delay_apply so we could pass it as a function argument: > > + * delay, avoid having positions of the flushed and apply LSN > > +overwritten by > > + * the latest LSN. > > + */ > > +static bool in_delaying_apply = false; > > +static XLogRecPtr last_received = InvalidXLogRecPtr; > > + > I have removed the first variable and make it one of the arguments for > send_feedback(). > > > 2) -1 gets converted to -1000 > > > > +int64 > > +interval2ms(const Interval *interval) > > +{ > > + int64 days; > > + int64 ms; > > + int64 result; > > + > > + days = interval->month * INT64CONST(30); > > + days += interval->day; > > + > > + /* Detect whether the value of interval can cause an overflow. */ > > + if (pg_mul_s64_overflow(days, MSECS_PER_DAY, &result)) > > + ereport(ERROR, > > + > > (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), > > + errmsg("bigint out of range"))); > > + > > + /* Adds portion time (in ms) to the previous result. */ > > + ms = interval->time / INT64CONST(1000); > > + if (pg_add_s64_overflow(result, ms, &result)) > > + ereport(ERROR, > > + > > (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), > > + errmsg("bigint out of range"))); > > > > create subscription sub7 connection 'dbname=regression host=localhost > > port=5432' publication pub1 with (min_apply_delay = '-1'); > > ERROR: -1000 ms is outside the valid range for parameter "min_apply_delay" > Good catch! Fixed in order to make input '-1' interpretted as -1 ms. > > > 3) This can be slightly reworded: > > + <para> > > + The length of time (ms) to delay the application of changes. > > + </para></entry> > > to: > > Delay applying the changes by a specified amount of time(ms). > This has been suggested in [1] by Peter Smith. So, I'd like to keep the > current patch's description. > Then, I didn't change this. > > > 4) maybe_delay_apply can be moved from apply_handle_stream_prepare to > > apply_spooled_messages so that it is consistent with > > maybe_start_skipping_changes: > > @@ -1120,6 +1240,19 @@ apply_handle_stream_prepare(StringInfo s) > > > > elog(DEBUG1, "received prepare for streamed transaction %u", > > prepare_data.xid); > > > > + /* > > + * Should we delay the current prepared transaction? > > + * > > + * Although the delay is applied in BEGIN PREPARE messages, > > streamed > > + * prepared transactions apply the delay in a STREAM PREPARE > > message. > > + * That's ok because no changes have been applied yet > > + * (apply_spooled_messages() will do it). The STREAM START message > > does > > + * not contain a prepare time (it will be available when the > > in-progress > > + * prepared transaction finishes), hence, it was not possible to > > apply a > > + * delay at that time. > > + */ > > + maybe_delay_apply(prepare_data.prepare_time); > > > > > > That way the call from apply_handle_stream_commit can also be removed. > Sounds good. I moved the call of maybe_delay_apply() to the > apply_spooled_messages(). > Now it's aligned with maybe_start_skipping_changes(). > > > 5) typo transfering should be transferring > > + publisher and the current time on the subscriber. Time > > spent in logical > > + decoding and in transfering the transaction may reduce the > > actual wait > > + time. If the system clocks on publisher and subscriber are > > + not > Fixed. > > > 6) feedbacks can be changed to feedback messages > > + * it's necessary to keep sending feedbacks during the delay from the > > + worker > > + * process. Meanwhile, the feature delays the apply before starting the > Fixed. > > > 7) > > + /* > > + * Suppress overwrites of flushed and writtten positions by the lastest > > + * LSN in send_feedback(). > > + */ > > > > 7a) typo writtten should be written > > > > 7b) lastest should latest > I have removed this sentence. So, those typos are removed. > > Please have a look at the updated patch. > > [1] - > https://www.postgresql.org/message-id/CAHut%2BPttQdFMNM2c6WqKt2c9G6r3ZKYRGHm04RR-4p4fyA4WRg%40mail.gmail.com > >
Hi, 1. + errmsg("min_apply_delay must not be set when streaming = parallel"))); we give the same error msg for both the cases: a. when subscription is created with streaming=parallel but we are trying to alter subscription to set min_apply_delay >0 b. when subscription is created with some min_apply_delay and we are trying to alter subscription to make it streaming=parallel. For case a, error msg looks fine but for case b, I think error msg should be changed slightly. ALTER SUBSCRIPTION regress_testsub SET (streaming = parallel); ERROR: min_apply_delay must not be set when streaming = parallel This gives the feeling that we are trying to modify min_apply_delay but we are not. Maybe we can change it to: "subscription with min_apply_delay must not be allowed to stream parallel" (or something better) thanks Shveta