Re: [bug?] Missed parallel safety checks, and wrong parallel safety

On Tue, Apr 20, 2021 at 2:23 PM tsunakawa(dot)takay(at)fujitsu(dot)com
<tsunakawa(dot)takay(at)fujitsu(dot)com> wrote:
> (2)
> I'm afraid the above phenomenon reveals that postgres overlooks parallel safety checks in some places. Specifically, we noticed the following:
>
> * User-defined aggregate
> CREATE AGGREGATE allows to specify parallel safety of the aggregate itself and the planner checks it, but the support function of the aggregate is not checked. OTOH, the document clearly says:
>
> https://www.postgresql.org/docs/devel/xaggr.html
>
> "Worth noting also is that for an aggregate to be executed in parallel, the aggregate itself must be marked PARALLEL SAFE. The parallel-safety markings on its support functions are not consulted."
>
> https://www.postgresql.org/docs/devel/sql-createaggregate.html
>
> "An aggregate will not be considered for parallelization if it is marked PARALLEL UNSAFE (which is the default!) or PARALLEL RESTRICTED. Note that the parallel-safety markings of the aggregate's support functions are not consulted by the planner, only the marking of the aggregate itself."

IMO, the reason for not checking the parallel safety of the support
functions is that the functions themselves can have whole lot of other
functions (can be nested as well) which might be quite hard to check
at the planning time. That is why the job of marking an aggregate as
parallel safe is best left to the user. They have to mark the aggreage
parallel unsafe if at least one support function is parallel unsafe,
otherwise parallel safe.

> Can we check the parallel safety of aggregate support functions during statement execution and error out? Is there any reason not to do so?

And if we were to do above, within the function execution API, we need
to know where the function got called from(?). It is best left to the
user to decide whether a function/aggregate is parallel safe or not.
This is the main reason we have declarative constructs like parallel
safe/unsafe/restricted.

For core functions, we definitely should properly mark parallel
safe/restricted/unsafe tags wherever possible.

Please correct me If I miss something.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com