Re: [HACKERS] Optional message to user when terminating/cancelling backend

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Onder Kalaci <onder(at)citusdata(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend
Date: 2018-11-12 19:35:47
Message-ID: 30321.1542051347@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> I’ve split the patch into two logical parts, the signalling functionality and
> the userfacing terminate/cancel part. For extra clarity I’ve also included the
> full v19 patch, in case you prefer that instead when seeing the two.

I'm a bit befuddled by this patch, or at least the proposed test cases.
Those show no proof that the feature actually works, ie, delivers a
message to the target backend. Also, what am I to make of the test cases
involving NULL arguments?

Personally, rather than messing around with defaulted arguments and
detecting nulls at runtime, I'd make two C functions that are both strict.

The signal_message stuff seems both overly complicated and overly fragile.
You have, for example, largely ignored the coding rule that says to have
only straight-line code within a spinlock hold. I am also not very
enamored of setting up Yet Another per-backend data structure in shared
memory, especially one that can so easily get out of sync with the
ProcArray. If we're going to expend dedicated shared memory on this
feature, let's just add the necessary fields to the PGPROC structs.
That would also provide some opportunity to interlock things in a way that
would fix the race conditions, ie, once we've found the relevant PGPROC
entry, hold a lock on it till we've inserted the message and sent the
signal.

I don't especially like orig_length: that information is of no use
to the recipient backend, and what the field is actually being used
as, ie a boolean "slot is full" indicator, is nowhere to be guessed
from the field's documentation.

The current patch fails to apply against parallel_schedule, which
reminds me that you forgot to include an update to serial_schedule.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-11-12 19:45:48 Re: PostgreSQL BuildFarm Client Release 9
Previous Message Elvis Pranskevichus 2018-11-12 19:29:48 Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.