On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote: > Nathan Bossart <nathandboss...@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
