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

On Sat, Jun 12, 2021 at 1:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Jun 11, 2021 at 12:13 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Do we invalidate relcache entry if someone changes say trigger or some
> > index AM function property via Alter Function (in our case from safe
> > to unsafe or vice-versa)? Tsunakawa-San has mentioned this as the
> > reason in his email [1] why we can't rely on caching this property in
> > relcache entry. I also don't see anything in AlterFunction which would
> > suggest that we invalidate the relation with which the function might
> > be associated via trigger.
>
> Hmm. I am not sure index that AM functions really need to be checked,
> but triggers certainly do.
>

Why do you think we don't need to check index AM functions? Say we
have an index expression that uses function and if its parallel safety
is changed then probably that also impacts whether we can do insert in
parallel. Because otherwise, we will end up executing some parallel
unsafe function in parallel mode during index insertion.

> I think if you are correct that an ALTER
> FUNCTION wouldn't invalidate the relcache entry, which is I guess
> pretty much the same problem Tom was pointing out in the thread to
> which you linked.
>
> But ... thinking out of the box as Tom suggests, what if we came up
> with some new kind of invalidation message that is only sent when a
> function's parallel-safety marking is changed? And every backend in
> the same database then needs to re-evaluate the parallel-safety of
> every relation for which it has cached a value. Such recomputations
> might be expensive, but they would probably also occur very
> infrequently. And you might even be able to make it a bit more
> fine-grained if it's worth the effort to worry about that: say that in
> addition to caching the parallel-safety of the relation, we also cache
> a list of the pg_proc OIDs upon which that determination depends. Then
> when we hear that the flag's been changed for OID 123456, we only need
> to invalidate the cached value for relations that depended on that
> pg_proc entry.
>

Yeah, this could be one idea but I think even if we use pg_proc OID,
we still need to check all the rel cache entries to find which one
contains the invalidated OID and that could be expensive. I wonder
can't we directly find the relation involved and register invalidation
for the same? We are able to find the relation to which trigger
function is associated during drop function via findDependentObjects
by scanning pg_depend. Assuming, we are able to find the relation for
trigger function by scanning pg_depend, what kinds of problems do we
envision in registering the invalidation for the same?

I think we probably need to worry about the additional cost to find
dependent objects and if there are any race conditions in doing so as
pointed out by Tom in his email [1]. The concern related to cost could
be addressed by your idea of registering such an invalidation only
when the user changes the parallel safety of the function which we
don't expect to be a frequent operation. Now, here the race condition,
I could think of could be that by the time we change parallel-safety
(say making it unsafe) of a function, some of the other sessions might
have already started processing insert on a relation where that
function is associated via trigger or check constraint in which case
there could be a problem. I think to avoid that we need to acquire an
Exclusive lock on the relation as we are doing in Rename Policy kind
of operations.


> There are ways that a relation could become
> parallel-unsafe without changing the parallel-safety marking of any
> function, but perhaps all of the other ways involve a relcache
> invalidation?
>

Probably, but I guess we can once investigate/test those cases as well
if we find/agree on the solution for the functions stuff.

[1] - https://www.postgresql.org/message-id/1030301.1616560249%40sss.pgh.pa.us

-- 
With Regards,
Amit Kapila.