Re: Patch proposal: New hooks in the connection path

On 7/5/22 03:37, Bharath Rupireddy wrote:
> On Mon, Jul 4, 2022 at 6:23 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> On 7/2/22 1:00 AM, Nathan Bossart wrote:
>> > Could we model this after fmgr_hook?  The first argument in that hook
>> > indicates where it is being called from.  This doesn't alleviate the need
>> > for several calls to the hook in the authentication logic, but extension
>> > authors would only need to define one hook.
>>
>> I like the idea and indeed fmgr.h looks a good place to model it.
>>
>> Attached a new patch version doing so.

I was thinking along the same lines, so +1 for the general approach

> Thanks for the patch. Can we think of enhancing
> ClientAuthentication_hook_type itself i.e. make it a generic hook for
> all sorts of authentication metrics, info etc. with the type parameter
> embedded to it instead of new hook FailedConnection_hook?We can either
> add a new parameter for the "event" (the existing
> ClientAuthentication_hook_type implementers will have problems), or
> embed/multiplex the "event" info to existing Port structure or status
> variable (macro or enum) (existing implementers will not have
> compatibility problems).  IMO, this looks cleaner going forward.

Not sure I like this though -- I'll have to think about that

> On the v2 patch:
> 
> 1. Why do we need to place the hook and structures in fmgr.h? Why not in auth.h?

agreed -- it does not belong in fmgr.h

> 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);
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("timeout while processing startup packet")));

Why add the ereport()?

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?

Also, a teeny nit:
8<--------------
+    if (status != STATUS_OK) {
+        if (FailedConnection_hook)
8<--------------

does not follow usual practice and probably should be:

8<--------------
+    if (status != STATUS_OK)
+    {
+        if (FailedConnection_hook)
8<--------------


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