Re: [BUG] orphaned function

Le 18/12/2020 à 00:26, Tom Lane a écrit :
> Gilles Darold <gilles(at)darold(dot)net> writes:
>> The same problem applies if the returned type or the procedural language
>> is dropped. I have tried to fix that in ProcedureCreate() by using an
>> AccessSharedLock for the data types and languages involved in the
>> function declaration. With this patch the race condition with parameters
>> types, return types and PL languages are fixed. Only data types and
>> procedural languages with Oid greater than FirstBootstrapObjectId will
>> be locked locked. But this is probably more complex than this fix so it
>> is proposed as a separate patch
>> (v1-003-orphaned_function_types_language.patch) to not interfere with
>> the applying of Bertran's patch.
> Indeed. This points up one of the things that is standing in the way
> of any serious consideration of applying this patch. To my mind there
> are two fundamental, somewhat interrelated considerations that haven't
> been meaningfully addressed:
>
> 1. What set of objects is it worth trying to remove this race condition
> for, and with respect to what dependencies? Bertrand gave no
> justification for worrying about function-to-namespace dependencies
> specifically, and you've likewise given none for expanding the patch
> to consider function-to-datatype dependencies. There are dozens more
> cases that could be considered; but I sure don't want to be processing
> another ad-hoc fix every time someone thinks they're worried about
> another specific case.
>
> Taking a practical viewpoint, I'm far more worried about dependencies
> of tables than those of any other kind of object. A messed-up function
> definition doesn't cost you much: you can just drop and recreate the
> function. A table full of data is way more trouble to recreate, and
> indeed the data might be irreplaceable. So it seems pretty silly to
> propose that we try to remove race conditions for functions' dependencies
> on datatypes without removing the same race condition for tables'
> dependencies on their column datatypes.
>
> But any of these options lead to the same question: why stop there?
> An approach that would actually be defensible, perhaps, is to incorporate
> this functionality into the dependency mechanism: any time we're about to
> create a pg_depend or pg_shdepend entry, take out an AccessShareLock on
> the referenced object, and then check to make sure it still exists.
> This would improve the dependency mechanism so it prevents creation-time
> race conditions, not just attempts to drop dependencies of
> already-committed objects. It would mean that the one patch would be
> the end of the problem, rather than looking forward to a steady drip of
> new fixes.
>
> 2. How much are we willing to pay for this added protection? The fact
> that we've gotten along fine without it for years suggests that the
> answer needs to be "not much". But I'm not sure that that actually
> is the answer, especially if we don't have a policy that says "we'll
> protect against these race conditions but no other ones". I think
> there are possibly-serious costs in three different areas:
>
> * Speed. How much do all these additional lock acquisitions slow
> down a typical DDL command?
>
> * Number of locks taken per transaction. This'd be particularly an
> issue for pg_restore runs using single-transaction mode: they'd take
> not only locks on the objects they create, but also a bunch of
> reference-protection locks. It's not very hard to believe that this'd
> make a difference in whether restoring a large database is possible
> without increasing max_locks_per_transaction.
>
> * Risk of deadlock. The reference locks themselves should be sharable,
> so maybe there isn't much of a problem, but I want to see this question
> seriously analyzed not just ignored.
>
> Obviously, the magnitude of these costs depends a lot on how many
> dependencies we want to remove the race condition for. But that's
> exactly the reason why I don't want a piecemeal approach of fixing
> some problems now and some other problems later. That's way too
> much like the old recipe for boiling a frog: we could gradually get
> into serious performance problems without anyone ever having stopped
> to consider the issue.
>
> In short, I think we should either go for a 100% solution if careful
> analysis shows we can afford it, or else have a reasoned policy
> why we are going to close these specific race conditions and no others
> (implying that we'll reject future patches in the same area). We
> haven't got either thing in this thread as it stands, so I do not
> think it's anywhere near being ready to commit.
>
> regards, tom lane

Thanks for the review,

Honestly I have never met these races condition in 25 years of
PostgreSQL and will probably never but clearly I don't have the Amazon
database services workload. I let Bertrand decide what to do with the
patch and address the races condition with a more global approach
following your advices if more work is done on this topic. I tag the
patch as "Waiting on author".

Best regards,

--
Gilles Darold
LzLabs GmbH
http://www.lzlabs.com/