Re: [PATCHES] New shared memory hooks proposal (was Re: - Mailing list pgsql-hackers

On Sat, 2006-10-14 at 14:55 -0400, Tom Lane wrote:
> Marc Munro <marc@bloodnok.com> writes:
> > The attached patch provides add-ins with the means to register for
> > shared memory and LWLocks.
>
> I finally got around to reviewing this patch, and realized that it's got
> a pretty fundamental design flaw: it isn't useful under Windows (or any
> other EXEC_BACKEND platform), because there isn't any provision for
> backends to locate structs allocated by other backends by means of
> searching in shared memory.  AFAICS the code can only do something
> useful in a platform where allocations made in the postmaster process
> can be found by backends via fork inheritance of pointers.

Rats!  You are right.  I had quite overlooked the windows case.

> The right way to handle shmem allocations is to use ShmemInitStruct
> to either allocate a shared struct for the first time or attach to a
> previously made instance of the struct.  (This "struct" could be a
> memory allocation arena itself, but that's not the core code's problem.)
> Now we could extend the patch so that each "addin" has its own
> ShmemIndex within its private workspace, but I think that's probably
> overkill.  My inclination is to rip out ShmemAllocFromContext and expect
> addins to use ShmemInitStruct the same as everyone else.  The hook
> callable at shared_preload_libraries time should just serve to add
> the necessary amount to the computed size of the shared memory segment.

I think that works for me.

> RegisterAddinLWLock is broken in the same way: it could only be used
> safely if the registered lock ID were remembered in shared memory,
> but since shared memory doesn't exist at the time it's supposed to be
> called, there's no way to do that.  Again, it'd seem to work as long as
> the lock ID value were inherited via fork, but that's gonna fail on
> EXEC_BACKEND builds.  I think we should probably take this out in favor
> of something that just increments a counter that replaces
> NUM_USER_DEFINED_LWLOCKS, and expect people to use LWLockAssign() at an
> appropriate time while initializing their shared memory areas.

Agreed.

> It strikes me that there's a race condition here, which we've not seen
> in previous use because backends expect all standard shared memory
> structs to have already been initialized by the postmaster.  An add-on
> will instead have to operate under the regime of "first backend wanting
> to use the struct must init it".  Although ShmemInitStruct returns a
> "found" bool telling you you've got to init it, there's no interlock
> ensuring that you can do so before someone else comes along and tries to
> use the struct (which he'll assume is done because he sees found = true).
> And, per above discussion, an add-on can't solve this for itself using
> an add-on LWLock, because it really has to acquire its add-on locks
> while initializing that same shmem struct, which is where it's going to
> keep the locks' identity :-(
>
> So I think we need to provide a standard LWLock reserved for the purpose
> of synchronizing first-time use of a shmem struct.  The coding rules for
> an add-on would then look like:
>
> * in the shared_preload_libraries hook:
>
>     RequestAddinShmemSpace(size);
>     RequestAddinLWLocks(n);
>
> * in a backend, to access a shared memory struct:
>
>     static mystruct *ptr = NULL;
>
>     if (!ptr)
>     {
>         bool    found;
>
>         LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
>         ptr = ShmemInitStruct("my struct name", size, &found);
>         if (!ptr)
>             elog(ERROR, "out of shared memory");
>         if (!found)
>         {
>             initialize contents of shmem area;
>             acquire any requested LWLocks using:
>             ptr->mylockid = LWLockAssign();
>         }
>         LWLockRelease(AddinShmemInitLock);
>     }
>
>
> Thoughts?

I am content that what you suggest is the right way to go.  I will work
on a new patch immediately, unless you would prefer to do this yourself.

It seems to me that these coding rules should be documented in the main
documentation, I guess in the section that describes Dynamic Loading.
Would you like me to take a stab at that?

__
Marc