[pgpool-hackers: 3727] Re: add a feature: dml object level load balance
Muhammad Usama
m.usama at gmail.com
Wed Jul 22 13:24:59 JST 2020
On Wed, Jul 22, 2020 at 7:32 AM Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
> 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.
>
Thanks for pointing it out, I somehow overlooked it.
I will take care of it and move the simple query test case
to the standard regression directory.
Thanks
Best Regards
>
> 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
> >>>
> >>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200722/86eaf6ff/attachment-0001.html>
More information about the pgpool-hackers
mailing list