On Wed, Mar 24, 2021 at 12:14 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2021-03-22 08:47:47 -0400, Robert Haas wrote:
> > I find this fairly ugly. If you can't make the cost of checking
> > whether parallelism is safe low enough that you don't need a setting
> > for this, then I think perhaps you shouldn't have the feature at all.
> > In other words, I propose that you revert both this and 05c8482f7f and
> > come back when you have a better design that doesn't introduce so much
> > overhead.
>
> I started out wondering whether some of the caching David Rowley has been
> working on to reduce the overhead of the result cache planning shows a
> path for how to make parts of this cheaper.
> https://postgr.es/m/CAApHDvpw5qG4vTb%2BhwhmAJRid6QF%2BR9vvtOhX2HN2Yy9%2Bw20sw%40mail.gmail.com
>
> 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?
>
No, because I think we don't build IndexOptInfo for target tables
(tables in which insert is performed). It is only built for tables to
scan. However, I think here we could cache parallel-safety info in the
index rel descriptor and can use it for next time. This will avoid
checking expressions each time. Do you have something else in mind?
--
With Regards,
Amit Kapila.