Re: pgsql: Add a new GUC and a reloption to enable inserts in parallel-mode

Hi,

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? But also, why on earth
is that WARNING branch a good idea?

+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.

The pattern to rebuild information that we already have cached elsewhere
seems to repeat all over this patch.

This seems not even close to committable.

Greetings,

Andres Freund