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

Ahsan Hadi ahsan.hadi at enterprisedb.com
Wed Dec 11 23:21:34 JST 2013


On Wed, Dec 11, 2013 at 6:32 PM, Tatsuo Ishii <ishii at postgresql.org> wrote:

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

Yes please. That would be great.


>
> 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.
>



-- 
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20131211/26936f42/attachment-0001.html>


More information about the pgpool-hackers mailing list