Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Mar 23, 2024 at 12:31 PM Tom Lane <tgl@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.
> 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.
By that logic, we should rip out every Assert in the system, as well
as all of the (extensive) resource leak checking that already happens
during CommitTransaction. We've always felt that those leak checks
were worth the cost to help us find bugs --- which they have done and
still do from time to time. I don't see why this case is different,
especially when the added cost compared to HEAD is not much more than
one C function call.
Or in other words: the point is not about stopping malicious C code,
it's about recognizing that we make mistakes.
regards, tom lane