small_cleanups around login event triggers

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