Horton

Thread

Commits

  1. Repair ALTER EXTENSION ... SET SCHEMA.

  1. ALTER EXTENSION SET SCHEMA versus dependent types

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-08T21:52:31Z

    I happened to notice that the comment for AlterObjectNamespace_oid
claims that

 * ... it doesn't have to deal with certain special cases
 * such as not wanting to process array types --- those should never
 * be direct members of an extension anyway.

This struck me as probably broken in the wake of e5bc9454e
(Explicitly list dependent types as extension members in pg_depend),
and sure enough a moment's worth of testing showed it is:

regression=# create schema s1;
CREATE SCHEMA
regression=# create extension cube with schema s1;
CREATE EXTENSION
regression=# create schema s2;
CREATE SCHEMA
regression=# alter extension cube set schema s2;
ERROR:  cannot alter array type s1.cube[]
HINT:  You can alter type s1.cube, which will alter the array type as well.

So we need to do something about that; and the fact that this escaped
testing shows that our coverage for ALTER EXTENSION SET SCHEMA is
pretty lame.

The attached patch fixes up the code and adds a new test to
the test_extensions module.  The fix basically is to skip the
pg_depend entries for dependent types, assuming that they'll
get dealt with when we process their parent objects.

			regards, tom lane

  2. Re: ALTER EXTENSION SET SCHEMA versus dependent types

    Nathan Bossart <nathandbossart@gmail.com> — 2024-05-08T23:33:20Z

    On Wed, May 08, 2024 at 05:52:31PM -0400, Tom Lane wrote:
> The attached patch fixes up the code and adds a new test to
> the test_extensions module.  The fix basically is to skip the
> pg_depend entries for dependent types, assuming that they'll
> get dealt with when we process their parent objects.

Looks reasonable to me.  The added test coverage seems particularly
valuable.  If I really wanted to nitpick, I might complain about the three
consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
makes lines like

+		AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true,
+								   objsMoved);

difficult to interpret.  But that's not necessarily the fault of this patch
and probably needn't block it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  3. Re: ALTER EXTENSION SET SCHEMA versus dependent types

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-08T23:42:18Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> Looks reasonable to me.  The added test coverage seems particularly
> valuable.  If I really wanted to nitpick, I might complain about the three
> consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
> makes lines like

> +		AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true,
> +								   objsMoved);

> difficult to interpret.  But that's not necessarily the fault of this patch
> and probably needn't block it.

I considered merging ignoreDependent and errorOnTableType into a
single 3-valued enum, but didn't think it was worth the trouble
given the very small number of callers; also it wasn't quite clear
how to map that to AlterTypeNamespace_oid's API.  Perhaps a little
more thought is appropriate though.

One positive reason for increasing the number of parameters is that
that will be a clear API break for any outside callers, if there
are any.  If I just replace a bool with an enum, such callers might
or might not get any indication that they need to take a fresh
look.

			regards, tom lane

  4. Re: ALTER EXTENSION SET SCHEMA versus dependent types

    Nathan Bossart <nathandbossart@gmail.com> — 2024-05-08T23:54:59Z

    On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Looks reasonable to me.  The added test coverage seems particularly
>> valuable.  If I really wanted to nitpick, I might complain about the three
>> consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
>> makes lines like
> 
>> +		AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true,
>> +								   objsMoved);
> 
>> difficult to interpret.  But that's not necessarily the fault of this patch
>> and probably needn't block it.
> 
> I considered merging ignoreDependent and errorOnTableType into a
> single 3-valued enum, but didn't think it was worth the trouble
> given the very small number of callers; also it wasn't quite clear
> how to map that to AlterTypeNamespace_oid's API.  Perhaps a little
> more thought is appropriate though.
> 
> One positive reason for increasing the number of parameters is that
> that will be a clear API break for any outside callers, if there
> are any.  If I just replace a bool with an enum, such callers might
> or might not get any indication that they need to take a fresh
> look.

Agreed.  Another option could be to just annotate the arguments with the
parameter names.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  5. Re: ALTER EXTENSION SET SCHEMA versus dependent types

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-08T23:57:55Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote:
>> One positive reason for increasing the number of parameters is that
>> that will be a clear API break for any outside callers, if there
>> are any.  If I just replace a bool with an enum, such callers might
>> or might not get any indication that they need to take a fresh
>> look.

> Agreed.  Another option could be to just annotate the arguments with the
> parameter names.

At the call sites you mean?  Sure, I can do that.

			regards, tom lane

  6. Re: ALTER EXTENSION SET SCHEMA versus dependent types

    Nathan Bossart <nathandbossart@gmail.com> — 2024-05-09T00:01:16Z

    On Wed, May 08, 2024 at 07:57:55PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Agreed.  Another option could be to just annotate the arguments with the
>> parameter names.
> 
> At the call sites you mean?  Sure, I can do that.

Yes.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com