[pgpool-hackers: 3726] Re: add a feature: dml object level load balance
Tatsuo Ishii
ishii at sraoss.co.jp
Wed Jul 22 11:07:26 JST 2020
Hi Usama,
Why the test is under src/test/dml-adaptive-test/simple-query-test/test.sh?
It would be good if it's located under the standard regression test directory.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
> Hi Sunbiao,
>
>
> I have committed your patch after a few minor modifications and adjusting
> the documentation.
> https://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=761d30a17d04350fb3a67bce43440731c8ba3c90
>
>
> Best Regards
>
> On Tue, Jul 7, 2020 at 7:08 AM sunbiao at highgo.com <sunbiao at highgo.com>
> wrote:
>
>> Hi, Usama
>>
>> I have seen patch-v7 and tested it.
>> I think removing the parentheses () is a good way.
>>
>> Thanks
>> Best Regards
>> ------------------------------
>> sunbiao at highgo.com
>>
>>
>> *From:* Muhammad Usama <m.usama at gmail.com>
>> *Date:* 2020-07-07 01:19
>> *To:* sunbiao at highgo.com
>> *CC:* Tatsuo Ishii <ishii at sraoss.co.jp>; pgpool-hackers
>> <pgpool-hackers at pgpool.net>
>> *Subject:* Re: Re: [pgpool-hackers: 3592] add a feature: dml object level
>> load balance
>> Hi Sunbiao,
>>
>> Thanks for pointing the mistake.
>> So I have yet again changed the patch a little bit.
>> Instead of doing strncasecmp in
>> get_associated_object_from_dml_adaptive_relations function,
>> I have removed the parentheses () from the original token at the time of
>> parsing.
>> What do you think? see the new attached patch.
>>
>> Thanks
>> Best Regards
>> Muhammad Usama
>>
>> Thanks for the updated patch.
>> On Mon, Jul 6, 2020 at 10:37 AM sunbiao at highgo.com <sunbiao at highgo.com>
>> wrote:
>>
>>> Hi, Usama
>>>
>>> I think your idea is better then using json.
>>> I tested patch-v5. There was a bug in the function
>>> "get_associated_object_from_dml_adaptive_relations" , when sql calls a
>>> function.
>>>
>>
>> Can you please
>>
>>
>>> I fixed it in the new patch.
>>>
>>> Thanks
>>> Best regards.
>>> ------------------------------
>>> sunbiao at highgo.com
>>>
>>>
>>> *From:* Muhammad Usama <m.usama at gmail.com>
>>> *Date:* 2020-07-02 05:15
>>> *To:* sunbiao at highgo.com
>>> *CC:* Tatsuo Ishii <ishii at sraoss.co.jp>; pgpool-hackers
>>> <pgpool-hackers at pgpool.net>
>>> *Subject:* Re: [pgpool-hackers: 3592] add a feature: dml object level
>>> load balance
>>> Hi Sunbiao,
>>>
>>> Thanks for the patch, It is working as expected. Though while reviewing it
>>> I realized we are doing the dml_adaptive_relationship_list parsing at the
>>> start of every
>>> session, That is better than the previous approach but still has room for
>>> improvement.
>>>
>>> My idea is to do the parsing at the very beginning while loading the
>>> configuration parameter
>>> and then use that.
>>>
>>> Secondly, I do like your idea of using the JSON object for storing the
>>> parsed dml_adaptive_relationship_list
>>> but the string manipulation we were doing for searching the function
>>> names from the list was
>>> pinching me a little since it is done for each statement.
>>>
>>> So how about storing the object type at the time of parsing the list and
>>> use that instead
>>> of doing string manipulations for each function name searches.
>>>
>>> With these two changes, I have cooked up a quick patch on top of your
>>> 0001-dml-adaptive-patch-v4.
>>> Can you have a look at the attached patch and see if you have any
>>> reservations with this approach?
>>>
>>> Also, note that I have just made this patch and haven't tested it, so I
>>> am expecting that you may find
>>> some issues with it :-)
>>>
>>>
>>> Thanks
>>> Best regards.
>>>
>>>
>>>
>>> On Mon, Jun 29, 2020 at 6:36 AM sunbiao at highgo.com <sunbiao at highgo.com>
>>> wrote:
>>>
>>>> Hi, Usama
>>>>
>>>> I made a new patch.Including the following：
>>>> 1.Updated documentation
>>>> 2.Used json to save relationship list
>>>> 3.Changed 'dml_load_balance' to 'dml_adaptive'
>>>> 4.Added extended-query-test
>>>> 5.Tested 'git apply'
>>>>
>>>> Thanks
>>>> Best Regards
>>>>
>>>> ------------------------------
>>>> sunbiao at highgo.com
>>>>
>>>>
>>>> *From:* sunbiao at highgo.com
>>>> *Date:* 2020-06-22 10:57
>>>> *To:* Muhammad Usama <m.usama at gmail.com>
>>>> *CC:* Tatsuo Ishii <ishii at sraoss.co.jp>; pgpool-hackers
>>>> <pgpool-hackers at pgpool.net>
>>>> *Subject:* Re: [pgpool-hackers: 3592] add a feature: dml object level
>>>> load balance
>>>> Hi, Usama
>>>>
>>>> I use the raw_expression_tree_walker() function to find out relname.
>>>> This function is only valid for DML statements
>>>> (SELECT/INSERT/UPDATE/DELETE).
>>>> So I named this feature ‘dml_load_balance’.
>>>>
>>>> ‘adaptive’ it feels like that can parse all statements. It can't
>>>> actually.
>>>> Maybe we can call it 'dml_adaptive'.
>>>>
>>>> According to comments from you and Tatsuo Ishii, i will make a new patch.
>>>>
>>>> Thanks
>>>> Best Regards
>>>> ------------------------------
>>>> sunbiao at highgo.com
>>>>
>>>>
>>>> *From:* Tatsuo Ishii <ishii at sraoss.co.jp>
>>>> *Date:* 2020-06-20 17:50
>>>> *To:* m.usama at gmail.com
>>>> *CC:* sunbiao at highgo.com; pgpool-hackers at pgpool.net
>>>> *Subject:* Re: [pgpool-hackers: 3592] add a feature: dml object level
>>>> load balance
>>>> Hi Usama,
>>>>
>>>> > Hi Sunbiao,
>>>> >
>>>> > Thanks for the updated patch. Overall the patch looks good and works as
>>>> > expected.
>>>> >
>>>> > However, I am a little concerned about the performance aspect of
>>>> > the check_object_relationship_list() function.
>>>> > Since it is parsing the item in the
>>>> > dml_load_balance_object_relationship_list
>>>> > list every time it is invoked. So I think we need to fissure out the
>>>> way to
>>>> > store the pre-parsed list (which can be constructed at session start)
>>>> and
>>>> > try
>>>> > to save the parsing at each function call.
>>>> >
>>>> > Other than that you need to provide the documentation updates for the
>>>> > feature.
>>>> >
>>>> > Finally, one last comment is how about if we change the
>>>> > disable_load_balance_on_write
>>>> > setting name from 'dml_load_balance' to 'adaptive'?
>>>> >
>>>> >
>>>> > @Tatsuo Ishii <ishii at sraoss.co.jp>
>>>> > What do you think about this feature and patch? If you do not have any
>>>> > reservations then
>>>> > I will commit it after Sunbiao takes care of review comments.
>>>>
>>>> Looks good to me except subtle points below:
>>>>
>>>> - The test should include tests for extended query:
>>>> i.e. src/test/extended-query-test. There are some tests or
>>>> disable-load-balance. So it's better to add a test to them.
>>>>
>>>> - There are extra spaces in the patch. Also it needs rebase.
>>>>
>>>> t-ishii$ git apply ~/0001-dml-load-balance-patch-v3.patch
>>>> /home/t-ishii/0001-dml-load-balance-patch-v3.patch:327: space before tab
>>>> in indent.
>>>> * dml_load_balance_object_relationship_list */
>>>> /home/t-ishii/0001-dml-load-balance-patch-v3.patch:679: trailing
>>>> whitespace.
>>>> CREATE OR REPLACE FUNCTION insert_tb_t2_func() RETURNS TRIGGER AS
>>>> $example_table$
>>>> /home/t-ishii/0001-dml-load-balance-patch-v3.patch:680: trailing
>>>> whitespace.
>>>> BEGIN
>>>> /home/t-ishii/0001-dml-load-balance-patch-v3.patch:681: trailing
>>>> whitespace.
>>>> INSERT INTO tb_t2 VALUES (1);
>>>> /home/t-ishii/0001-dml-load-balance-patch-v3.patch:682: trailing
>>>> whitespace.
>>>> RETURN NEW;
>>>> error: patch failed: src/context/pool_session_context.c:170
>>>> error: src/context/pool_session_context.c: patch does not apply
>>>>
>>>> > Thanks
>>>> > Best Regards
>>>> > Muhammad Usama
>>>> >
>>>> >
>>>> > Thanks
>>>> > Best Regards
>>>> > Muhammad Usama
>>>> >
>>>> > On Mon, Jun 15, 2020 at 7:25 AM sunbiao at highgo.com <sunbiao at highgo.com
>>>> >
>>>> > wrote:
>>>> >
>>>> >> Hi, Usama
>>>> >> I found a problem in patch v2.
>>>> >> I can not use “pool show” to show new added parameter.
>>>> >> So i made patch v3.
>>>> >>
>>>> >> I make disable_load_balance_on_write to accept new value.
>>>> >> Set disable_load_balance_on_write = 'dml_load_balance' to enable this
>>>> >> feature.
>>>> >> This new patch contains a test script in path
>>>> >> ‘src/test/dml-load-balance-test’.
>>>> >> If pg installed by default in /usr/local/pgsql, just execute test.sh.
>>>> >> If pg is in other dir, execute ‘test.sh -p /path_to_pg_dir/’.
>>>> >> It will show “success: dml load balance test pass.” , when test pass.
>>>> >>
>>>> >> this script will test below sql:
>>>> >> show pool_nodes;
>>>> >>
>>>> >> -- test DML
>>>> >> begin ;
>>>> >> insert into tb_dml_insert values (1);
>>>> >> select * from tb_dml_insert ;
>>>> >> commit ;
>>>> >>
>>>> >> begin ;
>>>> >> update tb_dml_update SET a = 2;
>>>> >> select * from tb_dml_update ;
>>>> >> commit ;
>>>> >>
>>>> >> begin ;
>>>> >> delete from tb_dml_delete;
>>>> >> select * from tb_dml_delete;
>>>> >> commit ;
>>>> >>
>>>> >> -- test trigger
>>>> >> begin ;
>>>> >> insert into tb_t1 values (1);
>>>> >> select * from tb_t2 ;
>>>> >> commit ;
>>>> >>
>>>> >> -- test function
>>>> >> begin ;
>>>> >> select insert_tb_f_func(6);
>>>> >> select * from tb_f ;
>>>> >> commit ;
>>>> >>
>>>> >> -- test view
>>>> >> begin ;
>>>> >> insert into tb_v values (8);
>>>> >> select * from tb_v_view ;
>>>> >> commit ;
>>>> >>
>>>> >> Thanks
>>>> >> Best Regards
>>>> >> ------------------------------
>>>> >> sunbiao at highgo.com
>>>> >>
>>>> >>
>>>> >> *From:* Muhammad Usama <m.usama at gmail.com>
>>>> >> *Date:* 2020-06-12 18:08
>>>> >> *To:* sunbiao at highgo.com
>>>> >> *CC:* pgpool-hackers <pgpool-hackers at pgpool.net>
>>>> >> *Subject:* Re: [pgpool-hackers: 3592] add a feature: dml object level
>>>> >> load balance
>>>> >> Hi Sunbiao,
>>>> >>
>>>> >> Can you kindly resend the 0001-dml-load-balance-patch-v2.patch patch?
>>>> I
>>>> >> am not able to download the patch file.
>>>> >>
>>>> >> Thanks
>>>> >> Best regards
>>>> >> Muhammad Usama
>>>> >>
>>>> >>
>>>> >> On Fri, Apr 24, 2020 at 9:55 PM Muhammad Usama <m.usama at gmail.com>
>>>> wrote:
>>>> >>
>>>> >>> Hi Sunbiao,
>>>> >>>
>>>> >>> Thanks for the patch and it looks line an interesting feature. I have
>>>> >>> just skimmed
>>>> >>> through the patch and I have a few of small comments.
>>>> >>>
>>>> >>> 1 - Wouldn't it be better to add a new mode existing
>>>> >>> disable_load_balance_on_write parameter
>>>> >>> instead of adding a new configuration parameter i.e
>>>> >>> dml_object_level_load_balance ?
>>>> >>> You can make disable_load_balance_on_write to accept new value like
>>>> >>> disable_load_balance_on_write = 'object' to enable this feature.
>>>> >>>
>>>> >>> 2- The patch contains some invalid changes in
>>>> pgpool.conf.sample-stream
>>>> >>> file
>>>> >>> i.e it changes the default values of black_function_list and
>>>> >>> disable_load_balance_on_write
>>>> >>> configuration parameters which I am sure were not intended
>>>> >>>
>>>> >>> 3- The default value for new configuration parameter
>>>> >>> dml_object_level_load_balance_token_list
>>>> >>> better be an empty
>>>> >>>
>>>> >>> 4- Instead of attaching a separate test script you could include a
>>>> proper
>>>> >>> test case for the feature.
>>>> >>>
>>>> >>> Thanks
>>>> >>> Best Regards
>>>> >>>
>>>> >>> Muhammad Usama
>>>> >>> Highgo Software (Canada/China/Pakistan)
>>>> >>> URL : http://www.highgo.ca
>>>> >>> ADDR: 10318 WHALLEY BLVD, Surrey, BC
>>>> >>>
>>>> >>>
>>>> >>>
>>>> >>> On Fri, Apr 24, 2020 at 3:43 PM sunbiao at highgo.com <
>>>> sunbiao at highgo.com>
>>>> >>> wrote:
>>>> >>>
>>>> >>>> Hi Hackers,
>>>> >>>> If sql like below:
>>>> >>>>
>>>> >>>> begin ;
>>>> >>>> update tb_1 SET id = 1;
>>>> >>>> select * from tb_1 ;
>>>> >>>> select * from tb_2 ;
>>>> >>>> select * from tb_3 ;
>>>> >>>> select * from tb_4 ;
>>>> >>>> select * from tb_5 ;
>>>> >>>> commit ;
>>>> >>>>
>>>> >>>> when set disable_load_balance_on_write = 'transaction'. write
>>>> queries
>>>> >>>> appear in an explicit transaction, subsequent read queries are not
>>>> load
>>>> >>>> balanced until the transaction ends. so all sql will be sent to
>>>> primary
>>>> >>>> node.
>>>> >>>>
>>>> >>>> i think that “update tb_1 SET id = 1” and “select * from tb_1”
>>>> should be
>>>> >>>> sent to primary node.
>>>> >>>> actually, tb_2 tb_3 tb_4 tb_5 can be sent to standby node. if do
>>>> this,
>>>> >>>> will reduce primary load.
>>>> >>>>
>>>> >>>> so i made a patch to implement my idea.
>>>> >>>> when transaction start, i will initialize a list to save table name
>>>> of
>>>> >>>> write queries.
>>>> >>>> read queries will check the list, if find the table name in list,
>>>> read
>>>> >>>> queries will be sent to primary.
>>>> >>>> when transaction end, i destroy the list.
>>>> >>>>
>>>> >>>> i add two parameter:
>>>> >>>>
>>>> >>>> dml_object_level_load_balance = on
>>>> >>>> dml_object_level_load_balance_token_list=
>>>> >>>> 'tb_t1:tb_t2,insert_tb_f_func():tb_f,tb_v:tb_v_view'
>>>> >>>>
>>>> >>>> use dml_object_level_load_balance_token_list to set relationships
>>>> >>>> between objects, such as trigger, function, view.
>>>> >>>> If set dml_object_level_load_balance = on,
>>>> disable_load_balance_on_write
>>>> >>>> should be off.
>>>> >>>>
>>>> >>>> Is it possible to add this feature?
>>>> >>>> ------------------------------
>>>> >>>> sunbiao at highgo.com
>>>> >>>> _______________________________________________
>>>> >>>> pgpool-hackers mailing list
>>>> >>>> pgpool-hackers at pgpool.net
>>>> >>>> http://www.pgpool.net/mailman/listinfo/pgpool-hackers
>>>> >>>>
>>>> >>>
>>>>
>>>>
>>>
>>> --
>>> ...
>>> Muhammad Usama
>>> Highgo Software (Canada/China/Pakistan)
>>> URL : http://www.highgo.ca
>>> ADDR: 10318 WHALLEY BLVD, Surrey, BC
>>>
>>>
More information about the pgpool-hackers
mailing list