On Sat, Mar 23, 2024 at 12:31 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > However, the calling logic seems a bit shy of a load, in that it > trusts IsInParallelMode() completely to decide whether to check for > leaked parallel contexts. So we'd miss the case where somebody did > ExitParallelMode without having cleaned up workers. It's not like > AtEOXact_Parallel and AtEOSubXact_Parallel cost a lot when they have > nothing to do, so I think we should call them unconditionally, and > separately from that issue a warning if parallelModeLevel isn't zero > (and we're committing).
I wasn't worried about this case when I wrote this code. The general flow that I anticipated was that somebody would run a query, and ExecMain.c would enter parallel mode, and then maybe eventually reach some SQL-callable C function that hadn't gotten the memo about parallel query but had been mistakenly labelled as PARALLEL RESTRICTED or PARALLEL SAFE when it wasn't really, and so the goal was for core functions that such a function might reasonably attempt to call to notice that something bad was happening. But if the user puts a call to ExitParallelMode() inside such a function, it's hard to imagine what goal they have other than to deliberately circumvent the safeguards. And they're always going to be able to do that somehow, if they're coding in C. So I'm not convinced that the sanity checks you've added are really going to do anything other than burn a handful of CPU cycles. If there's some plausible case in which they protect us against a user who has legitimately made an error, fine; but if we're just wandering down the slippery slope of believing we can defend against malicious C code, we absolutely should not do that, not even a little bit. The first CPU instruction we burn in the service of a hopeless cause is already one too many. -- Robert Haas EDB: http://www.enterprisedb.com
