Fastpath while arranging the changes in LSN order in logical decoding

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 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

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

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 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.

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 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.

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 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.

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
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.

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