<div dir="ltr"><div>Hi,</div><div><br></div><div>While working on the pcp_stop_pgpool command to implement cluster mode,</div><div>I realized that we have a lot of clutter in few source files. Especially pool.h include</div><div>file. So I also did some code reorganization and added a bunch of new include</div><div>files in the hope to reduce the size of pool.h and also get rid of too many</div><div>global functions.</div><div><br></div><div>Along with that, I think there are few other files that need a little shredding,</div><div>src/main/pgpool_main.c and src/protocol/child.c are at the top of this</div><div>wanted to be trimmed list. </div><div>So as part of this patch, I have taken a few things related to internal commands</div><div>(interface between the child processes can and Pgpool-II main process) from</div><div>pgpool_main.c file and moved them into new <font color="#2fb41d" face="Meslo LG L DZ for Powerline"><span style="font-size:14px;font-variant-ligatures:no-common-ligatures">"</span></font>src/main/pgpool_internal_commands.c"</div>
<div><br></div><div>Similarly, src/protocol/child.c had and still has so many functions that do not fit</div><div>with the personality of child.c file. So I have moved some of the stuff related to</div><div>DB functions info src/protocol/pool_pg_utils.c file.</div><div><br></div><div>But I think this is still not enough and we may require another round ( if someone</div><div>is willing to work on it) of a source file shredding. </div>
<div><br></div>Also, I would like to suggest that we just try to refrain from putting everything in pool.h<div>and only do that if it is absolutely necessary. </div><div>As pool.h is like a global include for Pgpool-II source and in my opinion other than few</div><div>struct definitions, Macros, preprocessor Defines, global variable externs like pool_config</div><div>there are very few things (especially functions) that really needed to be globally</div><div>scoped in Pgpool-II.</div><div>So I would like to suggest that we should at least think twice before adding anything to</div><div>pool.h file and consider adding a new include file or even new source file if we</div><div>can't find an appropriate existing file for putting in the function we just worked up.</div><div><br></div><div>Other than lots and lots of core reorganization changes the patch implements</div><div>"--scope=cluster/local" option in the pcp_stop_pgpool command and adds a proper</div><div>infrastructure in watchdog for broadcasting the custom commands over the watchdog</div><div>cluster to make it easier to implement the --scope=cluster for other PCP utilities.</div><div><br></div><div>Note on implementing the new command to execute over the watchdog network.</div><div><br></div><div>Step 1: Define the command name string</div><div>============</div><div>Define a new string to identify the command name in <span style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0);font-family:"Meslo LG L DZ for Powerline";font-size:14px">src/include/watchdog/wd_ipc_defines.h</span></div>
<div><br></div><div>For example </div><div><br></div><font face="monospace" size="4">+#define WD_COMMAND_RESTART_CLUSTE "RESTART_CLUSTER"<br>+#define WD_COMMAND_REELECT_MASTER "REELECT_MASTER"<br>+#define WD_COMMAND_SHUTDOWN_CLUSTER "SHUTDOWN_CLUSTER"</font><div><br></div><div>Step 2: send the command to watchdog </div><div>=========</div>Construct the argument list using WDExecCommandArg struct<div>and just fire wd_execute_cluster_command() function with your new command name</div><div>and argument list.</div><div>See implementation of process_shutown_request() for details</div><div><br></div><div>....</div><div>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" style="" size="4" color="#000000"><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">WDExecCommandArg wdExecCommandArg;</span></font></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><br></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">strncpy(wdExecCommandArg.arg_name, "mode", sizeof(wdExecCommandArg.arg_name) - 1);</span></font></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">snprintf(wdExecCommandArg.arg_value, sizeof(wdExecCommandArg.arg_name) - 1, "%c",mode);</span></font></p>
<p class="gmail-p2" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><br></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">if (wd_execute_cluster_command(WD_COMMAND_SHUTDOWN_CLUSTER,1, &wdExecCommandArg) != COMMAND_OK)</span></font></p>
<p class="gmail-p2" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures"> <span class="gmail-Apple-converted-space"> </span></span><span class="gmail-s3" style="font-variant-ligatures:no-common-ligatures">ereport(ERROR,</span></font></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s1" style="font-variant-ligatures:no-common-ligatures"> <span class="gmail-Apple-converted-space"> </span></span><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">(errmsg("PCP: error while processing shutdown cluster request"),</span></font></p>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:normal"><font face="monospace" size="4" color="#000000"><span class="gmail-s1" style="font-variant-ligatures:no-common-ligatures"><span class="gmail-Apple-converted-space"> </span></span><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">errdetail("failed to propogate shutdown command through watchdog")));</span></font></p></div><div>...</div><div><br></div><div><br></div><div><div>Step 3: Implement the executor</div><div>===========</div><div><font face="monospace"><b>wd_execute_cluster_command_processor</b></font>(..) in watchdog.c is a place where a remote</div><div>Pgpool-II would receive a command. So Just verify if its the command you are implementing</div><div>by comparing the clusterCommand string as it is done for the shutdown command<br><font face="monospace" size="4"><br>if (strcasecmp(WD_COMMAND_SHUTDOWN_CLUSTER, clusterCommand) == 0)<br>{<br>...<br>}</font></div><div><br></div><div><br></div><div><a class="gmail_plusreply" id="plusReplyChip-1" href="mailto:ishii@sraoss.co.jp" tabindex="-1">@Tatsuo Ishii</a> what are your thoughts on using the "--scope=[local | cluster ]" argument<br></div><div>and also on the code reorganization. </div><div><br></div><div><br></div><div>Thanks</div><div>Best regards</div><div>...</div><div><div style="color:rgb(0,0,0)"><div id="gmail-Zm-_Id_-Sgn"><div><span class="gmail-colour" style="color:rgb(68,68,68)">Muhammad Usama<br></span></div><div><span class="gmail-highlight"><span class="gmail-font" style="font-family:Lato"><span class="gmail-size" style="font-size:14px"><span class="gmail-colour" style="color:rgb(68,68,68)">Highgo Software (Canada/China/Pakistan)</span><span class="gmail-Apple-converted-space"><span class="gmail-colour" style="color:rgb(68,68,68)"> </span></span></span></span></span><span class="gmail-colour" style="color:rgb(68,68,68)"><br></span></div><div><span class="gmail-highlight"><span class="gmail-font" style="font-family:Lato"><span class="gmail-size" style="font-size:14px"><span class="gmail-colour" style="color:rgb(68,68,68)">URL :</span><span class="gmail-Apple-converted-space"><span class="gmail-colour" style="color:rgb(68,68,68)"> </span></span></span></span></span><a target="_blank" href="http://www.highgo.ca/" style="color:rgb(89,143,222);font-family:Lato;font-size:14px"><span class="gmail-colour" style="color:rgb(68,68,68)">http://www.highgo.ca</span></a><span class="gmail-highlight"><span class="gmail-font" style="font-family:Lato"><span class="gmail-size" style="font-size:14px"><span class="gmail-Apple-converted-space"><span class="gmail-colour" style="color:rgb(68,68,68)"> </span></span></span></span></span><span class="gmail-colour" style="color:rgb(68,68,68)"><br></span></div><div><span class="gmail-highlight"><span class="gmail-font" style="font-family:Lato"><span class="gmail-size" style="font-size:14px"><span class="gmail-colour" style="color:rgb(68,68,68)">ADDR: 10318 WHALLEY BLVD, Surrey, BC</span><span class="gmail-Apple-converted-space"><span class="gmail-colour" style="color:rgb(68,68,68)"> </span></span></span></span></span><br></div></div></div></div><div><br></div><div><br></div><div><br><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 28, 2020 at 5:30 PM Muhammad Usama <<a href="mailto:m.usama@gmail.com">m.usama@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br>On Thu, Apr 23, 2020 at 2:06 PM Muhammad Usama <<a href="mailto:m.usama@gmail.com" target="_blank">m.usama@gmail.com</a>> wrote:<br>><br>> On Tue, Apr 21, 2020 at 12:33 PM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>> wrote:<br>> ><br>> > Usama,<br>> ><br>> > Thank you for the propsal.<br>> ><br>> > > Hi Hackers,<br>> > ><br>> > > When PCP was first added to Pgpool-II it was meant to manage/control<br>> > > the single Pgpool-II instance.<br>> > > And after the addition of watchdog more often than not multiple<br>> > > Pgpool-II nodes are installed the system<br>> > > and there is not much work/thoughts have gone into the enhancements of<br>> > > the PCP utilities to make<br>> > > them work on the Pgpool-II cluster. So lately I was thinking to<br>> > > enhance/add the PCP utilities to make them<br>> > > more cluster-mode friendly.<br>> > > And for that purpose, I think we would require to add/enhance the<br>> > > following PCP commands.<br>> > ><br>> > > 1- pcp_reload_config<br>> > > ==================<br>> > > The utility to make the whole Pgpool-II cluster to reload the<br>> > > configuration files at once.<br>> > > I think this is required because currently if some installation has<br>> > > let say 3 node Pgpool-II cluster<br>> > > and we want to change one specific configuration parameter ( for<br>> > > example: adding a new backend node)<br>> > > we would require to go on and edit Pgpool configuration files on each<br>> > > node separately and then<br>> > > issue pgpool reload on each node one by one.<br>> > ><br>> > > In my opinion, we need multiple enhancement in this area.<br>> > > We need some mechanisms to let pgpool-II cluster use the centralized<br>> > > configuration<br>> > > ( except watchdog config) or at least a mechanism to push the<br>> > > configuration settings using some<br>> > > new utility like "pcp_load_config" that could take a pgpool.conf file<br>> > > and propagate it to all nodes<br>> > > using watchdog.<br>> ><br>> > Yeah, we definitely need it.<br>> ><br>> > > The centralized configuration part of the problem is a big task and I<br>> > > believe we could take it on after the<br>> > > "simplifying watchdog config" feature that Peng is working on.<br>> > > Meanwhile, I think we can work on pcp_reload_config utility in<br>> > > parallel to make reloading part easier.<br>> ><br>> > +1.<br>> ><br>> > > 2- pcp_restart_pgpool [cluster]<br>> > > ==========================<br>> > > Sometimes, a simple restart solves more problems than any other remedy<br>> > > or debugging.<br>> > > So I think we should have a PCP utility to restart a Pgpool-II node or<br>> > > all node on Pgpool-II cluster.<br>> > > something similar in functionality like "pg_ctl restart" in postgres<br>> ><br>> > +1.<br>> ><br>> > > 3- pcp_watchdog_re-elect<br>> > > ======================<br>> > > What if we want to change the master/coordinator node of the watchdog cluster.<br>> > > There exists no good way for that currently. So I purpose we should<br>> > > have a command to make<br>> > > watchdog re-do the elections. I already had a discussion on this one<br>> > > with Ishii-San sometimes<br>> > > back so I will send a separate proposal for that as well<br>> ><br>> > Looking forward to seeing it.<br>> ><br>> > > 4- pcp_stop_pgpool enhancements<br>> > > ===================<br>> > > pcp_stop_pgpool should be able to stop all the Pgpool-II nodes in<br>> > > Pgpool-II cluster.<br>> > > And for that, I have also cooked up a POC patch which allows the<br>> > > [--all (-a)] option to<br>> > > pcp_stop_pgpool utility and when this --all is specified then the<br>> > > utility stops all the Pgpool-II<br>> > > nodes part of the watchdog cluster.<br>> > > So can you please try the patch and also suggest about the command<br>> > > line option we should<br>> > > use for making the PCP commands interact with cluster rather than the<br>> > > single node.<br>> > > Because I think --all would not be the best choice since it is used by<br>> > > other utilities for a different purpose,<br>> > > and we should come up with new standard command-line switch for all<br>> > > utilities if they want to operate on<br>> > > the cluster rather than on a single node.<br>> ><br>> > +1.<br>> ><br>> > What about:<br>> ><br>> > --cluster, -c /* affect to whole watchdog cluster */<br>> > --local, -l /* affect only to local watchdog cluster */<br>><br>> Yes seems like a good option since both -c and -l are currently not<br>> used by any PCP utility.<br>><br>><br>> ><br>> > We actually need --cluster only but for completeness I feel like we'd<br>> > want to have --local as well (it exactly behaves as PCP commands do<br>> > today).<br>> ><br>> > > Or another way would be to leave the pcp_stop_pgpool utility untouched<br>> > > and add a new utility pcp_stop_cluster ?<br>> > ><br>> > > So what are your suggestions and thoughts on this and do you have<br>> > > other functions especially related to<br>> > > cluster mode of pgpool-II that needed controlling/management using PCP system?<br>> ><br>> > I have tested your patch and it worked as expected. Great!<br>><br>> Thanks, I will make the argument changes (-c and -l) and push it.<br><br><br>I have been thinking about this further, Instead of introducing -c and -l<br>how about adding new argument scope which can be assigned either local or cluster<div><br></div><div>i.e.</div><div><br><div><font size="4">--scope=[local | cluster]<br>and the short form would be -s=[l|c]<br><br></font>Regards</div><div>Muhammad Usama</div><div> <br>><br>><br>> Best regards<br>> Muhammad Usama<br>><br>> ><br>> > Best regards,<br>> > --<br>> > Tatsuo Ishii<br>> > SRA OSS, Inc. Japan<br>> > English: <a href="http://www.sraoss.co.jp/index_en.php" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>> > Japanese:<a href="http://www.sraoss.co.jp" target="_blank">http://www.sraoss.co.jp</a></div></div></div>
</blockquote></div></div></div></div></div>