[pgpool-hackers: 431] Re: WIP patch for exception and memory manager integration

Wed Dec 11 22:32:36 JST 2013

If you don't mind, I would like to be one of the reviewers.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

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