<div dir="ltr">Hi Tatsuo/Bruce,<div><br></div><div>Can we find a reviewer for this patch so we can get this in good shape and ready to be included for pgpool 3.4? </div><div><br></div><div>-- Ahsan<br><div><br></div><div>
<br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 10, 2013 at 11:20 PM, Muhammad Usama <span dir="ltr"><<a href="mailto:m.usama@gmail.com" target="_blank">m.usama@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi<div><br></div><div>I have been working on adoption of memory and exception managers in pgpool code and keeping</div>
<div>the track of my work in development branch named EXCEPTION_MGR, And this patch is basically</div>
<div>the diff of dev branch with current master.</div><div><br></div><div>Here is a little background on this front.</div><div>===============<span style="font-family:arial,sans-serif;font-size:13px">===============</span></div>
<div>Although some of the below is already shared on pgpool hackers but to give a little refresher the</div><div><span style="font-family:arial,sans-serif;font-size:13px">plan is to use the PostgreSQL</span><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,204)">exception</span><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,204)">manager</span><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px">API (elog and friends) and integrate it into pgpool.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">Since the PG's</span><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,204)">exception</span><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,204)">manager </span><span style="font-family:arial,sans-serif;font-size:13px">uses the long jump to handle exceptions, so importing that API</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">in pgpool will effect all existing pgpool code flows especially in case of an error. and lot of care</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">will be required for this integration, and secondly the elog API along with its friends will touch almost</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">all parts of pgpool source code which will add up to a very huge patch, </span><span style="font-family:arial,sans-serif;font-size:13px">So I am keeping the work</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">in the development repository </span>EXCEPTION_MGR.</div><div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
Current state of things.<br></div><div><div style="font-size:13px;font-family:arial,sans-serif"><div><div>=================</div><div><br></div><div>-- Code for both Exception Manager borrowed from PG is added to pgpool, The API consists</div>
<div>of elog.c and elog.h files. Since this API is very extensive and is designed for PostgreSQL so</div><div>to fit it properly into pgpool I have modified it a little bit, and most of the modifications are</div><div>related to removal of code which is not required for pgpool. </div>
<div><br></div><div>-- Added on_proc_exit callback mechanism of Postgres. To facilitate the cleanup at exit time.</div><div><br></div></div></div><div style="font-size:13px;font-family:arial,sans-serif">-- Added PostgreSQL's memory <span style="background-color:rgb(255,255,204)">manager</span> (palloc API). This includes the client side palloc</div>
<div style="font-size:13px;font-family:arial,sans-serif">functions placed in 'src/tools' directory (fe_memutils)</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">
-- Removed the existing memory <span style="background-color:rgb(255,255,204)">manager</span> which was very minimalistic and was not</div><div style="font-size:13px;font-family:arial,sans-serif">integrated in all parts of the code.</div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">-- I have also refectored some portions of code to make the code more readable at first glance.</div>
<div style="font-size:13px;font-family:arial,sans-serif">This includes</div><div style="font-size:13px;font-family:arial,sans-serif"><b>- Splitting of main.c file</b>: The main.c file is divided into two files main.c and pgpool_main.c, </div>
<div style="font-size:13px;font-family:arial,sans-serif">Now the main.c file only contains the code related to early initialisations of pgpool and parsing</div><div style="font-size:13px;font-family:arial,sans-serif">the command line options and related code. The actual logic of the pgpool main process is</div>
<div style="font-size:13px;font-family:arial,sans-serif">moved to new pgpool_main.c file.</div><div style="font-size:13px;font-family:arial,sans-serif"><b>- Splitting of some larger functions</b>:</div><div style="font-size:13px;font-family:arial,sans-serif">
<b>- Rewriting of the pgpool main loop logic:</b></div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">
What is left on this front.</div><div style="font-size:13px;font-family:arial,sans-serif">==================</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">
-- There are still some portions of code that have not yet got these managers hooked in. and most</div><div style="font-size:13px;font-family:arial,sans-serif">of that code belongs to the query parsing functionality.</div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">-- elog.c and elog.h files needs some cleanups and changes ( to remove unwanted functions and</div>
<div style="font-size:13px;font-family:arial,sans-serif">data members of ErrorData structure) but this will be done at the end when we will have 100%</div><div style="font-size:13px;font-family:arial,sans-serif">surety if something in there is required or not.<br>
</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">Patch summary for review</div><div style="font-size:13px;font-family:arial,sans-serif">======================</div>
<div style="font-size:13px;font-family:arial,sans-serif">Attached is the diff of dev repository with the current master, since the patch is very large mainly</div><div style="font-size:13px;font-family:arial,sans-serif">
because of new files added from PostgreSQL's code and replacing of pool_error(), pool_debug() and pool_log()</div>
<div style="font-size:13px;font-family:arial,sans-serif">function calls to ereport(ERROR,..) ereport(DEBUG,..) and ereport(LOG,..),</div><div style="font-size:13px;font-family:arial,sans-serif">So it would be little time consuming to review the patch. Although all the above also needs the thorough</div>
<div style="font-size:13px;font-family:arial,sans-serif">review, as replacing pool_error() with ereport(ERROR) or ereport(FATAL) is just not a cosmetic</div><div style="font-size:13px;font-family:arial,sans-serif">but changes the flow of code, but there are few things, that requires the more attention, comments, and suggestions.</div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">Process Loops and Memory Contexts</div><div style="font-size:13px;font-family:arial,sans-serif">
===========================<br>
</div><div style="font-size:13px;font-family:arial,sans-serif">On same footings as of PostgreSQL, the patch creates TOP MEMORY CONTEXT for pgpool process,</div><div style="font-size:13px;font-family:arial,sans-serif">which remains untouched till the life time of the process. and a per loop memory context for each process</div>
<div style="font-size:13px;font-family:arial,sans-serif">which gets resets on every loop iteration. Along with these coded also creates the ERROR CONTEXT</div><div style="font-size:13px;font-family:arial,sans-serif">which is used by elog api same as in PostgreSQL.</div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">pgpool parent process loop </div>
<div style="font-size:13px;font-family:arial,sans-serif">====================</div><div style="font-size:13px;font-family:arial,sans-serif">pgpool_main.c PgpoolMain()<br></div><div style="font-size:13px;font-family:arial,sans-serif">
<br></div><div style="font-size:13px;font-family:arial,sans-serif">The patch changes the flow of how health check failures are processed, health_check() function which</div><div style="font-size:13px;font-family:arial,sans-serif">
previously used to return health check failing node number or success is changed to do_health_check(),</div><div style="font-size:13px;font-family:arial,sans-serif">which reports an error through ereport in case of failed health check on backend node and than the failure is</div>
<div style="font-size:13px;font-family:arial,sans-serif">processed by "process_backend_health_check_failure()" function.</div><div style="font-size:13px;font-family:arial,sans-serif">This portion of the code requires the most review and suggestions, Since there are many different</div>
<div style="font-size:13px;font-family:arial,sans-serif">ways to implement this health checking.</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">
Another way could be to use PG_TRY and PG_CATCH inside health_check function and if</div><div style="font-size:13px;font-family:arial,sans-serif">"make_persistent_db_connection()" throws an error, the PG_CATCH block catches the error</div>
<div style="font-size:13px;font-family:arial,sans-serif">and return the failing node number</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">
something like</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px"><font face="courier new, monospace">health_check()</font></div><div style="font-size:13px"><font face="courier new, monospace">{</font></div>
<div style="font-size:13px"><font face="courier new, monospace">....</font></div><div style="font-size:13px"><font face="courier new, monospace">for(i =0; i< MAX_BACKEND; i++)</font></div><div style="font-size:13px"><font face="courier new, monospace">PG_TRY()</font></div>
<div style="font-size:13px"><font face="courier new, monospace">{</font></div><div style="font-size:13px"><font face="courier new, monospace"> make_persistent_db_connection();</font></div><div style="font-size:13px"><font face="courier new, monospace">...</font></div>
<div style="font-size:13px"><font face="courier new, monospace">}</font></div><div style="font-size:13px"><font face="courier new, monospace">PG_CATCH();</font></div><div style="font-size:13px"><font face="courier new, monospace">{</font></div>
<div style="font-size:13px"><font face="courier new, monospace">....</font></div><div style="font-size:13px"><font face="courier new, monospace"> return i;</font></div><div style="font-size:13px"><font face="courier new, monospace">}</font></div>
<div style="font-size:13px"><font face="courier new, monospace">PG_END_TRY();</font></div><div style="font-size:13px"><font face="courier new, monospace">return 0;</font></div><div style="font-size:13px"><font face="courier new, monospace">}</font></div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">And than the main loop process the failure, the similar way it it processing it now (with out the patch). </div>
<div style="font-size:13px;font-family:arial,sans-serif">But the current approach followed by the patch seems to come more natural with the exception manager</div><div style="font-size:13px;font-family:arial,sans-serif">
and yet on the negative side the current approach requires to add a state machine to the main loop.</div>
<div style="font-size:13px;font-family:arial,sans-serif">So suggestions and comments on this portion of code and implementation approach would be very helpful.<br></div><div style="font-size:13px;font-family:arial,sans-serif">
<br></div><div style="font-size:13px;font-family:arial,sans-serif">pgpool child process loop</div><div style="font-size:13px;font-family:arial,sans-serif">==================</div><div style="font-size:13px;font-family:arial,sans-serif">
src/protocol/child.c do_child()</div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">do_child() function is the work horse of pg_pool child process, and basically contains two loops those</div>
<div style="font-size:13px;font-family:arial,sans-serif">spans the life cycle of child process. First step is to wait for the client connection, and once this part</div><div style="font-size:13px;font-family:arial,sans-serif">
is successful, the child is connected with the front end, the process goes into the query process mode.</div><div style="font-size:13px;font-family:arial,sans-serif">So I have used two memory contexts instead of one per loop memory context for this process.</div>
<div style="font-size:13px;font-family:arial,sans-serif">QueryProcessMemoryContext and ChildLoopMemoryContext, where later is used for the pahse</div><div style="font-size:13px;font-family:arial,sans-serif">where child is waiting for the front end connections and the former is for the query process phase.</div>
<div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif">Secondly for the query processing function all my current code browsing leads me to the believe that</div>
<div style="font-size:13px;font-family:arial,sans-serif">although "pool_process_query()" caller has handled five possible return values from the function</div><div style="font-size:13px;font-family:arial,sans-serif">
i.e POOL_END, POOL_ERROR, POOL_FATAL, POOL_IDLE and POOL_CONTINUE.</div><div style="font-size:13px;font-family:arial,sans-serif">But in actual the function returns only POOL_END, POOL_ERROR and the success which is</div>
<div style="font-size:13px;font-family:arial,sans-serif">
POOL_CONTINUE. and to squeeze in the managers in query processing function I have moved forward</div><div style="font-size:13px;font-family:arial,sans-serif">with the above mentioned assumptions. Also my current code understanding on child process flow is when</div>
<div style="font-size:13px;font-family:arial,sans-serif">ever some error occurs in query process phase of child process that error actually terminates that particular client</div><div style="font-size:13px;font-family:arial,sans-serif">
So I have added the below code in elog.c file to treat the ERROR in child process as FATAL in case the</div><div style="font-size:13px;font-family:arial,sans-serif">front end clint is connected with the child process.</div>
</div><div><font face="courier new, monospace">if(elevel == ERROR)</font></div><div><font face="courier new, monospace">{<br> if(processType == PT_CHILD)<br> {<br> /*<br> * If the frontend connection exists, treat this error as FATAL<br>
*/<br> if(is_session_connected())<br></font><span style="font-family:'courier new',monospace"> elevel = FATAL;</span><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace"> }<br>
}</font><div style="font-size:13px;font-family:arial,sans-serif"><br></div><p style="font-size:13px;font-family:arial,sans-serif">Reviews comments and suggestions are most welcome</p><p style="font-size:13px;font-family:arial,sans-serif">
Thanks</p><span class="HOEnZb"><font color="#888888"><p style="font-size:13px;font-family:arial,sans-serif">Muhammad Usama</p><p style="font-size:13px;font-family:arial,sans-serif"><br></p><div style="font-size:13px;font-family:arial,sans-serif">
<br></div><div style="font-size:13px;font-family:arial,sans-serif"><br></div><div style="font-size:13px;font-family:arial,sans-serif"><br></div></font></span></div></div>
</div>
<br>_______________________________________________<br>
pgpool-hackers mailing list<br>
<a href="mailto:pgpool-hackers@pgpool.net">pgpool-hackers@pgpool.net</a><br>
<a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>Ahsan Hadi<br>Snr Director Product Development<br>EnterpriseDB Corporation<br>The Enterprise Postgres Company<br><br>Phone: +92-51-8358874 <br>Mobile: +92-333-5162114<br>
<br>Website: <a href="http://www.enterprisedb.com">www.enterprisedb.com</a><br>EnterpriseDB Blog: <a href="http://blogs.enterprisedb.com/">http://blogs.enterprisedb.com/</a><br>Follow us on Twitter: <a href="http://www.twitter.com/enterprisedb">http://www.twitter.com/enterprisedb</a><br>
<br>This e-mail message (and any attachment) is intended for the use of the individual or entity to whom it is addressed. This message contains information from EnterpriseDB Corporation that may be privileged, confidential, or exempt from disclosure under applicable law. If you are not the intended recipient or authorized to receive this for the intended recipient, any use, dissemination, distribution, retention, archiving, or copying of this communication is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately by reply e-mail and delete this message.
</div>