On Thu, Jan 7, 2021 at 12:55 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> * 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.
Oops, and thanks! Very happy to see this one in the tree.
> * 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.)
Hmm, it is a bit confusing, but having them separate is indeed more flexible.
> * 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 don't have a strong view here, but 08 with a P doesn't seem crazy to
me. Unlike eg 57014 (statement_timeout), 57017 (deadlock_timeout),
55P03 (lock_timeout... huh, I just noticed that DB2 uses 57017 for
that, distinguished from deadlock by another error field), after these
timeouts you don't have a session/connection anymore. The two are a
bit different though: in the older one, you were in a transaction, and
it seems to me quite newsworthy that your transaction has been
aborted, information that is not conveyed quite so clearly with a
connection-related error class.
> * 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.
+1
