Re: [HACKERS] Effect of changing the value for PARALLEL_TUPLE_QUEUE_SIZE
От | Rafia Sabih |
---|---|
Тема | Re: [HACKERS] Effect of changing the value for PARALLEL_TUPLE_QUEUE_SIZE |
Дата | |
Msg-id | CAOGQiiMOWJwfaegpERkvv3t6tY2CBdnhWHWi1iCfuMsCC98a4g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Effect of changing the value for PARALLEL_TUPLE_QUEUE_SIZE (Dilip Kumar <dilipbalaut@gmail.com>) |
Список | pgsql-hackers |
On Thu, Sep 21, 2017 at 10:34 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Sep 21, 2017 at 4:50 PM, Rafia Sabih > <rafia.sabih@enterprisedb.com> wrote: >> On Sun, Sep 17, 2017 at 9:10 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >>> On Wed, Sep 6, 2017 at 4:14 PM, Rafia Sabih >>> <rafia.sabih@enterprisedb.com> wrote: >>> >> >> Please find the attached file for the revised version. > > Thanks for the updated patch, I have some more comments. > Again, thanks for the review. Please find the attached file for updated patch. > +static shm_mq_handle *local_mq_attach(shm_mq_handle *mqh); > +static uint64 space_in_local(local_mq * lq, Size tuple_size); > +static bool read_local_queue(local_mq * lq, bool shm_mq_full); > > local_mq * lq -> local_mq *lq > same for other places as well. > > --- > Done. This is something pgindent does, anyhow corrected it. > +static uint64 space_in_shm(shm_mq *mq); > + > +static uint64 space_in_local(local_mq * lq, Size tuple_size); > > we better use Size here instead if uint64 > > --- Done > > + available = ringsize - used; > + > + ringsize = lq->mq_ring_size; > + writer_offset = lq->mq_bytes_written % ringsize; > + reader_offset = lq->mq_bytes_read % ringsize; > + > + if (writer_offset + tuple_size < ringsize && reader_offset < writer_offset) > + available = (ringsize - writer_offset); > > even though there is space in queue but tuple need rotation then we > are not allowing it to > write into the local queue. If there is some strong reason behind > that, please add comments > to explain the same. > --- > Corrected, it will just return available space now. > + if (shm_mq_full || (written - read) >= .05 * lq->mq_ring_size) > + return true; > + > + else > + return true; > +} > > Seems like you want to return 'false' in the else case. > > ---- Yes and done. > > + read_offset = lq->mq_bytes_read % lq->mq_ring_size; > + available = space_in_shm(mqh->mqh_queue); > + > + /* always read data in the aligned form */ > + to_read = MAXALIGN_DOWN(Min(used, available)); > + > + /* > + * if the amount of data to be send from local queue involves wrapping of > + * local queue, then send only the data till the end of queue right now > + * and rest later. > + */ > + if (lq->mq_bytes_read % lq->mq_ring_size + to_read > lq->mq_ring_size) > > You can directly use "read_offset" instead of recalculating > lq->mq_bytes_read % lq->mq_ring_size. > > ---- Done. > + do > + { > + if (shm_mq_is_detached(mqh->mqh_queue)) > + return SHM_MQ_DETACHED; > + > + shm_space = space_in_shm(mqh->mqh_queue); > + > + /* > + * cannot send data to shared queue, unless there is required > + * space, so wait till we get some space, since we cannot > + * write anymore in local queue as of now > + */ > + WaitLatch(MyLatch, WL_LATCH_SET, 0, WAIT_EVENT_MQ_SEND); > + > + /* Reset the latch so we don't spin. */ > + ResetLatch(MyLatch); > + > + /* An interrupt may have occurred while we were waiting. */ > + CHECK_FOR_INTERRUPTS(); > + > + shm_space = space_in_shm(mqh->mqh_queue); > + > + if (read_local_queue(lq, true) && shm_space > 0) > + copy_local_to_shared(lq, mqh, false); > + > + local_space = space_in_local(lq, tuple_size); > + > + } while (local_space <= tuple_size); > + > > 1. Just after getting the shm_space, you are calling WaitLatch, > without even checking whether > that space is sufficient to send the tuple. > 2. Every time after latch is set (maybe some space freed in the shared > queue) you are calling > copy_local_to_shared to send as much data as possible from local to > shared queue, but that doesn't > even guarantee that we will have sufficient space in the local queue > to accommodate the current tuple. > > I think calling copy_local_to_shared multiple time (which will > internally acquire mutex), after latch > is set you can check the shared queue space, don't attempt > copy_local_to_shared unless > shm_space >=tuple_size > Done. > -- > Regards, > Dilip Kumar > EnterpriseDB: http://www.enterprisedb.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Michael PaquierДата:
Сообщение: Re: [HACKERS] Shaky coding for vacuuming partitioned relations