Обсуждение: Re: pg_background (and more parallelism infrastructure patches)

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?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jul 25, 2014 at 4:16 PM, Alvaro Herrera
<alvherre@2ndquadrant.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.  (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.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 8, 2014 at 1:09 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > 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.

Probably true.  But I think we generally count on that to be no-fail,
or close to it, so I'm not really worried about it.

>> (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?

They have to check the logs.  To reiterate what I said before, there
is no reasonable way to both have the background worker terminate
quickly and also guarantee that the full error message is received by
the process that started it.  You have to pick one, and I stick by the
one I picked.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 8, 2014 at 2:05 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> 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.

Yeah.

>> 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.

Can you be more specific?

I'm generally of the view that there's little point in spending time
cleaning things up that will go away anyway when the process exits.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company