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
<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 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
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index d7257e4056..d9e1e3b4c1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -567,6 +567,8 @@ int postmaster_alive_fds[2] = {-1, -1};
HANDLE PostmasterHandle;
#endif
+FailedConnection_hook_type FailedConnection_hook = NULL;
+
/*
* Postmaster main entry point
*/
@@ -4462,7 +4464,11 @@ BackendInitialize(Port *port)
* already did any appropriate error reporting.
*/
if (status != STATUS_OK)
+ {
+ if (FailedConnection_hook)
+ (*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET,
port);
proc_exit(0);
+ }
/*
* Now that we have the user and database name, we can set the process
diff --git a/src/backend/utils/init/postinit.c
b/src/backend/utils/init/postinit.c
index 6b9082604f..e9bbd185f4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -360,10 +360,14 @@ CheckMyDatabase(const char *name, bool am_superuser, bool
override_allow_connect
if (!am_superuser &&
pg_database_aclcheck(MyDatabaseId, GetUserId(),
ACL_CONNECT)
!= ACLCHECK_OK)
+ {
+ if (FailedConnection_hook)
+ (*FailedConnection_hook)
(FCET_BAD_DATABASE_PERMISSION, MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied for database
\"%s\"", name),
errdetail("User does not have CONNECT
privilege.")));
+ }
/*
* Check connection limit for this database.
@@ -918,9 +922,13 @@ InitPostgres(const char *in_dbname, Oid dboid, const char
*username,
tuple = GetDatabaseTuple(in_dbname);
if (!HeapTupleIsValid(tuple))
+ {
+ if (FailedConnection_hook)
+ (*FailedConnection_hook)
(FCET_BAD_DATABASE_NAME, MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not
exist", in_dbname)));
+ }
dbform = (Form_pg_database) GETSTRUCT(tuple);
MyDatabaseId = dbform->oid;
MyDatabaseTableSpace = dbform->dattablespace;
@@ -935,9 +943,13 @@ InitPostgres(const char *in_dbname, Oid dboid, const char
*username,
tuple = GetDatabaseTupleByOid(dboid);
if (!HeapTupleIsValid(tuple))
+ {
+ if (FailedConnection_hook)
+ (*FailedConnection_hook)
(FCET_BAD_DATABASE_OID, MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database %u does not exist",
dboid)));
+ }
dbform = (Form_pg_database) GETSTRUCT(tuple);
MyDatabaseId = dbform->oid;
MyDatabaseTableSpace = dbform->dattablespace;
diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h
index d3c189efe3..a604b3f1e6 100644
--- a/src/include/libpq/auth.h
+++ b/src/include/libpq/auth.h
@@ -28,4 +28,20 @@ extern void sendAuthRequest(Port *port, AuthRequest areq,
const char *extradata,
typedef void (*ClientAuthentication_hook_type) (Port *, int);
extern PGDLLIMPORT ClientAuthentication_hook_type ClientAuthentication_hook;
+/*
+ * The failed connection events to be used in the FailedConnection_hook.
+ */
+typedef enum FailedConnectionEventType
+{
+ FCET_BAD_STARTUP_PACKET,
+ FCET_BAD_DATABASE_NAME,
+ FCET_BAD_DATABASE_OID,
+ FCET_BAD_DATABASE_PERMISSION
+} FailedConnectionEventType;
+
+/* kluge to avoid including libpq/libpq-be.h here */
+struct Port;
+typedef void (*FailedConnection_hook_type) (FailedConnectionEventType event,
const struct Port *port);
+extern PGDLLIMPORT FailedConnection_hook_type FailedConnection_hook;
+
#endif /* AUTH_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 4fb746930a..4fadafbddb 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -717,6 +717,7 @@ FPI
FSMAddress
FSMPage
FSMPageData
+FailedConnection_hook_type
FakeRelCacheEntry
FakeRelCacheEntryData
FastPathStrongRelationLockData
