RE: Support logical replication of DDLs
Hi On Tuesday, March 14, 2023 1:17 PM Ajin Cherian wrote: > I found out that the option ONLY was not parsed in the "CREATE INDEX" > command, for eg: CREATE UNIQUE INDEX ... ON ONLY table_name ... > > I've fixed this in patch 0002. FYI, cfbot reports a failure of v80 on linux [1]. Could you please check ? [17:39:49.745] == running regression test queries == [17:39:49.745] test test_ddl_deparse ... ok 27 ms [17:39:49.745] test create_extension ... ok 60 ms [17:39:49.745] test create_schema... ok 28 ms [17:39:49.745] test aggregate... ok 19 ms [17:39:49.745] test create_table ... FAILED 245 ms [17:39:49.745] test constraints ... FAILED 420 ms [17:39:49.745] test alter_table ... FAILED 448 ms [17:39:49.745] == shutting down postmaster == [17:39:49.745] [17:39:49.745] == [17:39:49.745] 3 of 7 tests failed. [17:39:49.745] == [17:39:49.745] [17:39:49.745] The differences that caused some tests to fail can be viewed in the [17:39:49.745] file "/tmp/cirrus-ci-build/src/test/modules/test_ddl_deparse_regress/regression.diffs". A copy of the test summary that you see [17:39:49.745] above is saved in the file "/tmp/cirrus-ci-build/src/test/modules/test_ddl_deparse_regress/regression.out". [1] - https://cirrus-ci.com/task/5420096810647552 Best Regards, Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
Hi, On Wednesday, March 15, 2023 2:34 PM Amit Kapila wrote: > On Tue, Mar 14, 2023 at 8:50 PM Takamichi Osumi (Fujitsu) > wrote: > > > > On Tuesday, March 14, 2023 8:02 PM Melih Mutlu > wrote: > > (3) copy_table() > > > > + /* > > +* If the publisher version is earlier than v14, it COPY command > doesn't > > +* support the binary option. > > +*/ > > > > This sentence doesn't look correct grammatically. We can replace "it COPY > command" with "subscription" for example. Kindly please fix it. > > > > How about something like: "The binary option for replication is supported > since > v14."? Yes, this looks best to me. I agree with this suggestion. Best Regards, Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
On Tuesday, March 14, 2023 8:02 PM Melih Mutlu wrote: > Attached v13. Hi, Thanks for sharing v13. Few minor review comments. (1) create_subscription.sgml + column types as opposed to text format. Even when this option is enabled, + only data types having binary send and receive functions will be + transferred in binary. Note that the initial synchronization requires (1-1) I think it's helpful to add a reference for the description about send and receive functions (e.g. to the page of CREATE TYPE). (1-2) Also, it would be better to have a cross reference from there to this doc as one paragraph probably in "Notes". I suggested this, because send and receive functions are described as "optional" there and missing them leads to error in the context of binary table synchronization. (3) copy_table() + /* +* If the publisher version is earlier than v14, it COPY command doesn't +* support the binary option. +*/ This sentence doesn't look correct grammatically. We can replace "it COPY command" with "subscription" for example. Kindly please fix it. Best Regards, Takamichi Osumi
RE: Rework LogicalOutputPluginWriterUpdateProgress
Hi, On Friday, March 10, 2023 6:32 PM Wang, Wei/王 威 wrote: > Attach the new patch set. Thanks for updating the patch ! One review comment on v7-0005. stream_start_cb_wrapper and stream_stop_cb_wrapper don't call the pair of threshold check and UpdateProgressAndKeepalive unlike other write wrapper functions like below. But, both of them write some data to the output plugin, set the flag of did_write and thus it updates the subscriber's last_recv_timestamp used for timeout check in LogicalRepApplyLoop. So, it looks adding the pair to both functions can be more accurate, in order to reset the counter in changes_count on the publisher ? @@ -1280,6 +1282,8 @@ stream_start_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn, /* Pop the error context stack */ error_context_stack = errcallback.previous; + + /* No progress has been made, so don't call UpdateProgressAndKeepalive */ } Best Regards, Takamichi Osumi
RE: Rework LogicalOutputPluginWriterUpdateProgress
Hi,
On Wednesday, March 8, 2023 11:54 AM From: wangw.f...@fujitsu.com
wrote:
> Attach the new patch.
Thanks for sharing v6 ! Few minor comments for the same.
(1) commit message
The old function name 'is_skip_threshold_change' is referred currently. We need
to update it to 'is_keepalive_threshold_exceeded' I think.
(2) OutputPluginPrepareWrite
@@ -662,7 +656,8 @@ void
OutputPluginPrepareWrite(struct LogicalDecodingContext *ctx, bool last_write)
{
if (!ctx->accept_writes)
- elog(ERROR, "writes are only accepted in commit, begin and
change callbacks");
+ elog(ERROR, "writes are only accepted in output plugin
callbacks, "
+"except startup, shutdown, filter_by_origin, and
filter_prepare.");
We can remove the period at the end of error string.
(3) is_keepalive_threshold_exceeded's comments
+/*
+ * Helper function to check whether a large number of changes have been skipped
+ * continuously.
+ */
+static bool
+is_keepalive_threshold_exceeded(LogicalDecodingContext *ctx)
I suggest to update the comment slightly something like below.
From:
...whether a large number of changes have been skipped continuously
To:
...whether a large number of changes have been skipped without being sent to
the output plugin continuously
(4) term for 'keepalive'
+/*
+ * Update progress tracking and send keep alive (if required).
+ */
The 'keep alive' might be better to be replaced with 'keepalive', which looks
commonest in other source codes. In the current patch, there are 3 different
ways to express it (the other one is 'keep-alive') and it would be better to
unify the term, at least within the same patch ?
Best Regards,
Takamichi Osumi
RE: Rework LogicalOutputPluginWriterUpdateProgress
Hi,
On Monday, February 27, 2023 6:30 PM wangw.f...@fujitsu.com
wrote:
> Attach the new patch.
Thanks for sharing v3. Minor review comments and question.
(1) UpdateDecodingProgressAndKeepalive header comment
The comment should be updated to explain maybe why we reset some other flags as
discussed in [1] and the functionality to update and keepalive of the function
simply.
(2) OutputPluginPrepareWrite
Probably the changed error string is too wide.
@@ -662,7 +657,7 @@ void
OutputPluginPrepareWrite(struct LogicalDecodingContext *ctx, bool last_write)
{
if (!ctx->accept_writes)
- elog(ERROR, "writes are only accepted in commit, begin and
change callbacks");
+ elog(ERROR, "writes are only accepted in callbacks in the
OutputPluginCallbacks structure (except startup, shutdown, filter_by_origin and
filter_prepare callbacks)");
I thought you can break the error message into two string lines. Or, you can
rephrase it to different expression.
(3) Minor question
The patch introduced the goto statements into the cb_wrapper functions. Is the
purpose to call the update_progress_and_keepalive after pop the error stack,
even if the corresponding callback is missing ? I thought we can just have
"else" clause for the check of the existence of callback, but did you choose
the current goto style for readability ?
(4) Name of is_skip_threshold_change
I also feel the name of is_skip_threshold_change can be changed to
"exceeded_keepalive_threshold" or something. Other candidates are proposed by
Peter-san in [2].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB6275374EBE7C8CABBE6730099EAF9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[2] -
https://www.postgresql.org/message-id/CAHut%2BPt3ZEMo-KTF%3D5KJSU%2BHdWJD19GPGGCKOmBeM47484Ychw%40mail.gmail.com
Best Regards,
Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
On Monday, February 20, 2023 8:47 PM Melih Mutlu wrote:
> Thanks for letting me know.
> Attached the fixed version of the patch.
Hi, Melih
Thanks for updating the patch. Minor comments on v9.
(1) commit message
"The patch introduces a new parameter, copy_format, to CREATE SUBSCRIPTION to
allow to choose
the format used in initial table synchronization."
This patch introduces the new parameter not only to CREATE SUBSCRIPTION and
ALTER SUBSCRIPTION, then this description should be more general, something
like below.
"The patch introduces a new parameter, copy_format, as subscription option to
allow user to choose the format of initial table synchronization."
(2) copy_table
We don't need to check the publisher's version below.
+
+ /* If the publisher is v16 or later, specify the format to copy data. */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 16)
+ {
+ char *format = MySubscription->copyformat ==
LOGICALREP_COPY_AS_BINARY ? "binary" : "text";
+ appendStringInfo(, " WITH (FORMAT %s)", format);
+ options = lappend(options, makeDefElem("format", (Node *)
makeString(format), -1));
+ }
+
We don't have this kind of check for "binary" option and it seems this is
user's responsibility to avoid any errors during replication. If we want to add
this kind of check, then we can add checks for both "binary" and "copy_format"
option together as an independent patch.
(3) subscription.sql/out
The other existing other subscription options check the invalid input for newly
created option (e.g. "foo" for disable_on_error, streaming mode). So, I think
we can add this type of test for this feature.
Best Regards,
Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
Hi, Melih On Monday, January 30, 2023 7:50 PM Melih Mutlu wrote: > Thanks for reviewing. ... > Well, with this patch, it will begin to fail in the table copy phase... The latest patch doesn't get updated for more than two weeks after some review comments. If you don't have time, I would like to help updating the patch for you and other reviewers. Kindly let me know your status. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Andres-san On Tuesday, February 14, 2023 1:47 AM Andres Freund wrote: > On 2023-02-11 05:44:47 +0000, Takamichi Osumi (Fujitsu) wrote: > > On Saturday, February 11, 2023 11:10 AM Andres Freund > wrote: > > > Has there been any discussion about whether this is actually best > > > implemented on the client side? You could alternatively implement it > > > on the sender. > > > > > > That'd have quite a few advantages, I think - you e.g. wouldn't > > > remove the ability to *receive* and send feedback messages. We'd > > > not end up filling up the network buffer with data that we'll not process > anytime soon. > > Thanks for your comments ! > > > > We have discussed about the publisher side idea around here [1] but, > > we chose the current direction. Kindly have a look at the discussion. > > > > If we apply the delay on the publisher, then it can lead to extra > > delay where we don't need to apply. > > The current proposed approach can take other loads or factors > > (network, busyness of the publisher, etc) into account because it > > calculates the required delay on the subscriber. > > I don't think it's OK to just loose the ability to read / reply to keepalive > messages. > > I think as-is we seriously consider to just reject the feature, adding too > much > complexity, without corresponding gain. Thanks for your comments ! Could you please tell us about your concern a bit more? The keepalive/reply messages are currently used for two purposes, (a) send the updated wrte/flush/apply locations; (b) avoid timeouts incase of idle times. Both the cases shouldn't be impacted with this time-delayed LR patch because during the delay there won't be any progress and to avoid timeouts, we allow to send the alive message during the delay. This is just we would like to clarify the issue you have in mind. OTOH, if we want to implement the functionality on publisher-side, I think we need to first consider the interface. We can think of two options (a) Have it as a subscription parameter as the patch has now and then pass it as an option to the publisher which it will use to delay; (b) Have it defined on publisher-side, say via GUC or some other way. The basic idea could be that while processing commit record (in DecodeCommit), we can somehow check the value of delay and then use it there to delay sending the xact. Also, during delay, we need to somehow send the keepalive and process replies, probably via a new callback or by some existing callback. We also need to handle in-progress and 2PC xacts in a similar way. For the former, probably we would need to apply the delay before sending the first stream. Could you please share what you feel on this direction as well ? Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Horiguchi-san On Monday, February 13, 2023 10:26 AM Kyotaro Horiguchi wrote: > At Fri, 10 Feb 2023 10:34:49 +0530, Amit Kapila > wrote in > > On Fri, Feb 10, 2023 at 10:11 AM Amit Kapila > wrote: > > > > > > On Fri, Feb 10, 2023 at 6:27 AM Kyotaro Horiguchi > > > wrote: > > > > We have suffered this kind of feedback silence many times. Thus I > > > > don't want to rely on luck here. I had in mind of exposing > > > > last_send itself or providing interval-calclation function to the logic. > > > > > > I think we have last_send time in send_feedback(), so we can expose > > > it if we want but how would that solve the problem you are worried > about? > > Wal receiver can avoid a too-long sleep by knowing when to wake up for the > next feedback. > > > I have an idea to use last_send time to avoid walsenders being > > timeout. Instead of directly using wal_receiver_status_interval as a > > minimum interval to send the feedback, we should check if it is > > greater than last_send time then we should send the feedback after > > (wal_receiver_status_interval - last_send). I think they can probably > > be different only on the very first time. Any better ideas? > > If it means MyLogicalRepWorker->last_send_time, it is not the last time when > walreceiver sent a feedback but the last time when > wal*sender* sent a data. So I'm not sure that works. > > We could use the variable that way, but AFAIU in turn when so many changes > have been spooled that the control doesn't return to LogicalRepApplyLoop > longer than wal_r_s_interval, maybe_apply_delay() starts calling > send_feedback() at every call after the first feedback timing. Even in that > case, send_feedback() won't send one actually until the next feedback timing, > I don't think that behavior is great. > > The only packets walreceiver sends back is the feedback packets and > currently only send_feedback knows the last feedback time. Thanks for your comments ! As described in your last sentence, in the latest patch v34 [1], we use the last time set in send_feedback() and based on it, we calculate and adjust the first timing of feedback message in maybe_apply_delay() so that we can send the feedback message following the interval of wal_receiver_status_interval. I wasn't sure if the above concern is still valid for this implementation. Could you please have a look at the latest patch and share your opinion ? [1] - https://www.postgresql.org/message-id/TYCPR01MB83736C50C98CB2153728A7A8EDDE9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi On Saturday, February 11, 2023 11:10 AM Andres Freund wrote: > On 2023-02-10 11:26:21 +0000, Takamichi Osumi (Fujitsu) wrote: > > Subject: [PATCH v34] Time-delayed logical replication subscriber > > > > Similar to physical replication, a time-delayed copy of the data for > > logical replication is useful for some scenarios (particularly to fix > > errors that might cause data loss). > > > > This patch implements a new subscription parameter called > 'min_apply_delay'. > Has there been any discussion about whether this is actually best > implemented on the client side? You could alternatively implement it on the > sender. > > That'd have quite a few advantages, I think - you e.g. wouldn't remove the > ability to *receive* and send feedback messages. We'd not end up filling up > the network buffer with data that we'll not process anytime soon. Thanks for your comments ! We have discussed about the publisher side idea around here [1] but, we chose the current direction. Kindly have a look at the discussion. If we apply the delay on the publisher, then it can lead to extra delay where we don't need to apply. The current proposed approach can take other loads or factors (network, busyness of the publisher, etc) into account because it calculates the required delay on the subscriber. [1] - https://www.postgresql.org/message-id/20221215.105200.268327207020006785.horikyota.ntt%40gmail.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, February 10, 2023 2:05 PM Friday, February 10, 2023 2:05 PM wrote: > On Fri, Feb 10, 2023 at 10:11 AM Amit Kapila > wrote: > > > > On Fri, Feb 10, 2023 at 6:27 AM Kyotaro Horiguchi > > wrote: > > > > > > At Thu, 9 Feb 2023 13:48:52 +0530, Amit Kapila > > > wrote in > > > > On Thu, Feb 9, 2023 at 10:45 AM Kyotaro Horiguchi > > > > wrote: > > > > > > > > > > At Wed, 8 Feb 2023 09:03:03 +, "Hayato Kuroda (Fujitsu)" > > > > > wrote in > > > > > > Thank you for reviewing! PSA new version. > > > > > > > > > > + if (statusinterval_ms > 0 && diffms > > > > > > + statusinterval_ms) > > > > > > > > > > The next expected feedback time is measured from the last status > > > > > report. Thus, it seems to me this may suppress feedbacks from > > > > > being sent for an unexpectedly long time especially when > > > > > min_apply_delay is shorter than wal_r_s_interval. > > > > > > > > > > > > > I think the minimum time before we send any feedback during the > > > > wait is wal_r_s_interval. Now, I think if there is no transaction > > > > for a long time before we get a new transaction, there should be > > > > keep-alive messages in between which would allow us to send > > > > feedback at regular intervals (wal_receiver_status_interval). So, > > > > I think we should be > > > > > > Right. > > > > > > > able to send feedback in less than 2 * > > > > wal_receiver_status_interval unless wal_sender/receiver timeout is > > > > very large and there is a very low volume of transactions. Now, we > > > > can try to send the feedback > > > > > > We have suffered this kind of feedback silence many times. Thus I > > > don't want to rely on luck here. I had in mind of exposing last_send > > > itself or providing interval-calclation function to the logic. > > > > > > > I think we have last_send time in send_feedback(), so we can expose it > > if we want but how would that solve the problem you are worried about? > > > > I have an idea to use last_send time to avoid walsenders being timeout. > Instead of directly using wal_receiver_status_interval as a minimum interval > to send the feedback, we should check if it is greater than last_send time > then we should send the feedback after (wal_receiver_status_interval - > last_send). I think they can probably be different only on the very first > time. > Any better ideas? This idea sounds good to me and implemented this idea in an attached patch v34. In the previous patch, we couldn't solve the timeout of the publisher, when we conduct a scenario suggested by Horiguchi-san and reproduced in the scenario attached test file 'test.sh'. But now we handle it by adjusting the timing of the first wait time. FYI, we thought to implement the new variable 'send_time' in the LogicalRepWorker structure at first. But, this structure is used when launcher controls workers or reports statistics and it stores TimestampTz recorded in the received WAL, so not sure if the struct is the right place to implement the variable. Moreover, there are other similar variables such as last_recv_time or reply_time. So, those will be confusing when we decide to have new variable together. Then, it's declared separately. The new patch also includes some changes for wait event. Kindly have a look at the v34 patch. Best Regards, Takamichi Osumi v34-0001-Time-delayed-logical-replication-subscriber.patch Description: v34-0001-Time-delayed-logical-replication-subscriber.patch test.sh Description: test.sh
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Thursday, February 9, 2023 4:56 PM Amit Kapila
wrote:
> On Thu, Feb 9, 2023 at 12:17 AM Peter Smith
> wrote:
> >
> > On Wed, Feb 8, 2023 at 8:03 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > ...
> > > > ==
> > > >
> > > > src/backend/replication/logical/worker.c
> > > >
> > > > 2. maybe_apply_delay
> > > >
> > > > + if (wal_receiver_status_interval > 0 && diffms >
> > > > + wal_receiver_status_interval * 1000L) { WaitLatch(MyLatch,
> > > > + WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > > > + wal_receiver_status_interval * 1000L,
> > > > + WAIT_EVENT_RECOVERY_APPLY_DELAY);
> send_feedback(last_received,
> > > > + true, false, true); }
> > > >
> > > > I felt that introducing another variable like:
> > > >
> > > > long statusinterval_ms = wal_receiver_status_interval * 1000L;
> > > >
> > > > would help here by doing 2 things:
> > > > 1) The condition would be easier to read because the ms units
> > > > would be the same
> > > > 2) Won't need * 1000L repeated in two places.
> > > >
> > > > Only, do take care to assign this variable in the right place in
> > > > this loop in case the configuration is changed.
> > >
> > > Fixed. Calculations are done on two lines - first one is the
> > > entrance of the loop, and second one is the after SIGHUP is detected.
> > >
> >
> > TBH, I expected you would write this as just a *single* variable
> > assignment before the condition like below:
> >
> > SUGGESTION (tweaked comment and put single assignment before
> > condition)
> > /*
> > * Call send_feedback() to prevent the publisher from exiting by
> > * timeout during the delay, when the status interval is greater than
> > * zero.
> > */
> > status_interval_ms = wal_receiver_status_interval * 1000L; if
> > (status_interval_ms > 0 && diffms > status_interval_ms) { ...
> >
> > ~
> >
> > I understand in theory, your code is more efficient, but in practice,
> > I think the overhead of a single variable assignment every loop
> > iteration (which is doing WaitLatch anyway) is of insignificant
> > concern, whereas having one assignment is simpler than having two IMO.
> >
>
> Yeah, that sounds better to me as well.
OK, fixed.
The comment adjustment suggested by Peter-san above
was also included in this v33.
Please have a look at the attached patch.
Best Regards,
Takamichi Osumi
v33-0001-Time-delayed-logical-replication-subscriber.patch
Description: v33-0001-Time-delayed-logical-replication-subscriber.patch
RE: Exit walsender before confirming remote flush in logical replication
On Wednesday, February 8, 2023 6:47 PM Hayato Kuroda (Fujitsu) wrote: > PSA rebased patch that supports updated time-delayed patch[1]. Hi, Thanks for creating the patch ! Minor review comments on v5-0002. (1) + Decides the condition for exiting the walsender process. + 'wait_flush', which is the default, the walsender + will wait for all the sent WALs to be flushed on the subscriber side, + before exiting the process. 'immediate' will exit + without confirming the remote flush. This may break the consistency + between publisher and subscriber, but it may be useful for a system + that has a high-latency network to reduce the amount of time for + shutdown. (1-1) The first part "exiting the walsender process" can be improved. Probably, you can say "the exiting walsender process" or "Decides the behavior of the walsender process at shutdown" instread. (1-2) Also, the next sentence can be improved something like "If the shutdown mode is wait_flush, which is the default, the walsender waits for all the sent WALs to be flushed on the subscriber side. If it is immediate, the walsender exits without confirming the remote flush". (1-3) We don't need to wrap wait_flush and immediate by single quotes within the literal tag. (2) + /* minapplydelay affects SHUTDOWN_MODE option */ I think we can move this comment to just above the 'if' condition and combine it with the existing 'if' conditions comments. (3) 001_rep_changes.pl (3-1) Question In general, do we add this kind of check when we extend the protocol (STREAM_REPLICATION command) or add a new condition for apply worker exit ? In case when we would like to know the restart of the walsender process in TAP tests, then could you tell me why the new test code matches the purpose of this patch ? (3-2) + "Timed out while waiting for apply to restart after changing min_apply_delay to non-zero value"; Probably, we can partly change this sentence like below, because we check walsender's pid. FROM: "... while waiting for apply to restart..." TO: "... while waiting for the walsender to restart..." Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Horiguchi-san Thanks for your review ! On Tuesday, February 7, 2023 1:43 PM From: Kyotaro Horiguchi wrote: > At Mon, 6 Feb 2023 13:10:01 +, "Takamichi Osumi (Fujitsu)" > wrote in > subscriptioncmds.c > > + if (opts.streaming == > LOGICALREP_STREAM_PARALLEL && > + !IsSet(opts.specified_opts, > SUBOPT_MIN_APPLY_DELAY) && > +sub->minapplydelay > 0) > .. > + if (opts.min_apply_delay > 0 && > + !IsSet(opts.specified_opts, > SUBOPT_STREAMING) && sub->stream == > +LOGICALREP_STREAM_PARALLEL) > > Don't we wrap the lines? Yes, those lines should have looked nicer. Updated. Kindly have a look at the latest patch v31 in [1]. There are also other some changes in the patch. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373BA483A6D2C924C600968EDDB9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Tuesday, February 7, 2023 2:26 PM Amit Kapila
wrote:
> On Tue, Feb 7, 2023 at 10:13 AM Kyotaro Horiguchi
> wrote:
> >
> > At Mon, 6 Feb 2023 13:10:01 +, "Takamichi Osumi (Fujitsu)"
> > wrote in
> > > The attached patch v29 has included your changes.
> >
> > catalogs.sgml
> >
> > +
> > + The minimum delay (ms) for applying changes.
> > +
> >
> > I think we don't use unit symbols that way. Namely I think we would
> > write it as "The minimum delay for applying changes in milliseconds"
> >
>
> Okay, if we prefer to use milliseconds, then how about: "The minimum delay, in
> milliseconds, for applying changes"?
This looks good to me. Adopted.
>
> >
> > alter_subscription.sgml
> >
> >are slot_name,
> >synchronous_commit,
> >binary, streaming,
> > - disable_on_error, and
> > - origin.
> > + disable_on_error,
> > + origin, and
> > + min_apply_delay.
> >
> >
> > By the way, is there any rule for the order among the words?
> >
>
> Currently, it is in the order in which the corresponding features are added.
Yes. So, I keep it as it is.
>
> > They
> > don't seem in alphabetical order nor in the same order to the
> > create_sbuscription page.
> >
>
> In create_subscription page also, it appears to be in the order in which those
> are added with a difference that they are divided into two categories
> (parameters that control what happens during subscription creation and
> parameters that control the subscription's replication behavior after it has
> been
> created)
Same as here. The current order should be fine.
>
> > (I seems like in the order of SUBOPT_* symbols, but I'm not sure it's
> > a good idea..)
> >
> >
> > subscriptioncmds.c
> >
> > + if (opts.streaming ==
> LOGICALREP_STREAM_PARALLEL &&
> > +
> > + !IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> > + sub->minapplydelay > 0)
> > ..
> > + if (opts.min_apply_delay > 0 &&
> > +
> > + !IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream ==
> > + LOGICALREP_STREAM_PARALLEL)
> >
> > Don't we wrap the lines?
> >
> >
> > worker.c
> >
> > + if (wal_receiver_status_interval > 0 &&
> > + diffms > wal_receiver_status_interval * 1000L)
> > + {
> > + WaitLatch(MyLatch,
> > + WL_LATCH_SET |
> WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > + wal_receiver_status_interval *
> 1000L,
> > +
> WAIT_EVENT_RECOVERY_APPLY_DELAY);
> > + send_feedback(last_received, true, false, true);
> > + }
> > + else
> > + WaitLatch(MyLatch,
> > + WL_LATCH_SET |
> WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > + diffms,
> > +
> > + WAIT_EVENT_RECOVERY_APPLY_DELAY);
> >
> > send_feedback always handles the case where
> > wal_receiver_status_interval == 0.
> >
>
> It only handles when force is false but here we are using that as true. So,
> not
> sure, if what you said would be an improvement.
Agreed. So, I keep it as it is.
>
> > thus we can simply wait for
> > min(wal_receiver_status_interval, diffms) then call send_feedback()
> > unconditionally.
> >
> >
> > -start_apply(XLogRecPtr origin_startpos)
> > +start_apply(void)
> >
> > -LogicalRepApplyLoop(XLogRecPtr last_received)
> > +LogicalRepApplyLoop(void)
> >
> > Does this patch requires this change?
> >
>
> I think this is because the scope of last_received has been changed so that it
> can be used to pass in send_feedback() during the delay.
Yes, that's our intention.
Kindly have a look at the latest patch v31 shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373BA483A6D2C924C600968EDDB9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Tuesday, February 7, 2023 6:56 PM Amit Kapila
wrote:
> On Tue, Feb 7, 2023 at 8:22 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Thank you for reviewing! PSA new version.
> >
>
> Few comments:
> =
Thanks for your comments !
> 1.
> @@ -74,6 +74,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
> BKI_SHARED_RELATION BKI_ROW
>
> Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */
>
> + int32 subminapplydelay; /* Replication apply delay (ms) */
> +
> bool subenabled; /* True if the subscription is enabled (the
> * worker should be running) */
>
> @@ -120,6 +122,7 @@ typedef struct Subscription
> * in */
> XLogRecPtr skiplsn; /* All changes finished at this LSN are
> * skipped */
> + int32 minapplydelay; /* Replication apply delay (ms) */
> char*name; /* Name of the subscription */
> Oid owner; /* Oid of the subscription owner */
>
> Why the new parameter is placed at different locations in above two
> strcutures? I think it should be after owner in both cases and accordingly its
> order should be changed in GetSubscription() or any other place it is used.
Fixed.
>
> 2. A minor comment change suggestion:
> /*
> * Common spoolfile processing.
> *
> - * The commit/prepare time (finish_ts) for streamed transactions is required
> - * for time-delayed logical replication.
> + * The commit/prepare time (finish_ts) is required for time-delayed
> + logical
> + * replication.
> */
Fixed.
> 3. I find the newly added tests take about 8s on my machine which is close
> highest in the subscription folder. I understand that it can't be less than 3s
> because of the delay but checking multiple cases makes it take that long. I
> think we can avoid the tests for streaming and disable the subscription. Also,
> after removing those, I think it would be better to add the remaining test in
> 001_rep_changes to save set-up and tear-down costs as well.
Sounds good to me. Moved the test to 001_rep_changes.pl.
> 4.
> +$node_publisher->append_conf('postgresql.conf',
> + 'logical_decoding_work_mem = 64kB');
>
> I think this setting is also not required.
Yes. And, in the process to move the test, removed.
Attached the v31 patch.
Note that regarding the translator style,
I chose to export the parameters from the errmsg to outside
at this stage. If there is a need to change it, then I'll follow it.
Other changes are minor alignments to make 'if' conditions
that exceeded 80 characters folded and look nicer.
Also conducted pgindent and pgperltidy.
Best Regards,
Takamichi Osumi
v31-0001-Time-delayed-logical-replication-subscriber.patch
Description: v31-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Monday, February 6, 2023 8:57 PM Amit Kapila wrote: > On Tue, Jan 24, 2023 at 5:02 AM Euler Taveira wrote: > > > > > > - elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, > write %X/%X, flush %X/%X in-delayed: %d", > > + elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, write > > + %X/%X, flush %X/%X, apply delay: %s", > > force, > > LSN_FORMAT_ARGS(recvpos), > > LSN_FORMAT_ARGS(writepos), > > LSN_FORMAT_ARGS(flushpos), > > -in_delayed_apply); > > +in_delayed_apply? "yes" : "no"); > > > > It is better to use a string to represent the yes/no option. > > > > I think it is better to be consistent with the existing force parameter which > is > also boolean, otherwise, it will look odd. Agreed. The latest patch v29 posted in [1] followed this suggestion. Kindly have a look at it. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373A59E7B74AA4F96B62BEAEDDA9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, February 6, 2023 8:51 PM Amit Kapila wrote: > On Mon, Feb 6, 2023 at 12:36 PM Takamichi Osumi (Fujitsu) > wrote: > > > > I have made a couple of changes in the attached: (a) changed a few error and > LOG messages; (a) added/changed comments. See, if these look good to you > then please include them in the next version. Hi, thanks for sharing the patch ! The proposed changes make comments easier to understand and more aligned with other existing comments. So, LGTM. The attached patch v29 has included your changes. Best Regards, Takamichi Osumi v29-0001-Time-delayed-logical-replication-subscriber.patch Description: v29-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, February 6, 2023 12:03 PM Peter Smith wrote:
> On Sat, Feb 4, 2023 at 5:04 PM Takamichi Osumi (Fujitsu)
> wrote:
> >
> ...
> >
> > Kindly have a look at the attached v27.
> >
>
> Here are some review comments for patch v27-0001.
Thanks for checking !
> ==
> src/test/subscription/t/032_apply_delay.pl
>
> 1.
> +# Confirm the time-delayed replication has been effective from the
> +server log # message where the apply worker emits for applying delay.
> +Moreover, verify # that the current worker's remaining wait time is
> +sufficiently bigger than the # expected value, in order to check any update
> of
> the min_apply_delay.
> +sub check_apply_delay_log
>
> ~
>
> "has been effective from the server log" --> "worked, by inspecting the server
> log"
Sounds good to me. Also,
this is an unique part for time-delayed logical replication.
So, we can update those as we want. Fixed.
> ~~~
>
> 2.
> +my $delay = 3;
>
> Might be better to name this variable as 'min_apply_delay'.
I named this variable by following the test of recovery_min_apply_delay
(src/test/recovery/005_replay_delay.pl). So, this is aligned
with the test and I'd like to keep it as it is.
> ~~~
>
> 3.
> +# Now wait for replay to complete on publisher. We're done waiting when
> +the # subscriber has applyed up to the publisher LSN.
> +$node_publisher->wait_for_catchup($appname);
>
> 3a.
> Something seemed wrong with the comment.
>
> Was it meant to say more like? "The publisher waits for the replication to
> complete".
>
> Typo: "applyed"
Your wording looks better than mine. Fixed.
> ~
>
> 3b.
> Instead of doing this wait_for_catchup stuff why don't you just use a
> synchronous pub/sub and then the publication will just block internally like
> you require but without you having to block using test code?
This is the style of 005_reply_delay.pl. Then, this is also aligned with it.
So, I'd like to keep the current way of times comparison as it is.
Even if we could omit wait_for_catchup(), there will be new codes
for synchronous replication and that would make the min_apply_delay tests
more different from the corresponding one. Note that if we use
the synchronous mode, we need to turn it off for the last
ALTER SUBSCRIPTION DISABLE test case whose min_apply_delay to 1 day 5 min
and execute one record insert after that. This will make the tests confusing.
> ~~~
>
> 4.
> +# Run a query to make sure that the reload has taken effect.
> +$node_publisher->safe_psql('postgres', q{SELECT 1});
>
> SUGGESTION (for the comment)
> # Running a dummy query causes the config to be reloaded.
Fixed.
> ~~~
>
> 5.
> +# Confirm the record is not applied expectedly my $result =
> +$node_subscriber->safe_psql('postgres',
> + "SELECT count(a) FROM tab_int WHERE a = 0;"); is($result, qq(0),
> +"check the delayed transaction was not applied");
>
> "expectedly" ??
>
> SUGGESTION (for comment)
> # Confirm the record was not applied
Fixed.
Best Regards,
Takamichi Osumi
v28-0001-Time-delayed-logical-replication-subscriber.patch
Description: v28-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On wangw.f...@fujitsu.com Amit Kapila wrote: > On Fri, Feb 3, 2023 at 3:12 PM wangw.f...@fujitsu.com > wrote: > > > > Here is a comment: > > > > 1. The checks in function AlterSubscription > > + /* > > +* The combination of parallel > streaming mode and > > +* min_apply_delay is not > allowed. See > > +* parse_subscription_options > for details of the reason. > > +*/ > > + if (opts.streaming == > LOGICALREP_STREAM_PARALLEL) > > + if > ((IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) && > opts.min_apply_delay > 0) || > > + > > + (!IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) && > > + sub->minapplydelay > 0)) > > and > > + /* > > +* The combination of parallel > streaming mode and > > +* min_apply_delay is not > allowed. > > +*/ > > + if (opts.min_apply_delay > 0) > > + if > ((IsSet(opts.specified_opts, SUBOPT_STREAMING) && opts.streaming == > LOGICALREP_STREAM_PARALLEL) || > > + > > + (!IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream == > > + LOGICALREP_STREAM_PARALLEL)) > > > > I think the case where the options "min_apply_delay>0" and > "streaming=parallel" > > are set at the same time seems to have been checked in the function > > parse_subscription_options, how about simplifying these two > > if-statements here to the following: > > ``` > > if (opts.streaming == LOGICALREP_STREAM_PARALLEL && > > !IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) && > > sub->minapplydelay > 0) > > > > and > > > > if (opts.min_apply_delay > 0 && > > !IsSet(opts.specified_opts, SUBOPT_STREAMING) && > > sub->stream == LOGICALREP_STREAM_PARALLEL) ``` > > > > Won't just checking if ((opts.streaming == > LOGICALREP_STREAM_PARALLEL && sub->minapplydelay > 0) || > (opts.min_apply_delay > 0 && sub->stream == > LOGICALREP_STREAM_PARALLEL)) be sufficient in that case? We need checks for !IsSet(). If we don't have those, we error out when executing the alter subscription with min_apply_delay = 0 and streaming = parallel, at the same time for a subscription whose min_apply_delay setting is bigger than 0, for instance. In this case, we pass (don't error out) parse_subscription_options()'s test for the combination of mutual exclusive options and then, error out the condition by matching the first condition opts.streaming == parallel and sub->minapplydelay > 0 above. Also, the Wang-san's refactoring proposal makes sense. Adopted. Regarding the style how to write min_apply_delay > 0 (or just putting min_apply_delay in 'if' conditions) for checking parameters, I agreed with Amit-san so I keep them as it is in the latest patch v27. Kindly have a look at v27 posted in [1] [1] - https://www.postgresql.org/message-id/TYCPR01MB83738F2BEF83DE525410E3ACEDD49%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, February 3, 2023 3:35 PM I wrote: > On Friday, February 3, 2023 2:21 PM Amit Kapila > wrote: > > On Fri, Feb 3, 2023 at 6:41 AM Peter Smith > wrote: > > > On Thu, Feb 2, 2023 at 7:21 PM Takamichi Osumi (Fujitsu) > > > wrote: > > > ... > > > > > Besides, I am not sure it's a stable test to check the log. Is > > > > > it possible that there's no such log on a slow machine? I > > > > > modified the code to sleep 1s at the beginning of > > > > > apply_dispatch(), then the new added test failed because the server > log cannot match. > > > > To get the log by itself is necessary to ensure that the delay is > > > > conducted by the apply worker, because we emit the diffms only if > > > > it's bigger than 0 in maybe_apply_delay(). If we omit the step, we > > > > are not sure the delay is caused by other reasons or the > > > > time-delayed > > feature. > > > > > > > > As you mentioned, it's possible that no log is emitted on slow > > > > machine. Then, the idea to make the test safer for such machines > > > > should > > be to make the delayed time longer. > > > > But we shortened the delay time to 1 second to mitigate the long > > > > test > > execution time of this TAP test. > > > > So, I'm not sure if it's a good idea to make it longer again. > > > > > > I think there are a couple of things that can be done about this problem: > > > > > > 1. If you need the code/test to remain as-is then at least the test > > > message could include some comforting text like "(this can fail on > > > slow machines when the delay time is already exceeded)" so then a > > > test failure will not cause undue alarm. > > > > > > 2. Try moving the DEBUG2 elog (in function maybe_apply_delay) so > > > that it will *always* log the remaining wait time even if that wait > > > time becomes negative. Then I think the test cases can be made > > > deterministic instead of relying on good luck. This seems like the > > > better option. > > > > > > > I don't understand why we have to do any of this instead of using 3s > > as min_apply_delay similar to what we are doing in > > src/test/recovery/t/005_replay_delay. Also, I think we should use > > exactly the same way to verify the test even though we want to keep > > the log level as > > DEBUG2 to check logs in case of any failures. > OK, will try to make our tests similar to the tests in 005_replay_delay as > much > as possible. I've updated the TAP test and made it aligned with 005_reply_delay.pl. For coverage, I have the stream of in-progress transaction test case and ALTER SUBSCRIPTION DISABLE behavior, which is unique to logical replication. Also, conducted pgindent and pgperltidy. Note that the latter half of the 005_reply_delay.pl doesn't seem to match with the test for time-delayed logical replication (e.g. promotion). So, I don't have those points. Kindly have a look at the attached v27. Best Regards, Takamichi Osumi v27-0001-Time-delayed-logical-replication-subscriber.patch Description: v27-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, February 3, 2023 2:21 PM Amit Kapila wrote: > On Fri, Feb 3, 2023 at 6:41 AM Peter Smith wrote: > > On Thu, Feb 2, 2023 at 7:21 PM Takamichi Osumi (Fujitsu) > > wrote: > > > > > ... > > > > Besides, I am not sure it's a stable test to check the log. Is it > > > > possible that there's no such log on a slow machine? I modified > > > > the code to sleep 1s at the beginning of apply_dispatch(), then > > > > the new added test failed because the server log cannot match. > > > To get the log by itself is necessary to ensure that the delay is > > > conducted by the apply worker, because we emit the diffms only if > > > it's bigger than 0 in maybe_apply_delay(). If we omit the step, we > > > are not sure the delay is caused by other reasons or the time-delayed > feature. > > > > > > As you mentioned, it's possible that no log is emitted on slow > > > machine. Then, the idea to make the test safer for such machines should > be to make the delayed time longer. > > > But we shortened the delay time to 1 second to mitigate the long test > execution time of this TAP test. > > > So, I'm not sure if it's a good idea to make it longer again. > > > > I think there are a couple of things that can be done about this problem: > > > > 1. If you need the code/test to remain as-is then at least the test > > message could include some comforting text like "(this can fail on > > slow machines when the delay time is already exceeded)" so then a test > > failure will not cause undue alarm. > > > > 2. Try moving the DEBUG2 elog (in function maybe_apply_delay) so that > > it will *always* log the remaining wait time even if that wait time > > becomes negative. Then I think the test cases can be made > > deterministic instead of relying on good luck. This seems like the > > better option. > > > > I don't understand why we have to do any of this instead of using 3s as > min_apply_delay similar to what we are doing in > src/test/recovery/t/005_replay_delay. Also, I think we should use exactly the > same way to verify the test even though we want to keep the log level as > DEBUG2 to check logs in case of any failures. OK, will try to make our tests similar to the tests in 005_replay_delay as much as possible. > Also, I don't see the need to add more tests like the ones below: > +# Test whether ALTER SUBSCRIPTION changes the delayed time of the apply > +worker # (1 day 5 minutes). Note that the extra 5 minute is to account > +for any # decoding/network overhead. > > Let's try to add tests similar to what we have for recovery_min_apply_delay > unless there is some functionality in this patch that is not there in the > recovery_min_apply_delay feature. The above command is a preparation part to check a behavior unique to time-delayed logical replication, which is to DISABLE a subscription causes the apply worker not to apply the suspended (delayed) transaction. So, it will be OK to have this test. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Wednesday, February 1, 2023 6:41 PM Shi, Yu/侍 雨 wrote: > On Mon, Jan 30, 2023 6:05 PM Takamichi Osumi (Fujitsu) > wrote: > > > > On Saturday, January 28, 2023 1:28 PM I wrote: > > > Attached the updated patch v24. > > Hi, > > > > > > I've conducted the rebase affected by the commit(1e8b61735c) by > > renaming the GUC to logical_replication_mode accordingly, because it's > > utilized in the TAP test of this time-delayed LR feature. > > There is no other change for this version. > > > > Kindly have a look at the attached v25. > > > > Thanks for your patch. Here are some comments. Thank you for your review ! > 2. > +# Make sure the apply worker knows to wait for more than 500ms > +check_apply_delay_log($node_subscriber, $offset, "0.5"); > > I think the last parameter should be 500. Good catch ! Fixed. > Besides, I am not sure it's a stable test to check the log. Is it possible > that there's > no such log on a slow machine? I modified the code to sleep 1s at the > beginning > of apply_dispatch(), then the new added test failed because the server log > cannot match. To get the log by itself is necessary to ensure that the delay is conducted by the apply worker, because we emit the diffms only if it's bigger than 0 in maybe_apply_delay(). If we omit the step, we are not sure the delay is caused by other reasons or the time-delayed feature. As you mentioned, it's possible that no log is emitted on slow machine. Then, the idea to make the test safer for such machines should be to make the delayed time longer. But we shortened the delay time to 1 second to mitigate the long test execution time of this TAP test. So, I'm not sure if it's a good idea to make it longer again. Please have a look at the latest v26 in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB83730A45925B9680C40D92AFEDD69%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Wednesday, February 1, 2023 1:37 PM Peter Smith wrote: > Here are my review comments for the patch v25-0001. Thank you for your review ! > == > Commit Message > > 1. > The other possibility is to apply the delay at the end of the parallel apply > transaction but that would cause issues related to resource bloat and locks > being > held for a long time. > > ~ > > SUGGESTION > We chose not to apply the delay at the end of the parallel apply transaction > because that would cause issues related to resource bloat and locks being held > for a long time. I prefer the current description. So, I just changed one word from "The other possibility is..." to "The other possibility was" to indicate both two paragraphs (this paragraph and the previous paragraph) are related. > == > doc/src/sgml/config.sgml > > 2. > + > + For time-delayed logical replication, the apply worker sends a > feedback > + message to the publisher every > + wal_receiver_status_interval milliseconds. > Make sure > + to set wal_receiver_status_interval less than > the > + wal_sender_timeout on the publisher, > otherwise, the > + walsender will repeatedly terminate due to timeout > + error. Note that if wal_receiver_status_interval > is > + set to zero, the apply worker sends no feedback messages during the > + min_apply_delay period. > + > > 2a. > "due to timeout error." --> "due to timeout errors." Fixed. > ~ > > 2b. > Shouldn't this also cross-ref to CREATE SUBSCRIPTION docs? Because the > above mentions 'min_apply_delay' but that is not defined on this page. Makes sense. Added. > == > doc/src/sgml/ref/create_subscription.sgml > > 3. > + > + By default, the subscriber applies changes as soon as possible. > This > + parameter allows the user to delay the application of changes by a > + given time period. If the value is specified without units, it is > + taken as milliseconds. The default is zero (no delay). See > + for details on the > + available valid time unites. > + > > Typo: "unites" Fixed it to "units". > ~~~ > > 4. > + > + Any delay becomes effective after all initial table synchronization > + has finished and occurs before each transaction starts to get > applied > + on the subscriber. The delay is calculated as the difference > between > + the WAL timestamp as written on the publisher and the current time > on > + the subscriber. Any overhead of time spent in logical decoding and > in > + transferring the transaction may reduce the actual wait time. It is > + also possible that the overhead already exceeds the requested > + min_apply_delay value, in which case no delay is > + applied. If the system clocks on publisher and subscriber are not > + synchronized, this may lead to apply changes earlier than expected, > + but this is not a major issue because this parameter is typically > + much larger than the time deviations between servers. Note that if > + this parameter is set to a long delay, the replication will stop if > + the replication slot falls behind the current LSN by more than > + linkend="guc-max-slot-wal-keep-size">max_slot_wal_keep_size al>. > + > > "Any delay becomes effective after all initial table synchronization..." --> > "Any > delay becomes effective only after all initial table synchronization..." Agreed. Fixed. > ~~~ > > 5. > + > + > +Delaying the replication means there is a much longer time > between > +making a change on the publisher, and that change being > committed > +on the subscriber. This can impact the performance of > synchronous > +replication. See > +parameter. > + > + > > > I'm not sure why this was text changed to say "means there is a much longer > time" instead of "can mean there is a much longer time". > > IMO the previous wording was better because this current text makes an > assumption about what the user has configured -- e.g. if they configured only > 1ms delay then the warning text is not really relevant. Yes, I changed here. The reason is that the purpose of this feature is to address unintentional wrong operations on the pub and for that purpose, I didn't feel quite very short time like you mentioned might not be set for this parameter after some community's comments from hackers. Either was fine, but I chose the current description, depending on the purpose. > ~~~ > > 6. > Why was the example (it existed when I last looked at patch v19) removed? > Personally, I found that example to be a useful reminder that the > min_apply_delay can specify units other than just 'ms'. Removed because the example was one variation that used one difference value
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Wednesday, February 1, 2023 5:40 PM Kyotaro Horiguchi wrote: > At Wed, 1 Feb 2023 08:38:11 +0530, Amit Kapila > wrote in > > On Wed, Feb 1, 2023 at 8:13 AM Kyotaro Horiguchi > > wrote: > > > > > > At Tue, 31 Jan 2023 15:12:14 +0530, Amit Kapila > > > wrote in > > > > So, shall we check if the result of parse_int is in the range 0 > > > > and PG_INT32_MAX to ameliorate this concern? > > > > > > Yeah, it is exactly what I wanted to suggest. > > > > > > > If this works then we need to > > > > probably change the return value of defGetMinApplyDelay() to int32. > > > > > > I didn't thought doing that, int can store all values in the valid > > > range (I'm assuming we implicitly assume int >= int32 in bit width) > > > and it is the natural integer in C. Either will do for me but I > > > slightly prefer to use int there. > > > > > > > I think it would be clear to use int32 because the parameter where we > > store the return value is also int32. > > I'm fine with that. Thank you for confirming. Attached the updated patch v26 accordingly. I slightly adjusted the comments in defGetMinApplyDelay on this point as well. Best Regards, Takamichi Osumi v26-0001-Time-delayed-logical-replication-subscriber.patch Description: v26-0001-Time-delayed-logical-replication-subscriber.patch
RE: Allow logical replication to copy tables in binary format
On Monday, January 30, 2023 7:50 PM Melih Mutlu wrote:
> Thanks for reviewing.
...
> Please see [1] and you'll get the following error in your case:
> "ERROR: incorrect binary data format in logical replication column 1"
Hi, thanks for sharing v7.
(1) general comment
I wondered if the addition of the new option/parameter can introduce some
confusion to the users.
case 1. When binary = true and copy_format = text, the table sync is conducted
by text.
case 2. When binary = false and copy_format = binary, the table sync is
conducted by binary.
(Note that the case 2 can be accepted after addressing the 3rd comment of
Bharath-san in [1].
I agree with the 3rd comment by itself.)
The name of the new subscription parameter looks confusing.
How about "init_sync_format" or something ?
(2) The commit message doesn't get updated.
The commit message needs to mention the new subscription option.
(3) whitespace errors.
$ git am v7-0001-Allow-logical-replication-to-copy-table-in-binary.patch
Applying: Allow logical replication to copy table in binary
.git/rebase-apply/patch:95: trailing whitespace.
copied to the subscriber. Supported formats are
.git/rebase-apply/patch:101: trailing whitespace.
that data will not be copied if copy_data = false.
warning: 2 lines add whitespace errors.
(4) pg_dump.c
if (fout->remoteVersion >= 16)
- appendPQExpBufferStr(query, " s.suborigin\n");
+ appendPQExpBufferStr(query, " s.suborigin,\n");
else
- appendPQExpBuffer(query, " '%s' AS suborigin\n",
LOGICALREP_ORIGIN_ANY);
+ appendPQExpBuffer(query, " '%s' AS suborigin,\n",
LOGICALREP_ORIGIN_ANY);
+
+ if (fout->remoteVersion >= 16)
+ appendPQExpBufferStr(query, " s.subcopyformat\n");
+ else
+ appendPQExpBuffer(query, " '%c' AS subcopyformat\n",
LOGICALREP_COPY_AS_TEXT);
This new branch for v16 can be made together with the previous same condition.
(5) describe.c
+
+ /* Copy format is only supported in v16 and higher */
+ if (pset.sversion >= 16)
+ appendPQExpBuffer(,
+ ", subcopyformat AS
\"%s\"\n",
+ gettext_noop("Copy
Format"));
Similarly to (4), this creates a new branch for v16. Please see the above codes
of this part.
(6)
+ * Extract the copy format value from a DefElem.
+ */
+char
+defGetCopyFormat(DefElem *def)
Shouldn't this function be static and remove the change of subscriptioncmds.h ?
(7) catalogs.sgml
The subcopyformat should be mentioned here and the current description for
subbinary
should be removed.
(8) create_subscription.sgml
+ text.
+
+ binary format can be selected only if
Unnecessary blank line.
[1] -
https://www.postgresql.org/message-id/CALj2ACW5Oa7_v25iZb326UXvtM_tjQfw0Tc3hPJ8zN4FZqc9cw%40mail.gmail.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, January 30, 2023 7:05 PM I wrote: > On Saturday, January 28, 2023 1:28 PM I wrote: > > Attached the updated patch v24. > I've conducted the rebase affected by the commit(1e8b61735c) by renaming > the GUC to logical_replication_mode accordingly, because it's utilized in the > TAP test of this time-delayed LR feature. > There is no other change for this version. > > Kindly have a look at the attached v25. Hi, The v25 caused a failure on windows of cfbot in [1]. But, the failure happened in the tests of pg_upgrade and the failure message looks the same one reported in the ongoing discussion of [2]. Then, it's an issue independent from the v25. [1] - https://cirrus-ci.com/task/5484559622471680 [2] - https://www.postgresql.org/message-id/20220919213217.ptqfdlcc5idk5xup%40awork3.anarazel.de Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, January 30, 2023 12:02 PM Kyotaro Horiguchi wrote: > At Sat, 28 Jan 2023 04:28:29 +, "Takamichi Osumi (Fujitsu)" > wrote in > > On Friday, January 27, 2023 8:00 PM Amit Kapila > wrote: > > > So, you have changed min_apply_delay from int64 to int32, but you > > > haven't mentioned the reason for the same? We use 'int' for the > > > similar parameter recovery_min_apply_delay, so, ideally, it makes > > > sense but still better to tell your reason explicitly. > > Yes. It's because I thought I need to make this feature consistent with the > recovery_min_apply_delay. > > This feature handles the range same as the recovery_min_apply delay > > from 0 to INT_MAX now so should be adjusted to match it. > > INT_MAX can stick out of int32 on some platforms. (I'm not sure where that > actually happens, though.) We can use PG_INT32_MAX instead. > > IMHO, I think we don't use int as a catalog column and I agree that > int32 is sufficient since I don't think more than 49 days delay is practical. > On > the other hand, maybe I wouldn't want to use int32 for intermediate > calculations. Hi, Horiguchi-san. Thanks for your comments ! IIUC, in the last sentence, you proposed the type of SubOpts min_apply_delay should be change to "int". But I couldn't find actual harm of the current codes, because we anyway insert the SubOpts value to the catalog after holding it in SubOpts. Also, it seems there is no explicit rule where we should use "int" local variables for "int32" system catalog values internally. I had a look at other variables for int32 system catalog members and either looked fine. So, I'd like to keep the current code as it is, until actual harm is found. The latest patch can be seen in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373E26884C385EFFFB8965FEDD39%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Saturday, January 28, 2023 1:28 PM I wrote: > Attached the updated patch v24. Hi, I've conducted the rebase affected by the commit(1e8b61735c) by renaming the GUC to logical_replication_mode accordingly, because it's utilized in the TAP test of this time-delayed LR feature. There is no other change for this version. Kindly have a look at the attached v25. Best Regards, Takamichi Osumi v25-0001-Time-delayed-logical-replication-subscriber.patch Description: v25-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, January 27, 2023 8:00 PM Amit Kapila wrote: > On Fri, Jan 27, 2023 at 1:39 PM Takamichi Osumi (Fujitsu) > wrote: > > > > On Wednesday, January 25, 2023 11:24 PM I wrote: > > > Attached the updated v22. > > Hi, > > > > During self-review, I noticed some changes are required for some > > variable types related to 'min_apply_delay' value, so have conducted > > the adjustment changes for the same. > > > > So, you have changed min_apply_delay from int64 to int32, but you haven't > mentioned the reason for the same? We use 'int' for the similar parameter > recovery_min_apply_delay, so, ideally, it makes sense but still better to > tell your > reason explicitly. Yes. It's because I thought I need to make this feature consistent with the recovery_min_apply_delay. This feature handles the range same as the recovery_min_apply delay from 0 to INT_MAX now so should be adjusted to match it. > Few comments > = > 1. > @@ -70,6 +70,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) > BKI_SHARED_RELATION BKI_ROW > XLogRecPtr subskiplsn; /* All changes finished at this LSN are > * skipped */ > > + int32 subminapplydelay; /* Replication apply delay (ms) */ > + > NameData subname; /* Name of the subscription */ > > Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */ > > Why are you placing this after subskiplsn? Earlier it was okay because we want > the 64 bit value to be aligned but now, isn't it better to keep it after > subowner? Moved it after subowner. > 2. > + > + diffms = TimestampDifferenceMilliseconds(GetCurrentTimestamp(), > + TimestampTzPlusMilliseconds(finish_ts, > + MySubscription->minapplydelay)); > > The above code appears a bit unreadable. Can we store the result of > TimestampTzPlusMilliseconds() in a separate variable say "TimestampTz > delayUntil;"? Agreed. Fixed. Attached the updated patch v24. Best Regards, Takamichi Osumi v24-0001-Time-delayed-logical-replication-subscriber.patch Description: v24-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, January 25, 2023 11:24 PM I wrote: > Attached the updated v22. Hi, During self-review, I noticed some changes are required for some variable types related to 'min_apply_delay' value, so have conducted the adjustment changes for the same. Additionally, I made some comments for translator and TAP test better. Note that I executed pgindent and pgperltidy for the patch. Now the updated patch should be more refined. Best Regards, Takamichi Osumi v23-0001-Time-delayed-logical-replication-subscriber.patch Description: v23-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, January 25, 2023 3:55 PM Amit Kapila
wrote:
> On Wed, Jan 25, 2023 at 11:23 AM Takamichi Osumi (Fujitsu)
> wrote:
> >
> >
> > Thank you for checking the patch !
> > On Wednesday, January 25, 2023 10:17 AM Kyotaro Horiguchi
> wrote:
> > > In short, I'd like to propose renaming the parameter
> > > in_delayed_apply of send_feedback to "has_unprocessed_change".
> > >
> > > At Tue, 24 Jan 2023 12:27:58 +0530, Amit Kapila
> > > wrote in
> > > > > send_feedback():
> > > > > +* If the subscriber side apply is delayed (because of
> > > time-delayed
> > > > > +* replication) then do not tell the publisher that the
> > > > > + received
> > > latest
> > > > > +* LSN is already applied and flushed, otherwise, it leads to
> the
> > > > > +* publisher side making a wrong assumption of logical
> > > replication
> > > > > +* progress. Instead, we just send a feedback message to
> > > > > + avoid a
> > > publisher
> > > > > +* timeout during the delay.
> > > > > */
> > > > > - if (!have_pending_txes)
> > > > > + if (!have_pending_txes && !in_delayed_apply)
> > > > > flushpos = writepos = recvpos;
> > > > >
> > > > > Honestly I don't like this wart. The reason for this is the
> > > > > function assumes recvpos = applypos but we actually call it
> > > > > while holding unapplied changes, that is, applypos < recvpos.
> > > > >
> > > > > Couldn't we maintain an additional static variable "last_applied"
> > > > > along with last_received?
> > > > >
> > > >
> > > > It won't be easy to maintain the meaning of last_applied because
> > > > there are cases where we don't apply the change directly. For
> > > > example, in case of streaming xacts, we will just keep writing it
> > > > to the file, now, say, due to some reason, we have to send the
> > > > feedback, then it will not allow you to update the latest write
> > > > locations. This would then become different then what we are doing
> without the patch.
> > > > Another point to think about is that we also need to keep the
> > > > variable updated for keep-alive ('k') messages even though we
> > > > don't apply anything in that case. Still, other cases to consider
> > > > are where we have mix of streaming and non-streaming transactions.
> > >
> > > Yeah. Even though I named it as "last_applied", its objective is to
> > > have get_flush_position returning the correct have_pending_txes
> > > without a hint from callers, that is, "let g_f_position know if
> > > store_flush_position has been called with the last received data".
> > >
> > > Anyway I tried that but didn't find a clean and simple way. However,
> > > while on it, I realized what the code made me confused.
> > >
> > > +static void send_feedback(XLogRecPtr recvpos, bool force, bool
> > > requestReply,
> > > + bool
> > > + in_delayed_apply);
> > >
> > > The name "in_delayed_apply" doesn't donsn't give me an idea of what
> > > the function should do for it. If it is named
> > > "has_unprocessed_change", I think it makes sense that send_feedback
> > > should think there may be an outstanding transaction that is not known to
> the function.
> > >
> > >
> > > So, my conclusion here is I'd like to propose changing the parameter
> > > name to "has_unapplied_change".
> > Renamed the variable name to "has_unprocessed_change".
> > Also, removed the first argument of the send_feedback() which isn't
> necessary now.
> >
>
> Why did you remove the first argument of the send_feedback() when that is not
> added by this patch? If you really think that is an improvement, feel free to
> propose that as a separate patch.
> Personally, I don't see a value in it.
Oh, sorry for that. I have made the change back.
Kindly have a look at the v22 shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB837305BD31FA317256BC7B1FEDCE9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, January 25, 2023 3:27 PM Kyotaro Horiguchi wrote: > At Tue, 24 Jan 2023 12:19:04 +, "Takamichi Osumi (Fujitsu)" > wrote in > > Attached the patch v20 that has incorporated all comments so far. > > Thanks! I looked thourgh the documentation part. Thank you for your review ! > + > + subminapplydelay int8 > + > + > + Total time spent delaying the application of changes, in milliseconds. > + > > I was confused becase it reads as this column shows the summarized actual > waiting time caused by min_apply_delay. IIUC actually it shows the > min_apply_delay setting for the subscription. Thus shouldn't it be something > like this? > > "The minimum amount of time to delay applying changes, in milliseconds" > And it might be better to mention the corresponding subscription paramter. This description looks much better to me than the past description. Fixed. OTOH, other parameters don't mention about its subscription parameters. So, I didn't add the mention. > + error. If wal_receiver_status_interval is set to > + zero, the apply worker doesn't send any feedback messages during > the > + min_apply_delay period. > > I took a bit longer time to understand what this sentence means. I'd like to > suggest something like the follwoing. > > "Since no status-update messages are sent while delaying, note that > wal_receiver_status_interval is the only source of keepalive messages during > that period." The current patch's description is precise and I prefer that. I would say "the only source" would be confusing to readers. However, I slightly adjusted the description a bit. Could you please check ? > + > + A logical replication subscription can delay the application of changes by > + specifying the min_apply_delay subscription > parameter. > + See for details. > + > > I'm not sure "logical replication subscription" is a common term. > Doesn't just "subscription" mean the same, especially in that context? > (Note that 31.2 starts with "A subscription is the downstream.."). I think you are right. Fixed. > + Any delay occurs only on WAL records for transaction begins after > all > + initial table synchronization has finished. The delay is > + calculated > > There is no "transaction begin" WAL records. Maybe it is "logical replication > transaction begin message". The timestamp is of "commit time". (I took > "transaction begins" as a noun, but that might be > wrong..) Yeah, we can improve here. But, we need to include not only "commit" but also "prepare" as nuance in this part. In short, I think we should change here to mention (1) the delay happens after all initial table synchronization (2) how delay is applied for non-streaming and streaming transactions in general. By the way, WAL timestamp is a word used in the recovery_min_apply_delay. So, I'd like to keep it to make the description more aligned with it, until there is a better description. Updated the doc. I adjusted the commit message according to this fix. > > +may reduce the actual wait time. It is also possible that the > overhead > +already exceeds the requested min_apply_delay > value, > +in which case no additional wait is necessary. If the system > + clocks > > I'm not sure it is right to say "necessary" here. IMHO it might be better be > "in > which case no delay is applied". Agreed. Fixed. > + in which case no additional wait is necessary. If the system clocks > + on publisher and subscriber are not synchronized, this may lead to > + apply changes earlier than expected, but this is not a major issue > + because this parameter is typically much larger than the time > + deviations between servers. Note that if this parameter is > + set to a > > This doesn't seem to fit our documentation. It is not our business whether a > certain amount deviation is critical or not. How about somethig like the > following? > > "Note that the delay is measured between the timestamp assigned by > publisher and the system clock on subscriber. You need to manage the > system clocks to be in sync so that the delay works properly." As discussed, this is aligned with recovery_min_apply_delay. So, I keep it. > +Delaying the replication can mean there is a much longer time > +between making a change on the publisher, and that change > being > +committed on the subscriber. This can impact the performance > of > +synchronous replication.
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Horiguchi-san
Thank you for checking the patch !
On Wednesday, January 25, 2023 10:17 AM Kyotaro Horiguchi
wrote:
> In short, I'd like to propose renaming the parameter in_delayed_apply of
> send_feedback to "has_unprocessed_change".
>
> At Tue, 24 Jan 2023 12:27:58 +0530, Amit Kapila
> wrote in
> > > send_feedback():
> > > +* If the subscriber side apply is delayed (because of
> time-delayed
> > > +* replication) then do not tell the publisher that the received
> latest
> > > +* LSN is already applied and flushed, otherwise, it leads to the
> > > +* publisher side making a wrong assumption of logical
> replication
> > > +* progress. Instead, we just send a feedback message to avoid a
> publisher
> > > +* timeout during the delay.
> > > */
> > > - if (!have_pending_txes)
> > > + if (!have_pending_txes && !in_delayed_apply)
> > > flushpos = writepos = recvpos;
> > >
> > > Honestly I don't like this wart. The reason for this is the function
> > > assumes recvpos = applypos but we actually call it while holding
> > > unapplied changes, that is, applypos < recvpos.
> > >
> > > Couldn't we maintain an additional static variable "last_applied"
> > > along with last_received?
> > >
> >
> > It won't be easy to maintain the meaning of last_applied because there
> > are cases where we don't apply the change directly. For example, in
> > case of streaming xacts, we will just keep writing it to the file,
> > now, say, due to some reason, we have to send the feedback, then it
> > will not allow you to update the latest write locations. This would
> > then become different then what we are doing without the patch.
> > Another point to think about is that we also need to keep the variable
> > updated for keep-alive ('k') messages even though we don't apply
> > anything in that case. Still, other cases to consider are where we
> > have mix of streaming and non-streaming transactions.
>
> Yeah. Even though I named it as "last_applied", its objective is to have
> get_flush_position returning the correct have_pending_txes without a hint
> from callers, that is, "let g_f_position know if store_flush_position has been
> called with the last received data".
>
> Anyway I tried that but didn't find a clean and simple way. However, while on
> it,
> I realized what the code made me confused.
>
> +static void send_feedback(XLogRecPtr recvpos, bool force, bool
> requestReply,
> + bool in_delayed_apply);
>
> The name "in_delayed_apply" doesn't donsn't give me an idea of what the
> function should do for it. If it is named "has_unprocessed_change", I think it
> makes sense that send_feedback should think there may be an outstanding
> transaction that is not known to the function.
>
>
> So, my conclusion here is I'd like to propose changing the parameter name to
> "has_unapplied_change".
Renamed the variable name to "has_unprocessed_change".
Also, removed the first argument of the send_feedback() which isn't necessary
now.
Kindly have a look at the patch shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373193B4331B7EB6276F682EDCE9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Wednesday, January 25, 2023 2:02 PM shveta malik wrote: > On Tue, Jan 24, 2023 at 5:49 PM Takamichi Osumi (Fujitsu) > wrote: > > > > > > Attached the patch v20 that has incorporated all comments so far. > > Kindly have a look at the attached patch. > Thank You for patch. My previous comments are addressed. Tested it and it > looks good. Logging is also fine now. > > Just one comment, in summary, we see : > If the subscription sets min_apply_delay parameter, the logical replication > worker will delay the transaction commit for min_apply_delay milliseconds. > > Is it better to write "delay the transaction apply" instead of "delay the > transaction commit" just to be consistent as we do not actually delay the > commit for regular transactions. Thank you for your review ! Agreed. Your description looks better. Attached the updated patch v21. Best Regards, Takamichi Osumi v21-0001-Time-delayed-logical-replication-subscriber.patch Description: v21-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, January 24, 2023 8:32 AM Euler Taveira wrote:
> Good to know that you keep improving this patch. I have a few suggestions that
> were easier to provide a patch on top of your latest patch than to provide an
> inline suggestions.
Thanks for your review ! We basically adopted your suggestions.
> There are a few documentation polishing. Let me comment some of them above.
>
> - The length of time (ms) to delay the application of changes.
> + Total time spent delaying the application of changes, in milliseconds
>
> I don't remember if I suggested this description for catalog but IMO the
> suggestion reads better for me.
Adopted the above change.
> - For time-delayed logical replication (i.e. when the subscription is
> - created with parameter min_apply_delay > 0), the apply worker sends a
> - Standby Status Update message to the publisher with a period of
> - wal_receiver_status_interval. Make sure to set
> - wal_receiver_status_interval less than the
> - wal_sender_timeout on the publisher, otherwise, the
> - walsender will repeatedly terminate due to the timeout errors. If
> - wal_receiver_status_interval is set to zero, the
> apply
> - worker doesn't send any feedback messages during the subscriber's
> - min_apply_delay period. See
> -for details.
> + For time-delayed logical replication, the apply worker sends a
> feedback
> + message to the publisher every
> + wal_receiver_status_interval milliseconds. Make
> sure
> + to set wal_receiver_status_interval less than the
> + wal_sender_timeout on the publisher, otherwise, the
> + walsender will repeatedly terminate due to timeout
> + error. If wal_receiver_status_interval is set to
> + zero, the apply worker doesn't send any feedback messages during the
> + min_apply_delay interval.
>
> I removed the parenthesis explanation about time-delayed logical replication.
> If you are reading the documentation and does not know what it means you
> should
> (a) read the logical replication chapter or (b) check the glossary (maybe a
> new
> entry should be added). I also removed the Standby status Update message but
> it
> is a low level detail; let's refer to it as feedback message as the other
> sentences do. I changed "literal" to "varname" that's the correct tag for
> parameters. I replace "period" with "interval" that was the previous
> terminology. IMO we should be uniform, use one or the other.
Adopted.
Also, I added the glossary for time-delayed replication (one for
applicable to both physical replication and logical replication).
Plus, I united the term "interval" to period, because it would clarify the type
for this feature.
I think this is better.
> - The subscriber replication can be instructed to lag behind the publisher
> - side changes by specifying the min_apply_delay
> - subscription parameter. See for
> - details.
> + A logical replication subscription can delay the application of changes by
> + specifying the min_apply_delay subscription parameter.
> + See for details.
>
> This feature refers to a specific subscription, hence, "logical replication
> subscription" instead of "subscriber replication".
Adopted.
> + if (IsSet(opts->specified_opts, SUBOPT_MIN_APPLY_DELAY))
> + errorConflictingDefElem(defel, pstate);
> +
>
> Peter S referred to this missing piece of code too.
Added.
> -int
> +static int
> defGetMinApplyDelay(DefElem *def)
> {
>
> It seems you forgot static keyword.
Fixed.
> - elog(DEBUG2, "time-delayed replication for txid %u, min_apply_delay =
> %lld ms, Remaining wait time: %ld ms",
> -xid, (long long) MySubscription->minapplydelay, diffms);
> + elog(DEBUG2, "time-delayed replication for txid %u, min_apply_delay =
> " INT64_FORMAT " ms, remaining wait time: %ld ms",
> +xid, MySubscription->minapplydelay, diffms);
> int64 should use format modifier INT64_FORMAT.
Fixed.
> - (long) wal_receiver_status_interval * 1000,
> + wal_receiver_status_interval * 1000L,
>
> Cast is not required. I added a suffix to the constant.
Fixed.
> - elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, write %X/%X,
> flush %X/%X in-delayed: %d",
> + elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, write %X/%X,
> flush %X/%X, apply delay: %s",
> force,
> LSN_FORMAT_ARGS(recvpos),
> LSN_FORMAT_ARGS(writepos),
> LSN_FORMAT_ARGS(flushpos),
> -in_delayed_apply);
> +in_delayed_apply? "yes" : "no");
>
> It is better to use a string to represent the yes/no option.
Fixed.
> - gettext_noop("Min apply delay (ms)"));
> + gettext_noop("Min apply delay"));
>
> I don't know if it was discussed but we don't add units to headers. When I
> think about this parameter
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, January 23, 2023 5:07 PM Peter Smith wrote:
> Here are my review comments for v19-0001.
Thanks for your review !
>
> ==
> Commit message
>
> 1.
> The combination of parallel streaming mode and min_apply_delay is not
> allowed. The subscriber in the parallel streaming mode applies each stream on
> arrival without the time of commit/prepare. So, the subscriber needs to depend
> on the arrival time of the stream in this case, if we apply the time-delayed
> feature for such transactions. Then there is a possibility where some
> unnecessary delay will be added on the subscriber by network communication
> break between nodes or other heavy work load on the publisher. On the other
> hand, applying the delay at the end of transaction with parallel apply also
> can
> cause issues of used resource bloat and locks kept in open for a long time.
> Thus, those features can't work together.
> ~
>
> I think the above is just cut/paste from a code comment within
> subscriptioncmds.c. See review comments #5 below -- so if the code is
> changed then this commit message should also change to match it.
Now, updated this. Kindly have a look at the latest patch in [1].
>
> ==
> doc/src/sgml/ref/create_subscription.sgml
>
> 2.
> +
> +min_apply_delay
> (integer)
> +
> +
> + By default, the subscriber applies changes as soon as possible.
> This
> + parameter allows the user to delay the application of changes by a
> + given time interval. If the value is specified without units, it is
> + taken as milliseconds. The default is zero (no delay).
> +
>
> 2a.
> The pgdocs says this is an integer default to “ms” unit. Also, the example on
> this same page shows it is set to '4h'. But I did not see any mention of what
> other units are available to the user. Maybe other time units should be
> mentioned here, or maybe a link should be given to the section “20.1.1.
> Parameter Names and Values".
Added.
> ~
>
> 2b.
> Previously the word "interval" was deliberately used because this parameter
> had interval support. But maybe now it should be changed so it is not
> misleading.
>
> "a given time interval" --> "a given time period" ??
Fixed.
> ==
> src/backend/commands/subscriptioncmds.c
>
> 3. Forward declare
>
> +static int defGetMinApplyDelay(DefElem *def);
>
> If the new function is implemented as static near the top of this source file
> then
> this forward declare would not even be necessary, right?
This declaration has been kept as discussed.
> ~~~
>
> 4. parse_subscription_options
>
> @@ -324,6 +328,12 @@ parse_subscription_options(ParseState *pstate, List
> *stmt_options,
> opts->specified_opts |= SUBOPT_LSN;
> opts->lsn = lsn;
> }
> + else if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + strcmp(defel->defname, "min_apply_delay") == 0) {
> + opts->specified_opts |= SUBOPT_MIN_APPLY_DELAY; min_apply_delay =
> + opts->defGetMinApplyDelay(defel);
> + }
>
> Should this code fragment be calling errorConflictingDefElem so it will report
> an error if the same min_apply_delay parameter is redundantly repeated?
> (IIUC, this appears to be the code pattern for other parameters nearby).
Added.
> ~~~
>
> 5. parse_subscription_options
>
> + /*
> + * The combination of parallel streaming mode and min_apply_delay is
> + not
> + * allowed. The subscriber in the parallel streaming mode applies each
> + * stream on arrival without the time of commit/prepare. So, the
> + * subscriber needs to depend on the arrival time of the stream in this
> + * case, if we apply the time-delayed feature for such transactions.
> + Then
> + * there is a possibility where some unnecessary delay will be added on
> + * the subscriber by network communication break between nodes or other
> + * heavy work load on the publisher. On the other hand, applying the
> + delay
> + * at the end of transaction with parallel apply also can cause issues
> + of
> + * used resource bloat and locks kept in open for a long time. Thus,
> + those
> + * features can't work together.
> + */
>
> IMO some re-wording might be warranted here. I am not sure quite how to do it.
> Perhaps like below?
>
> SUGGESTION
>
> The combination of parallel streaming mode and min_apply_delay is not
> allowed.
>
> Here are some reasons why these features are incompatible:
> a. In the parallel streaming mode the subscriber applies each stream on
> arrival
> without knowledge of the commit/prepare time. This means we cannot
> calculate the underlying network/decoding lag between publisher and
> subscriber, and so always waiting for the full 'min_apply_delay'
> period might include unnecessary delay.
> b. If we apply the delay at the end of the transaction of the parallel apply
> then
> that would cause issues related to resource bloat and locks being held for a
> long time.
Now, this has been changed to the one suggested by Amit-san.
Thanks for your help.
> ~~~
>
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, January 23, 2023 7:45 PM Amit Kapila wrote: > On Mon, Jan 23, 2023 at 1:36 PM Peter Smith > wrote: > > > > Here are my review comments for v19-0001. > > > ... > > > > 5. parse_subscription_options > > > > + /* > > + * The combination of parallel streaming mode and min_apply_delay is > > + not > > + * allowed. The subscriber in the parallel streaming mode applies > > + each > > + * stream on arrival without the time of commit/prepare. So, the > > + * subscriber needs to depend on the arrival time of the stream in > > + this > > + * case, if we apply the time-delayed feature for such transactions. > > + Then > > + * there is a possibility where some unnecessary delay will be added > > + on > > + * the subscriber by network communication break between nodes or > > + other > > + * heavy work load on the publisher. On the other hand, applying the > > + delay > > + * at the end of transaction with parallel apply also can cause > > + issues of > > + * used resource bloat and locks kept in open for a long time. Thus, > > + those > > + * features can't work together. > > + */ > > > > IMO some re-wording might be warranted here. I am not sure quite how > > to do it. Perhaps like below? > > > > SUGGESTION > > > > The combination of parallel streaming mode and min_apply_delay is not > allowed. > > > > Here are some reasons why these features are incompatible: > > a. In the parallel streaming mode the subscriber applies each stream > > on arrival without knowledge of the commit/prepare time. This means we > > cannot calculate the underlying network/decoding lag between publisher > > and subscriber, and so always waiting for the full 'min_apply_delay' > > period might include unnecessary delay. > > b. If we apply the delay at the end of the transaction of the parallel > > apply then that would cause issues related to resource bloat and locks > > being held for a long time. > > > > ~~~ > > > > How about something like: > The combination of parallel streaming mode and min_apply_delay is not > allowed. This is because we start applying the transaction stream as soon as > the first change arrives without knowing the transaction's prepare/commit > time. > This means we cannot calculate the underlying network/decoding lag between > publisher and subscriber, and so always waiting for the full 'min_apply_delay' > period might include unnecessary delay. > > The other possibility is to apply the delay at the end of the parallel apply > transaction but that would cause issues related to resource bloat and locks > being held for a long time. Thank you for providing a good description ! Adopted. The latest patch can be seen in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373DC1881F382B4703F26E0EDC99%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, January 23, 2023 9:06 PM Amit Kapila wrote:
> On Sun, Jan 22, 2023 at 6:12 PM Takamichi Osumi (Fujitsu)
> wrote:
> >
> >
> > Attached the updated patch v19.
> >
>
> Few comments:
> =
> 1.
> }
> +
> +
> +/*
>
> Only one empty line is sufficient between different functions.
Fixed.
> 2.
> + if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + opts->min_apply_delay > 0 && opts->streaming ==
> + opts->LOGICALREP_STREAM_PARALLEL)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s and %s are mutually exclusive options",
> +"min_apply_delay > 0", "streaming = parallel"));
> }
>
> I think here we should add a comment for the translator as we are doing in
> some other nearby cases.
Fixed.
> 3.
> + /*
> + * The combination of parallel streaming mode and
> + * min_apply_delay is not allowed.
> + */
> + if (opts.streaming == LOGICALREP_STREAM_PARALLEL) if
> + ((IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> opts.min_apply_delay > 0) ||
> + (!IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> sub->minapplydelay > 0))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot enable %s mode for subscription with %s",
> +"streaming = parallel", "min_apply_delay"));
> +
>
> A. When can second condition ((!IsSet(opts.specified_opts,
> SUBOPT_MIN_APPLY_DELAY) && sub->minapplydelay > 0)) in above check
> be true?
> B. In comments, you can say "See parse_subscription_options."
(1) In the alter statement, streaming = parallel is set.
Also, (2) in the alter statement, min_apply_delay isn't set.
and (3) an existing subscription has non-zero min_apply_delay.
Added the comment.
> 4.
> +/*
> + * When min_apply_delay parameter is set on the subscriber, we wait
> +long enough
> + * to make sure a transaction is applied at least that interval behind
> +the
> + * publisher.
>
> Shouldn't this part of the comment needs to be updated after the patch has
> stopped using interval?
Yes. I removed "interval" in descriptions so that we don't get
confused with types.
> 5. How does this feature interacts with the SKIP feature? Currently, it
> doesn't
> care whether the changes of a particular xact are skipped or not. I think that
> might be okay because anyway the purpose of this feature is to make
> subscriber lag from publishers. What do you think?
> I feel we can add some comments to indicate the same.
Added the comment in the commit message.
I didn't add this kind of comment as code comments,
since both features are independent. If there is a need to write it anywhere,
then please let me know. The latest patch is posted in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373DC1881F382B4703F26E0EDC99%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, January 24, 2023 3:58 PM Amit Kapila
wrote:
> > send_feedback():
> > +* If the subscriber side apply is delayed (because of time-delayed
> > +* replication) then do not tell the publisher that the received
> > latest
> > +* LSN is already applied and flushed, otherwise, it leads to the
> > +* publisher side making a wrong assumption of logical replication
> > +* progress. Instead, we just send a feedback message to avoid a
> publisher
> > +* timeout during the delay.
> > */
> > - if (!have_pending_txes)
> > + if (!have_pending_txes && !in_delayed_apply)
> > flushpos = writepos = recvpos;
> >
> > Honestly I don't like this wart. The reason for this is the function
> > assumes recvpos = applypos but we actually call it while holding
> > unapplied changes, that is, applypos < recvpos.
> >
> > Couldn't we maintain an additional static variable "last_applied"
> > along with last_received?
> >
>
> It won't be easy to maintain the meaning of last_applied because there are
> cases where we don't apply the change directly. For example, in case of
> streaming xacts, we will just keep writing it to the file, now, say, due to
> some
> reason, we have to send the feedback, then it will not allow you to update the
> latest write locations. This would then become different then what we are
> doing without the patch.
> Another point to think about is that we also need to keep the variable updated
> for keep-alive ('k') messages even though we don't apply anything in that
> case.
> Still, other cases to consider are where we have mix of streaming and
> non-streaming transactions.
Agreed. This will change some existing behaviors. So, didn't conduct this
change in the latest patch [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373DC1881F382B4703F26E0EDC99%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Tuesday, January 24, 2023 5:52 PM Amit Kapila wrote: > On Tue, Jan 24, 2023 at 12:44 PM Peter Smith > wrote: > > > > On Tue, Jan 24, 2023 at 5:58 PM Amit Kapila > wrote: > > > > > > On Tue, Jan 24, 2023 at 8:15 AM Kyotaro Horiguchi > > > wrote: > > > > > > > > > Attached the updated patch v19. > > > > > > > > + maybe_delay_apply(TransactionId xid, TimestampTz finish_ts) > > > > > > > > I look this spelling strange. How about maybe_apply_delay()? > > > > > > > > > > +1. > > > > It depends on how you read it. I read it like this: > > > > maybe_delay_apply === means "maybe delay [the] apply" > > (which is exactly what the function does) > > > > versus > > > > maybe_apply_delay === means "maybe [the] apply [needs a] delay" > > (which is also correct, but it seemed a more awkward way to say it > > IMO) > > > > This matches more with GUC and all other usages of variables in the patch. So, > I still prefer the second one. Okay. Fixed. Attached the patch v20 that has incorporated all comments so far. Kindly have a look at the attached patch. Best Regards, Takamichi Osumi v20-0001-Time-delayed-logical-replication-subscriber.patch Description: v20-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Saturday, January 21, 2023 3:36 AM I wrote: > Kindly have a look at the patch v18. I've conducted some refactoring for v18. Now the latest patch should be tidier and the comments would be clearer and more aligned as a whole. Attached the updated patch v19. Best Regards, Takamichi Osumi v19-0001-Time-delayed-logical-replication-subscriber.patch Description: v19-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Thursday, January 19, 2023 7:55 PM vignesh C wrote:
> On Thu, 19 Jan 2023 at 12:06, Takamichi Osumi (Fujitsu)
> wrote:
> >
> > Updated the comment and the function call.
> >
> > Kindly have a look at the updated patch v17.
>
> Thanks for the updated patch, few comments:
> 1) min_apply_delay was accepting values like '600 m s h', I was not sure if we
> should allow this:
> alter subscription sub1 set (min_apply_delay = ' 600 m s h');
>
> + /*
> +* If no unit was specified, then explicitly
> add 'ms' otherwise
> +* the interval_in function would assume 'seconds'.
> +*/
> + if (strspn(tmp, "-0123456789 ") == strlen(tmp))
> + val = psprintf("%sms", tmp);
> + else
> + val = tmp;
> +
> + interval =
> DatumGetIntervalP(DirectFunctionCall3(interval_in,
> +
>
> CStringGetDatum(val),
> +
>
> ObjectIdGetDatum(InvalidOid),
> +
> Int32GetDatum(-1)));
>
FYI, the input can be accepted by the interval type.
Now we changed the direction of the type from interval to integer
but plus some unit can be added like recovery_min_apply_delay.
Please check.
> 3) There is one check at parse_subscription_options and another check in
> AlterSubscription, this looks like a redundant check in case of alter
> subscription, can we try to merge and keep in one place:
> /*
> * The combination of parallel streaming mode and min_apply_delay is not
> * allowed.
> */
> if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> opts->min_apply_delay > 0)
> {
> if (opts->streaming == LOGICALREP_STREAM_PARALLEL) ereport(ERROR,
> errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually
> exclusive options",
>"min_apply_delay > 0", "streaming = parallel")); }
>
> if (IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY)) {
> /*
> * The combination of parallel streaming mode and
> * min_apply_delay is not allowed.
> */
> if (opts.min_apply_delay > 0)
> if ((IsSet(opts.specified_opts, SUBOPT_STREAMING) && opts.streaming ==
> LOGICALREP_STREAM_PARALLEL) ||
> (!IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream ==
> LOGICALREP_STREAM_PARALLEL))
> ereport(ERROR,
> errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("cannot enable %s for subscription in %s mode",
>"min_apply_delay", "streaming = parallel"));
>
> values[Anum_pg_subscription_subminapplydelay - 1] =
> Int64GetDatum(opts.min_apply_delay);
> replaces[Anum_pg_subscription_subminapplydelay - 1] = true; }
We can't. For create subscription, we need to check the patch
from parse_subscription_options, while for alter subscription,
we need to refer the current MySubscription value for those tests
in AlterSubscription.
> 4) typo "execeeds" should be "exceeds"
>
> + time on the subscriber. Any overhead of time spent in
> logical decoding
> + and in transferring the transaction may reduce the actual wait
> time.
> + It is also possible that the overhead already execeeds the
> requested
> + min_apply_delay value, in which case no
> additional
> + wait is necessary. If the system clocks on publisher and subscriber
> + are not synchronized, this may lead to apply changes earlier
> + than
Fixed.
Kindly have a look at the v18 patch in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373BED9E390C4839AF56685EDC59%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Thursday, January 19, 2023 10:17 PM Amit Kapila wrote: > On Thu, Jan 19, 2023 at 12:06 PM Takamichi Osumi (Fujitsu) > wrote: > > > > Kindly have a look at the updated patch v17. > > > > Can we try to optimize the test time for this test? On my machine, it is the > second highest time-consuming test in src/test/subscription. It seems you are > waiting twice for apply_delay and both are for streaming cases by varying the > number of changes. I think it should be just once and that too for the > non-streaming case. I think it would be good to test streaming code path > interaction but not sure if it is important enough to have two test cases for > apply_delay. The first insert test is for non-streaming case and we need both cases for coverage. Regarding the time of test, conducted some optimization such as turning off the initial table sync, shortening the time of wait, and so on. > > One minor comment that I observed while going through the patch. > + /* > + * The combination of parallel streaming mode and min_apply_delay is > + not > + * allowed. > + */ > + if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) && > + opts->min_apply_delay > 0) > > I think it would be good if you can specify the reason for not allowing this > combination in the comments. Added. Please have a look at the latest v18 patch in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373BED9E390C4839AF56685EDC59%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Friday, January 20, 2023 12:47 PM shveta malik wrote: > 1) > Tried different variations of altering 'min_apply_delay'. All passed except > one > below: > > postgres=# alter subscription mysubnew set (min_apply_delay = '10.9min > 1ms'); ALTER SUBSCRIPTION postgres=# alter subscription mysubnew set > (min_apply_delay = '10.9min 2s 1ms'); ALTER SUBSCRIPTION --very similar to > above but fails, postgres=# alter subscription mysubnew set > (min_apply_delay = '10.9s 1ms'); > ERROR: invalid input syntax for type interval: "10.9s 1ms" FYI, this was because the interval type couldn't accept this format. But now we changed the input format from interval to integer alinged with recovery_min_apply_delay. Thus, we don't face this issue now. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Friday, January 20, 2023 5:54 PM shveta malik wrote: > On Fri, Jan 20, 2023 at 1:08 PM Peter Smith wrote: > > > a) the message should say that this is the *remaining* time to left to wait. > > > > b) it might be convenient to know from the log what was the original > > min_apply_delay value in the 1st place. > > > > For example, the logs might look something like this: > > > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > 16 ms. Remaining wait time: 159972 ms > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > 16 ms. Remaining wait time: 142828 ms > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > 16 ms. Remaining wait time: 129994 ms > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > 16 ms. Remaining wait time: 110001 ms ... > > > > +1 > This will also help when min_apply_delay is set to a new value in between the > current wait. Lets say, I started with min_apply_delay=5 min, when the worker > was half way through this, I changed min_apply_delay to 3 min or say 10min, I > see the impact of that change i.e. new wait-time is adjusted, but log becomes > confusing. So, please keep this scenario as well in mind while improving > logging. Yes, now the change of min_apply_delay value can be detected since I followed the format provided above. So, this scenario is also covered. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, January 20, 2023 6:13 PM shveta malik wrote: > On Fri, Jan 20, 2023 at 2:23 PM shveta malik wrote: > > > > On Fri, Jan 20, 2023 at 1:08 PM Peter Smith > wrote: > > > > > a) the message should say that this is the *remaining* time to left to > > > wait. > > > > > > b) it might be convenient to know from the log what was the original > > > min_apply_delay value in the 1st place. > > > > > > For example, the logs might look something like this: > > > > > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > > 16 ms. Remaining wait time: 159972 ms > > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > > 16 ms. Remaining wait time: 142828 ms > > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > > 16 ms. Remaining wait time: 129994 ms > > > DEBUG: time-delayed replication for txid 1234, min_apply_delay = > > > 16 ms. Remaining wait time: 110001 ms ... > > > > > > > +1 > > This will also help when min_apply_delay is set to a new value in > > between the current wait. Lets say, I started with min_apply_delay=5 > > min, when the worker was half way through this, I changed > > min_apply_delay to 3 min or say 10min, I see the impact of that change > > i.e. new wait-time is adjusted, but log becomes confusing. So, please > > keep this scenario as well in mind while improving logging. > > > > > when we send-feedback during apply-delay after every > wal_receiver_status_interval , the log comes as: > 023-01-19 17:12:56.000 IST [404795] DEBUG: sending feedback (force 1) to > recv 0/1570840, write 0/1570840, flush 0/1570840 > > Shall we have some info here to indicate that it is sent while waiting for > apply_delay to distinguish it from other such send-feedback logs? > It will > make apply_delay flow clear in logs. This additional tip of log information has been added in the latest v18. Kindly have a look at it in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373BED9E390C4839AF56685EDC59%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Friday, January 20, 2023 3:56 PM Peter Smith wrote:
> Hi Osumi-san, here are my review comments for the latest patch v17-0001.
Thanks for your review !
> ==
> Commit Message
>
> 1.
> Prohibit the combination of this feature and parallel streaming mode.
>
> SUGGESTION (using the same wording as in the code comments) The
> combination of parallel streaming mode and min_apply_delay is not allowed.
Okay. Fixed.
> ==
> doc/src/sgml/ref/create_subscription.sgml
>
> 2.
> +
> + By default, the subscriber applies changes as soon as possible.
> This
> + parameter allows the user to delay the application of changes by a
> + specified amount of time. If the value is specified without units,
> it
> + is taken as milliseconds. The default is zero (no delay).
> +
>
> Looking at this again, it seemed a bit strange to repeat "specified"
> twice in 2 sentences. Maybe change one of them.
>
> I’ve also suggested using the word "interval" because I don’t think docs yet
> mentioned anywhere (except in the example) that using intervals is possible.
>
> SUGGESTION (for the 2nd sentence)
> This parameter allows the user to delay the application of changes by a given
> time interval.
Adopted.
> ~~~
>
> 3.
> +
> + Any delay occurs only on WAL records for transaction begins after
> all
> + initial table synchronization has finished. The delay is calculated
> + between the WAL timestamp as written on the publisher and the
> current
> + time on the subscriber. Any overhead of time spent in
> logical decoding
> + and in transferring the transaction may reduce the actual wait
> time.
> + It is also possible that the overhead already execeeds the
> requested
> + min_apply_delay value, in which case no
> additional
> + wait is necessary. If the system clocks on publisher and subscriber
> + are not synchronized, this may lead to apply changes earlier than
> + expected, but this is not a major issue because this parameter is
> + typically much larger than the time deviations between servers.
> Note
> + that if this parameter is set to a long delay, the replication will
> + stop if the replication slot falls behind the current LSN
> by more than
> + linkend="guc-max-slot-wal-keep-size">max_slot_wal_keep_size literal>.
> +
>
> 3a.
> Typo "execeeds" (I think Vignesh reported this already)
Fixed.
> ~
>
> 3b.
> SUGGESTION (for the 2nd sentence)
> BEFORE
> The delay is calculated between the WAL timestamp...
> AFTER
> The delay is calculated as the difference between the WAL timestamp...
Fixed.
> ~~~
>
> 4.
> +
> +
> +Delaying the replication can mean there is a much longer
> time between making
> +a change on the publisher, and that change being
> committed on the subscriber.
> +v
> +See .
> +
> +
>
> IMO maybe there is a better way to express the 2nd sentence:
>
> BEFORE
> This can have a big impact on synchronous replication.
> AFTER
> This can impact the performance of synchronous replication.
Fixed.
> ==
> src/backend/commands/subscriptioncmds.c
>
> 5. parse_subscription_options
>
> @@ -324,6 +328,43 @@ parse_subscription_options(ParseState *pstate, List
> *stmt_options,
> opts->specified_opts |= SUBOPT_LSN;
> opts->lsn = lsn;
> }
> + else if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + strcmp(defel->defname, "min_apply_delay") == 0) {
> + char*val,
> +*tmp;
> + Interval *interval;
> + int64 ms;
>
> IMO 'delay_ms' (or similar) would be a friendlier variable name than just 'ms'
The variable name has been changed which is more clear to the feature.
> ~~~
>
> 6.
> @@ -404,6 +445,20 @@ parse_subscription_options(ParseState *pstate, List
> *stmt_options,
> "slot_name = NONE", "create_slot = false")));
> }
> }
> +
> + /*
> + * The combination of parallel streaming mode and min_apply_delay is
> + not
> + * allowed.
> + */
> + if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + opts->min_apply_delay > 0)
> + {
> + if (opts->streaming == LOGICALREP_STREAM_PARALLEL)
> ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually
> + exclusive options",
> +"min_apply_delay > 0", "streaming = parallel")); }
>
> This could be expressed as a single condition using &&, maybe also with the
> brackets eliminated. (Unless you feel the current code is more readable)
The current style is intentional. We feel the code is more readable.
> ~~~
>
> 7.
>
> + if (opts.min_apply_delay > 0)
> + if ((IsSet(opts.specified_opts, SUBOPT_STREAMING) && opts.streaming
> == LOGICALREP_STREAM_PARALLEL) ||
> + (!IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream ==
> LOGICALREP_STREAM_PARALLEL))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +
Re: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Horiguchi-san and Amit-san On Wednesday, November 9, 2022 3:41 PM Kyotaro Horiguchi wrote: > Using interval is not standard as this kind of parameters but it seems > convenient. On the other hand, it's not great that the unit month introduces > some subtle ambiguity. This patch translates a month to 30 days but I'm not > sure it's the right thing to do. Perhaps we shouldn't allow the units upper > than > days. In the past discussion, we talked about the merits to utilize the interval type. On the other hand, now we are facing some incompatibility issues of parsing between this time-delayed feature and physical replication's recovery_min_apply_delay. For instance, the interval type can accept '600 m s h', '1d 10min' and '1m', but the recovery_min_apply_delay makes the server failed to start by all of those. Therefore, this would confuse users and I'm going to make the feature's input compatible with recovery_min_apply_delay in the next version. Best Regards, Takamichi Osumi
RE: Modify the document of Logical Replication configuration settings
Hi, On Thursday, January 19, 2023 3:14 PM Michael Paquier wrote: > On Wed, Jan 18, 2023 at 02:04:16PM +0530, Bharath Rupireddy wrote: > > [1] > > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index > > 89d53f2a64..6f9509267c 100644 > > --- a/doc/src/sgml/config.sgml > > +++ b/doc/src/sgml/config.sgml > > @@ -4326,7 +4326,8 @@ restore_command = 'copy > > "C:\\server\\archivedir\\%f" "%p"' # Windows > > > > Terminate replication connections that are inactive for longer > > than this amount of time. This is useful for > > -the sending server to detect a standby crash or network outage. > > +the sending server to detect a standby crash or logical replication > > +subscriber crash or network outage. > > If this value is specified without units, it is taken as > > milliseconds. > > The default value is 60 seconds. > > A value of zero disables the timeout mechanism. > > Perhaps we could do that, I am not sure whether this brings much in this > section, though. This might increase comprehensiveness of the doc slightly. If we want to do this, it might be better to add this kind of additions to other parameters such as wal_receiver_timeout, wal_retrieve_retry_interval and wal_receiver_status_interval, too. BTH, thank you for having taken care of my patch, Michael-san! Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Thursday, January 19, 2023 10:42 AM Peter Smith wrote: > On Wed, Jan 18, 2023 at 6:06 PM Peter Smith > wrote: > > > > Here are my review comments for the latest patch v16-0001. (excluding > > the test code) > > > ... > > > > 8. AlterSubscription (general) > > > > I observed during testing there are 3 different errors…. > > > > At subscription CREATE time you can get this error: > > ERROR: min_apply_delay > 0 and streaming = parallel are mutually > > exclusive options > > > > If you try to ALTER the min_apply_delay when already streaming = > > parallel you can get this error: > > ERROR: cannot enable min_apply_delay for subscription in streaming = > > parallel mode > > > > If you try to ALTER the streaming to be parallel if there is already a > > min_apply_delay > 0 then you can get this error: > > ERROR: cannot enable streaming = parallel mode for subscription with > > min_apply_delay > > > > ~ > > > > IMO there is no need to have 3 different error message texts. I think > > all these cases are explained by just the first text (ERROR: > > min_apply_delay > 0 and streaming = parallel are mutually exclusive > > options) > > > > > > After checking the regression test output I can see the merit of your separate > error messages like this, even if they are maybe not strictly necessary. So > feel > free to ignore my previous review comment. Thank you for your notification. I wrote another reason why we wrote those messages in [1]. So, please have a look at it. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373447440202B248BB63805EDC49%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, January 18, 2023 4:06 PM Peter Smith wrote: > Here are my review comments for the latest patch v16-0001. (excluding the > test code) Hi, thank you for your review ! > == > > General > > 1. > > Since the value of min_apply_delay cannot be < 0, I was thinking probably it > should have been declared everywhere in this patch as a > uint64 instead of an int64, right? No, we won't be able to adopt this idea. It seems that we are not able to use uint for catalog type. So, can't applying it to the pg_subscription.h definitions and then similarly Int64GetDatum to store catalog variables and the argument variable of Int64GetDatum. Plus, there is a possibility that type Interval becomes negative value, then we are not able to change the int64 variable to get the return value of interval2ms(). > == > > Commit message > > 2. > > If the subscription sets min_apply_delay parameter, the logical replication > worker will delay the transaction commit for min_apply_delay milliseconds. > > ~ > > IMO there should be another sentence before this just to say that a new > parameter is being added: > > e.g. > This patch implements a new subscription parameter called > 'min_apply_delay'. Added. > == > > doc/src/sgml/config.sgml > > 3. > > + > + For time-delayed logical replication, the apply worker sends a Standby > + Status Update message to the corresponding publisher per the > indicated > + time of this parameter. Therefore, if this parameter is longer than > + wal_sender_timeout on the publisher, then the > + walsender doesn't get any update message during the delay and > repeatedly > + terminates due to the timeout errors. Hence, make sure this parameter > is > + shorter than the wal_sender_timeout of the > publisher. > + If this parameter is set to zero with time-delayed replication, the > + apply worker doesn't send any feedback messages during the > + min_apply_delay. > + > > > This paragraph seemed confusing. I think it needs to be reworded to change all > of the "this parameter" references because there are at least 3 different > parameters mentioned in this paragraph. e.g. maybe just change them to > explicitly name the parameter you are talking about. > > I also think it needs to mention the ‘min_apply_delay’ subscription parameter > up-front and then refer to it appropriately. > > The end result might be something like I wrote below (this is just my guess ? > probably you can word it better). > > SUGGESTION > For time-delayed logical replication (i.e. when the subscription is created > with > parameter min_apply_delay > 0), the apply worker sends a Standby Status > Update message to the publisher with a period of wal_receiver_status_interval > . > Make sure to set wal_receiver_status_interval less than the > wal_sender_timeout on the publisher, otherwise, the walsender will repeatedly > terminate due to the timeout errors. If wal_receiver_status_interval is set > to zero, > the apply worker doesn't send any feedback messages during the subscriber’s > min_apply_delay period. Applied. Also, I added one reference for min_apply_delay parameter at the end of this description. > == > > doc/src/sgml/ref/create_subscription.sgml > > 4. > > + > + By default, the subscriber applies changes as soon as possible. As > + with the physical replication feature > + (), it can be > useful to > + have a time-delayed logical replica. This parameter lets the user > to > + delay the application of changes by a specified amount of > time. If this > + value is specified without units, it is taken as milliseconds. The > + default is zero(no delay). > + > > 4a. > As with the physical replication feature (recovery_min_apply_delay), it can be > useful to have a time-delayed logical replica. > > IMO not sure that the above sentence is necessary. It seems only to be saying > that this parameter can be useful. Why do we need to say that? Removed the sentence. > ~ > > 4b. > "This parameter lets the user to delay" -> "This parameter lets the user > delay" > OR > "This parameter lets the user to delay" -> "This parameter allows the user to > delay" Fixed. > ~ > > 4c. > "If this value is specified without units" -> "If the value is specified > without > units" Fixed. > ~ > > 4d. > "zero(no delay)." -> "zero (no delay)." Fixed. > > > 5. > > + > + The delay occurs only on WAL records for transaction begins and > after > + the initial table synchronization. It is possible that the > + replication delay between publisher and subscriber exceeds the > value > + of this parameter, in which case no delay is added. Note that the > + delay is calculated between the WAL time stamp as written on > + publisher and the current time on the subscriber. Time > spent in
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Thursday, January 19, 2023 10:49 AM Peter Smith
wrote:
> On Wed, Jan 18, 2023 at 6:06 PM Peter Smith
> wrote:
> >
> > Here are my review comments for the latest patch v16-0001. (excluding
> > the test code)
> >
>
> And here are some review comments for the v16-0001 test code.
Hi, thanks for your review !
> ==
>
> src/test/regress/sql/subscription.sql
>
> 1. General
> For all comments
>
> "time delayed replication" -> "time-delayed replication" maybe is better?
Fixed.
> ~~~
>
> 2.
> -- fail - utilizing streaming = parallel with time delayed replication is not
> supported.
>
> For readability please put a blank line before this test.
Fixed.
> ~~~
>
> 3.
> -- success -- value without unit is taken as milliseconds
>
> "value" -> "min_apply_delay value"
Fixed.
> ~~~
>
> 4.
> -- success -- interval is converted into ms and stored as integer
>
> "interval" -> "min_apply_delay interval"
>
> "integer" -> "an integer"
Both are fixed.
> ~~~
>
> 5.
> You could also add another test where min_apply_delay is 0
>
> Then the following combination can be confirmed OK -- success create
> subscription with (streaming=parallel, min_apply_delay=0)
This combination is added with the modification for #6.
> ~~
>
> 6.
> -- fail - alter subscription with min_apply_delay should fail when streaming =
> parallel is set.
> CREATE SUBSCRIPTION regress_testsub CONNECTION
> 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
> streaming = parallel);
>
> There is another way to do this test without creating a brand-new
> subscription.
> You could just alter the existing subscription like:
> ALTER ... SET (min_apply_delay = 0)
> then ALTER ... SET (parallel = streaming) then ALTER ... SET (min_apply_delay
> = 123)
Fixed.
> ==
>
> src/test/subscription/t/032_apply_delay.pl
>
> 7. sub check_apply_delay_log
>
> my ($node_subscriber, $message, $expected) = @_;
>
> Why pass in the message text? I is always the same so can be hardwired in this
> function, right?
Fixed.
> ~~~
>
> 8.
> # Get the delay time in the server log
>
> "int the server log" -> "from the server log" (?)
Fixed.
> ~~~
>
> 9.
> qr/$message: (\d+) ms/
> or die "could not get delayed time";
> my $logged_delay = $1;
>
> # Is it larger than expected?
> cmp_ok($logged_delay, '>', $expected,
> "The wait time of the apply worker is long enough expectedly"
> );
>
> 9a.
> "could not get delayed time" -> "could not get the apply worker wait time"
>
> 9b.
> "The wait time of the apply worker is long enough expectedly" -> "The apply
> worker wait time has expected duration"
Both are fixed.
> ~~~
>
> 10.
> sub check_apply_delay_time
>
>
> Maybe a brief explanatory comment for this function is needed to explain the
> unreplicated column c.
Added.
> ~~~
>
> 11.
> $node_subscriber->safe_psql('postgres',
> "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
> application_name=$appname' PUBLICATION tap_pub WITH (streaming = on,
> min_apply_delay = '3s')"
>
>
> I think there should be a comment here highlighting that you are setting up a
> subscriber time delay of 3 seconds, and then later you can better describe the
> parameters for the checking functions...
Added a comment for CREATE SUBSCRIPTION command.
> e.g. (add this comment)
> # verifies that the subscriber lags the publisher by at least 3 seconds
> check_apply_delay_time($node_publisher, $node_subscriber, '5', '3');
>
> e.g.
> # verifies that the subscriber lags the publisher by at least 3 seconds
> check_apply_delay_time($node_publisher, $node_subscriber, '8', '3');
Added.
> ~~~
>
> 12.
> # Test whether ALTER SUBSCRIPTION changes the delayed time of the apply
> worker # (1 day 1 minute).
> $node_subscriber->safe_psql('postgres',
> "ALTER SUBSCRIPTION tap_sub SET (min_apply_delay = 8646)"
> );
>
> Update the comment with another note.
> # Note - The extra 1 min is to account for any decoding/network overhead.
Okay, added the comment. In general, TAP tests
fail if we wait for more than 3 minutes. Then,
we should think setting the maximum consumed time
more than 3 minutes is safe. For example, if
(which should not happen usually, but)
we consumed more than 1 minutes between this ALTER SUBSCRIPTION SET
and below check_apply_delay_log() then, the test will fail.
So made the extra time bigger.
> ~~~
>
> 13.
> # Make sure we have long enough min_apply_delay after the ALTER command
> check_apply_delay_log($node_subscriber, "logical replication apply delay",
> "8000");
>
> IMO the expectation of 1 day (8646 ms) wait time might be a better number
> for your "expected" value.
>
> So update the comment/call like this:
>
> # Make sure the apply worker knows to wait for more than 1 day (8640 ms)
> check_apply_delay_log($node_subscriber, "logical replication apply delay",
> "8640");
Updated the comment and the function call.
Kindly have a look at the updated
RE: Modify the document of Logical Replication configuration settings
On Wednesday, January 18, 2023 4:11 PM Michael Paquier wrote: > On Wed, Jan 18, 2023 at 07:00:43AM +0000, Takamichi Osumi (Fujitsu) wrote: > > The attached patch includes below two changes for the description of > > Logical Replication "Configuration Settings". > > > > 1. Add one brief description about wal_sender_timeout. > >I made it similar to one other sentence for subscriber. > > 2. Fix a wrong GUC name "wal_receiver_retry_interval". > >I think this doesn't seem to exist and would mean > "wal_retrieve_retry_interval". > > > > Kindly have a look at it. > > Looks right to me, thanks! Thank you for checking, too ! Best Regards, Takamichi Osumi
Modify the document of Logical Replication configuration settings
Hi, hackers The attached patch includes below two changes for the description of Logical Replication "Configuration Settings". 1. Add one brief description about wal_sender_timeout. I made it similar to one other sentence for subscriber. 2. Fix a wrong GUC name "wal_receiver_retry_interval". I think this doesn't seem to exist and would mean "wal_retrieve_retry_interval". Kindly have a look at it. Best Regards, Takamichi Osumi v1-0001-Make-the-description-of-the-LR-configuration-sett.patch Description: v1-0001-Make-the-description-of-the-LR-configuration-sett.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Wednesday, January 18, 2023 2:19 PM Amit Kapila wrote: > On Wed, Jan 18, 2023 at 6:37 AM Takamichi Osumi (Fujitsu) > wrote: > > > > > > > > Can you please explain a bit more as asked above to understand the > > > difference? > > So, the current difference is that the time-delayed apply worker of > > logical replication doesn't apply the delayed transaction on the > > subscriber when the subscription has been disabled during the delay, > > while (in one example of a promotion) the physical replication does the > > apply > of the delayed transaction. > > > > I don't see any particular reason here to allow the transaction apply to > complete > if the subscription is disabled. Note, that here we are waiting at the > beginning > of the transaction and for large transactions, it might cause a significant > delay if > we allow applying the xact. OTOH, if someone comes up with a valid use case > to allow the transaction apply to get completed after the subscription is > disabled then we can anyway do it later as well. This makes sense. I agree with you. So, I'll keep the current behavior of the patch. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Tuesday, January 17, 2023 9:54 PM Amit Kapila
wrote:
> On Tue, Jan 17, 2023 at 4:30 PM Takamichi Osumi (Fujitsu)
> wrote:
> >
> > On Saturday, January 14, 2023 3:27 PM vignesh C
> wrote:
> >
> > > 2) I'm not sure if this will add any extra coverage as the altering
> > > value of min_apply_delay is already tested in the regression, if so
> > > this test can be
> > > removed:
> > > +# Test ALTER SUBSCRIPTION. Delay 86460 seconds (1 day 1 minute).
> > > +$node_subscriber->safe_psql('postgres',
> > > + "ALTER SUBSCRIPTION tap_sub SET (min_apply_delay =
> > > 8646)"
> > > +);
> > > +
> > > +# New row to trigger apply delay.
> > > +$node_publisher->safe_psql('postgres',
> > > + "INSERT INTO test_tab VALUES (0, 'foobar')");
> > > +
> > > +check_apply_delay_log("logical replication apply delay",
> > > +"8000");
> > While addressing this point, I've noticed that there is a behavior
> > difference between physical replication's recovery_min_apply_delay and
> > this feature when stopping the replication during delays.
> >
> > At present, in the latter case,
> > the apply worker exits without applying the suspended transaction
> > after ALTER SUBSCRIPTION DISABLE command for the subscription.
> >
>
> In the previous paragraph, you said the behavior difference while stopping the
> replication but it is not clear from where this DISABLE command comes in that
> scenario.
Sorry for my unclear description. I mean "stopping the replication" is
to disable the subscription during the "min_apply_delay" wait time on logical
replication setup.
I proposed and mentioned this discussion point to define
how the time-delayed apply worker should behave when there is a transaction
delayed by "min_apply_delay" parameter and additionally the user issues
ALTER SUBSCRIPTION ... DISABLE during the delay. When it comes to physical
replication, it's hard to find a perfect correspondent for LR's ALTER
SUBSCRIPTION
DISABLE command, but I chose a scenario to promote a secondary during
"recovery_min_apply_delay" for comparison this time. After the promotion of
the secondary in the physical replication, the transaction
committed on the publisher but delayed on the secondary can be seen.
This would be because CheckForStandbyTrigger in recoveryApplyDelay returns true
and we apply the record by breaking the wait.
I checked and got the LOG message "received promote request" in the secondary
log
when I tested this case.
> > Meanwhile, there is no "disabling" command for physical replication,
> > but I checked the behavior about what happens for promoting a
> > secondary during the delay of recovery_min_apply_delay for physical
> replication as one example.
> > The transaction has become visible even in the promoting in the middle of
> delay.
> >
>
> What causes such a transaction to be visible after promotion? Ideally, if the
> commit doesn't succeed, the transaction shouldn't be visible.
> Do, we allow the transaction waiting due to delay to get committed on
> promotion?
The commit succeeded on the primary and then I promoted the secondary
during the "recovery_min_apply_delay" wait of the transaction. Then, the result
is the transaction turned out to be available on the promoted secondary.
> > I'm not sure if I should make the time-delayed LR aligned with this
> > behavior.
> > Does someone has an opinion for this ?
> >
>
> Can you please explain a bit more as asked above to understand the
> difference?
So, the current difference is that the time-delayed apply
worker of logical replication doesn't apply the delayed transaction on the
subscriber
when the subscription has been disabled during the delay, while (in one example
of a promotion) the physical replication does the apply of the delayed
transaction.
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi,
On Saturday, January 14, 2023 3:27 PM vignesh C wrote:
> 1) Since the min_apply_delay = 3, but you have specified 2s, there might be a
> possibility that it can log delay as 1000ms due to pub/sub/network delay and
> the test can fail randomly, If we cannot ensure this log file value,
> check_apply_delay_time verification alone should be sufficient.
> +is($result, qq(5|1|5), 'check if the new rows were applied to
> +subscriber');
> +
> +check_apply_delay_log("logical replication apply delay", "2000");
You are right. Removed the left-time check of the 1st call of
check_apply_delay_log().
> 2) I'm not sure if this will add any extra coverage as the altering value of
> min_apply_delay is already tested in the regression, if so this test can be
> removed:
> +# Test ALTER SUBSCRIPTION. Delay 86460 seconds (1 day 1 minute).
> +$node_subscriber->safe_psql('postgres',
> + "ALTER SUBSCRIPTION tap_sub SET (min_apply_delay =
> 8646)"
> +);
> +
> +# New row to trigger apply delay.
> +$node_publisher->safe_psql('postgres',
> + "INSERT INTO test_tab VALUES (0, 'foobar')");
> +
> +check_apply_delay_log("logical replication apply delay", "8000");
While addressing this point, I've noticed that there is a
behavior difference between physical replication's recovery_min_apply_delay
and this feature when stopping the replication during delays.
At present, in the latter case,
the apply worker exits without applying the suspended transaction
after ALTER SUBSCRIPTION DISABLE command for the subscription.
Meanwhile, there is no "disabling" command for physical replication,
but I checked the behavior about what happens for promoting a secondary
during the delay of recovery_min_apply_delay for physical replication as one
example.
The transaction has become visible even in the promoting in the middle of delay.
I'm not sure if I should make the time-delayed LR aligned with this behavior.
Does someone has an opinion for this ?
By the way, the above test code can be used for the test case
when the apply worker is in a delay but the transaction has been canceled by
ALTER SUBSCRIPTION DISABLE command. So, I didn't remove it at this stage.
> 3) We generally keep the subroutines before the tests, it can be kept
> accordingly:
> 3.a)
> +sub check_apply_delay_log
> +{
> + my ($message, $expected) = @_;
> + $expected = 0 unless defined $expected;
> +
> + my $old_log_location = $log_location;
>
> 3.b)
> +sub check_apply_delay_time
> +{
> + my ($primary_key, $expected_diffs) = @_;
> +
> + my $inserted_time_on_pub = $node_publisher->safe_psql('postgres',
> qq[
> + SELECT extract(epoch from c) FROM test_tab WHERE a =
> $primary_key;
> + ]);
> +
Fixed.
> 4) typo "more then once" should be "more than once"
> + regress_testsub | regress_subscription_user | f |
> {testpub,testpub1,testpub2} | f | off | d|
> f| any|0 | off
> | dbname=regress_doesnotexist | 0/0
> (1 row)
>
> -- fail - publication used more then once @@ -316,10 +316,10 @@ ERROR:
> publication "testpub3" is not in subscription "regress_testsub"
> -- ok - delete publications
> ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1,
> testpub2 WITH (refresh = false);
> \dRs+
This was an existing typo on HEAD. Addressed in other thread in [1].
> 5) This can be changed to "Is it larger than expected?"
> + # Is it larger than expected ?
> + cmp_ok($logged_delay, '>', $expected,
> + "The wait time of the apply worker is long
> enough expectedly"
> + );
Fixed.
> 6) 2022 should be changed to 2023
> +++ b/src/test/subscription/t/032_apply_delay.pl
> @@ -0,0 +1,170 @@
> +
> +# Copyright (c) 2022, PostgreSQL Global Development Group
> +
> +# Test replication apply delay
Fixed.
> 7) Termination full stop is not required for single line comments:
> 7.a)
> +use Test::More;
> +
> +# Create publisher node.
> +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
>
> 7.b) +
> +# Create subscriber node.
> +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
>
> 7.c) +
> +# Create some preexisting content on publisher.
> +$node_publisher->safe_psql('postgres',
>
> 7.d) similarly in rest of the files
Removed the periods for single line comments.
> 8) Is it possible to add one test for spooling also?
There is a streaming transaction case in the TAP test already.
I conducted some minor comment modifications along with above changes.
Kindly have a look at the v16.
[1] -
https://www.postgresql.org/message-id/flat/TYCPR01MB83737EA140C79B7D099F65E8EDC69%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
v16-0001-Time-delayed-logical-replication-subscriber.patch
Description: v16-0001-Time-delayed-logical-replication-subscriber.patch
typo in the subscription command tests
Hi, While working on some logical replication patch, I've find a typo on HEAD. Attached the modification patch for this. Best Regards, Takamichi Osumi v1-0001-Fix-a-typo-in-subscription-command-tests.patch Description: v1-0001-Fix-a-typo-in-subscription-command-tests.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Melih On Thursday, January 12, 2023 10:12 PM Melih Mutlu wrote: > I've a question about 032_apply_delay.pl. > ... > I couldn't quite see how these lines test whether ALTER SUBSCRIPTION > successfully worked. > Don't we need to check that min_apply_delay really changed as a result? Yeah, we should check it from the POV of apply worker's debug logs. The latest patch posted in [1] addressed your concern, by checking the logged delay time in the server log. I'd say what we could do is to check the logged time is long enough after the ALTER SUBSCRIPTION command. Please have a look at the patch. [1] - https://www.postgresql.org/message-id/TYCPR01MB83739C6133B50DDA8BAD1601EDFD9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, Shveta Thanks for your comments! On Thursday, January 12, 2023 6:51 PM shveta malik wrote: > > Yes, DBAs may set wal_receiver_status_interval to more than > > wal_sender_timeout by mistake. > > > > But to handle the scenario we must compare between min_apply_delay *on > > subscriber* and wal_sender_timeout *on publisher*. Both values are not > > transferred to opposite sides, so the WARNING cannot be raised. I > > considered that such a mechanism seemed to be complex. The discussion > around [1] may be useful. > > > > [1]: > > > https://www.postgresql.org/message-id/CAA4eK1Lq%2Bh8qo%2BrqGU-E%2B > hwJK > > AHYocV54y4pvou4rLysCgYD-g%40mail.gmail.com > > > > okay, I see. So even when 'wal_receiver_status_interval' is set to 0, no > log/warning is needed when the user tries to set min_apply_delay>0? > Are we good with doc alone? Yes. As far as I can remember, we don't emit log or warning for some kind of combination of those parameters (in the context of timeout too). So, it should be fine. > One trivial correction in config.sgml: > + terminates due to the timeout errors. Hence, make sure this parameter > + shorter than the wal_sender_timeout of the > publisher. > Hence, make sure this parameter is shorter... Fixed. Kindly have a look at the latest patch shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB83739C6133B50DDA8BAD1601EDFD9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Thursday, January 12, 2023 12:04 PM Kyotaro Horiguchi
wrote:
> At Wed, 11 Jan 2023 12:46:24 +, "Hayato Kuroda (Fujitsu)"
> wrote in
> > them. Which version is better?
>
>
> Some comments by a quick loock, different from the above.
Horiguchi-san, thanks for your review !
> + CONNECTION 'host=192.168.1.50 port=5432 user=foo
> dbname=foodb'
>
> I understand that we (not PG people, but IT people) are supposed to use in
> documents a certain set of special addresses that is guaranteed not to be
> routed in the field.
>
> > TEST-NET-1 : 192.0.2.0/24
> > TEST-NET-2 : 198.51.100.0/24
> > TEST-NET-3 : 203.0.113.0/24
>
> (I found 192.83.123.89 in the postgres_fdw doc, but it'd be another issue..)
Fixed. If necessary we can create another thread for this.
> + if (strspn(tmp, "-0123456789 ") == strlen(tmp))
>
> Do we need to bother spending another memory block for apparent non-digits
> here?
Yes. The characters are necessary to handle an issue reported in [1].
The issue happened if the user inputs a negative value,
then the length comparison became different between strspn and strlen
and the input value was recognized as seconds, when
the unit wasn't described. This led to a wrong error message for the user.
Those addition of such characters solve the issue.
> + errmsg(INT64_FORMAT " ms
> is outside the valid range for parameter
> +\"%s\"",
>
> We don't use INT64_FORMAT in translatable message strings. Cast then
> use %lld instead.
Thanks for teaching us. Fixed.
> This message looks unfriendly as it doesn't suggest the valid range, and it
> shows the input value in a different unit from what was in the input. A I
> think we
> can spell it as "\"%s\" is outside the valid range for subsciription parameter
> \"%s\" (0 .. in millisecond)"
Makes sense. I incorporated the valid range with the aligned format of
recovery_min_apply_delay.
FYI, the physical replication's GUC doesn't write the unites for the range like
below.
I followed and applied this style.
---
LOG: -1 ms is outside the valid range for parameter "recovery_min_apply_delay"
(0 .. 2147483647)
FATAL: configuration file
"/home/k5user/new/pg/l/make_v15/slave/postgresql.conf" contains errors
---
> + int64 min_apply_delay;
> ..
> + if (ms < 0 || ms > INT_MAX)
>
> Why is the variable wider than required?
You are right. Fixed.
> + errmsg("%s and %s are mutually
> exclusive options",
> +"min_apply_delay > 0",
> "streaming = parallel"));
>
> Mmm. Couldn't we refuse 0 as min_apply_delay?
Sorry, the previous patch's behavior wasn't consistent with this error message.
In the previous patch, if we conducted alter subscription
with stream = parallel and min_apply_delay = 0 (from a positive value) at the
same time,
the alter command failed, although this should succeed by this time-delayed
feature specification.
We fixed this part accordingly by some more tests in AlterSubscription().
By the way, we should allow users to change min_apply_dealy to 0
whenever they want from different value. Then, we didn't restrict
this kind of operation.
> + sub->minapplydelay > 0)
> ...
> + if (opts.min_apply_delay > 0 &&
>
> Is there any reason for the differenciation?
Yes. The former is the object for an existing subscription configuration.
For example, if we alter subscription with setting streaming = 'parallel'
for a subscription created with min_apply_delay = '1 day', we
need to reject the alter command. The latter is new settings.
> +
> errmsg("cannot set %s for subscription with %s",
> +
> "streaming = parallel", "min_apply_delay > 0"));
>
> I think that this shoud be more like human-speking. Say, "cannot enable
> min_apply_delay for subscription in parallel streaming mode" or something..
> The same is applicable to the nearby message.
Reworded the error messages. Please check.
> +static void maybe_delay_apply(TimestampTz ts);
>
> apply_spooled_messages(FileSet *stream_fileset, TransactionId xid,
> -XLogRecPtr lsn)
> +XLogRecPtr lsn, TimestampTz ts)
>
> "ts" looks too generic. Couldn't it be more specific?
> We need a explanation for the parameter in the function comment.
Changed it to finish_ts, since it indicates commit/prepare time.
This terminology should be aligned with finish lsn.
> + if (!am_parallel_apply_worker())
> + {
> + Assert(ts > 0);
> + maybe_delay_apply(ts);
>
> It seems to me better that the if condition and assertion are checked inside
> maybe_delay_apply().
Fixed.
[1] -
https://www.postgresql.org/message-id/CALDaNm3Bpzhh60nU-keuGxMPb-OhcqsfpCN3ysfCfCJ-2ShYPA%40mail.gmail.com
Best Regards,
Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
On Wednesday, January 11, 2023 7:45 PM Melih Mutlu
wrote:
> Thanks for your review.
> Done.
Hi, minor comments on v5.
(1) publisher's version check
+ /* If the publisher is v16 or later, copy in binary format.*/
+ server_version = walrcv_server_version(LogRepWorkerWalRcvConn);
+ if (server_version >=16 && MySubscription->binary)
+ {
+ appendStringInfoString(, " WITH (FORMAT binary)");
+ options = lappend(options, makeDefElem("format", (Node *)
makeString("binary"), -1));
+ }
+
+ elog(LOG, "version: %i, %s", server_version, cmd.data);
(1-1) There is no need to log the version and the query by elog here.
(1-2) Also, I suggest we can remove the server_version variable itself,
because we have only one actual reference for it.
There is a style that we call walrcv_server_version in the
'if condition' directly like existing codes in fetch_remote_table_info().
(1-3) Suggestion to improve comments.
FROM:
/* If the publisher is v16 or later, copy in binary format.*/
TO:
/* If the publisher is v16 or later, copy data in the required data
format. */
(2) Minor suggestion for some test code alignment.
$result =
$node_subscriber->safe_psql('postgres',
"SELECT sum(a) FROM tst_dom_constr");
-is($result, '21', 'sql-function constraint on domain');
+is($result, '33', 'sql-function constraint on domain');
+
+$result_binary =
+ $node_subscriber->safe_psql('postgres',
+ "SELECT sum(a) FROM tst_dom_constr");
+is($result_binary, '33', 'sql-function constraint on domain');
I think if we change the order of this part of check like below, then
it would look more aligned with other existing test codes introduced by this
patch.
---
my $domain_check = 'SELECT sum(a) FROM tst_dom_constr';
$result = $node_subscriber->safe_psql('postgres', $domain_check);
$result_binary = $node_subscriber->safe_psql('postgres', $domain_check);
is($result, '33', 'sql-function constraint on domain');
is($result_binary, '33', 'sql-function constraint on domain in binary');
---
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, January 10, 2023 11:28 AM I wrote: > On Tuesday, December 27, 2022 6:29 PM Tuesday, December 27, 2022 6:29 PM > wrote: > > Thanks for reviewing our patch! PSA new version patch set. > Now, the patches fails to apply to the HEAD, because of recent commits > (c6e1f62e2c and 216a784829c) as reported in [1]. > > I'll rebase the patch with other changes when I post a new version. This is done in the patch in [1]. Please note that because of the commit c6e1f62e2c, we don't need the 1st patch we borrowed from another thread in [2] any more. [1] - https://www.postgresql.org/message-id/TYCPR01MB837340F78F4A16F542589195EDFF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com [2] - https://www.postgresql.org/message-id/flat/20221122004119.GA132961%40nathanxps13 Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, January 3, 2023 8:22 PM shveta malik wrote: > Please find a few minor comments. Thanks for your review ! > 1. > + diffms = TimestampDifferenceMilliseconds(GetCurrentTimestamp(), > + > >TimestampTzPlusMilliseconds(ts, MySubscription->minapplydelay)); on > unix, above code looks unaligned (copied from unix) > > 2. same with: > + interval = DatumGetIntervalP(DirectFunctionCall3(interval_in, > + > >CStringGetDatum(val), > + > >ObjectIdGetDatum(InvalidOid), > + > >Int32GetDatum(-1))); > perhaps due to tabs? Those patches indentation look OK. I checked them by pgindent and less command described in [1]. So, I didn't change those. > 2. comment not clear: > + * During the time delayed replication, avoid reporting the suspended > + * latest LSN are already flushed and written, to the publisher. You are right. I fixed this part to make it clearer. Could you please check ? > 3. > + * Call send_feedback() to prevent the publisher from exiting by > + * timeout during the delay, when wal_receiver_status_interval is > + * available. The WALs for this delayed transaction is neither > + * written nor flushed yet, Thus, we don't make the latest LSN > + * overwrite those positions of the update message for this delay. > > yet, Thus, we --> yet, thus, we/ yet. Thus, we Yeah, you are right. But, I have removed the last sentence, because the last one explains some internals of send_feedback(). I judged that it would be awkward to describe it in maybe_delay_apply(). Now this part has become concise. > 4. > + /* Adds portion time (in ms) to the previous result. */ > + ms = interval->time / INT64CONST(1000); > Is interval->time always in micro-seconds here? Yeah, it seems so. Some internal codes indicate it. Kindly have a look at functions such as make_interval() and interval2itm(). Please have a look at the latest patch v12 in [2]. [1] - https://www.postgresql.org/docs/current/source-format.html [2] - https://www.postgresql.org/message-id/TYCPR01MB837340F78F4A16F542589195EDFF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, January 3, 2023 4:01 PM vignesh C 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, ))
> + 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, ))
> + 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:
> +
> + The length of time (ms) to delay the application of changes.
> +
> 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
Best Regards,
Takamichi Osumi
v13-0001-Time-delayed-logical-replication-subscriber.patch
Description: v13-0001-Time-delayed-logical-replication-subscriber.patch
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, December 27, 2022 6:29 PM Tuesday, December 27, 2022 6:29 PM wrote: > Thanks for reviewing our patch! PSA new version patch set. Now, the patches fails to apply to the HEAD, because of recent commits (c6e1f62e2c and 216a784829c) as reported in [1]. I'll rebase the patch with other changes when I post a new version. [1] - http://cfbot.cputube.org/patch_41_3581.log Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Thursday, December 22, 2022 3:02 PM Takamichi Osumi (Fujitsu) wrote: > Attached the updated patch. > Again, I used one basic patch in another thread to wake up logical replication > worker shared in [2] for TAP tests. The v11 caused a cfbot failure in [1]. But, failed tests looked irrelevant to the feature to me at present. While waiting for another test execution of cfbot, I'd like to check the detailed reason and update the patch if necessary. [1] - https://cirrus-ci.com/task/4580705867399168 Best Regards, Takamichi Osumi
RE: Support logical replication of DDLs
On Thursday, December 22, 2022 2:22 AM vignesh C wrote: > I have handled most of the comments for [1] in the v55 version patch attached. > I will handle the pending comments in the upcoming version and reply to it. > [1] - > https://www.postgresql.org/message-id/20221207122041.hbfj4hen3ibhdzgn% > 40alvherre.pgsql Hi, Vignesh FYI, cfbot causes a failure for v55 in [1]. Could you check it ? [1] - https://cirrus-ci.com/task/6366800263249920 Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Thursday, December 15, 2022 12:53 PM Amit Kapila wrote: > On Thu, Dec 15, 2022 at 7:16 AM Kyotaro Horiguchi > wrote: > > > > At Wed, 14 Dec 2022 10:46:17 +, "Hayato Kuroda (Fujitsu)" > > wrote in > > > I have implemented and tested that workers wake up per > > > wal_receiver_timeout/2 and send keepalive. Basically it works well, but I > found two problems. > > > Do you have any good suggestions about them? > > > > > > 1) > > > > > > With this PoC at present, workers calculate sending intervals based > > > on its wal_receiver_timeout, and it is suppressed when the parameter is > > > set > to zero. > > > > > > This means that there is a possibility that walsender is timeout > > > when wal_sender_timeout in publisher and wal_receiver_timeout in > subscriber is different. > > > Supposing that wal_sender_timeout is 2min, wal_receiver_tiemout is > > > 5min, > > > > It seems to me wal_receiver_status_interval is better for this use. > > It's enough for us to docuemnt that "wal_r_s_interval should be > > shorter than wal_sener_timeout/2 especially when logical replication > > connection is using min_apply_delay. Otherwise you will suffer > > repeated termination of walsender". > > > > This sounds reasonable to me. Okay, I changed the time interval to wal_receiver_status_interval and added this description about timeout. FYI, wal_receiver_status_interval by definition specifies the minimum frequency for the WAL receiver process to send information to the upstream. So I utilized the value for WaitLatch directly. My descriptions of the documentation change follow it. > > > and min_apply_delay is 10min. The worker on subscriber will wake up > > > per 2.5min and send keepalives, but walsender exits before the message > arrives to publisher. > > > > > > One idea to avoid that is to send the min_apply_delay subscriber > > > option to publisher and compare them, but it may be not sufficient. > > > Because XXX_timout GUC parameters could be modified later. > > > > # Anyway, I don't think such asymmetric setup is preferable. > > > > > 2) > > > > > > The issue reported by Vignesh-san[1] has still remained. I have > > > already analyzed that [2], the root cause is that flushed WAL is not > > > updated and sent to the publisher. Even if workers send keepalive > > > messages to pub during the delay, the flushed position cannot be modified. > > > > I didn't look closer but the cause I guess is walsender doesn't die > > until all WAL has been sent, while logical delay chokes replication > > stream. For the (2) issue, a new thread has been created independently from this thread in [1]. I'll leave any new changes to the thread on this point. Attached the updated patch. Again, I used one basic patch in another thread to wake up logical replication worker shared in [2] for TAP tests. [1] - https://www.postgresql.org/message-id/tyapr01mb586668e50fc2447ad7f92491f5...@tyapr01mb5866.jpnprd01.prod.outlook.com [2] - https://www.postgresql.org/message-id/flat/20221122004119.GA132961%40nathanxps13 Best Regards, Takamichi Osumi v11-0001-wake-up-logical-workers-as-needed-instead-of-rel.patch Description: v11-0001-wake-up-logical-workers-as-needed-instead-of-rel.patch v11-0002-Time-delayed-logical-replication-subscriber.patch Description: v11-0002-Time-delayed-logical-replication-subscriber.patch
assertion failures on BuildFarm that happened in slab.c
Hi, hackers
I've found two assertion failures on BuildFarm [1][2].
The call stack can be found in [2].
TRAP: failed Assert("dlist_is_empty(blocklist)"), File: "slab.c", Line: 564,
PID: 16148
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(ExceptionalCondition+0x54)[0x983564]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(SlabAlloc+0x50d)[0x9b554d]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(MemoryContextAlloc+0x4c)[0x9b2b2c]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(ReorderBufferGetChange+0x15)[0x7d62d5]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(heap_decode+0x4d1)[0x7cb071]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(LogicalDecodingProcessRecord+0x63)[0x7ca033]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION[0x7f1962]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION[0x7f42e2]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(exec_replication_command+0xa88)[0x7f4ec8]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(PostgresMain+0x524)[0x847a44]
2022-12-20 13:18:39.725 UTC [16167:4] 015_stream.pl LOG: disconnection:
session time: 0:00:00.019 user=postgres database=postgres host=[local]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION[0x7b57e6]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(PostmasterMain+0xe27)[0x7b6757]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(main+0x683)[0x4c74e3]
/lib/libc.so.6(__libc_start_main+0xf0)[0x7f5179a8d070]
postgres: publisher: walsender postgres postgres [local]
START_REPLICATION(_start+0x2a)[0x4c75aa]
2022-12-20 13:18:39.795 UTC [16107:4] LOG: server process (PID 16148) was
terminated by signal 6: Aborted
The last failure for subscriptionCheck before those for HEAD happened 66 days
ago [3]
and I checked all failures there within 90 days. There is no similar failure.
I'm not sure, but this could be related to a recent commit (d21ded75fd) in [4].
[1] -
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=butterflyfish=2022-12-20%2013%3A00%3A19
[2] -
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=grassquit=2022-12-20%2010%3A34%3A39
[3] -
https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=90=subscriptionCheck=Submit
[4] -
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d21ded75fdbc18d68be6e6c172f0f842c50e9263
Best Regards,
Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, December 13, 2022 1:27 PM Kyotaro Horiguchi wrote: > At Tue, 13 Dec 2022 02:28:49 +, "Takamichi Osumi (Fujitsu)" > wrote in > > On Wednesday, December 7, 2022 12:00 PM Kyotaro Horiguchi > wrote: > > > > We couldn't reproduce this failure and find the same type of failure > > on the cfbot from the past failures. > > It seems no subtests run in your environment. > > Very sorry for that. The test script is found to be a left-over file in a > git-reset'ed > working tree. Please forget about it. > > FWIW, the latest patch passed make-world for me on Rocky8/x86_64. Hi, No problem at all. Also, thank you for your testing and confirming the latest one! Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, December 7, 2022 12:00 PM Kyotaro Horiguchi wrote: > At Tue, 6 Dec 2022 11:08:43 -0800, Andres Freund wrote > in > > Hi, > > > > The tests fail on cfbot: > > https://cirrus-ci.com/task/4533866329800704 > > > > They only seem to fail on 32bit linux. > > > > https://api.cirrus-ci.com/v1/artifact/task/4533866329800704/testrun/bu > > ild-32/testrun/subscription/032_apply_delay/log/regress_log_032_apply_ > > delay > > [06:27:10.628](0.138s) ok 2 - check if the new rows were applied to > > subscriber timed out waiting for match: (?^:logical replication apply > > delay) at > /tmp/cirrus-ci-build/src/test/subscription/t/032_apply_delay.pl line 124. > > It fails for me on 64bit Linux.. (Rocky 8.7) > > > t/032_apply_delay.pl ... Dubious, test returned 29 (wstat > > 7424, 0x1d00) No subtests run > .. > > t/032_apply_delay.pl (Wstat: 7424 Tests: 0 Failed: 0) > > Non-zero exit status: 29 > > Parse errors: No plan found in TAP output Hi, Horiguchi-san Sorry for being late. We couldn't reproduce this failure and find the same type of failure on the cfbot from the past failures. It seems no subtests run in your environment. Could you please share the log files, if you have or when you can reproduce this ? FYI, the latest patch is attached in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB83730C23CB7D29E57368BECDEDE29%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Saturday, December 10, 2022 12:08 AM Takamichi Osumi (Fujitsu) wrote: > On Friday, December 9, 2022 3:38 PM Kuroda, Hayato/黒田 隼人 > wrote: > > Thanks for reporting! I have analyzed the problem and found the root cause. > > > > This feature seemed not to work on 32-bit OSes. This was because the > > calculation of delay_time was wrong. The first argument of this should > > be TimestampTz datatype, not Datum: > > > > ``` > > + /* Set apply delay */ > > + delay_until = > > TimestampTzPlusMilliseconds(TimestampTzGetDatum(ts), > > + > > + MySubscription->applydelay); > > ``` > > > > In more detail, the datum representation of int64 contains the value > > itself on 64-bit OSes, but it contains the pointer to the value on 32-bit. > > > > After modifying the issue, this will work on 32-bit environments. > Thank you for your analysis. > > Yeah, it seems we conduct addition of values to the pointer value, which is > returned from the call of TimestampTzGetDatum(), on 32-bit machine by > mistake. > > I'll remove the call in my next version. Applied this fix in the last version, shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB83730C23CB7D29E57368BECDEDE29%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Friday, November 25, 2022 5:43 AM Peter Smith wrote: > On Fri, Nov 25, 2022 at 2:15 AM Takamichi Osumi (Fujitsu) > wrote: > > > > On Wednesday, October 5, 2022 6:42 PM Peter Smith > wrote: > ... > > > > > == > > > > > > 5. src/backend/commands/subscriptioncmds.c - SubOpts > > > > > > @@ -89,6 +91,7 @@ typedef struct SubOpts > > > bool disableonerr; > > > char*origin; > > > XLogRecPtr lsn; > > > + int64 min_apply_delay; > > > } SubOpts; > > > > > > I feel it would be better to be explicit about the storage units. So > > > call this member ‘min_apply_delay_ms’. E.g. then other code in > > > parse_subscription_options will be more natural when you are > > > converting using and assigning them to this member. > > I don't think we use such names including units explicitly. > > Could you please tell me a similar example for this ? > > > > Regex search "\..*_ms[e\s]" finds some members where the unit is in the > member name. > > e.g. delay_ms (see EnableTimeoutParams in timeout.h) e.g. interval_in_ms (see > timeout_paramsin timeout.c) > > Regex search ".*_ms[e\s]" finds many local variables where the unit is in the > variable name > > > > == > > > > > > 16. src/include/catalog/pg_subscription.h > > > > > > + int64 subapplydelay; /* Replication apply delay */ > > > + > > > > > > Consider renaming this as 'subapplydelayms' to make the units perfectly > clear. > > Similar to the 5th comments, I can't find any examples for this. > > I'd like to keep it general, which makes me feel it is more aligned > > with existing codes. Hi, thank you for sharing this info. I searched the codes where I could feel the merits to add "ms" at the end of the variable names. Adding the unites would help to calculate or convert some time related values. In this patch there is only a couple of functions, like maybe_delay_apply() or for conversion of time, parse_subscription_options. I feel changing just a couple of structures might be awkward, while changing all internal structures is too much. So, I keep the names as those were after some modifications shared in [1]. If you have any better idea, please let me know. [1] - https://www.postgresql.org/message-id/TYCPR01MB83730C23CB7D29E57368BECDEDE29%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Tuesday, December 6, 2022 5:00 PM Peter Smith wrote: > Here are some review comments for patch v9-0001: Hi, thank you for your reviews ! > > == > > GENERAL > > 1. min_ prefix? > > What's the significance of the "min_" prefix for this parameter? I'm guessing > the > background is that at one time it was considered to be a GUC so took a name > similar to GUC recovery_min_apply_delay (??) > > But in practice, I think it is meaningless and/or misleading. For example, > suppose the user wants to defer replication by 1hr. IMO it is more natural to > just say "defer replication by 1 hr" (aka > apply_delay='1hr') Clearly it means replication will take place about > 1 hr into the future. OTHO saying "defer replication by a MINIMUM of 1 hr" > (aka > min_apply_delay='1hr') is quite vague because then it is equally valid if the > replication gets delayed by 1 hr or 2 hrs or 5 days or 3 weeks since all of > those > satisfy the minimum delay. The implementation could hardwire a delay of > INT_MAX ms but clearly, that's not really what the user would expect. > > ~ > > So, I think this parameter should be renamed just as 'apply_delay'. > > But, if you still decide to keep it as 'min_apply_delay' then there is a lot > of other > code that ought to be changed to be consistent with that name. > e.g. > - subapplydelay in catalogs.sgml --> subminapplydelay > - subapplydelay in system_views.sql --> subminapplydelay > - subapplydelay in pg_subscription.h --> subminapplydelay > - subapplydelay in dump.h --> subminapplydelay > - i_subapplydelay in pg_dump.c --> i_subminapplydelay > - applydelay member name of Form_pg_subscription --> minapplydelay > - "Apply Delay" for the column name displayed by describe.c --> "Min apply > delay" I followed the suggestion to keep the "min_" prefix in [1]. Fixed. > - more... > > (IMO the fact that so much code does not currently say 'min' at all is just > evidence that the 'min' prefix really didn't really mean much in the first > place) > > > == > > doc/src/sgml/catalogs.sgml > > 2. Section 31.2 Subscription > > + > + Time delayed replica of subscription is available by indicating > + min_apply_delay. See > +for details. > + > > How about saying like: > > SUGGESTION > The subscriber replication can be instructed to lag behind the publisher side > changes by specifying the min_apply_delay subscription > parameter. See XXX for details. Fixed. > == > > doc/src/sgml/ref/create_subscription.sgml > > 3. min_apply_delay > > + > + By default, subscriber applies changes as soon as possible. As with > + the physical replication feature > + (), it can be useful > to > + have a time-delayed logical replica. This parameter allows you to > + delay the application of changes by a specified amount of time. If > + this value is specified without units, it is taken as milliseconds. > + The default is zero, adding no delay. > + > > "subscriber applies" -> "the subscriber applies" > > "allows you" -> "lets the user" > > "The default is zero, adding no delay." -> "The default is zero (no delay)." Fixed. > ~ > > 4. > > + larger than the time deviations between servers. Note that > + in the case when this parameter is set to a long value, the > + replication may not continue if the replication slot falls behind > the > + current LSN by more than > max_slot_wal_keep_size. > + See more details in . > + > > 4a. > SUGGESTION > Note that if this parameter is set to a long delay, the replication will stop > if the > replication slot falls behind the current LSN by more than > max_slot_wal_keep_size. Fixed. > ~ > > 4b. > When it is rendered (like below) it looks a bit repetitive: > ... if the replication slot falls behind the current LSN by more than > max_slot_wal_keep_size. See more details in max_slot_wal_keep_size. Thanks! Fixed the redundancy. > ~ > > IMO the previous sentence should include the link. > > SUGGESTION > if the replication slot falls behind the current LSN by more than linkend = > "guc-max-slot-wal-keep-size">max_slot_wal_keep_size k>. Fixed. > ~ > > 5. > > + > +Synchronous replication is affected by this setting when > +synchronous_commit is set to > +remote_write; every COMMIT > +will need to wait to be applied. > + > > Yes, this deserves a big warning -- but I am just not quite sure of the > details. I > think this impacts more than just "remote_rewrite" -- e.g. the same problem > would happen if "synchronous_standby_names" is non-empty. > > I think this warning needs to be more generic to cover everything. > Maybe something like below > > SUGGESTION: > Delaying the replication can mean there is a much longer time between making > a change on the publisher, and that change being committed on the subscriber. > This can
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Monday, December 12, 2022 2:54 PM Kyotaro Horiguchi wrote: > I asked about unexpected walsender termination caused by this feature but I > think I didn't received an answer for it and the behavior is still exists. > > Specifically, if servers have the following settings, walsender terminates for > replication timeout. After that, connection is restored after the LR delay > elapses. > Although it can be said to be working in that sense, the error happens > repeatedly every about min_apply_delay internvals but is hard to distinguish > from network troubles. I'm not sure you're deliberately okay with it but, I > don't > think the behavior causing replication timeouts is acceptable. > > > wal_sender_timeout = 10s; > > wal_receiver_temeout = 10s; > > > > create subscription ... with (min_apply_delay='60s'); > > This is a kind of artificial but timeout=60s and delay=5m is not an uncommon > setup and that also causes this "issue". > > subscriber: > > 2022-12-12 14:17:18.139 JST LOG: terminating walsender process due to > > replication timeout > > 2022-12-12 14:18:11.076 JST LOG: starting logical decoding for slot "s1" > ... Hi, Horiguchi-san Thank you so much for your report! Yes. Currently, how to deal with the timeout issue is under discussion. Some analysis about the root cause are also there. Kindly have a look at [1]. [1] - https://www.postgresql.org/message-id/TYAPR01MB58669394A67F2340B82E42D1F5E29%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, December 7, 2022 2:07 PM Amit Kapila wrote: > On Tue, Dec 6, 2022 at 5:44 PM Takamichi Osumi (Fujitsu) > wrote: > > > > On Friday, December 2, 2022 4:05 PM Amit Kapila > wrote: > > > On Tue, Nov 15, 2022 at 12:33 PM Amit Kapila > > > > > > wrote: > > > > One more thing I would like you to consider is the point raised by > > > > me related to this patch's interaction with the parallel apply > > > > feature as mentioned in the email [1]. I am not sure the idea > > > > proposed in that email [1] is a good one because delaying after > > > > applying commit may not be good as we want to delay the apply of > > > > the transaction(s) on subscribers by this feature. I feel this needs > > > > more > thought. > > > > > > > > > > I have thought a bit more about this and we have the following > > > options to choose the delay point from. (a) apply delay just before > > > committing a transaction. As mentioned in comments in the patch this > > > can lead to bloat and locks held for a long time. (b) apply delay > > > before starting to apply changes for a transaction but here the > > > problem is which time to consider. In some cases, like for streaming > > > transactions, we don't receive the commit/prepare xact time in the start > message. (c) use (b) but use the previous transaction's commit time. > > > (d) apply delay after committing a transaction by using the xact's commit > time. > > > > > > At this stage, among above, I feel any one of (c) or (d) is worth > > > considering. Now, the difference between (c) and (d) is that if > > > after commit the next xact's data is already delayed by more than > > > min_apply_delay time then we don't need to kick the new logic of apply > delay. > > > > > > The other thing to consider whether we need to process any keepalive > > > messages during the delay because otherwise, walsender may think > > > that the subscriber is not available and time out. This may not be a > > > problem for synchronous replication but otherwise, it could be a problem. > > > > > > Thoughts? > > (1) About the timing to apply the delay > > > > One approach of (b) would be best. The idea is to delay all types of > > transaction's application based on the time when one transaction arrives at > the subscriber node. > > > > But I think it will unnecessarily add the delay when there is a delay in > sending > the transaction by the publisher (say due to the reason that publisher was > busy > handling other workloads or there was a temporary network communication > break between publisher and subscriber). This could probably be the reason > why physical replication (via recovery_min_apply_delay) uses the commit time > of > the sending side. You are right. The approach (b) adds additional (or unnecessary) delay due to network communication or machine troubles in streaming-in-progress cases. We agreed this approach (b) has the disadvantage. > > One advantage of this approach over (c) and (d) is that this can avoid > > the case where we might apply a transaction immediately without > > waiting, if there are two transactions sequentially and the time in between > exceeds the min_apply_delay time. > > > > I am not sure if I understand your point. However, I think even if the > transactions are sequential but if the time between them exceeds (say because > the publisher was down) min_apply_delay, there is no need to apply additional > delay. I'm sorry, my description was not accurate. As for the approach (c), kindly imagine two transactions (txn1, txn2) are executed on the publisher side and the publisher tries to send both of them to the subscriber. Here, there is no network trouble and the publisher isn't busy for other workloads. However, the diff of the time between txn1 and txn2 execeeds "min_apply_delay" (which is set to the subscriber). In this case, when the txn2 is a stream-in-progress transaction, we don't apply any delay for txn2 when it arrives on the subscriber. It's because before txn2 comes to the subscriber, "min_apply_delay" has already passed on the publisher side. This means there's a case we don't apply any delay when we choose approach (c). The approach (d) has also similar disadvantage. IIUC, in this approach the subscriber applies delay after committing a transaction, based on the commit/prepare time of publisher side. Kindly, imagine two transactions are executed on the publisher and the 2nd transaction completes after the subscriber's delay for the 1st transaction. Again, there is no net
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Friday, December 9, 2022 3:38 PM Kuroda, Hayato/黒田 隼人 wrote: > Thanks for reporting! I have analyzed the problem and found the root cause. > > This feature seemed not to work on 32-bit OSes. This was because the > calculation of delay_time was wrong. The first argument of this should be > TimestampTz datatype, not Datum: > > ``` > + /* Set apply delay */ > + delay_until = > TimestampTzPlusMilliseconds(TimestampTzGetDatum(ts), > + > + MySubscription->applydelay); > ``` > > In more detail, the datum representation of int64 contains the value itself on > 64-bit OSes, but it contains the pointer to the value on 32-bit. > > After modifying the issue, this will work on 32-bit environments. Thank you for your analysis. Yeah, it seems we conduct addition of values to the pointer value, which is returned from the call of TimestampTzGetDatum(), on 32-bit machine by mistake. I'll remove the call in my next version. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, December 7, 2022 12:00 PM Kyotaro Horiguchi wrote: > At Tue, 6 Dec 2022 11:08:43 -0800, Andres Freund wrote > in > > Hi, > > > > The tests fail on cfbot: > > https://cirrus-ci.com/task/4533866329800704 > > > > They only seem to fail on 32bit linux. > > > > https://api.cirrus-ci.com/v1/artifact/task/4533866329800704/testrun/bu > > ild-32/testrun/subscription/032_apply_delay/log/regress_log_032_apply_ > > delay > > [06:27:10.628](0.138s) ok 2 - check if the new rows were applied to > > subscriber timed out waiting for match: (?^:logical replication apply > > delay) at > /tmp/cirrus-ci-build/src/test/subscription/t/032_apply_delay.pl line 124. > > It fails for me on 64bit Linux.. (Rocky 8.7) > > > t/032_apply_delay.pl ... Dubious, test returned 29 (wstat > > 7424, 0x1d00) No subtests run > .. > > t/032_apply_delay.pl (Wstat: 7424 Tests: 0 Failed: 0) > > Non-zero exit status: 29 > > Parse errors: No plan found in TAP output > > regards. Hi, thank you so much for your notifications ! I'll look into the failures. Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
On Friday, December 2, 2022 4:05 PM Amit Kapila wrote: > On Tue, Nov 15, 2022 at 12:33 PM Amit Kapila > wrote: > > One more thing I would like you to consider is the point raised by me > > related to this patch's interaction with the parallel apply feature as > > mentioned in the email [1]. I am not sure the idea proposed in that > > email [1] is a good one because delaying after applying commit may not > > be good as we want to delay the apply of the transaction(s) on > > subscribers by this feature. I feel this needs more thought. > > > > I have thought a bit more about this and we have the following options to > choose the delay point from. (a) apply delay just before committing a > transaction. As mentioned in comments in the patch this can lead to bloat and > locks held for a long time. (b) apply delay before starting to apply changes > for a > transaction but here the problem is which time to consider. In some cases, > like > for streaming transactions, we don't receive the commit/prepare xact time in > the start message. (c) use (b) but use the previous transaction's commit time. > (d) apply delay after committing a transaction by using the xact's commit > time. > > At this stage, among above, I feel any one of (c) or (d) is worth > considering. Now, > the difference between (c) and (d) is that if after commit the next xact's > data is > already delayed by more than min_apply_delay time then we don't need to kick > the new logic of apply delay. > > The other thing to consider whether we need to process any keepalive > messages during the delay because otherwise, walsender may think that the > subscriber is not available and time out. This may not be a problem for > synchronous replication but otherwise, it could be a problem. > > Thoughts? Hi, Thank you for your comments ! Below are some analysis for the major points above. (1) About the timing to apply the delay One approach of (b) would be best. The idea is to delay all types of transaction's application based on the time when one transaction arrives at the subscriber node. One advantage of this approach over (c) and (d) is that this can avoid the case where we might apply a transaction immediately without waiting, if there are two transactions sequentially and the time in between exceeds the min_apply_delay time. When we receive stream-in-progress transactions, we'll check whether the time for delay has passed or not at first in this approach. (2) About the timeout issue When having a look at the physical replication internals, it conducts sending feedback and application of delay separately on different processes. OTOH, the logical replication needs to achieve those within one process. When we want to apply delay and avoid the timeout, we should not store all the transactions data into memory. So, one approach for this is to serialize the transaction data and after the delay, we apply the transactions data. However, this means if users adopt this feature, then all transaction data that should be delayed would be serialized. We are not sure if this sounds a valid approach or not. One another approach was to divide the time of delay in apply_delay() and utilize the divided time for WaitLatch and sends the keepalive messages from there. But, this approach requires some change on the level of libpq layer (like implementing a new function for wal receiver in order to monitor if the data from the publisher is readable or not there). Probably, the first idea to serialize the delayed transactions might be better on this point. Any feedback is welcome. Best Regards, Takamichi Osumi
RE: logical replication restrictions
Hi, On Thursday, August 11, 2022 7:33 PM Amit Kapila wrote: > On Tue, Aug 9, 2022 at 3:52 AM Euler Taveira wrote: > > > > On Wed, Aug 3, 2022, at 10:27 AM, Amit Kapila wrote: > > > > Your explanation makes sense to me. The other point to consider is > > that there can be cases where we may not apply operation for the > > transaction because of empty transactions (we don't yet skip empty > > xacts for prepared transactions). So, won't it be better to apply the > > delay just before we apply the first change for a transaction? Do we > > want to apply the delay during table sync as we sometimes do need to > > enter apply phase while doing table sync? > > > > I thought about the empty transactions but decided to not complicate > > the code mainly because skipping transactions is not a code path that > > will slow down this feature. As explained in the documentation, there > > is no harm in delaying a transaction for more than min_apply_delay; it > > cannot apply earlier. Having said that I decided to do nothing. I'm > > also not sure if it deserves a comment or if this email is a possible > > explanation > for this decision. > > > > I don't know what makes you think it will complicate the code. But anyway > thinking further about the way apply_delay is used at various places in the > patch, > as pointed out by Peter Smith it seems it won't work for the parallel apply > feature where we start applying the transaction immediately after start > stream. > I was wondering why don't we apply delay after each commit of the transaction > rather than at the begin command. We can remember if the transaction has > made any change and if so then after commit, apply the delay. If we can do > that > then it will alleviate the concern of empty and skipped xacts as well. I agree with this direction. I'll update this point in a subsequent patch. > Another thing I was wondering how to determine what is a good delay time for > tests and found that current tests in replay_delay.pl uses 3s, so should we > use > the same for apply delay tests in this patch as well? Fixed in the patch posted in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Monday, November 14, 2022 7:15 PM Amit Kapila wrote: > On Wed, Nov 9, 2022 at 12:11 PM Kyotaro Horiguchi > wrote: > > > > At Wed, 10 Aug 2022 17:33:00 -0300, "Euler Taveira" > > wrote in > > > On Wed, Aug 10, 2022, at 9:39 AM, osumi.takami...@fujitsu.com wrote: > > > > Minor review comments for v6. > > > Thanks for your review. I'm attaching v7. > > > > Using interval is not standard as this kind of parameters but it seems > > convenient. On the other hand, it's not great that the unit month > > introduces some subtle ambiguity. This patch translates a month to 30 > > days but I'm not sure it's the right thing to do. Perhaps we shouldn't > > allow the units upper than days. > > > > Agreed. Isn't the same thing already apply to recovery_min_apply_delay for > which the maximum unit seems to be in days? If so, there is no reason to do > something different here? The corresponding one of physical replication had the upper limit of INT_MAX(like it means 24 days is OK, but 25 days isn't). I added this test in the patch posted in [1]. > > > apply_delay() chokes the message-receiving path so that a not-so-long > > delay can cause a replication timeout to fire. I think we should > > process walsender pings even while delaying. Needing to make > > replication timeout longer than apply delay is not great, I think. > > > > Again, I think for this case also the behavior should be similar to how we > handle > recovery_min_apply_delay. Yes, I agree with you. This feature makes it easier to trigger the publisher's timeout, which can't be observed in the physical replication. I'll do the investigation and modify this point in a subsequent version. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Time delayed LR (WAS Re: logical replication restrictions)
Hi, On Tuesday, November 22, 2022 6:15 PM vignesh C wrote: > On Mon, 14 Nov 2022 at 12:14, Amit Kapila wrote: > > > > Hi, > > > > The thread title doesn't really convey the topic under discussion, so > > changed it. IIRC, this has been mentioned by others as well in the > > thread. > > > > On Sat, Nov 12, 2022 at 7:21 PM vignesh C wrote: > > > > > > Few comments: > > > 1) I feel if the user has specified a long delay there is a chance > > > that replication may not continue if the replication slot falls > > > behind the current LSN by more than max_slot_wal_keep_size. I feel > > > we should add this reference in the documentation of min_apply_delay > > > as the replication will not continue in this case. > > > > > > > This makes sense to me. Modified accordingly. The updated patch is in [1]. > > > > > 2) I also noticed that if we have to shut down the publisher server > > > with a long min_apply_delay configuration, the publisher server > > > cannot be stopped as the walsender waits for the data to be > > > replicated. Is this behavior ok for the server to wait in this case? > > > If this behavior is ok, we could add a log message as it is not very > > > evident from the log files why the server could not be shut down. > > > > > > > I think for this case, the behavior should be the same as for physical > > replication. Can you please check what is behavior for the case you > > are worried about in physical replication? Note, we already have a > > similar parameter for recovery_min_apply_delay for physical > > replication. > > In the case of physical replication by setting recovery_min_apply_delay, I > noticed that both primary and standby nodes were getting stopped successfully > immediately after the stop server command. In case of logical replication, > stop > server fails: > pg_ctl -D publisher -l publisher.log stop -c waiting for server to shut > down... > failed > pg_ctl: server does not shut down > > In case of logical replication, the server does not get stopped because the > walsender process is not able to exit: > ps ux | grep walsender > vignesh 1950789 75.3 0.0 8695216 22284 ? Rs 11:51 1:08 > postgres: walsender vignesh [local] START_REPLICATION Thanks, I could reproduce this and I'll update this point in a subsequent version. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
Re: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, November 16, 2022 12:58 PM Ian Lawrence Barwick
wrote:
> 2022年11月14日(月) 10:09 Takamichi Osumi (Fujitsu)
> :
> > I've simply rebased the patch to make it applicable on top of HEAD and
> > make the tests pass. Note there are still open pending comments and
> > I'm going to start to address those.
> Thanks for the updated patch.
>
> While reviewing the patch backlog, we have determined that this patch adds
> one or more TAP tests but has not added the test to the "meson.build" file.
>
> To do this, locate the relevant "meson.build" file for each test and add it
> in the
> 'tests' dictionary, which will look something like this:
>
> 'tap': {
> 'tests': [
> 't/001_basic.pl',
> ],
> },
>
> For some additional details please see this Wiki article:
>
> https://wiki.postgresql.org/wiki/Meson_for_patch_authors
>
> For more information on the meson build system for PostgreSQL see:
>
> https://wiki.postgresql.org/wiki/Meson
Hi, thanks for your notification.
You are right. Modified.
The updated patch can be seen in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
Re: Time delayed LR (WAS Re: logical replication restrictions)
On Wednesday, October 5, 2022 6:42 PM Peter Smith wrote:
> Hi Euler, a long time ago you ask me a few questions about my previous review
> [1].
>
> Here are my replies, plus a few other review comments for patch v7-0001.
Hi, thank you for your comments.
> ==
>
> 1. doc/src/sgml/catalogs.sgml
>
> +
> + Application delay of changes by a specified amount of time. The
> + unit is in milliseconds.
> +
>
> The wording sounds a bit strange still. How about below
>
> SUGGESTION
> The length of time (ms) to delay the application of changes.
Fixed.
> ===
>
> 2. Other documentation?
>
> Maybe should say something on the Logical Replication Subscription page
> about this? (31.2 Subscription)
Added.
> ===
>
> 3. doc/src/sgml/ref/create_subscription.sgml
>
> + synchronized, this may lead to apply changes earlier than expected.
> + This is not a major issue because a typical setting of this
> parameter
> + are much larger than typical time deviations between servers.
>
> Wording?
>
> SUGGESTION
> ... than expected, but this is not a major issue because this parameter is
> typically much larger than the time deviations between servers.
Fixed.
> ~~~
>
> 4. Q/A
>
> From [2] you asked:
>
> > Should there also be a big warning box about the impact if using
> > synchronous_commit (like the other streaming replication page has this
> > warning)?
> Impact? Could you elaborate?
>
> ~
>
> I noticed the streaming replication docs for recovery_min_apply_delay has a
> big
> red warning box saying that setting this GUC may block the synchronous
> commits. So I was saying won’t a similar big red warning be needed also for
> this min_apply_delay parameter if the delay is used in conjunction with a
> publisher wanting synchronous commit because it might block everything?
I agree with you. Fixed.
> ~~~
>
> 4. Example
>
> +
> +CREATE SUBSCRIPTION foo
> + CONNECTION 'host=192.168.1.50 port=5432 user=foo
> dbname=foodb'
> +PUBLICATION baz
> + WITH (copy_data = false, min_apply_delay = '4h');
> +
>
> If the example named the subscription/publication as ‘mysub’ and ‘mypub’ I
> think it would be more consistent with the existing examples.
Fixed.
> ==
>
> 5. src/backend/commands/subscriptioncmds.c - SubOpts
>
> @@ -89,6 +91,7 @@ typedef struct SubOpts
> bool disableonerr;
> char*origin;
> XLogRecPtr lsn;
> + int64 min_apply_delay;
> } SubOpts;
>
> I feel it would be better to be explicit about the storage units. So call this
> member ‘min_apply_delay_ms’. E.g. then other code in
> parse_subscription_options will be more natural when you are converting using
> and assigning them to this member.
I don't think we use such names including units explicitly.
Could you please tell me a similar example for this ?
> ~~~
>
> 6. - parse_subscription_options
>
> + /*
> + * If there is no unit, interval_in takes second as unit. This
> + * parameter expects millisecond as unit so add a unit (ms) if
> + * there isn't one.
> + */
>
> The comment feels awkward. How about below
>
> SUGGESTION
> If no unit was specified, then explicitly add 'ms' otherwise the interval_in
> function would assume 'seconds'
Fixed.
> ~~~
>
> 7. - parse_subscription_options
>
> (This is a repeat of [1] review comment #12)
>
> + if (opts->min_apply_delay < 0 && IsSet(supported_opts,
> SUBOPT_MIN_APPLY_DELAY))
> + ereport(ERROR,
> + errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> + errmsg("option \"%s\" must not be negative", "min_apply_delay"));
>
> Why is this code here instead of inside the previous code block where the
> min_apply_delay was assigned in the first place?
Changed.
> ==
>
> 8. src/backend/replication/logical/worker.c - apply_delay
>
> + * When min_apply_delay parameter is set on subscriber, we wait long
> + enough to
> + * make sure a transaction is applied at least that interval behind the
> + * publisher.
>
> "on subscriber" -> "on the subscription"
Fixed.
> ~~~
>
> 9.
>
> + * Apply delay only after all tablesync workers have reached READY
> + state. A
> + * tablesync worker are kept until it reaches READY state. If we allow
> + the
>
>
> Wording ??
>
> "A tablesync worker are kept until it reaches READY state." ??
I removed the sentence.
> ~~~
>
> 10.
>
> 10a.
> + /* nothing to do if no delay set */
>
> Uppercase comment
> /* Nothing to do if no delay set */
>
> ~
>
> 10b.
> + /* set apply delay */
>
> Uppercase comment
> /* Set apply delay */
Both are fixed.
> ~~~
>
> 11. - apply_handle_stream_prepare / apply_handle_stream_commit
>
> The previous concern about incompatibility with the "Parallel Apply"
> work (see [1] review comments #17, #18) is still a pending issue, isn't it?
Yes, I think so.
Kindly have a look at [1].
> ==
>
> 12. src/backend/utils/adt/timestamp.c interval_to_ms
>
> +/*
> + * Given an Interval returns the number of milliseconds.
> + */
> +int64
>
RE: wake up logical workers after ALTER SUBSCRIPTION
On Tuesday, November 22, 2022 1:39 PM Nathan Bossart wrote: > On Tue, Nov 22, 2022 at 03:03:52AM +, Hayato Kuroda (Fujitsu) wrote: > > Just One comment: IIUC the statement "ALTER SUBSCRIPTION" can be > > executed inside the transaction. So if two subscriptions are altered > > in the same transaction, only one of them will awake. Is it expected > behavior? > > > > I think we can hold a suboid list and record oids when the > > subscription are altered, and then the backend process can consume all > > of list cells at the end of the transaction. > > I think you are correct. I did it this way in v2. I've also moved the bulk > of > the logic to logical/worker.c. Hi, thanks for updating. I just quickly had a look at your patch and had one minor question. With this patch, when we execute alter subscription in a sub transaction and additionally rollback to it, is there any possibility that we'll wake up the workers that don't need to do so ? I'm not sure if this brings about some substantial issue, but just wondering if there is any need of improvement for this. Best Regards, Takamichi Osumi
RE: Assertion failure in SnapBuildInitialSnapshot()
Hi, On Tuesday, November 15, 2022 10:26 AM Andres Freund wrote: > On 2022-11-10 16:04:40 +0530, Amit Kapila wrote: > > I don't have any good ideas on how to proceed with this. Any thoughts > > on this would be helpful? > > One thing worth doing might be to convert the assertion path into an elog(), > mentioning both xids (or add a framework for things like AssertLT(), but that > seems hard). With the concrete values we could make a better guess at > what's going wrong. > > It'd probably not hurt to just perform this check independent of > USE_ASSERT_CHECKING - compared to the cost of creating a slot it's > neglegible. > > That'll obviously only help us whenever we re-encounter the issue, which will > likely be a while... > > > Have you tried reproducing the issue by running the test in a loop? Just FYI, I've tried to reproduce this failure in a loop, but I couldn't hit the same assertion failure. I extracted the #16643 of 100_bugs.pl only and executed the tests more than 500 times. My env and test was done in rhel7.9 and gcc 4.8 with configure option of --enable-cassert --enable-debug --enable-tap-tests --with-icu CFLAGS=-O0 and prefix. Best Regards, Takamichi Osumi
RE: logical replication restrictions
On Tuesday, November 8, 2022 2:27 PM Kuroda, Hayato/黒田 隼人 wrote: > If you could not allocate a time to discuss this problem because of other > important tasks or events, we would like to take over the thread and modify > your patch. > > We've planned that we will start to address comments and reported bugs if > you would not respond by the end of this week. Hi, I've simply rebased the patch to make it applicable on top of HEAD and make the tests pass. Note there are still open pending comments and I'm going to start to address those. I've written Euler as the original author in the commit message to note his credit. Best Regards, Takamichi Osumi v8-0001-Time-delayed-logical-replication-subscriber.patch Description: v8-0001-Time-delayed-logical-replication-subscriber.patch
RE: Data is copied twice when specifying both child and parent table in publication
On Monday, October 17, 2022 2:49 PM Wang, Wei/王 威 wrote: > Attach the new patch set. Hi, thank you for posting the new patches. Here are minor comments on the HEAD_v13-0002. (1) Suggestion for the document description + + If a root partitioned table is published by any subscribed publications which + set publish_via_partition_root = true, changes on this root partitioned table + (or on its partitions) will be published using the identity and schema of this + root partitioned table rather than that of the individual partitions. + + I suppose this sentence looks quite similar to the one in the previous paragraph and can be adjusted. IIUC the main value of the patch is to clarify what happens when we mix publications of different publish_via_partition_root settings for one partition hierarchy. If this is true, how about below sentence instead of the one above ? " There can be a case where a subscription combines publications with different publish_via_partition_root values for one same partition hierarchy (e.g. subscribe two publications indicating the root partitioned table and its child table respectively). In this case, the identity and schema of the root partitioned table take priority. " (2) Better documentation alignment I think we need to wrap publish_via_partition_root by "literal" tag in the documentation create_publication.sgml. Best Regards, Takamichi Osumi
