shared memory message queues
Right now, it's pretty hard to write code that does anything useful
with dynamic shared memory. Sure, you can allocate a dynamic shared
memory segment, and that's nice, but you won't get any help at all
figuring out what to store in it, or how to use it to communicate
effectively, which is not so nice. And some of the services we offer
around the main shared memory segment are conspicuously missing for
dynamic shared memory. The attached patches attempt to rectify some
of these problems. If you're not the patient type who wants to read
the whole email, patch #3 is the cool part.
Patch #1, on-dsm-detach-v1.patch, adds the concept of on_dsm_detach
hooks. These are basically like on_shmem_exit hooks, except that
detaching from a dsm can happen at any time, not just at backend exit.
But they're needed for the same reasons: when we detach from the main
shared memory segment, we need to make sure that we've released all
relevant locks, returned our PGPROC to the pool, etc. Dynamic shared
memory segments require the same sorts of cleanup when they contain
similarly complex data structures. The part of this patch which I
suppose will elicit some controversy is that I've had to rearrange
on_shmem_exit a bit. It turns out that during shmem_exit, we do
"user-level" cleanup, like aborting the transaction, first. We expect
that will probably release all of our shared-memory resources. Then,
just to make doubly sure, we do "low-level cleanup", where individual
modules return session-lifetime resources and make doubly sure that no
lwlocks, etc. have been leaked. on_dsm_exit callbacks properly happen
in the middle, after we've tried to abort the transaction but before
the main shared memory segment is finally shut down. I'm not sure
that the solution I've adopted here is optimal; see within for
details.
Patch #2, shm-toc-v1.patch, provides a facility for sizing a dynamic
shared memory segment before creation, and for dividing it up into
chunks after it's been created. It therefore serves a function quite
similar to RequestAddinShmemSpace, except of course that there is only
one main shared memory segment created at postmaster startup time,
whereas new dynamic shared memory segments can come into existence on
the fly; and it serves even more conspicuously the function of
ShmemIndex, which enables backends to locate particular data
structures within the shared memory segment. It is however quite a
bit simpler than the ShmemIndex mechanism: we don't need the same
level of extensibility here that we do for the main shared memory
segment, because a new extension need not piggyback on an existing
dynamic shared memory segment, but can create a whole segment of its
own.
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). Queues are single-reader and single-writer; they use process
latches to implement waiting for the queue to fill (in the case of the
reader) or drain (in the case of the writer). A non-blocking mode is
also available for situations where other options might lead to
deadlock. Even without this patch, backends can write messages to a
dynamic shared memory segment and wait for some other backend to read
them, but unless you know exactly how much data you want to send
before you create the shared memory segment, and unless you don't mind
storing all of it for the lifetime of the segment, you'll quickly run
into non-trivial problems around memory reuse and synchronization. So
this is an effort to create a higher-level infrastructure where one
process can simply declare that it wishes to a send series of messages
to a particular queue and another process can declare that it wishes
to read them out of that queue, and so it happens.
As far as parallelism is concerned, I anticipate that this code will
be useful for at least two purposes: (1) propagating errors that occur
inside a worker process back to the user backend that initiated the
parallel operation; and (2) streaming tuples from a worker performing
one part of the query (a scan or join, say) back to the user backend
or another worker performing a different part of the same query. I
suspect that this code will find applications outside parallelism as
well.
Patch #4, test-shm-mq-v1.patch, is a demonstration of how to use the
various background worker and dynamic shared memory facilities
introduced over the course of the 9.4 release cycle, and the
facilities introduced by patches #1-#3 of this series, to actually do
something interesting. Specifically, it sets up a ring of processes
connected by shared message queues and relays a user-specified message
around the ring repeatedly, then checks that it has the same message
at the end. This is obviously just a demonstration, but I find it
pretty cool, because the code here demonstrates that, with all of
these facilities in place, setting up a bunch of workers and having
them talk to each other can be done using what is really a pretty
modest amount of code. Importantly, this patch shows how to make the
start-up and shut-down sequences reliable, so that you don't end up
with the user backend hanging forever waiting for a worker that has
already died or will never start, or a worker backend waiting for a
user backend that has already aborted. Review of this logic is
particularly appreciated, as it's proven to be pretty complex: I think
the solutions I've worked out here are generally good, but there may
still be holes to plug. My hope is that people will take this test
code and use it as a basis for real applications. Including this
patch in our distribution will also serve as a useful regression test
of dynamic background workers and dynamic shared memory, which has so
far been lacking.
Particular thanks are due to Noah Misch for serving as my constant
sounding board during the development of this patch series.
Thanks,
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
on-dsm-detach-v1.patchtext/x-patch; charset=US-ASCII; name=on-dsm-detach-v1.patchDownload+268-81
shm-toc-v1.patchtext/x-patch; charset=US-ASCII; name=shm-toc-v1.patchDownload+288-1
shm-mq-v1.patchtext/x-patch; charset=US-ASCII; name=shm-mq-v1.patchDownload+926-1
test-shm-mq-v1.patchtext/x-patch; charset=US-ASCII; name=test-shm-mq-v1.patchDownload+932-0
This patch needs to be rebased.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 6, 2013 at 9:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
This patch needs to be rebased.
Thanks. You didn't mention which of the four needed rebasing, but it
seems that it's just the first one. New version of just that patch
attached; please use the prior versions of the remaining three.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
on-dsm-detach-v2.patchtext/x-patch; charset=US-ASCII; name=on-dsm-detach-v2.patchDownload+268-81
Hello,
I tried to look at the patch #1 and #2 at first, but I shall rest of
portion later.
* basic checks
All the patches (not only #1, #2) can be applied without any problem towards
the latest master branch. Its build was succeeded with Werror.
Regression test works fine on the core and contrib/test_shm_mq.
* on-dsm-detach-v2.patch
It reminded me the hook registration/invocation mechanism on apache/httpd.
It defines five levels for invocation order (REALLY_FIRST, FIRST, MIDDLE,
LAST, REALLY_LAST), but these are alias of integer values, in other words,
they are just kicks the hook in order to the priority value associated with a
function pointer.
These flexibility may make sense. We may want to control the order to
release resources more fine grained in the future. For example, isn't it
a problem if we have only two levels when a stuff in a queue needs to be
released earlier than the queue itself?
I'm not 100% certain on this suggestion because on_shmen_exit is a hook
that does not host so much callbacks, thus extension may implement
their own way on the SHMEM_EXIT_EARLY or SHMEM_EXIT_LATE stage
of course.
* shm-toc-v1.patch
From my experience, it makes sense to put a magic number on the tail of
toc segment to detect shared-memory usage overrun. It helps to find bugs
bug hard to find because of concurrent jobs.
Probably, shm_toc_freespace() is a point to put assertion.
How many entries is shm_toc_lookup() assumed to chain?
It assumes a liner search from the head of shm_toc segment, and it is
prerequisite for lock-less reference, isn't it?
Does it make a problem if shm_toc host many entries, like 100 or 1000?
Or, it is not an expected usage?
#3 and #4 should be looked later...
Thanks,
2013/11/8 Robert Haas <robertmhaas@gmail.com>:
On Wed, Nov 6, 2013 at 9:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
This patch needs to be rebased.
Thanks. You didn't mention which of the four needed rebasing, but it
seems that it's just the first one. New version of just that patch
attached; please use the prior versions of the remaining three.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 19, 2013 at 12:33 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
* on-dsm-detach-v2.patch
It reminded me the hook registration/invocation mechanism on apache/httpd.
It defines five levels for invocation order (REALLY_FIRST, FIRST, MIDDLE,
LAST, REALLY_LAST), but these are alias of integer values, in other words,
they are just kicks the hook in order to the priority value associated with a
function pointer.
These flexibility may make sense. We may want to control the order to
release resources more fine grained in the future. For example, isn't it
a problem if we have only two levels when a stuff in a queue needs to be
released earlier than the queue itself?
I'm not 100% certain on this suggestion because on_shmen_exit is a hook
that does not host so much callbacks, thus extension may implement
their own way on the SHMEM_EXIT_EARLY or SHMEM_EXIT_LATE stage
of course.
I don't really see much point in adding more flexibility here until we
need it, but I can imagine that we might someday need it, for reasons
that are not now obvious.
* shm-toc-v1.patch
From my experience, it makes sense to put a magic number on the tail of
toc segment to detect shared-memory usage overrun. It helps to find bugs
bug hard to find because of concurrent jobs.
Probably, shm_toc_freespace() is a point to put assertion.How many entries is shm_toc_lookup() assumed to chain?
It assumes a liner search from the head of shm_toc segment, and it is
prerequisite for lock-less reference, isn't it?
Does it make a problem if shm_toc host many entries, like 100 or 1000?
Or, it is not an expected usage?
It is not an expected usage. In typical usage, I expect that the
number of TOC entries will be about N+K, where K is a small constant
(< 10) and N is the number of cooperating parallel workers. It's
possible that we'll someday be in a position to leverage 100 or 1000
parallel workers on the same task, but I don't expect it to be soon.
And, actually, I doubt that revising the data structure would pay off
at N=100. At N=1000, maybe. At N=10000, probably. But we are
*definitely* not going to need that kind of scale any time soon, and I
don't think it makes sense to design a complex data structure to
handle that case when there are so many more basic problems that need
to be solved before we can go there.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
Planned to look at this for a while... Not a detailed review, just some
thoughts. I'll let what I read sink in and possibly comment later.
On 2013-10-31 12:21:31 -0400, Robert Haas wrote:
The attached patches attempt to rectify some of these problems.
Well, I wouldn't call it problems. Just natural, incremental development
;)
Patch #1, on-dsm-detach-v1.patch, adds the concept of on_dsm_detach
hooks
[snip]
The part of this patch which I
suppose will elicit some controversy is that I've had to rearrange
on_shmem_exit a bit. It turns out that during shmem_exit, we do
"user-level" cleanup, like aborting the transaction, first. We expect
that will probably release all of our shared-memory resources.
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?
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()?
Patch #2, shm-toc-v1.patch, provides a facility for sizing a dynamic
shared memory segment before creation, and for dividing it up into
chunks after it's been created. It therefore serves a function quite
similar to RequestAddinShmemSpace, except of course that there is only
one main shared memory segment created at postmaster startup time,
whereas new dynamic shared memory segments can come into existence on
the fly; and it serves even more conspicuously the function of
ShmemIndex, which enables backends to locate particular data
structures within the shared memory segment. It is however quite a
bit simpler than the ShmemIndex mechanism: we don't need the same
level of extensibility here that we do for the main shared memory
segment, because a new extension need not piggyback on an existing
dynamic shared memory segment, but can create a whole segment of its
own.
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?
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?
Queues are single-reader and single-writer; they use process
latches to implement waiting for the queue to fill (in the case of the
reader) or drain (in the case of the writer).
Hm. Too bad, I'd hoped for single-reader, multiple-writer :P
A non-blocking mode is also available for situations where other
options might lead to deadlock.
That's cool. I wonder if there's a way to discover, on the writing end,
when new data can be sent. For reading there's a comment about waiting
for the latch...
Couple of questions:
* Some introductory comments about the concurrency approach would be
good.
* shm_mq_attach() is talking about BackgroundWorkerHandles - is there a
limitation that a bgworker has to be involved?
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
Sorry for my late.
I checked the part-3 (shm-mq-v1.patc) portion as below.
Your comments towards part-1 and part-2 are reasonable for me,
so I have no argue on this portion.
Even though shm_mq_create() expects the "address" is aligned,
however, no mechanism to ensure. How about to put Assert() here?
shm_mq_send_bytes() has an Assert() to check the length to be
added to mq_bytes_written. Is the MAXALIGN64() right check in
32-bit architecture?
The sendnow value might be Min(ringsize - used, nbytes - sent),
and the ringsize came from mq->mq_ring_size being aligned
using MAXALIGN(_DOWN) that has 32bit width.
/*
* Update count of bytes written, with alignment padding. Note
* that this will never actually insert any padding except at the
* end of a run of bytes, because the buffer size is a multiple of
* MAXIMUM_ALIGNOF, and each read is as well.
*/
Assert(sent == nbytes || sendnow == MAXALIGN64(sendnow));
shm_mq_inc_bytes_written(mq, MAXALIGN64(sendnow));
What will happen if sender tries to send a large chunk that needs to
be split into multiple sub-chunks and receiver concurrently detaches
itself from the queue during the writes by sender?
It seems to me the sender gets SHM_MQ_DETACHED and only
earlier half of the chunk still remains on the queue even though
its total length was already in the message queue.
It may eventually lead infinite loop on the receiver side when another
receiver appeared again later, then read incomplete chunk.
Does it a feasible scenario? If so, it might be a solution to prohibit
enqueuing something without receiver, and reset queue when a new
receiver is attached.
The source code comments in shm_mq_wait_internal() is a little bit
misleading for me. It says nothing shall be written without attaching
the peer process, however, we have no checks as long as nsend is
less than queue size.
* If handle != NULL, the queue can be read or written even before the
* other process has attached. We'll wait for it to do so if needed. The
* handle must be for a background worker initialized with bgw_notify_pid
* equal to our PID.
Right now, that's all I can comment on. I'll do follow-up code reading
in the weekend.
Thanks,
2013/11/20 Robert Haas <robertmhaas@gmail.com>:
On Tue, Nov 19, 2013 at 12:33 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
* on-dsm-detach-v2.patch
It reminded me the hook registration/invocation mechanism on apache/httpd.
It defines five levels for invocation order (REALLY_FIRST, FIRST, MIDDLE,
LAST, REALLY_LAST), but these are alias of integer values, in other words,
they are just kicks the hook in order to the priority value associated with a
function pointer.
These flexibility may make sense. We may want to control the order to
release resources more fine grained in the future. For example, isn't it
a problem if we have only two levels when a stuff in a queue needs to be
released earlier than the queue itself?
I'm not 100% certain on this suggestion because on_shmen_exit is a hook
that does not host so much callbacks, thus extension may implement
their own way on the SHMEM_EXIT_EARLY or SHMEM_EXIT_LATE stage
of course.I don't really see much point in adding more flexibility here until we
need it, but I can imagine that we might someday need it, for reasons
that are not now obvious.* shm-toc-v1.patch
From my experience, it makes sense to put a magic number on the tail of
toc segment to detect shared-memory usage overrun. It helps to find bugs
bug hard to find because of concurrent jobs.
Probably, shm_toc_freespace() is a point to put assertion.How many entries is shm_toc_lookup() assumed to chain?
It assumes a liner search from the head of shm_toc segment, and it is
prerequisite for lock-less reference, isn't it?
Does it make a problem if shm_toc host many entries, like 100 or 1000?
Or, it is not an expected usage?It is not an expected usage. In typical usage, I expect that the
number of TOC entries will be about N+K, where K is a small constant
(< 10) and N is the number of cooperating parallel workers. It's
possible that we'll someday be in a position to leverage 100 or 1000
parallel workers on the same task, but I don't expect it to be soon.
And, actually, I doubt that revising the data structure would pay off
at N=100. At N=1000, maybe. At N=10000, probably. But we are
*definitely* not going to need that kind of scale any time soon, and I
don't think it makes sense to design a complex data structure to
handle that case when there are so many more basic problems that need
to be solved before we can go there.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 5, 2013 at 8:29 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Patch #1, on-dsm-detach-v1.patch, adds the concept of on_dsm_detach
hooks
[snip]
The part of this patch which I
suppose will elicit some controversy is that I've had to rearrange
on_shmem_exit a bit. It turns out that during shmem_exit, we do
"user-level" cleanup, like aborting the transaction, first. We expect
that will probably release all of our shared-memory resources.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.
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.
Patch #2, shm-toc-v1.patch, provides a facility for sizing a dynamic
shared memory segment before creation, and for dividing it up into
chunks after it's been created. It therefore serves a function quite
similar to RequestAddinShmemSpace, except of course that there is only
one main shared memory segment created at postmaster startup time,
whereas new dynamic shared memory segments can come into existence on
the fly; and it serves even more conspicuously the function of
ShmemIndex, which enables backends to locate particular data
structures within the shared memory segment. It is however quite a
bit simpler than the ShmemIndex mechanism: we don't need the same
level of extensibility here that we do for the main shared memory
segment, because a new extension need not piggyback on an existing
dynamic shared memory segment, but can create a whole segment of its
own.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.
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.
Queues are single-reader and single-writer; they use process
latches to implement waiting for the queue to fill (in the case of the
reader) or drain (in the case of the writer).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.
A non-blocking mode is also available for situations where other
options might lead to deadlock.That's cool. I wonder if there's a way to discover, on the writing end,
when new data can be sent. For reading there's a comment about waiting
for the latch...
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.
Couple of questions:
* Some introductory comments about the concurrency approach would be
good.
Not sure exactly what to write.
* shm_mq_attach() is talking about BackgroundWorkerHandles - is there a
limitation that a bgworker has to be involved?
No. The only point of that is that someone might want to start
writing into a message queue before the reader has connected, or
reading before the writer has connected. If they do that and the
counterparty never attaches, they'll hang forever. You can handle
that by writing your own code to make sure both parties have attached
before starting to read or write... but if you happen to have a
BackgroundWorkerHandle handy, you can also pass that to
shm_mq_attach() and it'll return an error code if the background
worker in question is determined to have stopped.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-12-05 14:07:39 -0500, Robert Haas wrote:
On Thu, Dec 5, 2013 at 8:29 AM, Andres Freund <andres@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
On Fri, Dec 6, 2013 at 8:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-12-05 14:07:39 -0500, Robert Haas wrote:
On Thu, Dec 5, 2013 at 8:29 AM, Andres Freund <andres@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?
Hmm, before_shmem_exit() and on_shmem_exit() doesn't sound bad.
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?
That stuff mostly pertains to shm_toc.c, not shm_mq.c. I can
certainly look at expanding the explanation in the former.
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.
I did initially implement it as lockless, but I figured I needed a
fallback with spinlocks for machines that didn't have atomic 8-bit
writes and/or memory barriers, both of which are required for this to
work without locks. When I implemented the fallback, I discovered
that it wasn't any slower than the lock-free implementation, so I
decided to simplify my life (and the code) by not bothering with it.
So the basic idea is that you have to take the spinlock to modify the
count of bytes read - if you're the reader - or written - if you're
the writer. You also need to take the spinlock to read the counter
the other process might be modifying, but you can read the counter
that only you can modify without the lock. You also don't need the
lock to read or write the data in the buffer, because you can only
write to the portion of the buffer that the reader isn't currently
allowed to read, and you can only read from the portion of the buffer
that the writer isn't currently allowed to write.
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.
I think you'd want to bump the generation every time the slot was
released rather than when it was reacquired, but it does sound
possibly sensible. However, it wouldn't obviate the separate need to
watch a BackgroundWorkerHandle, because what this lets you do is (say)
write to a queue after you've registered the reader but before it's
actually been started by the postmaster, let alone acquired a PGPROC.
I won't object if someone implements such a system for their needs,
but it's not on my critical path.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2013/12/6 Kohei KaiGai <kaigai@kaigai.gr.jp>:
What will happen if sender tries to send a large chunk that needs to
be split into multiple sub-chunks and receiver concurrently detaches
itself from the queue during the writes by sender?
It seems to me the sender gets SHM_MQ_DETACHED and only
earlier half of the chunk still remains on the queue even though
its total length was already in the message queue.
It may eventually lead infinite loop on the receiver side when another
receiver appeared again later, then read incomplete chunk.
Does it a feasible scenario? If so, it might be a solution to prohibit
enqueuing something without receiver, and reset queue when a new
receiver is attached.
Doesn't it an intended usage to attach a peer process on a message
queue that had once detached, does it?
If so, it may be a solution to put ereport() on shm_mq_set_receiver()
and shm_mq_set_sender() to prohibit to assign a process on the
message queue with mq_detached = true. It will make the situation
simplified.
Regarding to the test-shm-mq-v1.patch, setup_background_workers()
tries to launch nworkers of background worker processes, however,
may fail during the launching if max_worker_processes is not enough.
Is it a situation to attach the BGWORKER_EPHEMERAL flag when
your patch gets committed, isn't it?
I think it is waste of efforts to add error handling here instead of the
core support to be added, however, it makes sense to put a source
code comment not to forget to add this flag when it came.
Also, test_shm_mq_setup() waits for completion of starting up of
background worker processes. I'm uncertain whether it is really
needed, because this shared memory message queue allows to
send byte stream without receiver, and also blocks until byte
stream will come from the peer to be set later.
This module is designed for test purpose, so I think it makes more
sense if test condition is more corner case.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Dec 8, 2013 at 5:52 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
2013/12/6 Kohei KaiGai <kaigai@kaigai.gr.jp>:
What will happen if sender tries to send a large chunk that needs to
be split into multiple sub-chunks and receiver concurrently detaches
itself from the queue during the writes by sender?
It seems to me the sender gets SHM_MQ_DETACHED and only
earlier half of the chunk still remains on the queue even though
its total length was already in the message queue.
It may eventually lead infinite loop on the receiver side when another
receiver appeared again later, then read incomplete chunk.
Does it a feasible scenario? If so, it might be a solution to prohibit
enqueuing something without receiver, and reset queue when a new
receiver is attached.Doesn't it an intended usage to attach a peer process on a message
queue that had once detached, does it?
If so, it may be a solution to put ereport() on shm_mq_set_receiver()
and shm_mq_set_sender() to prohibit to assign a process on the
message queue with mq_detached = true. It will make the situation
simplified.
It's not intended that you should be able to attach a new reader or
writer in place of an old one that detached. That would in fact be
pretty tricky to support, because if the detached process was in the
middle of reading or writing a message at the time it died, then
there's no way to recover protocol sync. We could design some
mechanism for that, but in the case of background workers connected to
dynamic shared memory segments it isn't needed, because I assume that
when the background worker croaks, you're going to tear down the
dynamic shared memory segment and thus the whole queue will disappear;
if the user retries the query, we'll create a whole new segment
containing a whole new queue (or queues).
Now, if we wanted to use these queues in permanent shared memory, we'd
probably need to think a little bit harder about this. It is not
impossible to make it work even as things stand, because you could
reuse the same chunk of shared memory and just overwrite it with a
newly-initialized queue. You'd need some mechanism to figure out when
to do that, and it might be kind of ugly, but I think i'd be doable.
That wasn't the design center for this attempt, though, and if we want
to use it that way then we probably should spend some time figuring
out how to support both a "clean" detach, where the reader or writer
goes away at a message boundary, and possibly also a "dirty" detach,
where the reader or writer goes away in the middle of a message. I
view those as problems for future patches, though.
Regarding to the test-shm-mq-v1.patch, setup_background_workers()
tries to launch nworkers of background worker processes, however,
may fail during the launching if max_worker_processes is not enough.
Is it a situation to attach the BGWORKER_EPHEMERAL flag when
your patch gets committed, isn't it?
I dropped the proposal for BGWORKER_EPHEMERAL; I no longer think we
need that. If not all of the workers can be registered,
setup_background_workers() will throw an error when
RegisterDynamicBackgroundWorker returns false. If the workers are
successfully registered but don't get as far as connecting to the
shm_mq, wait_for_workers_to_become_ready() will detect that condition
and throw an error. If all of the workers start up and attached to
the shared memory message queues but then later one of them dies, the
fact that it got as far as connecting to the shm_mq means that the
message queue's on_dsm_detach callback will run, which will mark the
queues to which it is connected as detached. That will cause the
workers on either side of it to exit also until eventually the failure
propagates back around to the user backend. This is all a bit complex
but I don't see a simpler solution.
Also, test_shm_mq_setup() waits for completion of starting up of
background worker processes. I'm uncertain whether it is really
needed, because this shared memory message queue allows to
send byte stream without receiver, and also blocks until byte
stream will come from the peer to be set later.
That's actually a very important check. Suppose we've got just 3
worker processes, so that the message queues are connected like this:
user backend -> worker 1 -> worker 2 -> worker 3 -> user backend
When test_shm_mq_setup connects to the queues linking it to worker 1
and worker 3, it passes a BackgroundWorkerHandle to shm_mq_attach. As
a result, if either worker 1 or worker 3 fails during startup, before
attaching to the queue, the user backend would notice that and error
out right away, even if it didn't do
wait_for_workers_to_become_ready(). However, if worker 2 fails during
startup, neither the user backend nor either of the other workers
would notice that without wait_for_workers_to_become_ready(): the user
backend isn't connected to worker 2 by a shm_mq at all, and workers 1
and 3 have no BackgroundWorkerHandle to pass to shm_mq_attach(),
because they're not the process that registered worker 2 in the first
place. So everything would just hang. The arrangement I've actually
got here should ensure that no matter how many workers you have and
which ones die at what point in their life cycle, everything will shut
down properly. If that's not the case, it's a bug.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 7, 2013 at 3:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
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?Hmm, before_shmem_exit() and on_shmem_exit() doesn't sound bad.
So here's an updated patch that takes that approach. It has a
substantially reduced footprint compared to the previous version, and
probably less chance of breaking third-party code. I also incorporated
your suggestion of renaming on_dsm_detach_cancel to
cancel_on_dsm_detach.
Unless anyone wants to further kibitz the naming here, I'm thinking
this part is ready to commit. I'll rebase and update the remaining
patches after that's done.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
on-dsm-detach-v3.patchtext/x-patch; charset=US-ASCII; name=on-dsm-detach-v3.patchDownload+192-46
On Tue, Dec 17, 2013 at 11:51 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Unless anyone wants to further kibitz the naming here, I'm thinking
this part is ready to commit. I'll rebase and update the remaining
patches after that's done.
OK, so I committed that. Per Andres's gripe about lack of overall
design documentation, I added several large comment blocks to
shm-mq-v1.patch; the updated version is here attached as
shm-mq-v2.patch. test-shm-mq-v1.patch no longer compiles due to the
naming changes in that patch, so here's test-shm-mq-v2.patch with a
one-line change to compensate. These are intended to apply on top of
shm-toc-v1.patch, which I am not reposting here since it's unchanged,
and I'm unaware of any fixes that are needed there except possibly for
some further elaboration of the comments.
It sounds like most people who have looked at this stuff are broadly
happy with it, so I'd like to push on toward commit soon, but it'd be
helpful, Andres, if you could review the comment additions to
shm-mq-v2.patch and see whether those address your concerns. If so,
I'll see about improving the overall comments for shm-toc-v1.patch as
well to clarify the points that seem to have caused a bit of
confusion; specific thoughts on what ought to be covered there, or any
other review, is most welcome.
Thanks,
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2013-12-18 15:23:23 -0500, Robert Haas wrote:
It sounds like most people who have looked at this stuff are broadly
happy with it, so I'd like to push on toward commit soon, but it'd be
helpful, Andres, if you could review the comment additions to
shm-mq-v2.patch and see whether those address your concerns.
FWIW, I haven't looked carefully enough at the patch to consider myself
having reviewed it. I am not saying that it's not ready for committer,
just that I don't know whether it is.
I will have a look at the comment improvements, and if you don't beat me
to it, give the patch a read-over.
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
On 2013-10-31 12:21:31 -0400, Robert Haas wrote:
Patch #2, shm-toc-v1.patch, provides a facility for sizing a dynamic
shared memory segment before creation, and for dividing it up into
chunks after it's been created. It therefore serves a function quite
similar to RequestAddinShmemSpace, except of course that there is only
one main shared memory segment created at postmaster startup time,
whereas new dynamic shared memory segments can come into existence on
the fly; and it serves even more conspicuously the function of
ShmemIndex, which enables backends to locate particular data
structures within the shared memory segment. It is however quite a
bit simpler than the ShmemIndex mechanism: we don't need the same
level of extensibility here that we do for the main shared memory
segment, because a new extension need not piggyback on an existing
dynamic shared memory segment, but can create a whole segment of its
own.
So, without the argument of having per-extension dsm segments, I'd say
that a purely integer key sucks, because it's hard to manage and
debug. This way it's still not too nice, but I don't see a all that good
alternative.
Comments about shm-toc-v1.patch:
Since you're embedding spinlocks in struct shm_toc, this module will be
in conflict with platforms that do --disable-spinlocks, since the number
of spinlocks essentially needs to be predetermined there. I personally
still think the solution to that is getting rid of --disable-spinlocks.
I vote for removing all the shm_toc_estimator() knowledge from the
header, there seems little reason to expose it that way. That just
exposes unneccessary details and makes fixes after releases harder
(requiring extensions to recompile). Function call costs hardly can
matter, right?
Do you assume that lookup speed isn't that important? I have a bit of a
hard time imagining that linear search over the keys isn't going to bite
us. At the least there should be a comment somewhere documenting that
the indented usecase is having a relatively few keys.
Wouldn't it make sense to have a function that does the combined job of
shm_toc_insert() and shm_toc_allocate()?
What's the assumption about the use of the magic in create/attach? Just
statically defined things in user code?
Ok, cooking now, then I'll have a look at the queue itself,
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
On Fri, Dec 20, 2013 at 1:11 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-31 12:21:31 -0400, Robert Haas wrote:
Patch #2, shm-toc-v1.patch, provides a facility for sizing a dynamic
shared memory segment before creation, and for dividing it up into
chunks after it's been created. It therefore serves a function quite
similar to RequestAddinShmemSpace, except of course that there is only
one main shared memory segment created at postmaster startup time,
whereas new dynamic shared memory segments can come into existence on
the fly; and it serves even more conspicuously the function of
ShmemIndex, which enables backends to locate particular data
structures within the shared memory segment. It is however quite a
bit simpler than the ShmemIndex mechanism: we don't need the same
level of extensibility here that we do for the main shared memory
segment, because a new extension need not piggyback on an existing
dynamic shared memory segment, but can create a whole segment of its
own.So, without the argument of having per-extension dsm segments, I'd say
that a purely integer key sucks, because it's hard to manage and
debug. This way it's still not too nice, but I don't see a all that good
alternative.Comments about shm-toc-v1.patch:
Since you're embedding spinlocks in struct shm_toc, this module will be
in conflict with platforms that do --disable-spinlocks, since the number
of spinlocks essentially needs to be predetermined there. I personally
still think the solution to that is getting rid of --disable-spinlocks.
Yep. We can either deprecate --disable-spinlocks, or we can add an
API that is the reverse of S_INIT_LOCK().
I vote for removing all the shm_toc_estimator() knowledge from the
header, there seems little reason to expose it that way. That just
exposes unneccessary details and makes fixes after releases harder
(requiring extensions to recompile). Function call costs hardly can
matter, right?
Oh, you're saying to make it a function rather than a macro? Sure, I
could do that.
Do you assume that lookup speed isn't that important? I have a bit of a
hard time imagining that linear search over the keys isn't going to bite
us. At the least there should be a comment somewhere documenting that
the indented usecase is having a relatively few keys.
Well, as previously noted, in the use cases I imagine for this, it'll
be nworkers + somesmallconstant. I can add a comment to that effect.
Wouldn't it make sense to have a function that does the combined job of
shm_toc_insert() and shm_toc_allocate()?
We could, but I don't really feel compelled. It's not hard to call
them individually if that's the behavior you happen to want.
What's the assumption about the use of the magic in create/attach? Just
statically defined things in user code?
Yeah.
Ok, cooking now, then I'll have a look at the queue itself,
Thanks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-12-20 14:10:57 -0500, Robert Haas wrote:
Since you're embedding spinlocks in struct shm_toc, this module will be
in conflict with platforms that do --disable-spinlocks, since the number
of spinlocks essentially needs to be predetermined there. I personally
still think the solution to that is getting rid of --disable-spinlocks.Yep. We can either deprecate --disable-spinlocks, or we can add an
API that is the reverse of S_INIT_LOCK().
How is that going to help? Even if we'd deallocate unused spinlocks -
which sure is a good idea when they require persistent resources like
the PGSemaphore based one - the maximum is still going to be there.
I vote for removing all the shm_toc_estimator() knowledge from the
header, there seems little reason to expose it that way. That just
exposes unneccessary details and makes fixes after releases harder
(requiring extensions to recompile). Function call costs hardly can
matter, right?Oh, you're saying to make it a function rather than a macro? Sure, I
could do that.
Yea, keeping it private, as a function, seems like a good idea.
Do you assume that lookup speed isn't that important? I have a bit of a
hard time imagining that linear search over the keys isn't going to bite
us. At the least there should be a comment somewhere documenting that
the indented usecase is having a relatively few keys.Well, as previously noted, in the use cases I imagine for this, it'll
be nworkers + somesmallconstant. I can add a comment to that effect.
I primarily was wondering because you have gone through a bit of effort
making it lockless. A comment would be good, yes.
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
On Fri, Dec 20, 2013 at 2:33 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-12-20 14:10:57 -0500, Robert Haas wrote:
Since you're embedding spinlocks in struct shm_toc, this module will be
in conflict with platforms that do --disable-spinlocks, since the number
of spinlocks essentially needs to be predetermined there. I personally
still think the solution to that is getting rid of --disable-spinlocks.Yep. We can either deprecate --disable-spinlocks, or we can add an
API that is the reverse of S_INIT_LOCK().How is that going to help? Even if we'd deallocate unused spinlocks -
which sure is a good idea when they require persistent resources like
the PGSemaphore based one - the maximum is still going to be there.
Well, we can set the maximum to a bigger number, if that's what we
want to do, but I'll admit it's unclear what value would be
sufficient. We probably won't have more than one TOC per DSM, and the
maximum number of DSMs is capped based on MaxBackends, but it seems
likely that many applications of dynamic shared memory will require
spinlocks, and I don't think we can plausibly calculate an upper bound
there. If we could it would be a big number, and that might just make
startup fail anyway.
Personally, I don't have a big problem with the idea that if you
compile with --disable-spinlocks, some things may just fail, and
parallel whatever might be one of those things. We could just bump
the "30" in SpinlockSemas to "100", and if you happen to use more than
that, too bad for you: it'll fail.
But I also don't have a big problem with removing --disable-spinlocks
altogether. What do other people think?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2013-12-18 15:23:23 -0500, Robert Haas wrote:
It sounds like most people who have looked at this stuff are broadly
happy with it, so I'd like to push on toward commit soon, but it'd be
helpful, Andres, if you could review the comment additions to
shm-mq-v2.patch and see whether those address your concerns. If so,
I'll see about improving the overall comments for shm-toc-v1.patch as
well to clarify the points that seem to have caused a bit of
confusion; specific thoughts on what ought to be covered there, or any
other review, is most welcome.
Some things:
* shm_mq_minimum_size should be const
* the MAXALIGNing in shm_mq_create looks odd. We're computing
data_offset using MAXALIGN, determine the ring size using it, but then
set mq_ring_offset to data_offset - offsetof(shm_mq, mq_ring)?
* same problem as the toc stuff with a dynamic number of spinnlocks.
* you don't seem to to initialize the spinlock anywhere. That's clearly
not ok, independent of the spinlock implementation.
* The comments around shm_mq/shm_mq_handle are a clear improvement. I'd
be pretty happy if you additionally add someting akin to 'This struct
describes the shared state of a queue' and 'Backend-local reference to
a queue'.
* s/MQH_BUFSIZE/MQH_INITIAL_BUFSIZE/, I was already wondering whether
there's a limit on the max number of bytes.
* I think shm_mq_attach()'s documentation should mention that we'll
initially and later allocate memory in the current
CurrentMemoryContext when attaching. That will likely require some
care for some callers. Yes, it's documented in a bigger comment
somewhere else, but still.
* I don't really understand the mqh_consume_pending stuff. If we just
read a message spanning from the beginning to the end of the
ringbuffer, without wrapping, we might not confirm the read in
shm_mq_receive() until the next the it is called? Do I understand that
correctly?
I am talking about the following and other similar pieces of code:
+ if (rb >= needed)
+ {
+ /*
+ * Technically, we could consume the message length information at
+ * this point, but the extra write to shared memory wouldn't be
+ * free and in most cases we would reap no benefit.
+ */
+ mqh->mqh_consume_pending = needed;
+ *nbytesp = nbytes;
+ *datap = ((char *) rawdata) + MAXALIGN64(sizeof(uint64));
+ return SHM_MQ_SUCCESS;
+ }* ISTM the messages around needing to use the same arguments for
send/receive again after SHM_MQ_WOULD_BLOCK could stand to be more
forceful. "In this case, the caller should call this function again
after the process latch has been set." doesn't make it all that clear
that failure to do so will corrupt the next message. Maybe there also
should be an assert checking that the size is the same when retrying
as when starting a partial read?
* You have some CHECK_FOR_INTERRUPTS(), are you sure the code is safe to
longjmp out from there in all the cases? E.g. I am not sure it is for
shm_mq_send_bytes(), when we've first written a partial message, done
a shm_mq_inc_bytes_written() and then go into the available == 0
branch and jump out.
* Do I understand it correctly that mqh->mqh_counterparty_attached is
just sort of a cache, and we'll still detect a detached counterparty
when we're actually sending/receiving?
That's it for now.
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
