Re: shared memory message queues

On 2013-12-05 14:07:39 -0500, Robert Haas wrote:
> On Thu, Dec 5, 2013 at 8:29 AM, Andres Freund <andres(at)2ndquadrant(dot)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