RE: Terminate the idle sessions - Mailing list pgsql-hackers

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 <japinli@hotmail.com> 
Sent: Tuesday, November 17, 2020 6:02 PM
To: Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Thomas Munro
<thomas.munro@gmail.com>;bharath.rupireddyforpostgres@gmail.com; pgsql-hackers@lists.postgresql.org
 
Subject: Re: Terminate the idle sessions


On Nov 17, 2020, at 2:07 PM, mailto:kuroda.hayato@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 tohandle
 
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