Re: Improve LWLock tranche name visibility across backends - Mailing list pgsql-hackers

Hi,

On Sat, Aug 16, 2025 at 10:18:05PM -0500, Sami Imseih wrote:
> Attached is v8.

Thanks for the new version!

A few random comments about 0001:

1 ===

+}                      LWLockTrancheNamesShmem;

+}                      LWLockTrancheNamesStruct;

Add LWLockTrancheNamesShmem and LWLockTrancheNamesStruct to
src/tools/pgindent/typedefs.list?

2 ===

Maybe a comment before the above structs definitions to explain what they are
used for?

3 ===

+static void
+SetSharedTrancheName(int tranche_index, const char *tranche_name)
 {
-       /* This should only be called for user-defined tranches. */
-       if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED)
-               return;
+       dsa_pointer *name_ptrs;
+       dsa_pointer str_ptr;
+       char       *str_addr;
+       int                     len;
+       int                     current_allocated;
+       int                     new_alloc = 0;

-       /* Convert to array index. */
-       tranche_id -= LWTRANCHE_FIRST_USER_DEFINED;
+       LWLockAcquire(&LWLockTrancheNames.shmem->lock, LW_EXCLUSIVE);

Add Assert(IsUnderPostmaster || !IsPostmasterEnvironment);?

4 ===

+static void
+SetLocalTrancheName(int tranche_index, const char *tranche_name)
+{
+       int                     newalloc;
+
+       Assert(tranche_name);

should we add some assert on IsUnderPostmaster and/or IsPostmasterEnvironment too?

5 ===

+       while (i < NamedLWLockTrancheRequests)
+       {
+               NamedLWLockTranche *tranche;
+
+               tranche = &NamedLWLockTrancheArray[i];
+
+               SetLocalTrancheName(i, tranche->trancheName);
+
+               i++;
+       }

Maybe add a comment that those are the ones allocated by the postmaster during
startup?

Also, as this will be done during each sync and those tranches don't change,
so one could think there is room for improvement. Maybe add a comment that's
probably not worth optimizing (due to the fact that NamedLWLockTrancheRequests
should be small enough and the sync rare)?

6 ===

There is this existing comment:

/*
 * NamedLWLockTrancheRequests is both the valid length of the request array,
 * and the length of the shared-memory NamedLWLockTrancheArray later on.
 * This variable and NamedLWLockTrancheArray are non-static so that
 * postmaster.c can copy them to child processes in EXEC_BACKEND builds.
 */
int         NamedLWLockTrancheRequests = 0;

Maybe add something like? "Additional dynamic tranche names beyond this count
are stored in a DSA".

7 ===

+               old_ptrs = dsa_get_address(LWLockTrancheNames.dsa,
+                                                                  LWLockTrancheNames.shmem->list_ptr);
+
+               name_ptrs = dsa_get_address(LWLockTrancheNames.dsa, new_list);
+
+               memset(name_ptrs, InvalidDsaPointer, new_alloc);
+               memcpy(name_ptrs, old_ptrs, sizeof(dsa_pointer) * current_allocated);
+
+               dsa_free(LWLockTrancheNames.dsa, LWLockTrancheNames.shmem->list_ptr);

maybe use local variable for LWLockTrancheNames.shmem->list_ptr (and 
LWLockTrancheNames.dsa)?

8 ===

+       needs_sync = (trancheId >= LWLockTrancheNames.allocated ||
+                                 LWLockTrancheNames.local[trancheId] == NULL)
+               && (IsUnderPostmaster || !IsPostmasterEnvironment);

formating does not look good.

9 ===

-       if (trancheId >= LWLockTrancheNamesAllocated ||
-               LWLockTrancheNames[trancheId] == NULL)
-               return "extension";
+       if (trancheId < LWLockTrancheNames.allocated)
+               tranche_name = LWLockTrancheNames.local[trancheId];

-       return LWLockTrancheNames[trancheId];
+       if (!tranche_name)
+               elog(ERROR, "tranche ID %d is not registered", tranche_id_saved);

We now error out instead of returning "extension". That looks ok given the 
up-thread discussion but then the commit message needs some updates as it
states:

"
Additionally, while users should not pass arbitrary tranche IDs (that is,
IDs not created via LWLockNewTrancheId) to LWLockInitialize, nothing
technically prevents them from doing so. Therefore, we must continue to
handle such cases gracefully by returning a default "extension" tranche name.
"

10 ===

+LWLockTrancheNamesInitSize()
+{
+       Size            sz;
+
+       /*
+        * This value is used by other facilities, see pgstat_shmem.c, so it's
+        * good enough.
+        */
+       sz = 256 * 1024;

use DSA_MIN_SEGMENT_SIZE?

11 ===

+       if (!IsUnderPostmaster)
+       {
+               dsa_area   *dsa;
+               LWLockTrancheNamesShmem *ctl = LWLockTrancheNames.shmem;
+               char       *p = (char *) ctl;
+
+               /* Calculate layout within the shared memory region */
+               p += MAXALIGN(sizeof(LWLockTrancheNamesShmem));
+               ctl->raw_dsa_area = p;
+               p += MAXALIGN(LWLockTrancheNamesInitSize());

LWLockTrancheNamesInitSize() already does MAXALIGN() so the last one
above is not needed. But the last p advance seems not necessary as not used
after. I think the same is true in StatsShmemInit() (see [1]).

12 ===

+LWLockTrancheNamesBEInit(void)
+{
+       /* already attached, nothing to do */
+       if (LWLockTrancheNames.dsa)
+               return;
+
+       LWLockTrancheNamesAttach();
+
+       /* Set up a process-exit hook to clean up */

s/already/Already/?

For 0002, a quick review:

13 ===

+       if (log)
+               elog(DEBUG3, "current_allocated: %d in tranche names shared memory", new_alloc);

Instead of this, could we elog distinct messages where the patch currently sets
log = true?


14 ===

-                 xid_wraparound
+                 xid_wraparound \
+                 test_lwlock_tranches

breaks the ordering.

15 ===

 subdir('worker_spi')
 subdir('xid_wraparound')
+subdir('test_lwlock_tranches')

Same, breaks the ordering.

16 ===

+my $ENABLE_LOG_WAIT = 1;
+
+my $isession;
+my $log_location;
+
+# Helper function: wait for one or more logs if $ENABLE_LOG_WAIT is true
+sub maybe_wait_for_log {
+    my ($node, $logs, $log_loc) = @_;
+    return $log_loc unless $ENABLE_LOG_WAIT;

is ENABLE_LOG_WAIT needed as it does not change?


[1]: https://www.postgresql.org/message-id/aKLsu2sdpnyeuSSc%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com