Horton

Thread

Commits

  1. Delete deleteWhatDependsOn() in favor of more performDeletion() flag bits.

  2. Fix some issues with temp/transient tables in extension scripts.

  1. Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-26T16:25:14Z

    It suddenly struck me that the problem being complained of in bug #14434
is that dependency.c's findDependentObjects() is handling extension
dependencies incorrectly.  It has this logic:

                /*
                 * This object is part of the internal implementation of
                 * another object, or is part of the extension that is the
                 * other object.  We have three cases:
                 *
                 * 1. At the outermost recursion level, we normally disallow
                 * the DROP.  (We just ereport here, rather than proceeding,
                 * since no other dependencies are likely to be interesting.)
                 * However, there are exceptions.
                 */
                if (stack == NULL)
                {
                    /*
                     * Exception 1a: if the owning object is listed in
                     * pendingObjects, just release the caller's lock and
                     * return.  We'll eventually complete the DROP when we
                     * reach that entry in the pending list.
                     */
                    ...

                    /*
                     * Exception 1b: if the owning object is the extension
                     * currently being created/altered, it's okay to continue
                     * with the deletion.  This allows dropping of an
                     * extension's objects within the extension's scripts, as
                     * well as corner cases such as dropping a transient
                     * object created within such a script.
                     */
                    ...

                    /* No exception applies, so throw the error */
                    ...

The bug report occurs because the sequence's extension membership
pg_depend record is hit one recursion level down, so that stack!=NULL.
Instead, we should rearrange this so that "exception 1b" is allowed
whether or not we are at the outermost recursion level.  The assumption
is that the sequence creation/upgrade script knows what it's doing and
should not be forced to issue manual ALTER EXTENSION DROP commands
before it can do it.

While looking at that, I wonder if "exception 1a" isn't wrongly placed
as well.  Why shouldn't we let that case occur below top level?

			regards, tom lane

  2. Re: Wrong order of tests in findDependentObjects()

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-11-27T15:59:30Z

    On 11/26/16 10:25 AM, Tom Lane wrote:
> It suddenly struck me that the problem being complained of in bug #14434
> is that dependency.c's findDependentObjects() is handling extension
> dependencies incorrectly.

I suspect this is unrelated, but I've run into another oddity with 
extension dependency: if an extension creates any temporary objects the 
extension will install and function correctly... until the backend that 
created the extension quits. This is VERY confusing if you've never come 
across it before, because you'll do a bunch of work in a single script 
but when you try to use the extension for real it will "randomly" just 
vanish.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

  3. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-27T16:15:01Z

    Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> I suspect this is unrelated, but I've run into another oddity with 
> extension dependency: if an extension creates any temporary objects the 
> extension will install and function correctly... until the backend that 
> created the extension quits. This is VERY confusing if you've never come 
> across it before, because you'll do a bunch of work in a single script 
> but when you try to use the extension for real it will "randomly" just 
> vanish.

Yeah, I was wondering about that yesterday --- that comment mentions
the case of temporary objects, but it only fixes the problem while the
script runs.  Maybe there should be a separate test for "we're doing
temporary-object cleanup" that would similarly prevent recursion to
an extension?

			regards, tom lane

  4. Re: Wrong order of tests in findDependentObjects()

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-11-27T23:35:52Z

    On 11/27/16 10:15 AM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@bluetreble.com> writes:
>> I suspect this is unrelated, but I've run into another oddity with
>> extension dependency: if an extension creates any temporary objects the
>> extension will install and function correctly... until the backend that
>> created the extension quits. This is VERY confusing if you've never come
>> across it before, because you'll do a bunch of work in a single script
>> but when you try to use the extension for real it will "randomly" just
>> vanish.
>
> Yeah, I was wondering about that yesterday --- that comment mentions
> the case of temporary objects, but it only fixes the problem while the
> script runs.  Maybe there should be a separate test for "we're doing
> temporary-object cleanup" that would similarly prevent recursion to
> an extension?

I can't think of any reason you'd want the current behavior.

Though, it'd arguably be better to remove temp objects created by an 
extension after the script exits, so that they can't "leak" into the 
executing backend. Dunno if that's any harder or not...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

  5. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-28T17:04:44Z

    Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 11/27/16 10:15 AM, Tom Lane wrote:
>> Yeah, I was wondering about that yesterday --- that comment mentions
>> the case of temporary objects, but it only fixes the problem while the
>> script runs.  Maybe there should be a separate test for "we're doing
>> temporary-object cleanup" that would similarly prevent recursion to
>> an extension?

> I can't think of any reason you'd want the current behavior.

> Though, it'd arguably be better to remove temp objects created by an 
> extension after the script exits, so that they can't "leak" into the 
> executing backend. Dunno if that's any harder or not...

Sounds way harder to me.  There's no good way to separate temp objects
made by the script from those made earlier in the session.  Also, the
general theory of extension scripts is that they're just executed
normally, with the only additional bit of magic being that objects
created during the script are tied to the extension.  I'm not sure that
forcibly dropping temp objects at the end fits in that charter at all.

But I think fixing it to not recurse to extensions during temp namespace
cleanup might not be very hard.  I'll take a look.

			regards, tom lane

  6. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-28T19:48:10Z

    I wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> I can't think of any reason you'd want the current behavior.

> But I think fixing it to not recurse to extensions during temp namespace
> cleanup might not be very hard.  I'll take a look.

Here's a draft patch for that.  Rather than sticking yet another special
assumption into deleteWhatDependsOn, I thought it was time to get rid of
that function altogether in favor of selecting its few special behaviors
via flag bits for performDeletion.  So this adds PERFORM_DELETION_QUIETLY
and PERFORM_DELETION_SKIP_ORIGINAL flag bits to do that, plus a
PERFORM_DELETION_SKIP_EXTENSIONS bit that solves the problem at hand.
Treating this as a performDeletion flag bit also lets us disable extension
dropping in autovacuum's forced drop of temp tables, which would otherwise
be a nasty hole in the fix.

I'm not sure if this is a candidate for back-patching or not.  I think
what it's fixing is mostly a convenience thing, since extension scripts
that explicitly drop any temp objects they've created are not at risk,
and that would be good extension authoring practice anyway IMO.
If we do back-patch we'd need to figure out whether we want to preserve
deleteWhatDependsOn as a stub function in the back branches.  (I rather
doubt that any third party code is calling it, since it's so
special-purpose, but you never know ...)

Thoughts?

			regards, tom lane

  7. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-12-01T21:09:36Z

    I wrote:
>> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>>> I can't think of any reason you'd want the current behavior.

>> But I think fixing it to not recurse to extensions during temp namespace
>> cleanup might not be very hard.  I'll take a look.

I wrote a test case to try to demonstrate that this patch was fixing a
bug, and was surprised to find that it didn't fail.  The reason turns
out to be that we fixed this problem years ago in commit 08dd23cec:

    Also, arrange for explicitly temporary tables to not get linked as
    extension members in the first place, and the same for the magic
    pg_temp_nnn schemas that are created to hold them.  This prevents assorted
    unpleasant results if an extension script creates a temp table: the forced
    drop at session end would either fail or remove the entire extension, and
    neither of those outcomes is desirable.

Now, if you really try hard, say by creating a temp function, you can
break it.  But I don't have all that much sympathy for such use-cases.

I think that the patch I wrote is good cleanup, so I'm still inclined
to apply it in HEAD, but I no longer think it's fixing any case that's
significant in the field.  I wonder if you have a counterexample?

			regards, tom lane

  8. Re: Wrong order of tests in findDependentObjects()

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-12-01T23:14:36Z

    On 12/1/16 1:09 PM, Tom Lane wrote:
> I think that the patch I wrote is good cleanup, so I'm still inclined
> to apply it in HEAD, but I no longer think it's fixing any case that's
> significant in the field.  I wonder if you have a counterexample?

No; I'm sure I've run into this because of a temp object other than a 
table (probably a function, though possibly something else).
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

  9. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-12-01T23:28:56Z

    Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 12/1/16 1:09 PM, Tom Lane wrote:
>> I think that the patch I wrote is good cleanup, so I'm still inclined
>> to apply it in HEAD, but I no longer think it's fixing any case that's
>> significant in the field.  I wonder if you have a counterexample?

> No; I'm sure I've run into this because of a temp object other than a 
> table (probably a function, though possibly something else).

Makes sense.  The fact that we protect against this for temp tables and
views would make it all the more surprising when you get bit by some
less-common object type.

			regards, tom lane

  10. Re: Wrong order of tests in findDependentObjects()

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-12-02T20:05:00Z

    I wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> On 12/1/16 1:09 PM, Tom Lane wrote:
>>> I think that the patch I wrote is good cleanup, so I'm still inclined
>>> to apply it in HEAD, but I no longer think it's fixing any case that's
>>> significant in the field.  I wonder if you have a counterexample?

>> No; I'm sure I've run into this because of a temp object other than a 
>> table (probably a function, though possibly something else).

> Makes sense.  The fact that we protect against this for temp tables and
> views would make it all the more surprising when you get bit by some
> less-common object type.

It occurred to me that the hack installed in 08dd23cec, to not record
a pg_depend entry for a temp table, causes its own set of misbehaviors.
If you drop the extension later in the same session, the temp table isn't
automatically removed.  This would cause repeated drop/create cycles to
fail, which is kind of annoying since you might well do that during
extension development.  Even more annoying, if the temp table has another
sort of dependency on the extension --- say, it uses a data type defined
by the extension --- the DROP EXTENSION would fail unless you say CASCADE.

So I've pushed this patch with an addition to revert that hack.  I added
a regression test case showing that such usage behaves properly now.

			regards, tom lane