On Sat, Jun 4, 2016 at 8:43 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Thu, May 26, 2016 at 5:57 AM, Amit Kapila <amit.kapil...@gmail.com>
> >
> > I am able to reproduce the assertion (it occurs once in two to three
> > but always at same place) you have reported upthread with the above
> > It seems to me, issue here is that while workers are writing tuples in
> > tuple queue, the master backend has detached from the queues.  The
> > why master backend has detached from tuple queues is because of Limit
> > clause, basically after processing required tuples as specified by Limit
> > clause, it calls shutdown of nodes in below part of code:
> I can't reproduce this assertion failure on master.  I tried running
> 'make installcheck' and then running this query repeatedly in the
> regression database with and without
> parallel_setup_cost=parallel_tuple_cost=0, and got nowhere.  Does that
> work for you, or do you have some other set of steps?

I have tried by populating pg_statistic table after running make
installcheck.  The way to populate pg_statistic is to create lot of tables
and insert few rows in each table as mentioned in the end of mail upthread.

Today, again I tried reproducing it using same steps, but could not
reproduce it.  This is a timing issue and difficult to reproduce, last time
also I have spent quite some time to reproduce it.  I think we can fix the
issue as per analysis done by me last time and then let Andreas run his
tests to see if he could see the issue again.

> > I think the workers should stop processing tuples after the tuple
queues got
> > detached.  This will not only handle above situation gracefully, but
> > allow to speed up the queries where Limit clause is present on top of
> > node.  Patch for the same is attached with mail (this was part of
> > parallel seq scan patch, but was not applied and the reason as far as I
> > remember was we thought such an optimization might not be required for
> > initial version).
> This is very likely a good idea, but...
> > Another approach to fix this issue could be to reset mqh_partial_bytes
> > mqh_length_word_complete in shm_mq_sendv  in case of SHM_MQ_DETACHED.
> > are currently reset only incase of success.
> ...I think we should do this too, because it's intended that calling
> shm_mq_sendv again after it previously returned SHM_MQ_DETACHED should
> again return SHM_MQ_DETACHED, not fail an assertion.

  res = shm_mq_send_bytes(mqh, j, tmpbuf, nowait, &bytes_written);
  mqh->mqh_partial_bytes += bytes_written;
+ if (res == SHM_MQ_DETACHED)
+ {
+ mqh->mqh_partial_bytes = 0;
+ return res;
+ }

In the above change, you are first adding bytes_written and then doing the
SHM_MQ_DETACHED check, whereas other place the check is done first which
seems to be right.  I think it doesn't matter either way, but it is better
to be consistent.  Also isn't it better to set mqh_length_word_complete as
false as next time this API is called, it should first try to write length
into buffer.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Reply via email to