Li Japin <japi...@hotmail.com> writes:
> [ v9-0001-Allow-terminating-the-idle-sessions.patch ]
I've reviewed and pushed this. A few notes:
* Thomas' patch for improving timeout.c seems like a great idea, but
it did indeed have a race condition, and I felt the comments could do
with more work.
* I'm not entirely comfortable with the name "idle_session_timeout",
because it sounds like it applies to all idle states, but actually
it only applies when we're not in a transaction. I left the name
alone and tried to clarify the docs, but I wonder whether a different
name would be better. (Alternatively, we could say that it does
apply in all cases, making the effective timeout when in a transaction
the smaller of the two GUCs. But that'd be more complex to implement
and less flexible, so I'm not in favor of that answer.)
* The SQLSTATE you chose for the new error condition seems pretty
random. I do not see it in the SQL standard, so using a code that's
within the spec-reserved code range is certainly wrong. I went with
08P02 (the "P" makes it outside the reserved range), but I'm not
really happy either with using class 08 ("Connection Exception",
which seems to be mainly meant for connection-request failures),
or the fact that ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT is
practically identical but it's not even in the same major class.
Now 25 ("Invalid Transaction State") is certainly not right for
this new error, but I think what that's telling us is that 25 was a
misguided choice for the other error. In a green field I think I'd
put both of them in class 53 ("Insufficient Resources") or maybe class
57 ("Operator Intervention"). Can we get away with renumbering the
older error at this point? In any case I'd be inclined to move the
new error to 53 or 57 --- anybody have a preference which?
* I think the original intent in timeout.h was to have 10 slots
available for run-time-defined timeout reasons. This is the third
predefined one we've added since the header was created, so it's
starting to look a little tight. I adjusted the code to hopefully
preserve 10 free slots going forward.
* I noted the discussion about dropping setitimer in place of some
newer kernel API. I'm not sure that that is worth the trouble in
isolation, but it might be worth doing if we can switch the whole
module over to relying on CLOCK_MONOTONIC, so as to make its behavior
less flaky if the sysadmin steps the system clock. Portability
might be a big issue here though, plus we'd have to figure out how
we want to define enable_timeout_at(), which is unlikely to want to
use CLOCK_MONOTONIC values. In any case, that's surely material
for a whole new thread.
regards, tom lane
