On Wed, Mar 24, 2021 at 5:44 AM Andres Freund <andres@anarazel.de> wrote:
>
>
> But looking more, several of the checks just seem wrong to me.
>
> target_rel_index_max_parallel_hazard() deparses index expressions from
> scratch? With code like
>
> + index_rel = index_open(index_oid, lockmode);
> ...
> + index_oid_list = RelationGetIndexList(rel);
> + foreach(lc, index_oid_list)
> ...
> + ii_Expressions = RelationGetIndexExpressions(index_rel);
> ...
>
> + Assert(index_expr_item != NULL);
> + if (index_expr_item == NULL) /* shouldn't happen */
> + {
> + elog(WARNING, "too few entries in indexprs list");
> + context->max_hazard = PROPARALLEL_UNSAFE;
> + found_max_hazard = true;
> + break;
> + }
>
> Brrr.
>
> Shouldn't we have this in IndexOptInfo already?
The additional parallel-safety checks are (at least currently) invoked
as part of max_parallel_hazard(), which is called early on in the
planner, so I don't believe that IndexOptInfo/RelOptInfo has been
built at this point.
> But also, why on earth
> is that WARNING branch a good idea?
>
I think there are about half a dozen other places in the Postgres code
where the same check is done, in which case it calls elog(ERROR,...).
Is it a better strategy to immediately exit the backend with an error
in this case, like the other cases do?
My take on it was that if this "should never happen" condition is ever
encountered, let it back out of the parallel-safety checks and
error-out in the place it normally (currently) would.
>
> +static bool
> +target_rel_domain_max_parallel_hazard(Oid typid, max_parallel_hazard_context *context)
> ...
> + scan = systable_beginscan(con_rel, ConstraintTypidIndexId, true,
> + NULL, 1, key);
> +
> + while (HeapTupleIsValid((tup = systable_getnext(scan))))
>
> There's plenty other code in the planner that needs to know about
> domains. This stuff is precisely why the typecache exists.
>
>
OK, fair comment.
Regards,
Greg Nancarrow
Fujitsu Australia