[pgpool-hackers: 3608] Re: PCP command to interact with Pgpool-II cluster

Fri May 1 19:16:50 JST 2020

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/20200501/748cb87b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcp_stop_cluster.diff
Type: application/octet-stream
Size: 180204 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200501/748cb87b/attachment-0001.obj>