Allow streaming the changes after speculative aborts.
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or the next DML change record
after speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to
process them so we can allow streaming once we receive speculative
abort after speculative insertion. See attached.
I think this is a minor improvement in the logical replication of
in-progress transactions. I have verified this for speculative aborts
and it allows streaming once we receive the spec_abort change record.
--
With Regards,
Amit Kapila.
Attachments:
v1-0001-Allow-streaming-the-changes-after-speculative-abo.patchapplication/octet-stream; name=v1-0001-Allow-streaming-the-changes-after-speculative-abo.patchDownload
From 8c41842af00e6f422467aa1f8b10f623352637ea Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 25 Jun 2021 12:07:12 +0530
Subject: [PATCH v1] Allow streaming the changes after speculative aborts.
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or next DML change record after
speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to process
them so we can allow streaming once we receive speculative abort after
speculative insertion.
---
src/backend/replication/logical/reorderbuffer.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 19e96f3fd9..32816c12b8 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -182,9 +182,10 @@ typedef struct ReorderBufferDiskChange
( \
((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT) \
)
-#define IsSpecConfirm(action) \
+#define IsSpecConfirmOrAbort(action) \
( \
- ((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM) \
+ (((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM) || \
+ ((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT)) \
)
#define IsInsertOrUpdate(action) \
( \
@@ -731,12 +732,13 @@ ReorderBufferProcessPartialChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
/*
* Indicate a partial change for speculative inserts. The change will be
- * considered as complete once we get the speculative confirm token.
+ * considered as complete once we get the speculative confirm or abort
+ * token.
*/
if (IsSpecInsert(change->action))
toptxn->txn_flags |= RBTXN_HAS_PARTIAL_CHANGE;
else if (rbtxn_has_partial_change(toptxn) &&
- IsSpecConfirm(change->action))
+ IsSpecConfirmOrAbort(change->action))
toptxn->txn_flags &= ~RBTXN_HAS_PARTIAL_CHANGE;
/*
--
2.28.0.windows.1
On Fri, Jun 25, 2021 at 12:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or the next DML change record
after speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to
process them so we can allow streaming once we receive speculative
abort after speculative insertion. See attached.I think this is a minor improvement in the logical replication of
in-progress transactions. I have verified this for speculative aborts
and it allows streaming once we receive the spec_abort change record.
Yeah, this improvement makes sense. And the patch looks fine to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Tue, Jun 29, 2021 at 12:57 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Fri, Jun 25, 2021 at 12:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or the next DML change record
after speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to
process them so we can allow streaming once we receive speculative
abort after speculative insertion. See attached.I think this is a minor improvement in the logical replication of
in-progress transactions. I have verified this for speculative aborts
and it allows streaming once we receive the spec_abort change record.Yeah, this improvement makes sense. And the patch looks fine to me.
Thanks. Now, that the PG-15 branch is created, I think we should
commit this to both 15 and 14 as this is a minor change. What do you
think?
--
With Regards,
Amit Kapila.
On Wed, Jun 30, 2021 at 9:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jun 29, 2021 at 12:57 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Fri, Jun 25, 2021 at 12:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or the next DML change record
after speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to
process them so we can allow streaming once we receive speculative
abort after speculative insertion. See attached.I think this is a minor improvement in the logical replication of
in-progress transactions. I have verified this for speculative aborts
and it allows streaming once we receive the spec_abort change record.Yeah, this improvement makes sense. And the patch looks fine to me.
Thanks. Now, that the PG-15 branch is created, I think we should
commit this to both 15 and 14 as this is a minor change. What do you
think?
Yeah, this is a minor improvement so can be pushed to both 15 and 14.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 30, 2021 at 9:55 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Jun 30, 2021 at 9:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jun 29, 2021 at 12:57 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Fri, Jun 25, 2021 at 12:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Till now, we didn't allow to stream the changes in logical replication
till we receive speculative confirm or the next DML change record
after speculative inserts. The reason was that we never use to process
speculative aborts but after commit 4daa140a2f it is possible to
process them so we can allow streaming once we receive speculative
abort after speculative insertion. See attached.I think this is a minor improvement in the logical replication of
in-progress transactions. I have verified this for speculative aborts
and it allows streaming once we receive the spec_abort change record.Yeah, this improvement makes sense. And the patch looks fine to me.
Thanks. Now, that the PG-15 branch is created, I think we should
commit this to both 15 and 14 as this is a minor change. What do you
think?Yeah, this is a minor improvement so can be pushed to both 15 and 14.
Thanks, pushed!
--
With Regards,
Amit Kapila.
On Wed, Jun 30, 2021 at 4:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
Thanks. Now, that the PG-15 branch is created, I think we should
commit this to both 15 and 14 as this is a minor change. What do you
think?Yeah, this is a minor improvement so can be pushed to both 15 and 14.
Thanks, pushed!
I think if you're going to back-patch things that are arguably new
features into stable branches, you ought to give people more than 4
hours and 16 minutes to object. That's how much time passed between
the proposal to back-patch and the commit getting pushed.
I'm not objecting to the change as such - though someone else may wish
to - but I'm definitely objecting to the timing of the commit.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Jun 30, 2021 at 5:38 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jun 30, 2021 at 4:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
I'm not objecting to the change as such - though someone else may wish
to - but I'm definitely objecting to the timing of the commit.
Okay, I'll wait for more time going forward. Normally, I do wait but
this appeared straightforward to me so I went ahead.
--
With Regards,
Amit Kapila.