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

Reply via email to