On 7/5/22 03:37, Bharath Rupireddy wrote:
On Mon, Jul 4, 2022 at 6:23 PM Drouvot, Bertrand <bdrou...@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
