Re: pg_background (and more parallelism infrastructure patches)

On Mon, Sep 8, 2014 at 10:39 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Sat, Jul 26, 2014 at 9:32 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> > On Fri, Jul 25, 2014 at 4:16 PM, Alvaro Herrera
> > <alvherre(at)2ndquadrant(dot)com> wrote:
> > > On Fri, Jul 25, 2014 at 02:11:32PM -0400, Robert Haas wrote:
> > >> + pq_mq_busy = true;
> > >> +
> > >> + iov[0].data = &msgtype;
> > >> + iov[0].len = 1;
> > >> + iov[1].data = s;
> > >> + iov[1].len = len;
> > >> +
> > >> + Assert(pq_mq_handle != NULL);
> > >> + result = shm_mq_sendv(pq_mq_handle, iov, 2, false);
> > >> +
> > >> + pq_mq_busy = false;
> > >
> > > Don't you need a PG_TRY block here to reset pq_mq_busy?
> >
> > No. If shm_mq_sendv is interrupted, we can't use the shm_mq any more.
> > But since that should only happen if an interrupt arrives while the
> > queue is full, I think that's OK.
>
> I think here not only on interrupt, but any other error in this
> function shm_mq_sendv() path (one example is WaitLatch())
> could lead to same behaviour.
>
> > (Think about the alternatives: if
> > the queue is full, we have no way of notifying the launching process
> > without waiting for it to retrieve the results, but it might not do
> > that right away, and if we've been killed we need to die *now* not
> > later.)
>
> So in such cases what is the advise to users, currently they will
> see the below message:
> postgres=# select * from pg_background_result(5124) as (x int);
> ERROR: lost connection to worker process with PID 5124
>
> One way is to ask them to check logs, but what about if they want
> to handle error and take some action?
>
> Another point about error handling is that to execute the sql in
> function pg_background_worker_main(), it starts the transaction
> which I think doesn't get aborted if error occurs

For this I was just referring error handling code of
StartBackgroundWorker(), however during shutdown of process, it
will call AbortOutOfAnyTransaction() which will take care of aborting
transaction.

> and similarly handling
> for timeout seems to be missing in error path.

As we are anyway going to exit on error, so not sure, if this will be
required, however having it for clean exit seems to be better.

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