small_cleanups around login event triggers
I was taking a look at the login event triggers work (nice work btw)
and saw a couple of minor items that I thought would be worth cleaning
up. This is mostly just clarifying the exiting docs and code comments.
Robert Treat
https://xzilla.net
Attachments:
login_event_trigger_small_cleanups.patchapplication/octet-stream; name=login_event_trigger_small_cleanups.patchDownload
diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 23dbb80481..b6f719649a 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -39,10 +39,10 @@
<para>
The <literal>login</literal> event occurs when an authenticated user logs
into the system. Any bug in a trigger procedure for this event may
- prevent successful login to the system. Such bugs may be fixed by
- setting <xref linkend="guc-event-triggers"/> is set to <literal>false</literal>
- either in a connection string or configuration file. Alternative is
- restarting the system in single-user mode (as event triggers are
+ prevent successful login to the system. Such bugs may be worked around by
+ setting <xref linkend="guc-event-triggers"/> to <literal>false</literal>
+ either in a connection string or configuration file. Alternativly, you can
+ restart the system in single-user mode (as event triggers are
disabled in this mode). See the <xref linkend="app-postgres"/> reference
page for details about using single-user mode.
The <literal>login</literal> event will also fire on standby servers.
@@ -50,7 +50,7 @@
writing anything to the database when running on a standby.
Also, it's recommended to avoid long-running queries in
<literal>login</literal> event triggers. Note that, for instance,
- canceling connection in <application>psql</application> wouldn't cancel
+ canceling a connection in <application>psql</application> will not cancel
the in-progress <literal>login</literal> trigger.
</para>
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index ab11ab500b..b39a10a113 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -174,7 +174,7 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
else if (strcmp(stmt->eventname, "login") == 0 && tags != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("tag filtering is not supported for login event trigger")));
+ errmsg("tag filtering is not supported for login event triggers")));
/*
* Give user a nice error message if an event trigger of the same name
@@ -307,7 +307,7 @@ insert_event_trigger_tuple(const char *trigname, const char *eventname, Oid evtO
heap_freetuple(tuple);
/*
- * Login event triggers have an additional flag in pg_database to avoid
+ * Login event triggers have an additional flag in pg_database to enable
* faster lookups in hot codepaths. Set the flag unless already True.
*/
if (strcmp(eventname, "login") == 0)
@@ -376,7 +376,7 @@ filter_list_to_array(List *filterlist)
/*
* Set pg_database.dathasloginevt flag for current database indicating that
- * current database has on login triggers.
+ * current database has on login event triggers.
*/
void
SetDatatabaseHasLoginEventTriggers(void)
@@ -444,7 +444,7 @@ AlterEventTrigger(AlterEventTrigStmt *stmt)
CatalogTupleUpdate(tgrel, &tup->t_self, tup);
/*
- * Login event triggers have an additional flag in pg_database to avoid
+ * Login event triggers have an additional flag in pg_database to enable
* faster lookups in hot codepaths. Set the flag unless already True.
*/
if (namestrcmp(&evtForm->evtevent, "login") == 0 &&
@@ -695,7 +695,7 @@ EventTriggerCommonSetup(Node *parsetree,
}
}
- /* don't spend any more time on this if no functions to run */
+ /* Don't spend any more time on this if no functions to run */
if (runlist == NIL)
return NIL;
@@ -878,7 +878,7 @@ EventTriggerSQLDrop(Node *parsetree)
/*
* Fire login event triggers if any are present. The dathasloginevt
- * pg_database flag is left when an event trigger is dropped, to avoid
+ * pg_database flag is left unchanged when an event trigger is dropped to avoid
* complicating the codepath in the case of multiple event triggers. This
* function will instead unset the flag if no trigger is defined.
*/
@@ -891,7 +891,7 @@ EventTriggerOnLogin(void)
/*
* See EventTriggerDDLCommandStart for a discussion about why event
* triggers are disabled in single user mode or via a GUC. We also need a
- * database connection (some background workers doesn't have it).
+ * database connection (some background workers don't have it).
*/
if (!IsUnderPostmaster || !event_triggers ||
!OidIsValid(MyDatabaseId) || !MyDatabaseHasLoginEventTriggers)
@@ -920,7 +920,7 @@ EventTriggerOnLogin(void)
/*
* There is no active login event trigger, but our
- * pg_database.dathasloginevt was set. Try to unset this flag. We use the
+ * pg_database.dathasloginevt is set. Try to unset this flag. We use the
* lock to prevent concurrent SetDatatabaseHasLoginEventTriggers(), but we
* don't want to hang the connection waiting on the lock. Thus, we are
* just trying to acquire the lock conditionally.
@@ -931,7 +931,7 @@ EventTriggerOnLogin(void)
/*
* The lock is held. Now we need to recheck that login event triggers
* list is still empty. Once the list is empty, we know that even if
- * there is a backend, which concurrently inserts/enables login
+ * there is a backend which concurrently inserts/enables login
* trigger, it will update pg_database.dathasloginevt *afterwards*.
*/
runlist = EventTriggerCommonSetup(NULL,
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index 41fd856c65..27370c14a0 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -142,7 +142,7 @@ LockRelationOid(Oid relid, LOCKMODE lockmode)
* ConditionalLockRelationOid
*
* As above, but only lock if we can get the lock without blocking.
- * Returns true iff the lock was acquired.
+ * Returns true if the lock was acquired.
*
* NOTE: we do not currently need conditional versions of all the
* LockXXX routines in this file, but they could easily be added if needed.
On 14 Mar 2024, at 02:47, Robert Treat <rob@xzilla.net> wrote:
I was taking a look at the login event triggers work (nice work btw)
Thanks for reviewing committed code, that's something which doesn't happen
often enough and is much appreciated.
and saw a couple of minor items that I thought would be worth cleaning
up. This is mostly just clarifying the exiting docs and code comments.
+ either in a connection string or configuration file. Alternativly, you can
This should be "Alternatively" I think.
- canceling connection in <application>psql</application> wouldn't cancel
+ canceling a connection in <application>psql</application> will not cancel
Nitpickery (perhaps motivated by english not being my first language), but
since psql only deals with one connection I would expect this to read "the
connection".- * Returns true iff the lock was acquired.
+ * Returns true if the lock was acquired.
Using "iff" here is being consistent with the rest of the file (and technically
correct):$ grep -c "if the lock was" src/backend/storage/lmgr/lmgr.c
1
$ grep -c "iff the lock was" src/backend/storage/lmgr/lmgr.c
5
--
Daniel Gustafsson
On Thu, Mar 14, 2024 at 8:21 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 Mar 2024, at 02:47, Robert Treat <rob@xzilla.net> wrote:
I was taking a look at the login event triggers work (nice work btw)
Thanks for reviewing committed code, that's something which doesn't happen
often enough and is much appreciated.and saw a couple of minor items that I thought would be worth cleaning
up. This is mostly just clarifying the exiting docs and code comments.+ either in a connection string or configuration file. Alternativly, you can
This should be "Alternatively" I think.
Yes.
- canceling connection in <application>psql</application> wouldn't cancel + canceling a connection in <application>psql</application> will not cancel Nitpickery (perhaps motivated by english not being my first language), but since psql only deals with one connection I would expect this to read "the connection".
My interpretation of this is that "a connection" is more correct
because it could be your connection or someone else's connection (ie,
you are canceling one of many possible connections). Definitely
nitpickery either way.
- * Returns true iff the lock was acquired. + * Returns true if the lock was acquired. Using "iff" here is being consistent with the rest of the file (and technically correct):$ grep -c "if the lock was" src/backend/storage/lmgr/lmgr.c
1
$ grep -c "iff the lock was" src/backend/storage/lmgr/lmgr.c
5
Ah, yeah, I was pretty focused on the event trigger stuff and didn't
notice it being used elsewhere; thought it was a typo, but I guess
it's meant as shorthand for "if and only if", I wonder how many people
are familiar with that.
Robert Treat
https://xzilla.net
On 14 Mar 2024, at 14:21, Robert Treat <rob@xzilla.net> wrote:
On Thu, Mar 14, 2024 at 8:21 AM Daniel Gustafsson <daniel@yesql.se> wrote:
- canceling connection in <application>psql</application> wouldn't cancel + canceling a connection in <application>psql</application> will not cancel Nitpickery (perhaps motivated by english not being my first language), but since psql only deals with one connection I would expect this to read "the connection".My interpretation of this is that "a connection" is more correct
because it could be your connection or someone else's connection (ie,
you are canceling one of many possible connections). Definitely
nitpickery either way.
Fair point.
- * Returns true iff the lock was acquired. + * Returns true if the lock was acquired. Using "iff" here is being consistent with the rest of the file (and technically correct):
Ah, yeah, I was pretty focused on the event trigger stuff and didn't
notice it being used elsewhere; thought it was a typo, but I guess
it's meant as shorthand for "if and only if", I wonder how many people
are familiar with that.
I would like to think it's fairly widely understood among programmers, but I
might be dating myself in saying so.
I went ahead and applied this with the fixes mentioned here with one more tiny
change to the last hunk of the patch to make it say "login event trigger"
rather than just "login trigger". Thanks for the submission!
--
Daniel Gustafsson
On Thu, Mar 14, 2024 at 7:23 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 Mar 2024, at 14:21, Robert Treat <rob@xzilla.net> wrote:
On Thu, Mar 14, 2024 at 8:21 AM Daniel Gustafsson <daniel@yesql.se> wrote:- canceling connection in <application>psql</application> wouldn't cancel + canceling a connection in <application>psql</application> will not cancel Nitpickery (perhaps motivated by english not being my first language), but since psql only deals with one connection I would expect this to read "the connection".My interpretation of this is that "a connection" is more correct
because it could be your connection or someone else's connection (ie,
you are canceling one of many possible connections). Definitely
nitpickery either way.Fair point.
- * Returns true iff the lock was acquired. + * Returns true if the lock was acquired. Using "iff" here is being consistent with the rest of the file (and technically correct):Ah, yeah, I was pretty focused on the event trigger stuff and didn't
notice it being used elsewhere; thought it was a typo, but I guess
it's meant as shorthand for "if and only if", I wonder how many people
are familiar with that.I would like to think it's fairly widely understood among programmers, but I
might be dating myself in saying so.I went ahead and applied this with the fixes mentioned here with one more tiny
change to the last hunk of the patch to make it say "login event trigger"
rather than just "login trigger". Thanks for the submission!
LGTM, thanks!
Robert Treat
https://xzilla.net