From: | Nathan Bossart <nathandbossart(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: ALTER EXTENSION SET SCHEMA versus dependent types |
Date: | 2024-05-08 23:54:59 |
Message-ID: | 20240508235459.GA3212918@nathanxps13 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2024-05-08 23:57:32 | Re: bug: copy progress reporting of backends which run multiple COPYs |
Previous Message | Tom Lane | 2024-05-08 23:42:18 | Re: ALTER EXTENSION SET SCHEMA versus dependent types |