Re: Fastpath while arranging the changes in LSN order in logical decoding

On Sun, Mar 8, 2020 at 9:24 PM James Coleman <jtc331(at)gmail(dot)com> wrote:
>
> On Saturday, March 7, 2020, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>>
>> On Sat, Mar 7, 2020 at 9:59 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>> >
>> > On Sat, Mar 7, 2020 at 12:30 AM Andres Freund <andres(at)anarazel(dot)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(at)iki(dot)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