Re: Patch proposal: New hooks in the connection path

Hi,

On 7/6/22 12:11 AM, Joe Conway wrote:
>
> 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 review!

>
>> 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

Not sure about this one neither.

The "enhanced" ClientAuthentication_hook will have to be fired at the 
same places as the new FailedConnection_hook is, but i think those 
places are not necessary linked to real authentication per say (making 
the name confusing).

>
>> 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

Moved to auth.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()?

removed it.

>
> 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.

>
> 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<--------------
>
>
Thanks!, fixed.

-- 

Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com