Re: [UNVERIFIED SENDER] Re: [BUG] orphaned function

Hi,

On 12/1/20 10:32 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 11/30/20 11:29 PM, Tom Lane wrote:
>> "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> writes:
>>> here is a scenario that produces an orphaned function (means it does
>>> not
>>> belong to any namespace):
>>> [ drop schema before committing function creation ]
>> Hm.  Historically we've not been too fussed about preventing such race
>> conditions, and I wonder just how far is sane to take it. Should we
>> acquire lock on the function's argument/result data types?  Its
>> language?
>>
>> Given the precedent of RangeVarGetAndCheckCreationNamespace, I'm
>> willing to accept this patch's goals as stated.
>
> Thanks!
>
> Forgot to mention an observed side effect: pg_upgrade failing during
> the "Creating dump of database schemas" step, with things like:
>
> pg_dump: last built-in OID is 16383
> pg_dump: reading extensions
> pg_dump: identifying extension members
> pg_dump: reading schemas
> pg_dump: reading user-defined tables
> pg_dump: reading user-defined functions
> pg_dump: error: schema with OID 16385 does not exist
>
>> But it feels like
>> we need some clearer shared understanding of which things we are
>> willing to bother with preventing races for, and which we aren't.
>>
>>> Please find attached a patch that adds a LockDatabaseObject() call in
>>> QualifiedNameGetCreationNamespace() to prevent such orphaned
>>> situations.
>> I don't think that actually succeeds in preventing the race, it'll
>> just delay your process till the deletion is committed.
>
> oooh, i was thinking the other way around: it was protecting the other
> situation (when the drop schema is waiting for the create function to
> finish).
>
> The other scenario you are referring to needs to be addressed as well,
> thanks!
>
>>    But you
>> already have the namespaceId.  Note the complex retry logic in
>> RangeVarGetAndCheckCreationNamespace; without something on the same
>> order, you're not closing the hole in any meaningful way.
>
> understood with the scenario you were referring to previously (which
> was not the one I focused on).
>
>> Likely
>> what this patch should do is refactor that function so that its guts
>> can be used for other object-creation scenarios.
> Will have a look.
>>
I ended up making more changes in QualifiedNameGetCreationNamespace
<https://doxygen.postgresql.org/namespace_8c.html#a2cde9c8897e89ae47a99c103c4f96d31>()
to mimic the retry() logic that is in
RangeVarGetAndCheckCreationNamespace().

The attached v2 patch, now protects the function to be orphaned in those
2 scenarios:

Scenario 1: first, session 1 begin create function, then session 2 drops
the schema: drop schema is locked and would produce (once the create
function finishes):

bdt=# 2020-12-02 10:12:46.364 UTC [15810] ERROR:  cannot drop schema
tobeorph because other objects depend on it
2020-12-02 10:12:46.364 UTC [15810] DETAIL:  function tobeorph.bdttime()
depends on schema tobeorph
2020-12-02 10:12:46.364 UTC [15810] HINT:  Use DROP ... CASCADE to drop
the dependent objects too.
2020-12-02 10:12:46.364 UTC [15810] STATEMENT:  drop schema tobeorph;

Scenario 2: first, session 1 begin drop schema, then session 2 creates
the function: create function is locked and would produce (once the drop
schema finishes):

2020-12-02 10:14:29.468 UTC [15822] ERROR:  schema "tobeorph" does not exist

Thanks

Bertrand