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

> 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