Re: [HACKERS] parallelize queries containing initplans - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: [HACKERS] parallelize queries containing initplans
Date
Msg-id CAA4eK1JVtDyXQaL1x4MR-9qSMgrZt2jMhre6inLVNVTzXhWvyQ@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] parallelize queries containing initplans  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: [HACKERS] parallelize queries containing initplans
Re: [HACKERS] parallelize queries containing initplans
List pgsql-hackers
On Sat, Oct 7, 2017 at 7:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 7:08 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> I have fixed the other review comment related to using safe_param_list
>> in the attached patch.  I think I have fixed all comments given by
>> you, but let me know if I have missed anything or you have any other
>> comment.
>
> -        Param       *param = (Param *) node;
> +        if (list_member_int(context->safe_param_ids, ((Param *)
> node)->paramid))
> +            return false;
>
> -        if (param->paramkind != PARAM_EXEC ||
> -            !list_member_int(context->safe_param_ids, param->paramid))
> -        {
> -            if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> -                return true;
> -        }
> -        return false;            /* nothing to recurse to */
> +        if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> +            return true;
>
> I think your revised logic is wrong here because it drops the
> paramkind test, and I don't see any justification for that.
>

I have dropped the check thinking that Param extern params will be
always safe and for param exec params we now have a list of safe
params, so we don't need this check and now again thinking about it,
it seems I might not be right.  OTOH, I am not sure if the check as
written is correct for all cases, however, I think for the purpose of
this patch, we can leave it as it is and discuss separately what
should be the check (as suggested by you).  I have reverted the check
in the attached patch.

>
> But I'm also wondering if we're missing a trick here, because isn't a
> PARAM_EXTERN parameter always safe, given SerializeParamList?
>

Right.

>  If so,
> this || ought to be &&, but if that adjustment is needed, it seems
> like a separate patch.
>

How will it work if, during planning, we encounter param_extern param
in any list?  Won't it return true in that case, because param extern
params will not be present in safe_param_ids, so this check will be
true and then max_parallel_hazard_test will also return true?

How about always returning false for PARAM_EXTERN?


-- 
With Regards,
Amit Kapila.
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

Attachment

pgsql-hackers by date:

Previous
From: Konstantin Knizhnik
Date:
Subject: Re: [HACKERS] Slow synchronous logical replication
Next
From: Amit Kapila
Date:
Subject: Re: [HACKERS] Parallel Append implementation