Fastpath while arranging the changes in LSN order in logical decoding
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patchapplication/octet-stream; name=0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patchDownload
From 36e75baa88766f575ad5caa05cb4628023a8f164 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Mon, 25 Nov 2019 09:00:51 +0530
Subject: [PATCH] Fastpath for sending changes to output plugin in logical
decoding
In logical decoding before sending the changes to the output plugin, if
there is only a one transaction then no need to build the binary heap
becasue for one transaction they are already in the LSN order.
---
contrib/test_decoding/logical.conf | 2 +-
src/backend/replication/logical/reorderbuffer.c | 61 +++++++++++++++++--------
2 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/contrib/test_decoding/logical.conf b/contrib/test_decoding/logical.conf
index 07c4d3d..02595d9 100644
--- a/contrib/test_decoding/logical.conf
+++ b/contrib/test_decoding/logical.conf
@@ -1,3 +1,3 @@
wal_level = logical
max_replication_slots = 4
-logical_decoding_work_mem = 64kB
+logical_decoding_work_mem = 64MB
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 710a22f..ce92068 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1113,11 +1113,6 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn)
nr_txns++;
}
- /*
- * TODO: Consider adding fastpath for the rather common nr_txns=1 case, no
- * need to allocate/build a heap then.
- */
-
/* allocate iteration state */
state = (ReorderBufferIterTXNState *)
MemoryContextAllocZero(rb->context,
@@ -1133,10 +1128,11 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn)
state->entries[off].segno = 0;
}
- /* allocate heap */
- state->heap = binaryheap_allocate(state->nr_txns,
- ReorderBufferIterCompare,
- state);
+ /* allocate heap, if we have more than one transaction. */
+ if (nr_txns > 1)
+ state->heap = binaryheap_allocate(state->nr_txns,
+ ReorderBufferIterCompare,
+ state);
/*
* Now insert items into the binary heap, in an unordered fashion. (We
@@ -1165,7 +1161,9 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn)
state->entries[off].change = cur_change;
state->entries[off].txn = txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
/* add subtransactions if they contain changes */
@@ -1194,12 +1192,15 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn)
state->entries[off].change = cur_change;
state->entries[off].txn = cur_txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
}
/* assemble a valid binary heap */
- binaryheap_build(state->heap);
+ if (nr_txns > 1)
+ binaryheap_build(state->heap);
return state;
}
@@ -1217,11 +1218,24 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
ReorderBufferIterTXNEntry *entry;
int32 off;
- /* nothing there anymore */
- if (state->heap->bh_size == 0)
- return NULL;
+ /*
+ * If there is only one transaction then it will be at the offset 0.
+ * Otherwise get the offset from the binary heap.
+ */
+ if (state->nr_txns == 1)
+ {
+ off = 0;
+ if (state->entries[off].change == NULL)
+ return NULL;
+ }
+ else
+ {
+ /* nothing there anymore */
+ if (state->heap->bh_size == 0)
+ return NULL;
- off = DatumGetInt32(binaryheap_first(state->heap));
+ off = DatumGetInt32(binaryheap_first(state->heap));
+ }
entry = &state->entries[off];
/* free memory we might have "leaked" in the previous *Next call */
@@ -1251,7 +1265,8 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
state->entries[off].lsn = next_change->lsn;
state->entries[off].change = next_change;
- binaryheap_replace_first(state->heap, Int32GetDatum(off));
+ if (state->nr_txns > 1)
+ binaryheap_replace_first(state->heap, Int32GetDatum(off));
return change;
}
@@ -1281,14 +1296,19 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
/* txn stays the same */
state->entries[off].lsn = next_change->lsn;
state->entries[off].change = next_change;
- binaryheap_replace_first(state->heap, Int32GetDatum(off));
+
+ if (state->nr_txns > 1)
+ binaryheap_replace_first(state->heap, Int32GetDatum(off));
return change;
}
}
/* ok, no changes there anymore, remove */
- binaryheap_remove_first(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_remove_first(state->heap);
+ else
+ entry->change = NULL;
return change;
}
@@ -1319,7 +1339,8 @@ ReorderBufferIterTXNFinish(ReorderBuffer *rb,
Assert(dlist_is_empty(&state->old_change));
}
- binaryheap_free(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_free(state->heap);
pfree(state);
}
--
1.8.3.1
On Mon, Nov 25, 2019 at 9:22 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.
I have registered it in the next commitfest.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.
Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.
- Heikki
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.
I haven’t really measured the performance for this. I will try to do that
next week. Thanks for looking into this.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
1. Tried to apply the patch to PG 12.2 commit 45b88269a353ad93744772791feb6d01bc7e1e42 (HEAD -> REL_12_2, tag: REL_12_2), it doesn't work. Then tried to check the patch, and found the errors showing below.
$ git apply --check 0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patch
error: patch failed: contrib/test_decoding/logical.conf:1
error: contrib/test_decoding/logical.conf: patch does not apply
error: patch failed: src/backend/replication/logical/reorderbuffer.c:1133
error: src/backend/replication/logical/reorderbuffer.c: patch does not apply
2. Ran a further check for file "logical.conf", and found there is only one commit since 2014, which doesn't have the parameter, "logical_decoding_work_mem = 64kB"
3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple logical replication test. A connection issue is found like below,
"table public.pgbench_accounts: INSERT: aid[integer]:4071 bid[integer]:1 abalance[integer]:0 filler[character]:' '
pg_recvlogical: error: could not receive data from WAL stream: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
pg_recvlogical: disconnected; waiting 5 seconds to try again"
4. This connection issue can be reproduced on PG 12.2 commit mentioned above, the basic steps,
4.1 Change "wal_level = logical" in "postgresql.conf"
4.2 create a logical slot and listen on it,
$ pg_recvlogical -d postgres --slot test --create-slot
$ pg_recvlogical -d postgres --slot test --start -f -
4.3 from another terminal, run the command below,
$ pgbench -i -p 5432 -d postgres
Let me know if I did something wrong, and if a new patch is available, I can re-run the test on the same environment.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
After manually applied the patch, a diff regenerated is attached.
On 2020-02-18 4:16 p.m., David Zhang wrote:
1. Tried to apply the patch to PG 12.2 commit 45b88269a353ad93744772791feb6d01bc7e1e42 (HEAD -> REL_12_2, tag: REL_12_2), it doesn't work. Then tried to check the patch, and found the errors showing below.
$ git apply --check 0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patch
error: patch failed: contrib/test_decoding/logical.conf:1
error: contrib/test_decoding/logical.conf: patch does not apply
error: patch failed: src/backend/replication/logical/reorderbuffer.c:1133
error: src/backend/replication/logical/reorderbuffer.c: patch does not apply2. Ran a further check for file "logical.conf", and found there is only one commit since 2014, which doesn't have the parameter, "logical_decoding_work_mem = 64kB"
3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple logical replication test. A connection issue is found like below,
"table public.pgbench_accounts: INSERT: aid[integer]:4071 bid[integer]:1 abalance[integer]:0 filler[character]:' '
pg_recvlogical: error: could not receive data from WAL stream: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
pg_recvlogical: disconnected; waiting 5 seconds to try again"4. This connection issue can be reproduced on PG 12.2 commit mentioned above, the basic steps,
4.1 Change "wal_level = logical" in "postgresql.conf"
4.2 create a logical slot and listen on it,
$ pg_recvlogical -d postgres --slot test --create-slot
$ pg_recvlogical -d postgres --slot test --start -f -4.3 from another terminal, run the command below,
$ pgbench -i -p 5432 -d postgresLet me know if I did something wrong, and if a new patch is available, I can re-run the test on the same environment.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
0001-no-heap-manually.patchtext/plain; charset=UTF-8; name=0001-no-heap-manually.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/test_decoding/logical.conf b/contrib/test_decoding/logical.conf
index 367f706651..02595d99d5 100644
--- a/contrib/test_decoding/logical.conf
+++ b/contrib/test_decoding/logical.conf
@@ -1,2 +1,3 @@
wal_level = logical
max_replication_slots = 4
+logical_decoding_work_mem = 64MB
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index a74fd705b4..62b661dc4d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -989,11 +989,6 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
nr_txns++;
}
- /*
- * TODO: Consider adding fastpath for the rather common nr_txns=1 case, no
- * need to allocate/build a heap then.
- */
-
/* allocate iteration state */
state = (ReorderBufferIterTXNState *)
MemoryContextAllocZero(rb->context,
@@ -1009,10 +1004,11 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].segno = 0;
}
- /* allocate heap */
- state->heap = binaryheap_allocate(state->nr_txns,
- ReorderBufferIterCompare,
- state);
+ /* allocate heap, if we have more than one transaction. */
+ if (nr_txns > 1)
+ state->heap = binaryheap_allocate(state->nr_txns,
+ ReorderBufferIterCompare,
+ state);
/* Now that the state fields are initialized, it is safe to return it. */
*iter_state = state;
@@ -1044,7 +1040,9 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].change = cur_change;
state->entries[off].txn = txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
/* add subtransactions if they contain changes */
@@ -1073,12 +1071,15 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].change = cur_change;
state->entries[off].txn = cur_txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
}
/* assemble a valid binary heap */
- binaryheap_build(state->heap);
+ if (nr_txns > 1)
+ binaryheap_build(state->heap);
}
/*
@@ -1094,11 +1095,24 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
ReorderBufferIterTXNEntry *entry;
int32 off;
- /* nothing there anymore */
- if (state->heap->bh_size == 0)
- return NULL;
+ /*
+ * If there is only one transaction then it will be at the offset 0.
+ * Otherwise get the offset from the binary heap.
+ */
+ if (state->nr_txns == 1)
+ {
+ off = 0;
+ if (state->entries[off].change == NULL)
+ return NULL;
+ }
+ else
+ {
+ /* nothing there anymore */
+ if (state->heap->bh_size == 0)
+ return NULL;
+ off = DatumGetInt32(binaryheap_first(state->heap));
+ }
- off = DatumGetInt32(binaryheap_first(state->heap));
entry = &state->entries[off];
/* free memory we might have "leaked" in the previous *Next call */
@@ -1128,7 +1142,9 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
state->entries[off].lsn = next_change->lsn;
state->entries[off].change = next_change;
- binaryheap_replace_first(state->heap, Int32GetDatum(off));
+ if (state->nr_txns > 1)
+ binaryheap_replace_first(state->heap, Int32GetDatum(off));
+
return change;
}
@@ -1165,7 +1181,10 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
}
/* ok, no changes there anymore, remove */
- binaryheap_remove_first(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_remove_first(state->heap);
+ else
+ entry->change = NULL;
return change;
}
@@ -1196,7 +1215,8 @@ ReorderBufferIterTXNFinish(ReorderBuffer *rb,
Assert(dlist_is_empty(&state->old_change));
}
- binaryheap_free(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_free(state->heap);
pfree(state);
}
Hi Dilip,
On 2/18/20 7:30 PM, David Zhang wrote:
After manually applied the patch, a diff regenerated is attached.
David's updated patch applies but all logical decoding regression tests
are failing on cfbot.
Do you know when you will be able to supply an updated patch?
Regards,
--
-David
david@pgmasters.net
On Mon, Mar 2, 2020 at 7:27 PM David Steele <david@pgmasters.net> wrote:
Hi Dilip,
On 2/18/20 7:30 PM, David Zhang wrote:
After manually applied the patch, a diff regenerated is attached.
David's updated patch applies but all logical decoding regression tests
are failing on cfbot.Do you know when you will be able to supply an updated patch?
I will try to send in a day or two.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 3, 2020 at 8:42 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, Mar 2, 2020 at 7:27 PM David Steele <david@pgmasters.net> wrote:
Hi Dilip,
On 2/18/20 7:30 PM, David Zhang wrote:
After manually applied the patch, a diff regenerated is attached.
David's updated patch applies but all logical decoding regression tests
are failing on cfbot.Do you know when you will be able to supply an updated patch?
I will try to send in a day or two.
I have rebased the patch. check-world is passing.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v2-0001-Fastpath-for-sending-changes-to-output-plugin-in-.patchapplication/octet-stream; name=v2-0001-Fastpath-for-sending-changes-to-output-plugin-in-.patchDownload
From 10943afd0d57b4dbd97f1ebe896fa7459541c024 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Tue, 3 Mar 2020 09:40:10 +0530
Subject: [PATCH v2] Fastpath for sending changes to output plugin in logical
decoding
In logical decoding before sending the changes to the output plugin, if
there is only a one transaction then no need to build the binary heap
becasue for one transaction they are already in the LSN order.
---
src/backend/replication/logical/reorderbuffer.c | 61 +++++++++++++++++--------
1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 481277a..c6f1f89 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1037,11 +1037,6 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
nr_txns++;
}
- /*
- * TODO: Consider adding fastpath for the rather common nr_txns=1 case, no
- * need to allocate/build a heap then.
- */
-
/* allocate iteration state */
state = (ReorderBufferIterTXNState *)
MemoryContextAllocZero(rb->context,
@@ -1057,10 +1052,11 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].segno = 0;
}
- /* allocate heap */
- state->heap = binaryheap_allocate(state->nr_txns,
- ReorderBufferIterCompare,
- state);
+ /* allocate heap, if we have more than one transaction. */
+ if (nr_txns > 1)
+ state->heap = binaryheap_allocate(state->nr_txns,
+ ReorderBufferIterCompare,
+ state);
/* Now that the state fields are initialized, it is safe to return it. */
*iter_state = state;
@@ -1092,7 +1088,9 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].change = cur_change;
state->entries[off].txn = txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
/* add subtransactions if they contain changes */
@@ -1121,12 +1119,15 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
state->entries[off].change = cur_change;
state->entries[off].txn = cur_txn;
- binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
+ /* add to heap, only if we have more than one transaction. */
+ if (nr_txns > 1)
+ binaryheap_add_unordered(state->heap, Int32GetDatum(off++));
}
}
/* assemble a valid binary heap */
- binaryheap_build(state->heap);
+ if (nr_txns > 1)
+ binaryheap_build(state->heap);
}
/*
@@ -1142,11 +1143,24 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
ReorderBufferIterTXNEntry *entry;
int32 off;
- /* nothing there anymore */
- if (state->heap->bh_size == 0)
- return NULL;
+ /*
+ * If there is only one transaction then it will be at the offset 0.
+ * Otherwise get the offset from the binary heap.
+ */
+ if (state->nr_txns == 1)
+ {
+ off = 0;
+ if (state->entries[off].change == NULL)
+ return NULL;
+ }
+ else
+ {
+ /* nothing there anymore */
+ if (state->heap->bh_size == 0)
+ return NULL;
- off = DatumGetInt32(binaryheap_first(state->heap));
+ off = DatumGetInt32(binaryheap_first(state->heap));
+ }
entry = &state->entries[off];
/* free memory we might have "leaked" in the previous *Next call */
@@ -1176,7 +1190,8 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
state->entries[off].lsn = next_change->lsn;
state->entries[off].change = next_change;
- binaryheap_replace_first(state->heap, Int32GetDatum(off));
+ if (state->nr_txns > 1)
+ binaryheap_replace_first(state->heap, Int32GetDatum(off));
return change;
}
@@ -1206,14 +1221,19 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
/* txn stays the same */
state->entries[off].lsn = next_change->lsn;
state->entries[off].change = next_change;
- binaryheap_replace_first(state->heap, Int32GetDatum(off));
+
+ if (state->nr_txns > 1)
+ binaryheap_replace_first(state->heap, Int32GetDatum(off));
return change;
}
}
/* ok, no changes there anymore, remove */
- binaryheap_remove_first(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_remove_first(state->heap);
+ else
+ entry->change = NULL;
return change;
}
@@ -1244,7 +1264,8 @@ ReorderBufferIterTXNFinish(ReorderBuffer *rb,
Assert(dlist_is_empty(&state->old_change));
}
- binaryheap_free(state->heap);
+ if (state->nr_txns > 1)
+ binaryheap_free(state->heap);
pfree(state);
}
--
1.8.3.1
On Wed, Feb 19, 2020 at 6:00 AM David Zhang <david.zhang@highgo.ca> wrote:
After manually applied the patch, a diff regenerated is attached.
On 2020-02-18 4:16 p.m., David Zhang wrote:
1. Tried to apply the patch to PG 12.2 commit 45b88269a353ad93744772791feb6d01bc7e1e42 (HEAD -> REL_12_2, tag: REL_12_2), it doesn't work. Then tried to check the patch, and found the errors showing below.
$ git apply --check 0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patch
error: patch failed: contrib/test_decoding/logical.conf:1
error: contrib/test_decoding/logical.conf: patch does not apply
error: patch failed: src/backend/replication/logical/reorderbuffer.c:1133
error: src/backend/replication/logical/reorderbuffer.c: patch does not apply2. Ran a further check for file "logical.conf", and found there is only one commit since 2014, which doesn't have the parameter, "logical_decoding_work_mem = 64kB"
3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple logical replication test. A connection issue is found like below,
"table public.pgbench_accounts: INSERT: aid[integer]:4071 bid[integer]:1 abalance[integer]:0 filler[character]:' '
pg_recvlogical: error: could not receive data from WAL stream: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
pg_recvlogical: disconnected; waiting 5 seconds to try again"4. This connection issue can be reproduced on PG 12.2 commit mentioned above, the basic steps,
4.1 Change "wal_level = logical" in "postgresql.conf"
4.2 create a logical slot and listen on it,
$ pg_recvlogical -d postgres --slot test --create-slot
$ pg_recvlogical -d postgres --slot test --start -f -4.3 from another terminal, run the command below,
$ pgbench -i -p 5432 -d postgresLet me know if I did something wrong, and if a new patch is available, I can re-run the test on the same environment.
Thanks for testing and rebasing. I think one of the hunks is missing
in your rebased version. That could be the reason for failure. Can
you test on my latest version?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Hi Dilip,
I repeated the same test cases again and can't reproduce the
disconnection issue after applied your new patch.
Best regards,
David
On 2020-03-02 9:11 p.m., Dilip Kumar wrote:
On Wed, Feb 19, 2020 at 6:00 AM David Zhang <david.zhang@highgo.ca> wrote:
After manually applied the patch, a diff regenerated is attached.
On 2020-02-18 4:16 p.m., David Zhang wrote:
1. Tried to apply the patch to PG 12.2 commit 45b88269a353ad93744772791feb6d01bc7e1e42 (HEAD -> REL_12_2, tag: REL_12_2), it doesn't work. Then tried to check the patch, and found the errors showing below.
$ git apply --check 0001-Fastpath-for-sending-changes-to-output-plugin-in-log.patch
error: patch failed: contrib/test_decoding/logical.conf:1
error: contrib/test_decoding/logical.conf: patch does not apply
error: patch failed: src/backend/replication/logical/reorderbuffer.c:1133
error: src/backend/replication/logical/reorderbuffer.c: patch does not apply2. Ran a further check for file "logical.conf", and found there is only one commit since 2014, which doesn't have the parameter, "logical_decoding_work_mem = 64kB"
3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple logical replication test. A connection issue is found like below,
"table public.pgbench_accounts: INSERT: aid[integer]:4071 bid[integer]:1 abalance[integer]:0 filler[character]:' '
pg_recvlogical: error: could not receive data from WAL stream: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
pg_recvlogical: disconnected; waiting 5 seconds to try again"4. This connection issue can be reproduced on PG 12.2 commit mentioned above, the basic steps,
4.1 Change "wal_level = logical" in "postgresql.conf"
4.2 create a logical slot and listen on it,
$ pg_recvlogical -d postgres --slot test --create-slot
$ pg_recvlogical -d postgres --slot test --start -f -4.3 from another terminal, run the command below,
$ pgbench -i -p 5432 -d postgresLet me know if I did something wrong, and if a new patch is available, I can re-run the test on the same environment.
Thanks for testing and rebasing. I think one of the hunks is missing
in your rebased version. That could be the reason for failure. Can
you test on my latest version?
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
On Wed, Mar 4, 2020 at 3:02 AM David Zhang <david.zhang@highgo.ca> wrote:
Hi Dilip,
I repeated the same test cases again and can't reproduce the
disconnection issue after applied your new patch.
Thanks for the confirmation.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Hi,
On 2020-01-08 18:06:52 +0530, Dilip Kumar wrote:
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.I haven’t really measured the performance for this. I will try to do that
next week. Thanks for looking into this.
Did you do that?
Regards,
Andres
On Sat, Mar 7, 2020 at 12:30 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-01-08 18:06:52 +0530, Dilip Kumar wrote:
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.I haven’t really measured the performance for this. I will try to do that
next week. Thanks for looking into this.Did you do that?
I tried once in my local machine but could not produce consistent
results. I will try this once again in the performance machine and
report back.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Sat, Mar 7, 2020 at 9:59 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Mar 7, 2020 at 12:30 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-01-08 18:06:52 +0530, Dilip Kumar wrote:
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.I haven’t really measured the performance for this. I will try to do that
next week. Thanks for looking into this.Did you do that?
I tried once in my local machine but could not produce consistent
results. I will try this once again in the performance machine and
report back.
I have tried to decode changes for the 100,000 small transactions and
measured the time with head vs patch. I did not observe any
significant gain.
Head
-------
519ms
500ms
487ms
501ms
patch
------
501ms
492ms
486ms
489ms
IMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Saturday, March 7, 2020, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Mar 7, 2020 at 9:59 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Mar 7, 2020 at 12:30 AM Andres Freund <andres@anarazel.de>
wrote:
Hi,
On 2020-01-08 18:06:52 +0530, Dilip Kumar wrote:
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi>
wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output
plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoidbuilding the
binary heap. A small patch is attached for the same.
Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.I haven’t really measured the performance for this. I will try to
do that
next week. Thanks for looking into this.
Did you do that?
I tried once in my local machine but could not produce consistent
results. I will try this once again in the performance machine and
report back.I have tried to decode changes for the 100,000 small transactions and
measured the time with head vs patch. I did not observe any
significant gain.Head
-------
519ms
500ms
487ms
501mspatch
------
501ms
492ms
486ms
489msIMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.
Would you be able to share your test setup? It seems like it’d helpful to
get a larger sample size to better determine if it’s measurable or not.
Visually those 4 runs look to me like it’s possible, but objectively I’m
not sure we can yet conclude one way or the other.
James
On Sun, Mar 8, 2020 at 9:24 PM James Coleman <jtc331@gmail.com> wrote:
On Saturday, March 7, 2020, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Mar 7, 2020 at 9:59 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Mar 7, 2020 at 12:30 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-01-08 18:06:52 +0530, Dilip Kumar wrote:
On Wed, 8 Jan 2020 at 5:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 25/11/2019 05:52, Dilip Kumar wrote:
In logical decoding, while sending the changes to the output plugin we
need to arrange them in the LSN order. But, if there is only one
transaction which is a very common case then we can avoid building the
binary heap. A small patch is attached for the same.Does this make any measurable performance difference? Building a
one-element binary heap seems pretty cheap.I haven’t really measured the performance for this. I will try to do that
next week. Thanks for looking into this.Did you do that?
I tried once in my local machine but could not produce consistent
results. I will try this once again in the performance machine and
report back.I have tried to decode changes for the 100,000 small transactions and
measured the time with head vs patch. I did not observe any
significant gain.Head
-------
519ms
500ms
487ms
501mspatch
------
501ms
492ms
486ms
489msIMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.Would you be able to share your test setup? It seems like it’d helpful to get a larger sample size to better determine if it’s measurable or not. Visually those 4 runs look to me like it’s possible, but objectively I’m not sure we can yet conclude one way or the other.
Yeah, my test is very simple
CREATE TABLE t1 (a int, b int);
SELECT * FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
--run 100,000 small transactions with pgbench
./pgbench -f test.sql -c 1 -j 1 -t 100000 -P 1 postgres;
--measure time to decode the changes
time ./psql -d postgres -c "select count(*) from
pg_logical_slot_get_changes('regression_slot', NULL,NULL);
*test.sql is just one insert query like below
insert into t1 values(1,1);
I guess this should be the best case to test this patch because we are
decoding a lot of small transactions but it seems the time taken for
creating the binary heap is quite small.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On 2020-03-07 11:15:27 +0530, Dilip Kumar wrote:
IMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.
+1
On Mon, Mar 9, 2020 at 11:07 PM Andres Freund <andres@anarazel.de> wrote:
On 2020-03-07 11:15:27 +0530, Dilip Kumar wrote:
IMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.+1
Dilip, are you planning to do more tests for this? Anyone else wants
to do more tests? If not, based on current results, we can remove that
TODO and in future, if someone comes with a test case to show benefit
for adding fastpath, then we can consider the patch proposed by Dilip.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 6:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Mar 9, 2020 at 11:07 PM Andres Freund <andres@anarazel.de> wrote:
On 2020-03-07 11:15:27 +0530, Dilip Kumar wrote:
IMHO, if we conclude that because there is no performance gain so we
don't want to add one extra path in the code then we might want to
remove that TODO from the code so that we don't spend time for
optimizing this in the future.+1
Dilip, are you planning to do more tests for this? Anyone else wants
to do more tests? If not, based on current results, we can remove that
TODO and in future, if someone comes with a test case to show benefit
for adding fastpath, then we can consider the patch proposed by Dilip.
IMHO, I have tried the best case but did not see any performance gain
so I am not planning to test this further. I have attached the patch
for removing the TODO.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Remove-TODO-comments-for-fast-path.patchapplication/octet-stream; name=0001-Remove-TODO-comments-for-fast-path.patchDownload
From d9c4967662f09f68d8891d1056d78c612989b031 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Tue, 24 Mar 2020 18:23:06 +0530
Subject: [PATCH] Remove TODO comments for fast-path
Experiments shows that there is no performance gain by adding the
fast-path for the transactions which has no sub-transactions. So TODO
is removed.
---
src/backend/replication/logical/reorderbuffer.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 481277a..4594cf9 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1037,11 +1037,6 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
nr_txns++;
}
- /*
- * TODO: Consider adding fastpath for the rather common nr_txns=1 case, no
- * need to allocate/build a heap then.
- */
-
/* allocate iteration state */
state = (ReorderBufferIterTXNState *)
MemoryContextAllocZero(rb->context,
--
1.8.3.1
On 2020-03-24 18:36:03 +0530, Dilip Kumar wrote:
IMHO, I have tried the best case but did not see any performance gain
so I am not planning to test this further. I have attached the patch
for removing the TODO.
Pushed. Thanks!
On Wed, Mar 25, 2020 at 12:46 AM Andres Freund <andres@anarazel.de> wrote:
On 2020-03-24 18:36:03 +0530, Dilip Kumar wrote:
IMHO, I have tried the best case but did not see any performance gain
so I am not planning to test this further. I have attached the patch
for removing the TODO.Pushed. Thanks!
I have updated the CF entry. Thanks to all involved in this.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Mar 25, 2020 at 9:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Mar 25, 2020 at 12:46 AM Andres Freund <andres@anarazel.de> wrote:
On 2020-03-24 18:36:03 +0530, Dilip Kumar wrote:
IMHO, I have tried the best case but did not see any performance gain
so I am not planning to test this further. I have attached the patch
for removing the TODO.Pushed. Thanks!
I have updated the CF entry. Thanks to all involved in this.
Thanks!
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com