Re: strange parallel query behavior after OOM crashes

On Tue, Apr 11, 2017 at 12:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Apr 10, 2017 at 7:17 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> At first I was like 'WTF? Why do we need a new GUC just becase of an
>> assert?' but you're actually not adding a new GUC parameter, you're adding a
>> constant which is then used as a max value for max for the two existing
>> parallel GUCs.
>>
>> I think this is fine.
>
> I think it is pretty odd-looking. As written, it computes an unsigned
> -- and therefore necessarily non-negative -- value into a signed --
> and thus possibly neative -- value only to pass it back to abs() to
> make sure it's not negative:
>
> + Assert(!parallel ||
> abs((int)(BackgroundWorkerData->parallel_register_count -
> +
> BackgroundWorkerData->parallel_terminate_count)) <=
> + MAX_PARALLEL_WORKER_LIMIT);
>
> I think we can just say
>
> Assert(!parallel || BackgroundWorkerData->parallel_register_count -
> BackgroundWorkerData->parallel_terminate_count <= MAX_PARALLEL_WORKER_LIMIT);

Actually, there's an even simpler way: stick it inside the if () block
where we return false if we're outside the limit. Then we don't need
to test the "parallel" bool either, because it's already known to be
true. Committed that way.

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