Обсуждение: Re: [PATCHES] New shared memory hooks proposal (was Re: pre_load_libraries)
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. 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. 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. 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; acquireany requested LWLocks using: ptr->mylockid = LWLockAssign(); } LWLockRelease(AddinShmemInitLock);} Thoughts? regards, tom lane
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
Marc Munro <marc@bloodnok.com> writes: > 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. I've already got some of the relevant changes made in my local copy, so I might as well just go ahead and finish the coding work. But if you'd like to do the documentation part, be my guest. > It seems to me that these coding rules should be documented in the main > documentation, I guess in the section that describes Dynamic Loading. I'm not entirely sure where to put it, either. Subsection 32.9.1 (Dynamic Loading) is pretty basic stuff for first-time C-function authors --- this seems off-topic for that section. Maybe a new subsection down near the end of 32.9? Another possibility is somewhere in the Internals chapters, although I don't offhand see an obvious place there either. regards, tom lane