Re: Effect of changing the value for PARALLEL_TUPLE_QUEUE_SIZE

On Sun, Sep 17, 2017 at 9:10 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Wed, Sep 6, 2017 at 4:14 PM, Rafia Sabih
> <rafia(dot)sabih(at)enterprisedb(dot)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/