Subject: Re: Terminate the idle sessions

From: Cary Huang <[email-masked]>
Date: Mon, 10 Aug 2020 21:42:42 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

I applied this patch to the PG13 branch and generally this feature works as described. The new "idle_session_timeout" that controls the idle session disconnection is not in the default postgresql.conf and I think it should be included there with default value 0, which means disabled. 
There is currently no enforced minimum value for "idle_session_timeout" (except for value 0 for disabling the feature), so user can put any value larger than 0 and it could be very small like 500 or even 50 millisecond, this would make any psql connection to disconnect shortly after it has connected, which may not be ideal. Many systems I have worked with have 30 minutes inactivity timeout by default, and I think it would be better and safer to enforce a reasonable minimum timeout value

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 10 Aug 2020 14:49:36 -0700

On Mon, Aug 10, 2020 at 2:43 PM Cary Huang <[email-masked]> wrote:

> There is currently no enforced minimum value for "idle_session_timeout"
> (except for value 0 for disabling the feature), so user can put any value
> larger than 0 and it could be very small like 500 or even 50 millisecond,
> this would make any psql connection to disconnect shortly after it has
> connected, which may not be ideal. Many systems I have worked with have 30
> minutes inactivity timeout by default, and I think it would be better and
> safer to enforce a reasonable minimum timeout value

I'd accept a value of say 1,000 being minimum in order to reinforce the
fact that a unit-less input, while possible, is taken to be milliseconds
and such small values most likely mean the user has made a mistake.  I
would not choose a minimum allowed value solely based on our concept of
"reasonable".  I don't imagine a value of say 10 seconds, while seemingly
unreasonable, is going to be unsafe.

David J.

---

From: Li Japin <[email-masked]>
Date: Tue, 11 Aug 2020 03:14:58 +0000

Hi,

On Aug 11, 2020, at 5:42 AM, Cary Huang <[email-masked]<mailto:[email-masked]>> wrote:

Thanks for looking at it!

I’ve attached a new version that add “idle_session_timeout” in the default postgresql.conf.

On Mon, Aug 10, 2020 at 2:43 PM Cary Huang <[email-masked]<mailto:[email-masked]>> wrote:
There is currently no enforced minimum value for "idle_session_timeout" (except for value 0 for disabling the feature), so user can put any value larger than 0 and it could be very small like 500 or even 50 millisecond, this would make any psql connection to disconnect shortly after it has connected, which may not be ideal. Many systems I have worked with have 30 minutes inactivity timeout by default, and I think it would be better and safer to enforce a reasonable minimum timeout value

I'd accept a value of say 1,000 being minimum in order to reinforce the fact that a unit-less input, while possible, is taken to be milliseconds and such small values most likely mean the user has made a mistake.  I would not choose a minimum allowed value solely based on our concept of "reasonable".  I don't imagine a value of say 10 seconds, while seemingly unreasonable, is going to be unsafe.

I think David is right, see “idle_in_transaction_session_timeout”, it also doesn’t have a “reasonable” minimum value.

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 14 Aug 2020 11:45:28 +0530

> I’ve attached a new version that add “idle_session_timeout” in the default postgresql.conf.

Hi, I would like to just mention a use case I thought of while discussing [1]:

In postgres_fdw: assuming we use idle_in_session_timeout on remote
backends,  the remote sessions will be closed after timeout, but the
locally cached connection cache entries still exist and become stale.
The subsequent queries that may use the cached connections will fail,
of course these subsequent queries can retry the connections only at
the beginning of a remote txn but not in the middle of a remote txn,
as being discussed in [2]. For instance, in a long running local txn,
let say we used a remote connection at the beginning of the local
txn(note that it will open a remote session and it's entry is cached
in local connection cache), only we use the cached connection later at
some point in the local txn, by then let say the
idle_in_session_timeout has happened on the remote backend and the
remote session would have been closed, the long running local txn will
fail instead of succeeding.

I think, since the idle_session_timeout is by default disabled, we
have no problem. My thought is what if a user enables the
feature(knowingly or unknowingly) on the remote backend? If the user
knows about the above scenario, that may be fine. On the other hand,
either we can always the feature on the remote backend(at the
beginning of the remote txn, like we set for some other configuration
settings see - configure_remote_session() in connection.c) or how
about mentioning the above scenario in this feature documentation?

[1] - https://www.postgresql.org/message-id/CALj2ACU1NBQo9mihA15dFf6udkOi7m0u2_s5QJ6dzk%3DZQyVbwQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/flat/CALj2ACUAi23vf1WiHNar_LksM9EDOWXcbHCo-fD4Mbr1d%3D78YQ%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

---

From: Li Japin <[email-masked]>
Date: Fri, 14 Aug 2020 08:02:27 +0000

On Aug 14, 2020, at 2:15 PM, Bharath Rupireddy <[email-masked]<mailto:[email-masked]>> wrote:

Though we can disable the idle_session_timeout when using postgres_fdw,
there still has locally cached connection cache entries when the remote sessions
terminated by accident.  AFAIK, you have provided a patch to solve this
problem, and it is in current CF [1].

[1] - https://commitfest.postgresql.org/29/2651/

Best Regards,
Japin Li.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 17 Aug 2020 19:28:10 +0530

On Fri, Aug 14, 2020 at 1:32 PM Li Japin <[email-masked]> wrote:
>
> On Aug 14, 2020, at 2:15 PM, Bharath Rupireddy <
> [1] - https://commitfest.postgresql.org/29/2651/

Yes, that solution can retry the cached connections at only the beginning
of the remote txn and not at the middle of the remote txn and that makes
sense as we can not retry connecting to a different remote backend in the
middle of a remote txn.

+1 for disabling the idle_session_timeout feature in case of postgres_fdw.
This can avoid the remote backends to timeout during postgres_fdw usages.

With Regards,
Bharath Rupireddy.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 18 Aug 2020 10:19:44 +0900 (JST)

Hello.

At Mon, 17 Aug 2020 19:28:10 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> This can avoid the remote backends to timeout during postgres_fdw usages.

The same already happens for idle_in_transaction_session_timeout and
we can use "ALTER ROLE/DATABASE SET" to dislable or loosen them, it's
a bit cumbersome, though. I don't think we should (at least
implicitly) disable those timeouts ad-hockerly for postgres_fdw.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Li Japin <[email-masked]>
Date: Tue, 18 Aug 2020 02:12:51 +0000

On Aug 18, 2020, at 9:19 AM, Kyotaro Horiguchi <[email-masked]<mailto:[email-masked]>> wrote:

+1.

---

From: Thomas Munro <[email-masked]>
Date: Mon, 31 Aug 2020 12:51:20 +1200

On Tue, Aug 18, 2020 at 2:13 PM Li Japin <[email-masked]> wrote:
> On Aug 18, 2020, at 9:19 AM, Kyotaro Horiguchi <[email-masked]> wrote:

This seems like a reasonable feature to me.

The delivery of the error message explaining what happened is probably
not reliable, so to some clients and on some operating systems this
will be indistinguishable from a dropped network connection or other
error, but that's OK and we already have that problem with the
existing timeout-based disconnection feature.

The main problem I have with it is the high frequency setitimer()
calls.  If you enable both statement_timeout and idle_session_timeout,
then we get up to huge number of system calls, like the following
strace -c output for a few seconds of one backend under pgbench -S
workload shows:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 39.45    0.118685           0    250523           setitimer
 29.98    0.090200           0    125275           sendto
 24.30    0.073107           0    126235       973 recvfrom
  6.01    0.018068           0     20950           pread64
  0.26    0.000779           0       973           epoll_wait
------ ----------- ----------- --------- --------- ----------------
100.00    0.300839                523956       973 total

There's a small but measurable performance drop from this, as also
discussed in another thread about another kind of timeout[1].  Maybe
we should try to fix that with something like the attached?

[1] https://www.postgresql.org/message-id/flat/77def86b27e41f0efcba411460e929ae%40postgrespro.ru

---

From: Li Japin <[email-masked]>
Date: Mon, 31 Aug 2020 02:40:37 +0000

On Aug 31, 2020, at 8:51 AM, Thomas Munro <[email-masked]<mailto:[email-masked]>> wrote:

Hi, Thomas,

Could you give the more details about the test instructions?

---

From: Thomas Munro <[email-masked]>
Date: Mon, 31 Aug 2020 15:43:20 +1200

> Could you give the more details about the test instructions?

Hi Japin,

Sure.  Because I wasn't trying to get reliable TPS number or anything,
I just used a simple short read-only test with one connection, like
this:

pgbench -i -s10 postgres
pgbench -T60 -Mprepared -S postgres

Then I looked for the active backend and ran strace -c -p XXX for a
few seconds and hit ^C to get the counters.  I doubt the times are
very accurate, but the number of calls is informative.

If you do that on a server running with -c statement_timeout=10s, you
see one setitimer() per transaction.  If you also use -c
idle_session_timeout=10s at the same time, you see two.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 31 Aug 2020 13:49:05 +0900 (JST)

At Mon, 31 Aug 2020 12:51:20 +1200, Thomas Munro <[email-masked]> wrote in 
> % time     seconds  usecs/call     calls    errors syscall

I think it's worth doing. Maybe we can get rid of doing anything other
than removing an entry in the case where we disable a non-nearest
timeout.

regards.

---

From: Li Japin <[email-masked]>
Date: Mon, 31 Aug 2020 05:33:02 +0000

> On Aug 31, 2020, at 11:43 AM, Thomas Munro <[email-masked]> wrote:
> 

Hi, Thomas,

Thanks for your point out this problem, here is the comparison.

Without Optimize settimer usage:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 41.22    1.444851           1   1317033           setitimer
 28.41    0.995936           2    658622           sendto
 24.63    0.863316           1    659116       599 recvfrom
  5.71    0.200275           2    111055           pread64
  0.03    0.001152           2       599           epoll_wait
  0.00    0.000000           0         1           epoll_ctl
------ ----------- ----------- --------- --------- ----------------
100.00    3.505530               2746426       599 total

With Optimize settimer usage:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 49.89    1.464332           1   1091429           sendto
 40.83    1.198389           1   1091539       219 recvfrom
  9.26    0.271890           1    183321           pread64
  0.02    0.000482           2       214           epoll_wait
  0.00    0.000013           3         5           setitimer
  0.00    0.000010           2         5           rt_sigreturn
  0.00    0.000000           0         1           epoll_ctl
------ ----------- ----------- --------- --------- ----------------
100.00    2.935116               2366514       219 total

Here’s a modified version of Thomas’s patch.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 13 Nov 2020 10:27:47 +0000

Dear Li, 

I read your patch, and I think the documentation is too simple to avoid all problems.
(I think if some connection pooling is used, the same problem will occur.)
Could you add some explanations in the doc file? I made an example:

```
Note that this values should be set to zero if you use postgres_fdw or some
Connection-pooling software, because connections might be closed unexpectedly. 
```

I will send other comments if I find something.

Hayato Kuroda
FUJITSU LIMITED

---

From: Li Japin <[email-masked]>
Date: Sun, 15 Nov 2020 10:00:01 +0000

On Nov 13, 2020, at 6:27 PM, [email-masked]<mailto:[email-masked]> wrote:

```

Thanks for your advice! Attached v4.

--
Best regards
Japin Li

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 16 Nov 2020 05:22:35 +0000

Dear Li,

> Thanks for your advice! Attached v4.

I confirmed it. OK.

> @@ -30,6 +30,7 @@ typedef enum TimeoutId
>  	STANDBY_DEADLOCK_TIMEOUT,
>  	STANDBY_TIMEOUT,
>  	STANDBY_LOCK_TIMEOUT,
> +	IDLE_SESSION_TIMEOUT,
>  	IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
>  	/* First user-definable timeout reason */
>  	USER_TIMEOUT,

I'm not familiar with timeout, but I can see that the priority of idle-session is set lower than transaction-timeout.
Could you explain the reason? In my image this timeout locates at the lowest layer, so it might have the lowest 
priority.

Other codes are still checked :-(.

Hayato Kuroda
FUJITSU LIMITED

---

From: Li Japin <[email-masked]>
Date: Mon, 16 Nov 2020 12:40:57 +0000

Hi Kuroda,

On Nov 16, 2020, at 1:22 PM, [email-masked]<mailto:[email-masked]> wrote:

My apologies! I just add a enum for idle session and ignore the comments that says the enum has priority.
Fixed as follows:

@@ -30,8 +30,8 @@ typedef enum TimeoutId
        STANDBY_DEADLOCK_TIMEOUT,
        STANDBY_TIMEOUT,
        STANDBY_LOCK_TIMEOUT,
-       IDLE_SESSION_TIMEOUT,
        IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+       IDLE_SESSION_TIMEOUT,
        /* First user-definable timeout reason */
        USER_TIMEOUT,
        /* Maximum number of timeout reasons */

Thanks for your review! Attached.

--
Best regards
Japin Li

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 16 Nov 2020 16:59:15 -0700

> Thanks for your review! Attached.

Reading the doc changes:

I'd rather not name postgres_fdw explicitly, or at least not solely, as a
reason for setting this to zero.  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.

<note>
Consider setting this for specific users instead of as a server default.
Client connections managed by connection poolers, or initiated indirectly
like those by a remote postgres_fdw using server, should probably be
excluded from this timeout.

Text within <para> should be indented one space (you missed both under
listitem).

I'd suggest a comment that aside from a bit of resource consumption idle
sessions do not interfere with the long-running stability of the server,
unlike idle-in-transaction sessions which are controlled by the other
configuration setting.

David J.

---

From: Li Japin <[email-masked]>
Date: Tue, 17 Nov 2020 02:45:12 +0000

--
Best regards
Japin Li

On Nov 17, 2020, at 7:59 AM, David G. Johnston <[email-masked]<mailto:[email-masked]>> wrote:

On Mon, Nov 16, 2020 at 5:41 AM Li Japin <[email-masked]<mailto:[email-masked]>> wrote:
Thanks for your review! Attached.

Reading the doc changes:

I'd rather not name postgres_fdw explicitly, or at least not solely, as a reason for setting this to zero.  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.

<note>
Consider setting this for specific users instead of as a server default.  Client connections managed by connection poolers, or initiated indirectly like those by a remote postgres_fdw using server, should probably be excluded from this timeout.

Text within <para> should be indented one space (you missed both under listitem).

Thanks for your suggest! How about change document as follows:

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c4e2a1fdc..23e691a7c5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8281,17 +8281,17 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
       </term>
       <listitem>
        <para>
-       Terminate any session that has been idle for longer than the specified amount of time.
+        Terminate any session that has been idle for longer than the specified amount of time.
        </para>
        <para>
-       If this value is specified without units, it is taken as milliseconds.
-       A value of zero (the default) disables the timeout.
+        If this value is specified without units, it is taken as milliseconds.
+        A value of zero (the default) disables the timeout.
        </para>

        <note>
         <para>
-         This parameter should be set to zero if you use postgres_fdw or some
-         connection-pooling software, because connections might be closed unexpectedly.
+         This parameter should be set to zero if you use some connection-pooling software, or
+         PostgreSQL servers used by postgres_fdw, because connections might be closed unexpectedly.
         </para>
        </note>

I'd suggest a comment that aside from a bit of resource consumption idle sessions do not interfere with the long-running stability of the server, unlike idle-in-transaction sessions which are controlled by the other configuration setting.

Could you please explain how the idle-in-transaction interfere the long-running stability?

--
Best regards
Japin Li

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 16 Nov 2020 19:53:59 -0700

> might be closed unexpectedly.
Prefer mine, “or pg servers used by postgres_fdw”, doesn’t flow.

---

From: Li Japin <[email-masked]>
Date: Tue, 17 Nov 2020 03:27:05 +0000

On Nov 17, 2020, at 10:53 AM, David G. Johnston <[email-masked]<mailto:[email-masked]>> wrote:

On Monday, November 16, 2020, Li Japin <[email-masked]<mailto:[email-masked]>> wrote:

<note>

        <note>
         <para>

From the docs (next section):

This allows any locks held by that session to be released and the connection slot to be reused; it also allows tuples visible only to this transaction to be vacuumed. See Section 24.1<https://www.postgresql.org/docs/13/routine-vacuuming.html> for more details about this.

Thanks David! Attached.

--
Best regards
Japin Li

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 17 Nov 2020 06:07:35 +0000

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.

[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?

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.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Li Japin <[email-masked]>
Date: Tue, 17 Nov 2020 09:01:58 +0000

On Nov 17, 2020, at 2:07 PM, [email-masked]<mailto:[email-masked]> wrote:

Dear Li, David,

[besic consept]

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]

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.

```

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

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 18 Nov 2020 02:40:07 +0000

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

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 <[email-masked]> 
Sent: Tuesday, November 17, 2020 6:02 PM
To: Kuroda, Hayato/黒田 隼人 <[email-masked]>
Cc: David G. Johnston <[email-masked]>; Kyotaro Horiguchi <[email-masked]>; Thomas Munro <[email-masked]>; [email-masked]; [email-masked]
Subject: Re: Terminate the idle sessions

On Nov 17, 2020, at 2:07 PM, mailto:[email-masked] wrote:

Dear Li, David,

 
 
[besic consept]

[when call setitimer]

 

 
```

--
Best regards
Japin Li

---

From: Li Japin <[email-masked]>
Date: Wed, 18 Nov 2020 06:01:28 +0000

On Nov 18, 2020, at 10:40 AM, [email-masked]<mailto:[email-masked]> wrote:

Yeah, it might be occurred. Any suggestions to fix it?

--
Best regards
Japin Li

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 18 Nov 2020 06:22:50 +0000

Dear Li, 

> Yeah, it might be occurred. Any suggestions to fix it?

Oops.. I forgot putting my suggestion. Sorry.
How about substituting sigalrm_delivered to true in the reschedule_timeouts()?
Maybe this processing looks strange, so some comments should be put too.
Here is an example:

```diff
@@ -423,7 +423,14 @@ reschedule_timeouts(void)
 
        /* Reschedule the interrupt, if any timeouts remain active. */
        if (num_active_timeouts > 0)
+       {
+               /*
+                * sigalrm_delivered is set to true,
+                * because any intrreputions might be occured.
+                */
+               sigalrm_delivered = true;
                schedule_alarm(GetCurrentTimestamp());
+       }
 }
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Li Japin <[email-masked]>
Date: Wed, 18 Nov 2020 06:57:13 +0000

On Nov 18, 2020, at 2:22 PM, [email-masked]<mailto:[email-masked]> wrote:

```diff

Thanks for your suggestion.  Attached!

--
Best regards
Japin Li

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 19 Nov 2020 08:32:46 +0000

Dear Li,

> Thanks for your suggestion.  Attached!

I prefer your comments:-).

I think this patch is mostly good.
I looked whole the codes again and I found the following comment in the PostgresMain():

```c
		/*
		 * (5) turn off the idle-in-transaction timeout
		 */
```

Please mention about idle-session timeout and check another comment.

Best Regards, 
Hayato Kuroda

---

From: japin <[email-masked]>
Date: Thu, 19 Nov 2020 17:35:05 +0800

hi, Kuroda

> Please mention about idle-session timeout and check another comment.
Thanks! Add the comment for idle-session timeout.

--
Best regards
Japin Li

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 20 Nov 2020 02:05:09 +0000

Dear Li,

I confirmed it. OK.

I don't have any comments anymore. If no one has,
I will change the status few days later.
Other comments or suggestions to him?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 24 Nov 2020 00:01:49 +0000

No one have any comments, patch tester says OK, and I think this works well.
I changed status to "Ready for Committer."

Hayato Kuroda
FUJITSU LIMITED

-----Original Message-----
From: [email-masked] <[email-masked]> 
Sent: Friday, November 20, 2020 11:05 AM
To: 'japin' <[email-masked]>
Cc: David G. Johnston <[email-masked]>; Kyotaro Horiguchi <[email-masked]>; Thomas Munro <[email-masked]>; [email-masked]; [email-masked]
Subject: RE: Terminate the idle sessions

Dear Li,

I confirmed it. OK.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 23 Nov 2020 20:39:48 -0700

> I changed status to "Ready for Committer."
Some proof-reading:

v8-0001

Documentation:

My suggestion wasn't taken for the first note paragraph (review/author
disagreement) and the current has the following issues:

"if you use some connection-pooling" software doesn't need the word "some"
Don't substitute "pg" for the name of the product, PostgreSQL.
The word "used" is a more stylistic dislike, but "connected to using
postgres_fdw" would be a better choice IMO.

Code (minor, but if you are in there anyway):

(5) turn off ... timeout (there are now two, timeouts should be plural)

v8-0002 - No suggestions

David J.

---

From: Li Japin <[email-masked]>
Date: Tue, 24 Nov 2020 06:22:09 +0000

Hi, Kuroda

Thank for your review.

> On Nov 24, 2020, at 8:01 AM, [email-masked] wrote:
> 

--
Best regards
Japin Li

---

From: Li Japin <[email-masked]>
Date: Tue, 24 Nov 2020 06:22:23 +0000

Hi, David

Thanks for your suggestion!

On Nov 24, 2020, at 11:39 AM, David G. Johnston <[email-masked]<mailto:[email-masked]>> wrote:

On Mon, Nov 23, 2020 at 5:02 PM [email-masked]<mailto:[email-masked]> <[email-masked]<mailto:[email-masked]>> wrote:
No one have any comments, patch tester says OK, and I think this works well.
I changed status to "Ready for Committer."
Some proof-reading:

v8-0001

Documentation:

My suggestion wasn't taken for the first note paragraph (review/author disagreement) and the current has the following issues:

Sorry for ignoring this suggestion.

"if you use some connection-pooling" software doesn't need the word "some"
Don't substitute "pg" for the name of the product, PostgreSQL.
The word "used" is a more stylistic dislike, but "connected to using postgres_fdw" would be a better choice IMO.

How about use “foreign-data wrapper” replace “postgres_fdw”?

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b71a116be3..a3a50e7bdb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8293,8 +8293,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;

        <note>
         <para>
-         This parameter should be set to zero if you use some connection-pooling software,
-         or pg servers used by postgres_fdw, because connections might be closed unexpectedly.
+         This parameter should be set to zero if you use connection-pooling software,
+         or <productname>PostgreSQL</productname> servers connected to using foreign-data
+         wrapper, because connections might be closed unexpectedly.
         </para>
         <para>
          Aside from a bit of resource consumption idle sessions do not interfere with the

Fixed.

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index ba2369b72d..bcf8c610fd 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4278,7 +4278,7 @@ PostgresMain(int argc, char *argv[],
                DoingCommandRead = false;

                /*
-                * (5) turn off the idle-in-transaction and idle-session timeout
+                * (5) turn off the idle-in-transaction and idle-session timeouts
                 */
                if (disable_idle_in_transaction_timeout)
                {

I will send a new patch if there is not other comments.

--
Best Regards,
Japin Li

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 24 Nov 2020 08:20:15 -0700

> How about use “foreign-data wrapper” replace “postgres_fdw”?

I don't see much value in avoiding mentioning that specific term - my
proposal turned it into an example instead of being exclusive.

> +         wrapper, because connections might be closed unexpectedly.

Maybe:

+ or your PostgreSQL server receives connection from postgres_fdw or
similar middleware.
+ Such software is expected to self-manage its connections.
David J.

---

From: Li Japin <[email-masked]>
Date: Wed, 25 Nov 2020 02:18:28 +0000

On Nov 24, 2020, at 11:20 PM, David G. Johnston <[email-masked]<mailto:[email-masked]>> wrote:

On Mon, Nov 23, 2020 at 11:22 PM Li Japin <[email-masked]<mailto:[email-masked]>> wrote:

I don't see much value in avoiding mentioning that specific term - my proposal turned it into an example instead of being exclusive.

Maybe:

+ or your PostgreSQL server receives connection from postgres_fdw or similar middleware.
+ Such software is expected to self-manage its connections.

Thank you for your suggestion and patient! Fixed.

```
+        <para>
+         This parameter should be set to zero if you use connection-pooling software,
+         or <productname>PostgreSQL</productname> servers connected to using postgres_fdw
+         or similar middleware (such software is expected to self-manage its connections),
+         because connections might be closed unexpectedly.
+        </para>
```

--
Best regards
Japin Li

---

From: Anastasia Lubennikova <[email-masked]>
Date: Tue, 1 Dec 2020 16:55:49 +0300

On 25.11.2020 05:18, Li Japin wrote:
>
>
>
>> On Nov 24, 2020, at 11:20 PM, David G. Johnston 
>> <[email-masked] <mailto:[email-masked]>> wrote:
>>

Status update for a commitfest entry.
As far as I see, all recommendations from reviewers were addressed in 
the last version of the patch.

It passes CFbot successfully, so I move it to Ready For Committer.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company