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

Muhammad Usama m.usama at gmail.com
Mon Jan 6 23:29:20 JST 2014


Hi Tatsuo, Thanks for the review

your version of patch is causing the compilation problem on the OS/X, The
reason is the configure script is
defining HAVE_STRLCPY in config.h file while the pool.h is adding the
condition on HAVE_DECL_STRLCPY.
I have updated the patch to fix this issue can you please verify if the
attached version works on your linux box.


Regards
Usama

On Sat, Jan 4, 2014 at 4:44 PM, Tatsuo Ishii <ishii at postgresql.org> wrote:

> Sorry for delay. I have checked your patch.
>
> > Hi Tatsuo
> >
> > Thanks a bundle for looking into the patch.
> >
> >
> > On Sat, Dec 14, 2013 at 2:35 PM, Tatsuo Ishii <ishii at postgresql.org>
> wrote:
> >
> >> Usama,
> >>
> >> Thanks for the great work.
> >> I have take a look at your patches.
> >>
> >> Compiling the source code:
> >>
> >> I see some "//" comments. I believe they are temporary ones. We follow
> >> the PostgreSQL coding standard and it does not allow to use "//"
> >> comments. I also see some compiler warnings because of function
> >> prototypes are not declared.
> >>
> >
> > Yes I intend to follow the PostgreSQL coding style and standards, the
> "//"
> > style comments were left
> > in the code unintentionally, and I had this thing in the mind and last
> TODO
> > of this effort is to clean up
> > the code for these kind of mistakes.
> >
> >>
> >> I noticed you commented out following line which causes lots of
> >> compiler warnings. Is that intended?
> >>
> >> extern size_t strlcpy(char *dst, const char *src, size_t siz);
> >>
> >
> > Actually I use Mac OS/X for development and since OS/X is BSD based
>  which
> > already
> > contains the native strlcpy function defined in string.h, So I was
> getting
> > the compilation error.
> >
> > Please find the attached patch (strlcpy_fix.patch) to properly fix this
> > problem, The patch adds the check
> > in configure to test if strlcpy is provided by OS and in that case does
> not
> > externs the function in pool.h.
> > Since this issue also exists in the master branch so If you feel the
> > comfortable with the patch and the fix
> > ,can you please also apply this on master branch.
> > Note: You will require to run the autoconf after this patch.
>
> Unfortunately your patch did not work on my Linux box.
>
> AC_CHECK_DECLS([strlcat, strlcpy]) produces config.h like this if
> there's no strlcpy in system library.
>
> /* Define to 1 if you have the declaration of `strlcpy', and to 0 if you
>    don't. */
> #define HAVE_DECL_STRLCPY 0
>
> which makes following declaration in pool.h void, which I think you
> are not willing to do so.
>
> #ifndef HAVE_DECL_STRLCPY
> extern size_t strlcpy(char *dst, const char *src, size_t siz);
> #endif
>
> So I have changed AC_CHECK_DECLS([strlcat, strlcpy]) to
> AC_CHECK_FUNCS([strlcat, strlcpy]). Please verify attached patch works
> for Mac OS X.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp
>
> >> > 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.
> >>
> >> I saw followings:
> >>
> >> LOG:  starting health check
> >> LOCATION:  pgpool_main.c:369
> >> LOG:  after retry 0, All backends are returned to healthy state
> >> LOCATION:  pgpool_main.c:411
> >>
> >> Before "starting health check" message was debug message and should
> >> remain debug IMO. Otherwise they flood the log file.
> >>
> >> I also see problem with "after retry 0, All backends are returned to
> >> healthy state" message since the message appears even there's no retry
> >> (again before we do not spit the message if we are not doing retry).
> >>
> >
> > Thanks for pointing these. The 2nd attached patch "exmgr_fix.patch" tries
> > to fix these, which can be applied
> > on EXCEPTION_MGR development branch.
>
> Looks good.
>
> >> Most importantly, the message style is pretty different from current
> >> one. no timestamp, no pid.
> >>
> >
> > Yes this one is known thing, I some how missed to point it out
> > specifically. This is the part of the TODO item mentioned
> > upthread "changes and cleanup of elog.c and elog.h". I have intentionally
> > left it as a last item because it deals mostly with
> > cosmetic issues. Apart from that this TODO item also have couple of more
> > things which are needed to be done
> > by elog api. e.g. adding the configuration parameters similar to
> > PostgreSQL's
> > log_min_error_statement, log_min_messages, client_min_messages to control
> > the level of message generated and reported by pgpool.
> >
> >
> >
> > Thanks
> > Muhammad Usama
> >
> >> 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.
> >>
> >> Agreed.
> >>
> >> > 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
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20140106/cd10b99e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: strlcpy_fix-v3.patch
Type: application/octet-stream
Size: 1284 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20140106/cd10b99e/attachment.obj>


More information about the pgpool-hackers mailing list