Thread: doc patch: clarify the naming rule for injection_points

Dear hackers,

This is a forked thread from [1].

Naming rule of points is not determined yet, but most of them have lower cases
and each term are divided by dash "-". I think this is a good chance to formally
clarify it. PSA adding the description.

I was confused the correct place for the description. I added at the end of first
paragraph, because it describes how we add and use it. Suggestions are very
welcomed.

[1]:
https://www.postgresql.org/message-id/OSCPR01MB14966B78F3AF15C252EB9E02FF5B32%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

> Naming rule of points is not determined yet, but most of them have lower cases
> and each term are divided by dash "-". I think this is a good chance to formally
> clarify it. PSA adding the description.
>
> I was confused the correct place for the description. I added at the end of first
> paragraph, because it describes how we add and use it. Suggestions are very
> welcomed.

```
-     their own code using the same macro.
+     their own code using the same macro.  The name of injection points must be
+     lower characters, and dashes must separate its terms.
```

Perhaps "must" is a too strong statement. I suggest something like:

"""
By convention, the name of injection points ...
"""

I have mixed feelings about the patch overall though. This would mean
that we need to rename injection points like this:

``
AtEOXact_Inval-with-transInvalInfo
heap_update-before-pin
```

Otherwise we would contradict our own documentation.

I don't mind heap-update-before-pin, but not 100% sure if:

at-eo-xact-inval-with-trans-inval-info

... would be a better name than the current one.

--
Best regards,
Aleksander Alekseev

Dear Aleksander,


> ```
> -     their own code using the same macro.
> +     their own code using the same macro.  The name of injection points must
> be
> +     lower characters, and dashes must separate its terms.
> ```
> 
> Perhaps "must" is a too strong statement. I suggest something like:
> 
> """
> By convention, the name of injection points ...
> """

It is always difficult for me to distinguish them, thanks for comments.
PSA updated.

> 
> I have mixed feelings about the patch overall though. This would mean
> that we need to rename injection points like this:
> 
> ``
> AtEOXact_Inval-with-transInvalInfo
> heap_update-before-pin
> ```
> 
> Otherwise we would contradict our own documentation.

Thanks for telling these counterexamples. I grepped with "INJECTION_POINTS" and
"IS_INJECTION_POINT_ATTACHED", and no others are found.

> I don't mind heap-update-before-pin, but not 100% sure if:
> 
> at-eo-xact-inval-with-trans-inval-info
> 
> ... would be a better name than the current one.

I also feel just converting lower case is not good. The point seems to locate in
the end-of-transaction callback and it accepts invalidation messages. Based on the
fact, how about "inval-process-invalidation-messages"?
0002 did simple replacements of these words.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Apr 14, 2025 at 12:36:20PM +0000, Hayato Kuroda (Fujitsu) wrote:
> I also feel just converting lower case is not good. The point seems to locate in
> the end-of-transaction callback and it accepts invalidation messages. Based on the
> fact, how about "inval-process-invalidation-messages"?
> 0002 did simple replacements of these words.

-    INJECTION_POINT("heap_update-before-pin");
+    INJECTION_POINT("heap-update-before-pin");

The former name is better IMO.  heap_update() is the name of the
function where the point is defined.

-    INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo");
+    INJECTION_POINT("inval-process-invalidation-messages");

This naming has been puzzling me, though, so a rename looks fit.  I am
not sure that this is the best name that matches with this code path,
as this lacks a reference regarding the end of a transaction.  Perhaps
something like "end-transaction-process-inval" would be better?
--
Michael

Dear Michael,

> > I also feel just converting lower case is not good. The point seems to locate in
> > the end-of-transaction callback and it accepts invalidation messages. Based on
> the
> > fact, how about "inval-process-invalidation-messages"?
> > 0002 did simple replacements of these words.
>
> -    INJECTION_POINT("heap_update-before-pin");
> +    INJECTION_POINT("heap-update-before-pin");
>
> The former name is better IMO.  heap_update() is the name of the
> function where the point is defined.
>
> -    INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo");
> +    INJECTION_POINT("inval-process-invalidation-messages");
>
> This naming has been puzzling me, though, so a rename looks fit.  I am
> not sure that this is the best name that matches with this code path,
> as this lacks a reference regarding the end of a transaction.  Perhaps
> something like "end-transaction-process-inval" would be better?

Thanks for suggesting them. ISTM, you are correct. PSA updated version.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Apr 21, 2025 at 12:10:51PM +0000, Hayato Kuroda (Fujitsu) wrote:
> Thanks for suggesting them. ISTM, you are correct. PSA updated version.

Thanks, I've applied some slight tweaks, and applied the result down
to v17, leaving the heap_update point alone.
--
Michael

Dear Michael,

> Thanks, I've applied some slight tweaks, and applied the result down
> to v17, leaving the heap_update point alone.

Thanks, I confirmed your commit on HEAD and LGTM.

Best regards,
Hayato Kuroda
FUJITSU LIMITED