<div dir="ltr">Thanks Tatsuo for your review.<div><br></div><div>Usama is currently working on the last piece of this effort by integrating the memory/exception manager in the query parsing module. We are hoping to complete the coding by the end of next week. The goal is to get this functionality committed to the master branch before the end of Jan.</div>
<div><br></div><div>I don't think we need to write any new test cases to test the functionality. We need to ensure that the current test suite runs fine with the changes. </div><div><br></div><div>-- Ahsan <br><div><br>
</div><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jan 7, 2014 at 5:26 AM, Tatsuo Ishii <span dir="ltr"><<a href="mailto:ishii@postgresql.org" target="_blank">ishii@postgresql.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I have verified v3 patch and it complies fine on my Linux box. Thanks! I<br>
have committed and pushed to master. In addition to your v3 patch, I<br>
have changed copyright year of pool.h.<br>
<div class="HOEnZb"><div class="h5">--<br>
Tatsuo Ishii<br>
SRA OSS, Inc. Japan<br>
English: <a href="http://www.sraoss.co.jp/index_en.php" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
Japanese: <a href="http://www.sraoss.co.jp" target="_blank">http://www.sraoss.co.jp</a><br>
<br>
> Hi Tatsuo, Thanks for the review<br>
><br>
> your version of patch is causing the compilation problem on the OS/X, The<br>
> reason is the configure script is<br>
> defining HAVE_STRLCPY in config.h file while the pool.h is adding the<br>
> condition on HAVE_DECL_STRLCPY.<br>
> I have updated the patch to fix this issue can you please verify if the<br>
> attached version works on your linux box.<br>
><br>
><br>
> Regards<br>
> Usama<br>
><br>
> On Sat, Jan 4, 2014 at 4:44 PM, Tatsuo Ishii <<a href="mailto:ishii@postgresql.org">ishii@postgresql.org</a>> wrote:<br>
><br>
>> Sorry for delay. I have checked your patch.<br>
>><br>
>> > Hi Tatsuo<br>
>> ><br>
>> > Thanks a bundle for looking into the patch.<br>
>> ><br>
>> ><br>
>> > On Sat, Dec 14, 2013 at 2:35 PM, Tatsuo Ishii <<a href="mailto:ishii@postgresql.org">ishii@postgresql.org</a>><br>
>> wrote:<br>
>> ><br>
>> >> Usama,<br>
>> >><br>
>> >> Thanks for the great work.<br>
>> >> I have take a look at your patches.<br>
>> >><br>
>> >> Compiling the source code:<br>
>> >><br>
>> >> I see some "//" comments. I believe they are temporary ones. We follow<br>
>> >> the PostgreSQL coding standard and it does not allow to use "//"<br>
>> >> comments. I also see some compiler warnings because of function<br>
>> >> prototypes are not declared.<br>
>> >><br>
>> ><br>
>> > Yes I intend to follow the PostgreSQL coding style and standards, the<br>
>> "//"<br>
>> > style comments were left<br>
>> > in the code unintentionally, and I had this thing in the mind and last<br>
>> TODO<br>
>> > of this effort is to clean up<br>
>> > the code for these kind of mistakes.<br>
>> ><br>
>> >><br>
>> >> I noticed you commented out following line which causes lots of<br>
>> >> compiler warnings. Is that intended?<br>
>> >><br>
>> >> extern size_t strlcpy(char *dst, const char *src, size_t siz);<br>
>> >><br>
>> ><br>
>> > Actually I use Mac OS/X for development and since OS/X is BSD based<br>
>> which<br>
>> > already<br>
>> > contains the native strlcpy function defined in string.h, So I was<br>
>> getting<br>
>> > the compilation error.<br>
>> ><br>
>> > Please find the attached patch (strlcpy_fix.patch) to properly fix this<br>
>> > problem, The patch adds the check<br>
>> > in configure to test if strlcpy is provided by OS and in that case does<br>
>> not<br>
>> > externs the function in pool.h.<br>
>> > Since this issue also exists in the master branch so If you feel the<br>
>> > comfortable with the patch and the fix<br>
>> > ,can you please also apply this on master branch.<br>
>> > Note: You will require to run the autoconf after this patch.<br>
>><br>
>> Unfortunately your patch did not work on my Linux box.<br>
>><br>
>> AC_CHECK_DECLS([strlcat, strlcpy]) produces config.h like this if<br>
>> there's no strlcpy in system library.<br>
>><br>
>> /* Define to 1 if you have the declaration of `strlcpy', and to 0 if you<br>
>> don't. */<br>
>> #define HAVE_DECL_STRLCPY 0<br>
>><br>
>> which makes following declaration in pool.h void, which I think you<br>
>> are not willing to do so.<br>
>><br>
>> #ifndef HAVE_DECL_STRLCPY<br>
>> extern size_t strlcpy(char *dst, const char *src, size_t siz);<br>
>> #endif<br>
>><br>
>> So I have changed AC_CHECK_DECLS([strlcat, strlcpy]) to<br>
>> AC_CHECK_FUNCS([strlcat, strlcpy]). Please verify attached patch works<br>
>> for Mac OS X.<br>
>><br>
>> Best regards,<br>
>> --<br>
>> Tatsuo Ishii<br>
>> SRA OSS, Inc. Japan<br>
>> English: <a href="http://www.sraoss.co.jp/index_en.php" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
>> Japanese: <a href="http://www.sraoss.co.jp" target="_blank">http://www.sraoss.co.jp</a><br>
>><br>
>> >> > pgpool_main.c PgpoolMain()<br>
>> >> ><br>
>> >> > The patch changes the flow of how health check failures are processed,<br>
>> >> > health_check() function which<br>
>> >> > previously used to return health check failing node number or success<br>
>> is<br>
>> >> > changed to do_health_check(),<br>
>> >> > which reports an error through ereport in case of failed health check<br>
>> on<br>
>> >> > backend node and than the failure is<br>
>> >> > processed by "process_backend_health_check_failure()" function.<br>
>> >> > This portion of the code requires the most review and suggestions,<br>
>> Since<br>
>> >> > there are many different<br>
>> >> > ways to implement this health checking.<br>
>> >><br>
>> >> I saw followings:<br>
>> >><br>
>> >> LOG: starting health check<br>
>> >> LOCATION: pgpool_main.c:369<br>
>> >> LOG: after retry 0, All backends are returned to healthy state<br>
>> >> LOCATION: pgpool_main.c:411<br>
>> >><br>
>> >> Before "starting health check" message was debug message and should<br>
>> >> remain debug IMO. Otherwise they flood the log file.<br>
>> >><br>
>> >> I also see problem with "after retry 0, All backends are returned to<br>
>> >> healthy state" message since the message appears even there's no retry<br>
>> >> (again before we do not spit the message if we are not doing retry).<br>
>> >><br>
>> ><br>
>> > Thanks for pointing these. The 2nd attached patch "exmgr_fix.patch" tries<br>
>> > to fix these, which can be applied<br>
>> > on EXCEPTION_MGR development branch.<br>
>><br>
>> Looks good.<br>
>><br>
>> >> Most importantly, the message style is pretty different from current<br>
>> >> one. no timestamp, no pid.<br>
>> >><br>
>> ><br>
>> > Yes this one is known thing, I some how missed to point it out<br>
>> > specifically. This is the part of the TODO item mentioned<br>
>> > upthread "changes and cleanup of elog.c and elog.h". I have intentionally<br>
>> > left it as a last item because it deals mostly with<br>
>> > cosmetic issues. Apart from that this TODO item also have couple of more<br>
>> > things which are needed to be done<br>
>> > by elog api. e.g. adding the configuration parameters similar to<br>
>> > PostgreSQL's<br>
>> > log_min_error_statement, log_min_messages, client_min_messages to control<br>
>> > the level of message generated and reported by pgpool.<br>
>> ><br>
>> ><br>
>> ><br>
>> > Thanks<br>
>> > Muhammad Usama<br>
>> ><br>
>> >> Another way could be to use PG_TRY and PG_CATCH inside health_check<br>
>> >> > function and if<br>
>> >> > "make_persistent_db_connection()" throws an error, the PG_CATCH block<br>
>> >> > catches the error<br>
>> >> > and return the failing node number<br>
>> >> ><br>
>> >> > something like<br>
>> >> ><br>
>> >> > health_check()<br>
>> >> > {<br>
>> >> > ....<br>
>> >> > for(i =0; i< MAX_BACKEND; i++)<br>
>> >> > PG_TRY()<br>
>> >> > {<br>
>> >> > make_persistent_db_connection();<br>
>> >> > ...<br>
>> >> > }<br>
>> >> > PG_CATCH();<br>
>> >> > {<br>
>> >> > ....<br>
>> >> > return i;<br>
>> >> > }<br>
>> >> > PG_END_TRY();<br>
>> >> > return 0;<br>
>> >> > }<br>
>> >> ><br>
>> >> ><br>
>> >> > And than the main loop process the failure, the similar way it it<br>
>> >> > processing it now (with out the patch).<br>
>> >> > But the current approach followed by the patch seems to come more<br>
>> natural<br>
>> >> > with the exception manager<br>
>> >> > and yet on the negative side the current approach requires to add a<br>
>> state<br>
>> >> > machine to the main loop.<br>
>> >><br>
>> >> Agreed.<br>
>> >><br>
>> >> > So suggestions and comments on this portion of code and implementation<br>
>> >> > approach would be very helpful.<br>
>> >> ><br>
>> >> > pgpool child process loop<br>
>> >> > ==================<br>
>> >> > src/protocol/child.c do_child()<br>
>> >> ><br>
>> >> > do_child() function is the work horse of pg_pool child process, and<br>
>> >> > basically contains two loops those<br>
>> >> > spans the life cycle of child process. First step is to wait for the<br>
>> >> client<br>
>> >> > connection, and once this part<br>
>> >> > is successful, the child is connected with the front end, the process<br>
>> >> goes<br>
>> >> > into the query process mode.<br>
>> >> > So I have used two memory contexts instead of one per loop memory<br>
>> context<br>
>> >> > for this process.<br>
>> >> > QueryProcessMemoryContext and ChildLoopMemoryContext, where later is<br>
>> used<br>
>> >> > for the pahse<br>
>> >> > where child is waiting for the front end connections and the former is<br>
>> >> for<br>
>> >> > the query process phase.<br>
>> >> ><br>
>> >> > Secondly for the query processing function all my current code<br>
>> browsing<br>
>> >> > leads me to the believe that<br>
>> >> > although "pool_process_query()" caller has handled five possible<br>
>> return<br>
>> >> > values from the function<br>
>> >> > i.e POOL_END, POOL_ERROR, POOL_FATAL, POOL_IDLE and POOL_CONTINUE.<br>
>> >> > But in actual the function returns only POOL_END, POOL_ERROR and the<br>
>> >> > success which is<br>
>> >> > POOL_CONTINUE. and to squeeze in the managers in query processing<br>
>> >> function<br>
>> >> > I have moved forward<br>
>> >> > with the above mentioned assumptions. Also my current code<br>
>> understanding<br>
>> >> on<br>
>> >> > child process flow is when<br>
>> >> > ever some error occurs in query process phase of child process that<br>
>> error<br>
>> >> > actually terminates that particular client<br>
>> >> > So I have added the below code in elog.c file to treat the ERROR in<br>
>> child<br>
>> >> > process as FATAL in case the<br>
>> >> > front end clint is connected with the child process.<br>
>> >> ><br>
>> >> > if(elevel == ERROR)<br>
>> >> > {<br>
>> >> > if(processType == PT_CHILD)<br>
>> >> > {<br>
>> >> > /*<br>
>> >> > * If the frontend connection exists, treat this error as FATAL<br>
>> >> > */<br>
>> >> > if(is_session_connected())<br>
>> >> > elevel = FATAL;<br>
>> >> > }<br>
>> >> > }<br>
>> >> ><br>
>> >> > Reviews comments and suggestions are most welcome<br>
>> >> ><br>
>> >> > Thanks<br>
>> >> ><br>
>> >> > Muhammad Usama<br>
>> >><br>
>><br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
pgpool-hackers mailing list<br>
<a href="mailto:pgpool-hackers@pgpool.net">pgpool-hackers@pgpool.net</a><br>
<a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br>Ahsan Hadi<br>Snr Director Product Development<br>EnterpriseDB Corporation<br>The Enterprise Postgres Company<br><br>Phone: +92-51-8358874 <br>Mobile: +92-333-5162114<br>
<br>Website: <a href="http://www.enterprisedb.com">www.enterprisedb.com</a><br>EnterpriseDB Blog: <a href="http://blogs.enterprisedb.com/">http://blogs.enterprisedb.com/</a><br>Follow us on Twitter: <a href="http://www.twitter.com/enterprisedb">http://www.twitter.com/enterprisedb</a><br>
<br>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.
</div>