Re: Improve LWLock tranche name visibility across backends

> I haven't followed the latest discussion, but I took a look at the patch.
>
> +      It is possible to use a <literal>tranche_id</literal> that was not retrieved
> +      using <function>LWLockNewTrancheId</function>, but this is not recommended.
> +      The ID may clash with an already registered tranche name, or the specified
> +      name may not be found. In such cases, looking up the name will return a generic
> +      "extension" tranche name.
>
> Is there any reason to continue allowing this?  For example, maybe we could
> ERROR if LWLockInitialize()/GetLWTrancheName() are given a tranche_id
> greater than the number allocated.  I guess I'm not following why we should
> gracefully handle these kinds of coding errors, especially when they result
> in unhelpful behavior like an "extension" tranche.

We could definitely error out LWLockInitialize in this case. It may
break things out
there, so that is why I have been hesitant.

I don’t think we can use allocated in this case, since that value refers to
pre-allocated slots. Instead, we will need to do a lookup, and if no
tranche_name is found, then we should error out.

Essentially, we can call GetLWTrancheName within LWLockInitialize, but
GetLWTrancheName can no longer have a fallback value such as "extension".
It should error out instead. I believe that is what you mean. correct?

One thought I had was to change the signature of LWLockInitialize to take a
tranche name and no longer should LWLockNewTrancheId be no longer be global.
That will slightly complicate things with built-in tranches a bit (but
is doable).

On the other hand, an error seems like a reasonable approach.

> Attached v6 which addresses the feedback from the last review.

I spoke offline with Bertrand and discussed the synchronization of
the local memory from shared memory. After that discussion it is clear
we don't need to track the highest used index in shared memory. Because
the tranche_id comes from a shared counter, it is not possible that
the same tranche_id can be used twice, so we will not have overlap.
That means, when we sync, we just need to know the highest used
index in the local memory ( since that could be populated during
postmaster startup and inherited via fork ) and start syncing local
memory from that point. We can also cut off the sync once we encounter
an invalid dsa pointer, since there can't be any tranche names in shared
memory after that point. I did change SyncLWLockTrancheNames to
accept the tranche_id that triggered the sync, so we can make sure it
gets added to local memory, either as part of the sync or added with
the default "extension" tranche name.

I also corrected the INJECTION_POINT tests. I was missing a skip
if INJECTION_POINT is not enabled and removed

+elog(ERROR, "injection points not supported");

mentioned by Nathan earlier. To simplify the review, I also split the
test patch. It is not yet clear if there is consensus to include tests
( I think we should ), but having the tests will help the review.

More commentary was added to the code for the point Nathan
also raised.

See v7; which could be simplified further depending on the handling
of LWLockInitialize as mentioned at the top of this message, since we
don't need to account for out of range tranche IDs.


--
Sami