Re: Patch proposal: New hooks in the connection path

On 7/6/22 04:13, Drouvot, Bertrand wrote:
> On 7/6/22 12:11 AM, Joe Conway wrote:
>> On 7/5/22 03:37, Bharath Rupireddy wrote:
>>> 2. Timeout Handler is a signal handler, called as part of SIGALRM
>>> signal handler, most of the times, signal handlers ought to be doing
>>> small things, now that we are handing off the control to hook, which
>>> can do any long running work (writing to a remote storage, file,
>>> aggregate etc.), I don't think it's the right thing to do here.
>>>   static void
>>>   StartupPacketTimeoutHandler(void)
>>>   {
>>> + if (FailedConnection_hook)
>>> + (*FailedConnection_hook) (FCET_STARTUP_PACKET_TIMEOUT, MyProcPort);

>> But more to Bharath's point, perhaps this is a case that is better
>> served by incrementing a stat counter and not exposed as a hook?
>
> I think that the advantage of the hook is that it gives the extension
> author the ability/flexibility to aggregate the counter based on
> information available in the Port Struct (say the client addr for
> example) at this stage.
>
> What about to aggregate the stat counter based on the client addr? (Not
> sure if there is more useful information (than the client addr) at this
> stage though)
>
> That said, i agree that having a hook in a time out handler might not be
> the right thing to do (even if at the end that would be to the extension
> author responsibility to do "small things" in it), so it has been
> removed in the new attached version.

It isn't clear to me if having a hook in the timeout handler is a
nonstarter -- perhaps a comment with suitable warning for prospective
extension authors is enough? Anyone else want to weigh in on this issue
specifically?

--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com