Andres Freund <andres@anarazel.de> writes:
> On 2019-10-02 23:52:31 +0900, Fujii Masao wrote:
>> Regarding session end hook, you can do the almost same thing as that hook
>> by calling on_shmem_exit(), before_shmem_exit() or on_proc_exit() in
>> other hook like session start hook. This approach also has the same issue
>> discussed upthread, though. Anyway, I'm not sure if session end hook is
>> "actually" necessary.
> No, it's not actually the same (at least for
> before_shmem_exit).
Yeah. The important point here is that if you want to be able to execute
SQL, then you can't close down *ANY* subsystems before that. Even in the
simple scenario exercised by the test case, imagine that there are user
triggers attached to the table it wants to insert a row in. They could
execute anything at all.
Thus, we can't run any shutdown hooks before this one, if indeed we
consider it to be a shutdown hook at all.
A possible fix is to do it as the first action in proc_exit, but that will
fall foul of Andres' points about not wanting to do it in non-session
backends, nor in FATAL exits, nor in the case where a previous try failed.
Perhaps a better idea is to put it into PostgresMain's handling of client
EOF, despite the large comment saying not to add more code there.
(Or, maybe invent a new class of shutdown callbacks?
before_before_shmem_exit seems like a pretty grotty concept, but it's
more or less what we need here. It would sure be nice if we had an
actual specification for what is allowed to happen in these different
classes of callbacks...)
In any case, the hook would have to be responsible for cancelling any open
transaction for itself, in order to have a clean environment to run SQL
code in. It cannot piggyback on existing transaction-closing code to do
that, because any user-written code could throw an error inside the hook's
transaction and thereby break the invariant that we're not in a
transaction after that point.
Bottom line: there needs to be thought and testing of the case where the
executed SQL throws an error. We need to recover from that and exit
cleanly, and the committed patch surely would not have. Likewise there
should be a test for the case where we exit the session mid-transaction
or mid-failed-transaction.
BTW, my first thought about why the test case was failing was that it
had race conditions. We now see that the problems were worse than that,
but before a new on-session-exit test goes in, you need to think about
it. You have no guarantee that the callback will execute before the
client-side test script does its next action. Thus, for example, I
wondered if the SELECTs were failing because the expected insertion
hadn't happened yet, or if the DROP ROLEs were failing because there
was still a live session logged in under the role.
regards, tom lane
