Re: Proposal: Add a callback data parameter to GetNamedDSMSegment

> >> wouldn't the above be sufficient to create a DSM segment containing
> >> a flexible array?
> >
> > Yes, it creates it, but can I initialize it properly in
> > foo_init_state? How can I set the size member to the proper array
> > size, and how can I zero-initialize the array with the correct length
> > in it? What I can do currently is:
> >
> > 1. create the lwlock and set size to 0 in foo_init_state
> > 2. take the lwlock after GetNamedDSMSegment returns
> > 3. if size is 0 set it properly and zero-initialize the array
> >
> > That's why I said that there is a workaround, but it would be nicer if
> > I could do it properly in the init callback, by passing the array size
> > as a parameter to it.
>
> So, it seems like we've established at least 2 potential use-cases for this
> argument (tranche name and flexible arrays). And the amount of extension
> breakage doesn't seem too bad either. IMHO it'd be reasonable to proceed
> with this change.

I agree.

The workaround used above looks unsafe, because a backend could attach
to a partially initialized segment. Providing more flexibility in the
init callback
is a clear improvement.

The patch itself is straightforward. One thing that stands out is this:

```
static void
-init_tranche(void *ptr)
+init_tranche(void *ptr, void *arg)
{
int *tranche_id = (int *) ptr;

- *tranche_id = LWLockNewTrancheId("test_dsa");
+ *tranche_id = LWLockNewTrancheId(arg);
}

/* Test basic DSA functionality */
@@ -39,7 +39,7 @@ test_dsa_basic(PG_FUNCTION_ARGS)
dsa_pointer p[100];

tranche_id = GetNamedDSMSegment("test_dsa", sizeof(int),
-
init_tranche, &found);
+
init_tranche, "test_dsa", &found);

```

In almost all cases, the segment name is also used as the tranche name.
Currently, we set this name twice; in the GetNamedDSMSegment and inside
the init callback. The proposed patch does not improve this either.

I suggest passing the segment_name explicitly to the callback, and
reserving the extra argument for more complex data. If we are
changing the API, this seems like the right time to do so. So,
I think we should do something like:

```

static void
init_tranche(void *ptr, const char *segment_name, void *arg)
{
int *tranche_id = (int *) ptr;
*tranche_id = LWLockNewTrancheId(segment_name);
}

```
This works well for the first use-case identified. Instead of hard-
coding the tranche name in the callback, the name can be
retrieved as the segment name set in GetNamedDSMSegment.

The caller could still pass this name via the extra callback args, but
it's better to separate things a bit here, and reserve the extra callback
arguments for more complex data.

What do you think?

--
Sami Imseih
Amazon Web Services (AWS)