Logical WAL sender unresponsive during decoding commit
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace? Full session is attaches as file.
#0 pfree (pointer=0x561850bbee40) at ./build/../src/backend/utils/mmgr/mcxt.c:1032
#1 0x00005617712530d6 in ReorderBufferReturnTupleBuf (tuple=<optimized out>, rb=<optimized out>) at ./build/../src/backend/replication/logical/reorderbuffer.c:469
#2 ReorderBufferReturnChange (rb=<optimized out>, change=0x561772456048) at ./build/../src/backend/replication/logical/reorderbuffer.c:398
#3 0x0000561771253da1 in ReorderBufferRestoreChanges (rb=rb@entry=0x561771c14e10, txn=0x561771c0b078, file=file@entry=0x561771c15168, segno=segno@entry=0x561771c15178) at ./build/../src/backend/replication/logical/reorderbuffer.c:2570
#4 0x00005617712553ba in ReorderBufferIterTXNNext (state=0x561771c15130, rb=0x561771c14e10) at ./build/../src/backend/replication/logical/reorderbuffer.c:1146
#5 ReorderBufferCommit (rb=0x561771c14e10, xid=xid@entry=2976347782, commit_lsn=79160378448744, end_lsn=<optimized out>, commit_time=commit_time@entry=686095734290578, origin_id=origin_id@entry=0, origin_lsn=0) at ./build/../src/backend/replication/logical/reorderbuffer.c:1523
#6 0x000056177124a30a in DecodeCommit (xid=2976347782, parsed=0x7ffc3cb4c240, buf=0x7ffc3cb4c400, ctx=0x561771b10850) at ./build/../src/backend/replication/logical/decode.c:640
#7 DecodeXactOp (ctx=0x561771b10850, buf=buf@entry=0x7ffc3cb4c400) at ./build/../src/backend/replication/logical/decode.c:248
#8 0x000056177124a6a9 in LogicalDecodingProcessRecord (ctx=0x561771b10850, record=0x561771b10ae8) at ./build/../src/backend/replication/logical/decode.c:117
#9 0x000056177125d1e5 in XLogSendLogical () at ./build/../src/backend/replication/walsender.c:2893
#10 0x000056177125f5f2 in WalSndLoop (send_data=send_data@entry=0x56177125d180 <XLogSendLogical>) at ./build/../src/backend/replication/walsender.c:2242
#11 0x0000561771260125 in StartLogicalReplication (cmd=<optimized out>) at ./build/../src/backend/replication/walsender.c:1179
#12 exec_replication_command (cmd_string=cmd_string@entry=0x561771abe590 "START_REPLICATION SLOT dttsjtaa66crdhbm015h LOGICAL 0/0 ( \"include-timestamp\" '1', \"include-types\" '1', \"include-xids\" '1', \"write-in-chunks\" '1', \"add-tables\" '/* sanitized */.claim_audit,public.__consu"...) at ./build/../src/backend/replication/walsender.c:1612
#13 0x00005617712b2334 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x561771b2a438, dbname=<optimized out>, username=<optimized out>) at ./build/../src/backend/tcop/postgres.c:4267
#14 0x000056177123857c in BackendRun (port=0x561771b0d7a0, port=0x561771b0d7a0) at ./build/../src/backend/postmaster/postmaster.c:4484
#15 BackendStartup (port=0x561771b0d7a0) at ./build/../src/backend/postmaster/postmaster.c:4167
#16 ServerLoop () at ./build/../src/backend/postmaster/postmaster.c:1725
#17 0x000056177123954b in PostmasterMain (argc=9, argv=0x561771ab70e0) at ./build/../src/backend/postmaster/postmaster.c:1398
#18 0x0000561770fae8b6 in main (argc=9, argv=0x561771ab70e0) at ./build/../src/backend/main/main.c:228
What do you think?
Thank you!
Best regards, Andrey Borodin.
Attachments:
check_for_interrupts.diffapplication/octet-stream; name=check_for_interrupts.diff; x-unix-mode=0644Download
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 1c21a1d14b..53c8e633c6 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1340,6 +1340,8 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
ReorderBufferIterTXNEntry *entry;
int32 off;
+ CHECK_FOR_INTERRUPTS();
+
/* nothing there anymore */
if (state->heap->bh_size == 0)
return NULL;
On Tue, Aug 16, 2022 at 9:28 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace?
I think if we want to do this in this code path then it may be it is
better to add it in ReorderBufferProcessTXN where we are looping to
process each change.
--
With Regards,
Amit Kapila.
On Tue, Aug 16, 2022 at 2:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 9:28 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace?I think if we want to do this in this code path then it may be it is
better to add it in ReorderBufferProcessTXN where we are looping to
process each change.
+1
The same issue is recently reported[1]/messages/by-id/CAD21AoD+aNfLje+9JOqWbTiq1GL4BOp9_f7FxLADm8rS8cDhCQ@mail.gmail.com on -bugs and I proposed the
patch that adds CHECK_FOR_INTERRUPTS() to the loop in
ReorderBufferProcessTXN(). I think it should be backpatched.
Regards,
[1]: /messages/by-id/CAD21AoD+aNfLje+9JOqWbTiq1GL4BOp9_f7FxLADm8rS8cDhCQ@mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Tue, Aug 16, 2022 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 9:28 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace?I think if we want to do this in this code path then it may be it is
better to add it in ReorderBufferProcessTXN where we are looping to
process each change.+1
The same issue is recently reported[1] on -bugs and I proposed the
patch that adds CHECK_FOR_INTERRUPTS() to the loop in
ReorderBufferProcessTXN(). I think it should be backpatched.
I agree that it is better to backpatch this as well. Would you like to
verify if your patch works for all branches or if it need some tweaks?
--
With Regards,
Amit Kapila.
On Tue, Aug 16, 2022 at 2:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 9:28 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace?I think if we want to do this in this code path then it may be it is
better to add it in ReorderBufferProcessTXN where we are looping to
process each change.+1
The same issue is recently reported[1] on -bugs and I proposed the
patch that adds CHECK_FOR_INTERRUPTS() to the loop in
ReorderBufferProcessTXN(). I think it should be backpatched.I agree that it is better to backpatch this as well. Would you like to
verify if your patch works for all branches or if it need some tweaks?
Yes, I've confirmed v10 and master but will do that for other branches
and send patches for all supported branches.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On 16 Aug 2022, at 10:25, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
The same issue is recently reported[1] on -bugs
Oh, I missed that thread.
and I proposed the
patch that adds CHECK_FOR_INTERRUPTS() to the loop in
ReorderBufferProcessTXN().
I agree that it's a good place for check.
I think it should be backpatched.
Yes, I think so too.
[1] /messages/by-id/CAD21AoD+aNfLje+9JOqWbTiq1GL4BOp9_f7FxLADm8rS8cDhCQ@mail.gmail.com
The patch in this thread looks good to me.
Thank you!
Best regards, Andrey Borodin.
On Tue, Aug 16, 2022 at 2:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 9:28 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Hi hackers!
Some time ago I've seen a hanging logical replication that was trying to send transaction commit after doing table pg_repack.
I understand that those things do not mix well. Yet walsender was ignoring pg_terminate_backend() and I think this worth fixing.
Can we add CHECK_FOR_INTERRUPTS(); somewhere in this backtrace?I think if we want to do this in this code path then it may be it is
better to add it in ReorderBufferProcessTXN where we are looping to
process each change.+1
The same issue is recently reported[1] on -bugs and I proposed the
patch that adds CHECK_FOR_INTERRUPTS() to the loop in
ReorderBufferProcessTXN(). I think it should be backpatched.I agree that it is better to backpatch this as well. Would you like to
verify if your patch works for all branches or if it need some tweaks?Yes, I've confirmed v10 and master but will do that for other branches
and send patches for all supported branches.
I've attached patches for all supported branches.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Attachments:
REL11_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL11_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From f6d0610b06b377495a23b678e45b9fb3d0b4ba28 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 6c9b5dbced..1e297abf8d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1517,6 +1517,8 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
switch (change->action)
{
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
--
2.24.3 (Apple Git-128)
REL15_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL15_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From 20eb6721539379e0b0b34b116ab279fd723e1cbf Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 8da5f9089c..8971df140b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2085,6 +2085,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
/*
* We can't call start stream callback before processing first
* change.
--
2.24.3 (Apple Git-128)
REL14_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL14_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From 96060b86a01641e7c0a1e89c8b1faa116c01c2e9 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index e59d1396b5..3194e41851 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2078,6 +2078,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
/*
* We can't call start stream callback before processing first
* change.
--
2.24.3 (Apple Git-128)
REL13_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL13_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From 6374a096e235af93e1d5ce44a904f80e8f24ae24 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index ef8c2ea6df..368f60159d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1577,6 +1577,8 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
switch (change->action)
{
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
--
2.24.3 (Apple Git-128)
REL12_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL12_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From a1174ee282d10a1fc0be856606d4e69c78cded2f Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 475f76fa5e..4fe456cfa9 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1525,6 +1525,8 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
switch (change->action)
{
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
--
2.24.3 (Apple Git-128)
REL10_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=REL10_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From 6debeed1461a9f5baee3738652bb4d0bfa31183e Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 061555652c..a65ec84a69 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1525,6 +1525,8 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
switch (change->action)
{
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
--
2.24.3 (Apple Git-128)
master_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchapplication/octet-stream; name=master_v2-0001-Add-CHECK_FOR_INTERRUPTS-in-logical-decoding-s-pr.patchDownload
From 06ce2502a1c8b97a543426d8f9f7548bfc37e8fc Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 15 Aug 2022 12:35:02 +0900
Subject: [PATCH v2] Add CHECK_FOR_INTERRUPTS in logical decoding's processing
changes loop.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add a CHECK_FOR_INTERRUPTS() call in the main loop of processing
logical changes to make the logical decoding responsive to cancellations,
especially while the plugin skipping all changes.
Per bug #17580 reproted from 巨鲸 and Andrey Borodin. Backpatch to all
supported branches.
Author: Masahiko Sawada
Reviwed-by: Amit Kapila
Discussion: https://postgr.es/m/17580-849c1d5b6d7eb422@postgresql.org
Discussion: https://postgr.es/m/B319ECD6-9A28-4CDF-A8F4-3591E0BF2369@yandex-team.ru
Backpatch-through: 10
---
src/backend/replication/logical/reorderbuffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 1c21a1d14b..89cf9f9389 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2096,6 +2096,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Relation relation = NULL;
Oid reloid;
+ CHECK_FOR_INTERRUPTS();
+
/*
* We can't call start stream callback before processing first
* change.
--
2.24.3 (Apple Git-128)
On Tue, Aug 16, 2022 at 2:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached patches for all supported branches.
LGTM. I'll push this tomorrow unless there are comments/suggestions.
--
With Regards,
Amit Kapila.
On Mon, Aug 22, 2022 at 4:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached patches for all supported branches.
LGTM. I'll push this tomorrow unless there are comments/suggestions.
Pushed.
--
With Regards,
Amit Kapila.
On Tue, Aug 23, 2022 at 4:40 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Aug 22, 2022 at 4:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 16, 2022 at 2:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached patches for all supported branches.
LGTM. I'll push this tomorrow unless there are comments/suggestions.
Pushed.
I think this was a good change, but there's at least one other problem
here: within ReorderBufferRestoreChanges, the while (restored <
max_changes_in_memory && *segno <= last_segno) doesn't seem to contain
a CFI. Note that this can loop either by repeatedly failing to open a
file, or by repeatedly reading from a file and passing the data read
to ReorderBufferRestoreChange. So I think there should just be a CFI
at the top of this loop to make sure both cases are covered.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Oct 20, 2022 at 5:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
Pushed.
I think this was a good change, but there's at least one other problem
here: within ReorderBufferRestoreChanges, the while (restored <
max_changes_in_memory && *segno <= last_segno) doesn't seem to contain
a CFI. Note that this can loop either by repeatedly failing to open a
file, or by repeatedly reading from a file and passing the data read
to ReorderBufferRestoreChange. So I think there should just be a CFI
at the top of this loop to make sure both cases are covered.
Agreed. The failures due to file operations can make this loop
unpredictable in terms of time, so it is a good idea to have CFI at
the top of this loop.
I can take care of this unless there are any objections or you want to
do it. We have backpatched the previous similar change, so I think we
should backpatch this as well. What do you think?
--
With Regards,
Amit Kapila.
On Thu, Oct 20, 2022 at 1:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Oct 20, 2022 at 5:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
Pushed.
I think this was a good change, but there's at least one other problem
here: within ReorderBufferRestoreChanges, the while (restored <
max_changes_in_memory && *segno <= last_segno) doesn't seem to contain
a CFI. Note that this can loop either by repeatedly failing to open a
file, or by repeatedly reading from a file and passing the data read
to ReorderBufferRestoreChange. So I think there should just be a CFI
at the top of this loop to make sure both cases are covered.Agreed. The failures due to file operations can make this loop
unpredictable in terms of time, so it is a good idea to have CFI at
the top of this loop.I can take care of this unless there are any objections or you want to
do it. We have backpatched the previous similar change, so I think we
should backpatch this as well. What do you think?
Please go ahead. +1 for back-patching.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Oct 20, 2022 at 7:17 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Oct 20, 2022 at 1:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Oct 20, 2022 at 5:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
Pushed.
I think this was a good change, but there's at least one other problem
here: within ReorderBufferRestoreChanges, the while (restored <
max_changes_in_memory && *segno <= last_segno) doesn't seem to contain
a CFI. Note that this can loop either by repeatedly failing to open a
file, or by repeatedly reading from a file and passing the data read
to ReorderBufferRestoreChange. So I think there should just be a CFI
at the top of this loop to make sure both cases are covered.Agreed. The failures due to file operations can make this loop
unpredictable in terms of time, so it is a good idea to have CFI at
the top of this loop.I can take care of this unless there are any objections or you want to
do it. We have backpatched the previous similar change, so I think we
should backpatch this as well. What do you think?Please go ahead. +1 for back-patching.
Yesterday, I have pushed this change.
--
With Regards,
Amit Kapila.