Tender Wang <tndrwang@gmail.com> writes:
> Thanks for pushing the patch.
My apologies for not having paid much attention to this thread
earlier, but ...
I don't like anything about the committed patch. It's a band-aid
that doesn't really solve the problem: if I simply declare the
function as PARALLEL SAFE, the failure comes right back. Of
course you can argue that then I lied about the parallel safety
of the function, but I think a far better answer is to make such
functions actually parallel safe, which I have a patch for
awaiting review in the current commitfest [1].
Moreover I think this breaks a fairly long-standing principle of
the planner, namely that if we can inline or const-fold a function
then its original markings no longer matter. People are likely
to be surprised that that no longer applies to parallel-safety
decisions, and even more surprised that it only doesn't apply
in this one narrow context.
I also believe that this has introduced new bugs, caused by
passing is_parallel_safe() raw parse-analysis output rather
than a tree that's been through eval_const_expressions.
For example, that processing is responsible for inserting
function default-argument expressions. If a default argument
is parallel-unsafe, this coding would fail to notice that.
(Compare commit 743ddafc7.) There might be other structures
in an un-simplified tree that is_parallel_safe() isn't expecting
and would spit up on.
As far as HEAD goes, I think we should revert this and then look
to get [1] committed. I'm unsure what to do in the back branches,
but given the lack of prior complaints I suspect we could get away
with just reverting and leaving the issue unfixed. (I wouldn't
propose back-patching [1], it seems too risky.)
If we want to do something about the more generic issue that
maybe const-folding will succeed in the leader process but
fail in workers, then I think the way to do that is to adjust
parallel index build so that the processed expression trees are
passed to the workers from the leader. That is how it's done
with regular query trees, and index build is evidently taking
a not-very-safe shortcut by not doing it like that. But I'm not
excited enough about the mostly-theoretical hazard to put any
work into that myself.
Another line of thought is whether we should just forbid any
parallel-unsafe functions in index expressions. If we continue
to allow them, I think we have to reject parallelizing any
query or DDL operation that touches the associated table at all,
because there's too much risk of a worker trying to evaluate
such an expression somewhere along the line. Again though,
given the lack of complaints I'm not excited about imposing
such a draconian policy. (The fact that functions are marked
parallel unsafe by default means that if we did, we'd almost
certainly break cases that are working fine for users today.)
regards, tom lane
[1] https://commitfest.postgresql.org/47/4776/