Subject: RE: Logical replication timeout problem

From: "[email-masked]" <[email-masked]>
Date: Thu, 28 Apr 2022 10:01:31 +0000

On Wednesday, April 20, 2022 3:21 PM Masahiko Sawada <[email-masked]> wrote:
> 
> BTW the changes in
> REL_14_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch,
> adding end_xact to LogicalDecodingContext, seems good to me and it
> might be better than the approach of v17 patch from plugin developers’
> perspective? This is because they won’t need to pass true/false to
> end_xact of  OutputPluginUpdateProgress(). Furthermore, if we do what
> we do in update_replication_progress() in
> OutputPluginUpdateProgress(), what plugins need to do will be just to
> call OutputPluginUpdate() in every callback and they don't need to
> have the CHANGES_THRESHOLD logic. What do you think?

Hi Sawada-san, Wang

I was looking at the patch and noticed that we moved some logic from
update_replication_progress() to OutputPluginUpdateProgress() like
your suggestion.

I have a question about this change. In the patch we added some
restriction in this function OutputPluginUpdateProgress() like below:

+ /*
+ * If we are at the end of transaction LSN, update progress tracking.
+ * Otherwise, after continuously processing CHANGES_THRESHOLD changes, we
+ * try to send a keepalive message if required.
+ */
+ if (ctx->end_xact || ++changes_count >= CHANGES_THRESHOLD)
+ {
+ ctx->update_progress(ctx, ctx->write_location, ctx->write_xid,
+ skipped_xact);
+ changes_count = 0;
+ }

After the patch, we won't be able to always invoke the update_progress() if the
caller are at the middle of transaction(e.g. end_xact = false). The bebavior of the
public function OutputPluginUpdateProgress() is changed. I am thinking is it ok to
change this at back-branches ?

Because OutputPluginUpdateProgress() is a public function for the extension
developer, just a little concerned about the behavior change here.

Besides, the check of 'end_xact' and the 'CHANGES_THRESHOLD' seems specified to
the pgoutput. I am not very sure that if plugin author also needs these
logic(they might want to change the strategy), so I'd like to confirm it with
you.

Best regards,
Hou zj

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 2 May 2022 11:03:13 +0900

> developer, just a little concerned about the behavior change here.

Good point.

As you pointed out, it would not be good if we change the behavior of
OutputPluginUpdateProgress() in back branches. Also, after more
thought, it is not a good idea even for HEAD since there might be
background workers that use logical decoding and the timeout checking
might not be relevant at all with them.

BTW, I think you're concerned about the plugins that call
OutputPluginUpdateProgress() at the middle of the transaction (i.e.,
end_xact = false). We have the following change that makes the
walsender not update the progress at the middle of the transaction. Do
you think it is okay since it's not common usage to call
OutputPluginUpdateProgress() at the middle of the transaction by the
plugin that is used by the walsender?

 #define WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS 1000
-     if (!TimestampDifferenceExceeds(sendTime, now,
+     if (end_xact && TimestampDifferenceExceeds(sendTime, now,
      WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS))
-         return;
+     {
+         LagTrackerWrite(lsn, now);
+         sendTime = now;
+     }

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 May 2022 08:02:31 +0530

> might not be relevant at all with them.

So, shall we go back to the previous approach of using a separate
function update_replication_progress?

> plugin that is used by the walsender?

We have done that purposefully as otherwise, the lag tracker shows
incorrect information. See email [1]. The reason is that we always get
ack from subscribers for transaction end. Also, prior to this patch we
never call the lag tracker recording apart from the transaction end,
so as a bug fix we shouldn't try to change it.

[1] - https://www.postgresql.org/message-id/OS3PR01MB62755D216245199554DDC8DB9EEA9%40OS3PR01MB6275.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 2 May 2022 11:36:32 +0900

> function update_replication_progress?

Ok, agreed.

> so as a bug fix we shouldn't try to change it.

Make sense.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 4 May 2022 15:48:47 +0530

> > function update_replication_progress?

Attached, please find the updated patch accordingly. Currently, I have
prepared it for HEAD, if you don't see any problem with this, we can
prepare the back-branch patches based on this.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 6 May 2022 10:53:47 +0900

> prepare the back-branch patches based on this.

Thank you for updating the patch. Looks good to me.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 6 May 2022 07:11:56 +0000

> Thank you for updating the patch. Looks good to me.
Thanks for your review.

Improve the back-branch patches according to the discussion.
Move the CHANGES_THRESHOLD logic from function OutputPluginUpdateProgress to
new funcion update_replication_progress.
In addition, improve all patches formatting with pgindent.

Attach the patches.

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Mon, 9 May 2022 12:17:15 +0530

> Improve the back-branch patches according to the discussion.

Thanks. The patch LGTM. I'll push and back-patch this after the
current minor release is done unless there are more comments related
to this work.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 9 May 2022 17:46:35 +0900

> > Improve the back-branch patches according to the discussion.

The patches look good to me too.

Regards,

---

From: "Euler Taveira" <[email-masked]>
Date: Mon, 09 May 2022 10:30:52 -0300

> current minor release is done unless there are more comments related
Looks sane to me. (I only tested the HEAD version)

+   bool        end_xact = ctx->end_xact;

Do you really need a new variable here? It has the same name and the new one
isn't changed during the execution.

Does this issue deserve a test? A small wal_receiver_timeout. Although, I'm not
sure how stable the test will be.

--
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Amit Kapila <[email-masked]>
Date: Tue, 10 May 2022 08:52:38 +0530

> isn't changed during the execution.

I think both ways should be okay. I thought the proposed way is okay
because it is used in more than one place and is probably slightly
easier to follow by having a separate variable.

> sure how stable the test will be.

Yes, the main part is how to write a stable test because estimating
how many changes are enough for the configured wal_receiver_timeout to
pass on all the buildfarm machines is tricky. Also, I am not sure how
important is to test this behavior because based on this theory we
should have tests for all kinds of timeouts.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 10 May 2022 03:36:55 +0000

On Mon, May 9, 2022 at 11:23 AM Amit Kapila <[email-masked]> wrote:
> > ......
Yse, I think we could not guarantee the stability of this test.
In addition, if we set wal_receiver_timeout too small, it may cause timeout
unrelated to this bug. And if we set bigger wal_receiver_timeout and use larger
transaction in order to minimize the impact of machine performance, I think
this might take some time and might risk making the build farm slow.

Regards,
Wang wei

---

From: Amit Kapila <[email-masked]>
Date: Wed, 11 May 2022 17:03:54 +0530

> The patches look good to me too.

Pushed.

-- 
With Regards,
Amit Kapila.