[pgpool-hackers: 3723] Re: add a feature: dml object level load balance
Muhammad Usama
m.usama at gmail.com
Tue Jul 21 20:06:58 JST 2020
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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200721/62e53735/attachment-0001.html>
More information about the pgpool-hackers
mailing list