On 2013-12-05 14:07:39 -0500, Robert Haas wrote: > On Thu, Dec 5, 2013 at 8:29 AM, Andres Freund <and...@2ndquadrant.com> wrote: > > Hm. The API change of on_shmem_exit() is going to cause a fair bit of > > pain. There are a fair number of extensions out there using it and all > > would need to be littered by version dependent #ifdefs. What about > > providing a version of on_shmem_exit() that allows to specify the exit > > phase, and make on_shmem_exit() a backward compatible wrapper? > > Or, we could have on_user_exit() or so and leave on_shmem_exit() alone > altogether, which would probably be transparent for nearly everyone. > I kind of like that better, except that I'm not sure whether > on_user_exit() is any good as a name.
Leaving it as separate calls sounds good to me as well - but like you I don't like on_user_exit() that much. Not sure if I can up with something really good... on_shmem_exit_phase() or at_shmem_exit_phase() if we go for a function allowing to specify phases, and just before_shmem_exit() for the "user level" things? > > FWIW, I find on_dsm_detach_cancel() to be a confusing name. I thought it > > might be a variant that is called in different circumstances than > > on_dsm_detach(). Maybe go with cancel_on_shmem_detach(), like with > > cancel_shmem_exit()? > > It could be cancel_on_dsm_detach() if you like that better. Not > cancel_on_shmem_detach(), though. Yes, I like that better. The shm instead of dsm was just a thinko ,) > > Hm. A couple of questions/comments: > > * How do you imagine keys to be used/built? > > * Won't the sequential search over toc entries become problematic? > > * Some high level overview of how it's supposed to be used wouldn't > > hurt. > > * the estimator stuff doesn't seem to belong in the public header? > > The test-shm-mq patch is intended to illustrate these points. In that > case, for an N-worker configuration, there are N+1 keys; that is, N > message queues and one fixed-size control area. The estimator stuff > is critically needed in the public header so that you can size your > DSM to hold the stuff you intend to store in it; again, see > test-shm-mq. I still think shm_mq.c needs to explain more of that. That's where I look for a brief high-level explanation, no? > >> Patch #3, shm-mq-v1.patch, is the heart of this series. It creates an > >> infrastructure for sending and receiving messages of arbitrary length > >> using ring buffers stored in shared memory (presumably dynamic shared > >> memory, but hypothetically the main shared memory segment could be > >> used). > > > > The API seems to assume it's in dsm tho? > > The header file makes no reference to dsm anywhere, so I'm not sure > why you draw this conclusion. The reason I was asking was this reference to dsm: +shm_mq_handle * +shm_mq_attach(shm_mq *mq, dsm_segment *seg, BackgroundWorkerHandle *handle) I'd only looked at the header at that point, but looking at the function's comment it's otional. > > Hm. Too bad, I'd hoped for single-reader, multiple-writer :P > > Sure, that might be useful, too, as might multiple-reader, > multi-writer. But those things would come with performance and > complexity trade-offs of their own. I think it's appropriate to leave > the task of creating those things to the people that need them. If it > turns out that this can be enhanced to work like that without > meaningful loss of performance, that's fine by me, too, but I think > this has plenty of merit as-is. Yea, it's perfectly fine not to implement what I wished for ;). I just had hoped you would magically develop what I was dreaming about... > It's symmetric. The idea is that you try to read or write data; > should that fail, you wait on your latch and try again when it's set. Ok, good. That's what I thought. > > Couple of questions: > > * Some introductory comments about the concurrency approach would be > > good. > > Not sure exactly what to write. I had a very quick look at shm_mq_receive()/send() and shm_mq_receive_bytes()/shm_mq_send_bytes() - while the low level steps seem to be explained fairly well, the high level design of the queue doesn't seem to be explained much. I was first thinking that you were trying to implement a lockless queue (which is quite efficiently possible for 1:1 queues) even because I didn't see any locking in them till I noticed it's just delegated to helper functions. > > * shm_mq_attach() is talking about BackgroundWorkerHandles - is there a > > limitation that a bgworker has to be involved? > [sensible stuff] > > Possibly there could be some similar mechanism to wait for a > non-background-worker to stop, but I haven't thought much about what > that would look like. I wonder if we couldn't just generally store a "generation" number for each PGPROC which is increased everytime the slot is getting reused. Then one could simply check whether mq->mq_sender->generation == mq->mq_sender_generation. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
