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

Hi,

Hi,

On 11/30/20 11:29 PM, Tom Lane wrote:

"Drouvot, Bertrand" <bdrouvot@amazon.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() 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