On Sun, Sep 17, 2017 at 9:10 PM, Dilip Kumar <dilipbal...@gmail.com> wrote: > On Wed, Sep 6, 2017 at 4:14 PM, Rafia Sabih > <rafia.sa...@enterprisedb.com> wrote: > >> I worked on this idea of using local queue as a temporary buffer to >> write the tuples when master is busy and shared queue is full, and it >> gives quite some improvement in the query performance. >> > > I have done some initial review of this patch and I have some comments. > Thanks for the review.
> /* this is actual size for this tuple which will be written in queue */ > + tuple_size = MAXALIGN(sizeof(Size)) + MAXALIGN(iov.len); > + > + /* create and attach a local queue, if it is not yet created */ > + if (mqh->mqh_local_queue == NULL) > + mqh = local_mq_attach(mqh); > > I think we can create the local queue when first time we need it. So > basically you > can move this code inside else part where we first identify that there is no > space in the shared queue. > Done. > ------ > + /* write in local queue if there is enough space*/ > + if (local_space > tuple_size) > > I think the condition should be if (local_space >= tuple_size) > I did this to be on the safer side, anyhow modified. > ------ > + while(shm_space <= 0) > + { > + if (shm_mq_is_detached(mqh->mqh_queue)) > + return SHM_MQ_DETACHED; > + > + shm_space = space_in_shm(mqh->mqh_queue); > + } > > Instead of waiting in CPU intensive while loop we should wait on some latch, > why > can't we use wait latch of the shared queue and whenever some space > free, the latch will > be set then we can recheck the space and if it's enough we can write > to share queue > otherwise wait on the latch again. > > Check other similar occurrences. Done. > --------- > > + if (read_local_queue(lq, true) && shm_space > 0) > + copy_local_to_shared(lq, mqh, false); > + > > Instead of adding shm_space > 0 in check it can be Assert or nothing > because above loop will > only break if shm_space > 0. > ---- Done > > + /* > + * create a local queue, the size of this queue should be way higher > + * than PARALLEL_TUPLE_QUEUE_SIZE > + */ > + char *mq; > + Size len; > + > + len = 6553600; > > Create some macro which is multiple of PARALLEL_TUPLE_QUEUE_SIZE, > Done. > ------- > > + /* this local queue is not required anymore, hence free the space. */ > + pfree(mqh->mqh_local_queue); > + return; > +} > > I think you can remove the return at the end of the void function. > ----- Done. > > In empty_queue(shm_mq_handle *mqh) function I see that only last exit path > frees > the local queue but not all even though local queue is created. > ---- > Modified. > Other cosmetic issues. > ----------------------------- > +/* check the space availability in local queue */ > +static uint64 > +space_in_local(local_mq *lq, Size tuple_size) > +{ > + uint64 read, written, used, available, ringsize, writer_offset, > reader_offset; > + > + ringsize = lq->mq_ring_size; > + read = lq->mq_bytes_read; > + written = lq->mq_bytes_written; > + used = written - read; > + available = ringsize - used; > + > Alignment is not correct, I think you can run pgindent once. > ---- > > + /* check is there is required space in shared queue */ > statement need refactoring. "check if there is required space in shared > queue" ? > > ----- > > + /* write in local queue if there is enough space*/ > space missing before comment end. > > ----- > > + > +/* Routines required for local queue */ > + > +/* > + * Initialize a new local message queue, this is kept quite similar > to shm_mq_create. > + */ > > Header comments formatting is not in sync with other functions. > > ----- > > +} > +/* routine to create and attach local_mq to the shm_mq_handle */ > > one blank line between two functions. > > ----- > Ran pgindent for these issues. > + bool detached; > + > + detached = false; > > a better way is bool detached = false; > > ----- > Done. > Compilation warning > -------------------- > shm_mq.c: In function ‘write_in_local_queue’: > shm_mq.c:1489:32: warning: variable ‘tuple_size’ set but not used > [-Wunused-but-set-variable] > uint64 bytes_written, nbytes, tuple_size; > That might be in case not configured with cassert, however, it is removed in current version anyway. Please find the attached file for the revised version. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
faster_gather_v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers