<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 class="" 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 class="" 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 class="" 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 class="" 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 class="" 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 class="" 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 class="" style="font-size:13px;font-family:arial,sans-serif">Reviews comments and suggestions are most welcome</p><p class="" style="font-size:13px;font-family:arial,sans-serif">
Thanks</p><p class="" style="font-size:13px;font-family:arial,sans-serif">Muhammad Usama</p><p class="" 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></div></div>
</div>