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

[Tatsuo Ishii]

I have verified v3 patch and it complies fine on my Linux box. Thanks!  I
have committed and pushed to master. In addition to your v3 patch, I
have changed copyright year of pool.h.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

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