Re: parallelize queries containing initplans

On Fri, Oct 6, 2017 at 7:08 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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. IIUC,
param IDs are tracked separately for extern and exec parameters, so
why would we ignore that distinction here?

But I'm also wondering if we're missing a trick here, because isn't a
PARAM_EXTERN parameter always safe, given SerializeParamList? If so,
this || ought to be &&, but if that adjustment is needed, it seems
like a separate patch.

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