Re: Improve LWLock tranche name visibility across backends

> 1 ===
>
> +} LWLockTrancheNamesShmem;
>
> +} LWLockTrancheNamesStruct;
>
> Add LWLockTrancheNamesShmem and LWLockTrancheNamesStruct to
> src/tools/pgindent/typedefs.list?

done.

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

done.

> 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);?

done. This is required here since we should not be dealling with DSA
during postmaster or single-user.

> 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?

No, It's not needed here. SetLocalTrancheName is called during startup
and by normal
backends.

> 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)?

done.

> 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".

No. I don't like talking about that here. This is already described in:
```
* 3. Extensions can create new tranches using either RequestNamedLWLockTranche
* or LWLockNewTrancheId. Tranche names are reg
```

> 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)?

hmm, I don't see the point to this.

>
> 8 ===
>
> + needs_sync = (trancheId >= LWLockTrancheNames.allocated ||
> + LWLockTrancheNames.local[trancheId] == NULL)
> + && (IsUnderPostmaster || !IsPostmasterEnvironment);
>
> formating does not look good.

pgindent told me it's fine. But, I did add a parenthesis around the expression,
so pgindent now aligned the conditions in a better way.

> 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.
> "

done.

> 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?

done.

> 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]).

yeah, good point. done.

> 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/?

done.

> 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?

ok, that was my initial thought, but I did not like repeating the
messages. I went
back to distinct messages. I have no strong feelings either way.

>
> 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 ===

done and done.

> +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?

That was a remnant of my testing. removed.

see v9

--

Sami