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

Tatsuo Ishii ishii at postgresql.org
Sat Jan 4 20:44:21 JST 2014 

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 --------------
A non-text attachment was scrubbed...
Name: strlcpy_fix-v2.patch
Type: text/x-patch
Size: 1290 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20140104/586c7493/attachment.bin>

More information about the pgpool-hackers mailing list