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

[Tatsuo Ishii]

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.

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

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

Most importantly, the message style is pretty different from current
one. no timestamp, no pid.

> 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