Thread: 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
Attachment
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
Re: Fastpath while arranging the changes in LSN order in logicaldecoding
From
Heikki Linnakangas
Date:
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.
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
replicationtest. 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
replicationtest. 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
Attachment
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
Attachment
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
theparameter, "logical_decoding_work_mem = 64kB"
> >
> > 3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple
logicalreplication 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
theparameter, "logical_decoding_work_mem = 64kB"
>>>
>>> 3. Manually apply the patch including src/backend/replication/logical/reorderbuffer.c, and then ran a simple
logicalreplication 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
determineif it’s measurable or not. Visually those 4 runs look to me like it’s possible, but objectively I’m not sure
wecan 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
Attachment
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