Re: ALTER EXTENSION SET SCHEMA versus dependent types

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