On Sat, Oct 7, 2017 at 7:22 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Oct 6, 2017 at 7:08 AM, Amit Kapila <amit.kapil...@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
pq_pushdown_initplan_v12.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers