[pgpool-hackers: 3610] Re: PCP command to interact with Pgpool-II cluster
Muhammad Usama
m.usama at gmail.com
Mon May 4 00:38:52 JST 2020
Hi Ishii-San
Thanks for looking into the patch and valuable feedback.
Please find the V2 of the patch that fixes the mentioned issues
except the extended-query-test produces, that is failing because the
expected
output was generated by some different version of PostgreSQL and now the
line number in the expected notice messages are coming out from different
line numbers of the PG code.
So we may need to fix the test cases and I don't find any functionality
issues
or any issue that is caused by the patch
See the differences in the result and expected outputs
[highgo at 6132f8b15f3f extended-query-test]$ diff
results/disable-load-balance-off-black-function.data
expected/disable-load-balance-off-black-function.data
2c2
< <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M function f1() does not
exist, skipping F dropcmds.c L *491* R does_not_exist_skipping )
---
> <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M function f1() does not
exist, skipping F dropcmds.c L *483* R does_not_exist_skipping )
[highgo at 6132f8b15f3f extended-query-test]$ diff
results/disable-load-balance-off-black-function.data
expected/disable-load-balance-trans-black-function.data
2c2
< <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M function f1() does not
exist, skipping F dropcmds.c L *491* R does_not_exist_skipping )
---
> <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M function f1() does not
exist, skipping F dropcmds.c L *483* R does_not_exist_skipping )
[highgo at 6132f8b15f3f extended-query-test]$ diff
results/disable-load-balance-default.data
expected/disable-load-balance-default.data
2c2
< <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M table "pgproto_test1"
does not exist, skipping F tablecmds.c L *1186* R DropErrorMsgNonExistent )
---
> <= BE NoticeResponse(S NOTICE V NOTICE C 00000 M table "pgproto_test1"
does not exist, skipping F tablecmds.c L *914* R DropErrorMsgNonExistent )
Thanks
Best regards
...
Muhammad Usama
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
On Sat, May 2, 2020 at 6:28 PM Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
> Hi Usama,
>
> Here is a report on quick tests with your patch.
>
> 1) git apply produced following some warnings.
>
> /home/t-ishii/pcp_stop_cluster.diff:1984: trailing whitespace.
>
> /home/t-ishii/pcp_stop_cluster.diff:3919: trailing whitespace.
>
> /home/t-ishii/pcp_stop_cluster.diff:2882: new blank line at EOF.
> +
> /home/t-ishii/pcp_stop_cluster.diff:5068: new blank line at EOF.
> +
> warning: 4 lines add whitespace errors.
>
> 2) compiling produced following warnings.
>
> wd_escalation.c: In function ‘fork_escalation_process’:
> wd_escalation.c:79:2: warning: ‘sigsetmask’ is deprecated
> [-Wdeprecated-declarations]
> POOL_SETMASK(&UnBlockSig);
> ^~~~~~~~~~~~
> In file included from ../../src/include/utils/pool_signal.h:33:0,
> from wd_escalation.c:34:
> /usr/include/signal.h:173:12: note: declared here
> extern int sigsetmask (int __mask) __THROW __attribute_deprecated__;
> ^~~~~~~~~~
> wd_escalation.c: In function ‘fork_plunging_process’:
> wd_escalation.c:168:2: warning: ‘sigsetmask’ is deprecated
> [-Wdeprecated-declarations]
> POOL_SETMASK(&UnBlockSig);
> ^~~~~~~~~~~~
> In file included from ../../src/include/utils/pool_signal.h:33:0,
> from wd_escalation.c:34:
> /usr/include/signal.h:173:12: note: declared here
> extern int sigsetmask (int __mask) __THROW __attribute_deprecated__;
> ^~~~~~~~~~
> main/main.c: In function ‘main’:
> main/main.c:312:3: warning: implicit declaration of function
> ‘SSL_ServerSide_init’; did you mean ‘SSL_in_init’?
> [-Wimplicit-function-declaration]
> SSL_ServerSide_init();
> ^~~~~~~~~~~~~~~~~~~
> SSL_in_init
>
> 3) There is a one failure in the regression test.
>
> testing 010.rewrite_timestamp...failed.
>
> 4) extedend-query-test produces 10 errors.
>
> t-ishii$ ./test.sh
> creating pgpool-II temporary installation ...
> moving pgpool_setup to temporary installation path ...
> using pgpool-II at
> /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/extended-query-test/temp/installed
> *************************
> REGRESSION MODE : install
> PGPOOL-II :
> /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/extended-query-test/temp/installed
> PostgreSQL bin : /usr/local/pgsql/bin
> pgbench : /usr/local/pgsql/bin/pgbench
> PostgreSQL jdbc : /usr/local/pgsql/share/postgresql-9.2-1003.jdbc4.jar
> *************************
> *** creating test database with 2 nodes...done.
> statement_level_load_balance = off
> testing aborted-transaction.data ... ok.
> testing bug370-sql-error-followed-by-select.data ... ok.
> testing disable-load-balance-always-black-function.data ... ok.
> testing disable-load-balance-always.data ... ok.
> testing disable-load-balance-default-black-function.data ... extra test
> failed.
> testing disable-load-balance-default-simple.data ... ok.
> testing disable-load-balance-default.data ... extra test failed.
> testing disable-load-balance-off-black-function.data ... extra test failed.
> testing disable-load-balance-off.data ... ok.
> testing disable-load-balance-simple-black-function.data ... ok.
> testing disable-load-balance-simple.data ... ok.
> testing disable-load-balance-trans-black-function.data ... extra test
> failed.
> testing disable-load-balance-white-function.data ... ok.
> testing disable-load-balance.data ... extra test failed.
> testing node_js.data ... ok.
> testing parse-before-bind-2.data ... ok.
> testing parse-before-bind.data ... ok.
> testing query-cache-notrans.data ... ok.
> testing query-cache.data ... ok.
> testing select-multi-rows.data ... ok.
> testing sql-error.data ... ok.
> testing unable_to_bind.data ... ok.
> *** creating test database with 2 nodes...done.
> statement_level_load_balance = on
> testing aborted-transaction.data ... ok.
> testing bug370-sql-error-followed-by-select.data ... ok.
> testing disable-load-balance-always-black-function.data ... ok.
> testing disable-load-balance-always.data ... ok.
> testing disable-load-balance-default-black-function.data ... extra test
> failed.
> testing disable-load-balance-default-simple.data ... ok.
> testing disable-load-balance-default.data ... extra test failed.
> testing disable-load-balance-off-black-function.data ... extra test failed.
> testing disable-load-balance-off.data ... ok.
> testing disable-load-balance-simple-black-function.data ... ok.
> testing disable-load-balance-simple.data ... ok.
> testing disable-load-balance-trans-black-function.data ... extra test
> failed.
> testing disable-load-balance-white-function.data ... ok.
> testing disable-load-balance.data ... extra test failed.
> testing node_js.data ... ok.
> testing parse-before-bind-2.data ... ok.
> testing parse-before-bind.data ... ok.
> testing query-cache-notrans.data ... ok.
> testing query-cache.data ... ok.
> testing select-multi-rows.data ... ok.
> testing sql-error.data ... ok.
> testing unable_to_bind.data ... ok.
> creating test database with 3 nodes...done.
> testing statement_level_load_balance.data ... ok.
> out of 45 ok: 35 failed: 10 timeout: 0.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>
> From: Muhammad Usama <m.usama at gmail.com>
> Subject: Re: PCP command to interact with Pgpool-II cluster
> Date: Fri, 1 May 2020 15:16:50 +0500
> Message-ID: <
> CAEJvTzXWkQhWgJkM+cW1cCNeZYbaF_kcE7vVuHvWB5SmNnUeZw at mail.gmail.com>
>
> > Hi,
> >
> > While working on the pcp_stop_pgpool command to implement cluster mode,
> > I realized that we have a lot of clutter in few source
> > files. Especially pool.h include
> > file. So I also did some code reorganization and added a bunch of new
> > include
> > files in the hope to reduce the size of pool.h and also get rid of too
> many
> > global functions.
> >
> > Along with that, I think there are few other files that need a little
> > shredding,
> > src/main/pgpool_main.c and src/protocol/child.c are at the top of this
> > wanted to be trimmed list.
> > So as part of this patch, I have taken a few things related to internal
> > commands
> > (interface between the child processes can and Pgpool-II main process)
> from
> > pgpool_main.c file and moved them into new "
> > src/main/pgpool_internal_commands.c"
> >
> > Similarly, src/protocol/child.c had and still has so many functions that
> do
> > not fit
> > with the personality of child.c file. So I have moved some of the stuff
> > related to
> > DB functions info src/protocol/pool_pg_utils.c file.
> >
> > But I think this is still not enough and we may require another round (
> if
> > someone
> > is willing to work on it) of a source file shredding.
> >
> > Also, I would like to suggest that we just try to refrain from putting
> > everything in pool.h
> > and only do that if it is absolutely necessary.
> > As pool.h is like a global include for Pgpool-II source and in my opinion
> > other than few
> > struct definitions, Macros, preprocessor Defines, global variable externs
> > like pool_config
> > there are very few things (especially functions) that really needed to be
> > globally
> > scoped in Pgpool-II.
> > So I would like to suggest that we should at least think twice before
> > adding anything to
> > pool.h file and consider adding a new include file or even new source
> file
> > if we
> > can't find an appropriate existing file for putting in the function we
> just
> > worked up.
> >
> > Other than lots and lots of core reorganization changes the patch
> implements
> > "--scope=cluster/local" option in the pcp_stop_pgpool command and adds a
> > proper
> > infrastructure in watchdog for broadcasting the custom commands over the
> > watchdog
> > cluster to make it easier to implement the --scope=cluster for other PCP
> > utilities.
> >
> > Note on implementing the new command to execute over the watchdog
> network.
> >
> > Step 1: Define the command name string
> > ============
> > Define a new string to identify the command name in
> > src/include/watchdog/wd_ipc_defines.h
> >
> > For example
> >
> > +#define WD_COMMAND_RESTART_CLUSTE "RESTART_CLUSTER"
> > +#define WD_COMMAND_REELECT_MASTER "REELECT_MASTER"
> > +#define WD_COMMAND_SHUTDOWN_CLUSTER "SHUTDOWN_CLUSTER"
> >
> > Step 2: send the command to watchdog
> > =========
> > Construct the argument list using WDExecCommandArg struct
> > and just fire wd_execute_cluster_command() function with your new
> command
> > name
> > and argument list.
> > See implementation of process_shutown_request() for details
> >
> > ....
> >
> > WDExecCommandArg wdExecCommandArg;
> >
> >
> > strncpy(wdExecCommandArg.arg_name, "mode",
> > sizeof(wdExecCommandArg.arg_name) - 1);
> >
> > snprintf(wdExecCommandArg.arg_value, sizeof(wdExecCommandArg.arg_name) -
> 1,
> > "%c",mode);
> >
> >
> > if (wd_execute_cluster_command(WD_COMMAND_SHUTDOWN_CLUSTER,1,
> > &wdExecCommandArg) != COMMAND_OK)
> >
> > ereport(ERROR,
> >
> > (errmsg("PCP: error while processing shutdown cluster request"),
> >
> > errdetail("failed to propogate shutdown command through
> > watchdog")));
> > ...
> >
> >
> > Step 3: Implement the executor
> > ===========
> > *wd_execute_cluster_command_processor*(..) in watchdog.c is a place
> where a
> > remote
> > Pgpool-II would receive a command. So Just verify if its the command you
> > are implementing
> > by comparing the clusterCommand string as it is done for the shutdown
> > command
> >
> > if (strcasecmp(WD_COMMAND_SHUTDOWN_CLUSTER, clusterCommand) == 0)
> > {
> > ...
> > }
> >
> >
> > @Tatsuo Ishii <ishii at sraoss.co.jp> what are your thoughts on using the
> > "--scope=[local | cluster ]" argument
> > and also on the code reorganization.
> >
> >
> > Thanks
> > Best regards
> > ...
> > Muhammad Usama
> > Highgo Software (Canada/China/Pakistan)
> > URL : http://www.highgo.ca
> > ADDR: 10318 WHALLEY BLVD, Surrey, BC
> >
> >
> >
> >
> > On Tue, Apr 28, 2020 at 5:30 PM Muhammad Usama <m.usama at gmail.com>
> wrote:
> >
> >>
> >>
> >> On Thu, Apr 23, 2020 at 2:06 PM Muhammad Usama <m.usama at gmail.com>
> wrote:
> >> >
> >> > On Tue, Apr 21, 2020 at 12:33 PM Tatsuo Ishii <ishii at sraoss.co.jp>
> >> wrote:
> >> > >
> >> > > Usama,
> >> > >
> >> > > Thank you for the propsal.
> >> > >
> >> > > > Hi Hackers,
> >> > > >
> >> > > > When PCP was first added to Pgpool-II it was meant to
> manage/control
> >> > > > the single Pgpool-II instance.
> >> > > > And after the addition of watchdog more often than not multiple
> >> > > > Pgpool-II nodes are installed the system
> >> > > > and there is not much work/thoughts have gone into the
> enhancements
> >> of
> >> > > > the PCP utilities to make
> >> > > > them work on the Pgpool-II cluster. So lately I was thinking to
> >> > > > enhance/add the PCP utilities to make them
> >> > > > more cluster-mode friendly.
> >> > > > And for that purpose, I think we would require to add/enhance the
> >> > > > following PCP commands.
> >> > > >
> >> > > > 1- pcp_reload_config
> >> > > > ==================
> >> > > > The utility to make the whole Pgpool-II cluster to reload the
> >> > > > configuration files at once.
> >> > > > I think this is required because currently if some installation
> has
> >> > > > let say 3 node Pgpool-II cluster
> >> > > > and we want to change one specific configuration parameter ( for
> >> > > > example: adding a new backend node)
> >> > > > we would require to go on and edit Pgpool configuration files on
> each
> >> > > > node separately and then
> >> > > > issue pgpool reload on each node one by one.
> >> > > >
> >> > > > In my opinion, we need multiple enhancement in this area.
> >> > > > We need some mechanisms to let pgpool-II cluster use the
> centralized
> >> > > > configuration
> >> > > > ( except watchdog config) or at least a mechanism to push the
> >> > > > configuration settings using some
> >> > > > new utility like "pcp_load_config" that could take a pgpool.conf
> file
> >> > > > and propagate it to all nodes
> >> > > > using watchdog.
> >> > >
> >> > > Yeah, we definitely need it.
> >> > >
> >> > > > The centralized configuration part of the problem is a big task
> and I
> >> > > > believe we could take it on after the
> >> > > > "simplifying watchdog config" feature that Peng is working on.
> >> > > > Meanwhile, I think we can work on pcp_reload_config utility in
> >> > > > parallel to make reloading part easier.
> >> > >
> >> > > +1.
> >> > >
> >> > > > 2- pcp_restart_pgpool [cluster]
> >> > > > ==========================
> >> > > > Sometimes, a simple restart solves more problems than any other
> >> remedy
> >> > > > or debugging.
> >> > > > So I think we should have a PCP utility to restart a Pgpool-II
> node
> >> or
> >> > > > all node on Pgpool-II cluster.
> >> > > > something similar in functionality like "pg_ctl restart" in
> postgres
> >> > >
> >> > > +1.
> >> > >
> >> > > > 3- pcp_watchdog_re-elect
> >> > > > ======================
> >> > > > What if we want to change the master/coordinator node of the
> >> watchdog cluster.
> >> > > > There exists no good way for that currently. So I purpose we
> should
> >> > > > have a command to make
> >> > > > watchdog re-do the elections. I already had a discussion on this
> one
> >> > > > with Ishii-San sometimes
> >> > > > back so I will send a separate proposal for that as well
> >> > >
> >> > > Looking forward to seeing it.
> >> > >
> >> > > > 4- pcp_stop_pgpool enhancements
> >> > > > ===================
> >> > > > pcp_stop_pgpool should be able to stop all the Pgpool-II nodes in
> >> > > > Pgpool-II cluster.
> >> > > > And for that, I have also cooked up a POC patch which allows the
> >> > > > [--all (-a)] option to
> >> > > > pcp_stop_pgpool utility and when this --all is specified then the
> >> > > > utility stops all the Pgpool-II
> >> > > > nodes part of the watchdog cluster.
> >> > > > So can you please try the patch and also suggest about the command
> >> > > > line option we should
> >> > > > use for making the PCP commands interact with cluster rather than
> the
> >> > > > single node.
> >> > > > Because I think --all would not be the best choice since it is
> used
> >> by
> >> > > > other utilities for a different purpose,
> >> > > > and we should come up with new standard command-line switch for
> all
> >> > > > utilities if they want to operate on
> >> > > > the cluster rather than on a single node.
> >> > >
> >> > > +1.
> >> > >
> >> > > What about:
> >> > >
> >> > > --cluster, -c /* affect to whole watchdog cluster */
> >> > > --local, -l /* affect only to local watchdog cluster */
> >> >
> >> > Yes seems like a good option since both -c and -l are currently not
> >> > used by any PCP utility.
> >> >
> >> >
> >> > >
> >> > > We actually need --cluster only but for completeness I feel like
> we'd
> >> > > want to have --local as well (it exactly behaves as PCP commands do
> >> > > today).
> >> > >
> >> > > > Or another way would be to leave the pcp_stop_pgpool utility
> >> untouched
> >> > > > and add a new utility pcp_stop_cluster ?
> >> > > >
> >> > > > So what are your suggestions and thoughts on this and do you have
> >> > > > other functions especially related to
> >> > > > cluster mode of pgpool-II that needed controlling/management using
> >> PCP system?
> >> > >
> >> > > I have tested your patch and it worked as expected. Great!
> >> >
> >> > Thanks, I will make the argument changes (-c and -l) and push it.
> >>
> >>
> >> I have been thinking about this further, Instead of introducing -c and
> -l
> >> how about adding new argument scope which can be assigned either local
> or
> >> cluster
> >>
> >> i.e.
> >>
> >> --scope=[local | cluster]
> >> and the short form would be -s=[l|c]
> >>
> >> Regards
> >> Muhammad Usama
> >>
> >> >
> >> >
> >> > Best regards
> >> > Muhammad Usama
> >> >
> >> > >
> >> > > Best regards,
> >> > > --
> >> > > Tatsuo Ishii
> >> > > SRA OSS, Inc. Japan
> >> > > English: http://www.sraoss.co.jp/index_en.php
> >> > > Japanese:http://www.sraoss.co.jp
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200503/2ad28bd5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcp_stop_cluster_v2.diff
Type: application/octet-stream
Size: 181153 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200503/2ad28bd5/attachment-0001.obj>
More information about the pgpool-hackers
mailing list