Dear Li, > I’m not familiar with the system interrupt, however, > the sigalrm_due_at is subsutitue between HOLD_INTERRUPTS() > and RESUM_INTERRUPTS(), so I think it cannot be interrupted. > The following comments comes from miscadmin.h.
Right, but how about before HOLD_INTERRUPTS()? If so, only calling handle_sig_alarm() is occurred, and Setitimer will not be set, I think. > Not sure! I find that Win32 do not support setitimer(), > PostgreSQL emulate setitimer() by creating a persistent thread to handle > the timer setting and notification upon timeout. Yes, set/getitimer() is the systemcall, and implemented only in the unix-like system. But I rethink that such a fix is categorized in the refactoring and we should separate topic. Hence we can ignore here. Best Regards, Hayato Kuroda FUJITSU LIMITED From: Li Japin <japi...@hotmail.com> Sent: Tuesday, November 17, 2020 6:02 PM To: Kuroda, Hayato/黒田 隼人 <kuroda.hay...@fujitsu.com> Cc: David G. Johnston <david.g.johns...@gmail.com>; Kyotaro Horiguchi <horikyota....@gmail.com>; Thomas Munro <thomas.mu...@gmail.com>; bharath.rupireddyforpostg...@gmail.com; pgsql-hackers@lists.postgresql.org Subject: Re: Terminate the idle sessions On Nov 17, 2020, at 2:07 PM, mailto:kuroda.hay...@fujitsu.com wrote: Dear Li, David, > Additionally, using postgres_fdw within the server doesn't cause issues, > its using postgres_fdw and the remote server having this setting set to zero > that causes a problem. I didn't know the fact that postgres_fdw can use within the server... Thanks. I read optimize-setitimer patch, and looks basically good. I put what I understanding, so please confirm it whether your implementation is correct. (Maybe I missed some simultaneities, so please review anyone...) [besic consept] sigalrm_due_at means the time that interval timer will ring, and sigalrm_delivered means who calls schedule_alarm(). If fin_time of active_timeouts[0] is larger than or equal to sigalrm_due_at, stop calling setitimer because handle_sig_alarm() will be call sooner. Agree. The sigalrm_delivered means a timer has been handled by handle_sig_alarm(), so we should call setitimer() in next schedule_alarm(). [when call setitimer] In the attached patch, setitimer() will be only called the following scenarios: * when handle_sig_alarm() is called due to the pqsignal * when a timeout is registered and its fin_time is later than active_timeous[0] * when disable a timeout * when handle_sig_alarm() is interrupted and rescheduled(?) According to comments, handle_sig_alarm() may be interrupted because of the ereport. I think if handle_sig_alarm() is interrupted before subsutituting sigalrm_due_at to true, interval timer will be never set. Is it correct, or is my assumption wrong? I’m not familiar with the system interrupt, however, the sigalrm_due_at is subsutitue between HOLD_INTERRUPTS() and RESUM_INTERRUPTS(), so I think it cannot be interrupted. The following comments comes from miscadmin.h. > The HOLD_INTERRUPTS() and RESUME_INTERRUPTS() macros > allow code to ensure that no cancel or die interrupt will be accepted, > even if CHECK_FOR_INTERRUPTS() gets called in a subroutine. The interrupt > will be held off until CHECK_FOR_INTERRUPTS() is done outside any > HOLD_INTERRUPTS() ... RESUME_INTERRUPTS() section. Lastly, I found that setitimer is obsolete and should change to another one. According to my man page: ``` POSIX.1-2001, SVr4, 4.4BSD (this call first appeared in 4.2BSD). POSIX.1-2008 marks getitimer() and setitimer() obsolete, recommending the use of the POSIX timers API (timer_gettime(2), timer_settime(2), etc.) instead. ``` Do you have an opinion for this? I think it should be changed if all platform can support timer_settime system call, but this fix affects all timeouts, so more considerations might be needed. Not sure! I find that Win32 do not support setitimer(), PostgreSQL emulate setitimer() by creating a persistent thread to handle the timer setting and notification upon timeout. So if we want to replace it, I think we should open a new thread to achieve this. -- Best regards Japin Li
