On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote: > On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <pe...@eisentraut.org> wrote: >> Oops, there was a second commit in my branch that I neglected to send >> in. Here is my complete patch set.
Thanks for the new patch set. The gains are neat, giving nice numbers: 7 files changed, 200 insertions(+), 644 deletions(-) + default: + DropObjectById(object); + break; Hmm. I am not sure that this is a good idea. Wouldn't it be safer to use as default path something that generates an ERROR so as this code path would complain immediately when adding a new catalog? My point is to make people consider what they should do on deletion when adding a catalog that would take this code path, rather than assuming that a deletion is OK to happen. So I would recommend to keep the list of catalog OIDs for the DropObjectById case, keep the list for global objects, and add a third path with a new ERROR. - /* - * There's intentionally no default: case here; we want the - * compiler to warn if a new OCLASS hasn't been handled above. - */ In getObjectDescription() and getObjectTypeDescription() this was a safeguard, but we don't have that anymore. So it seems to me that this should be replaced with a default with elog(ERROR)? There is a third one in getObjectIdentityParts() that has not been removed, though, but same remark at the two others. RememberAllDependentForRebuilding() uses a default, so this one looks good to me. > there is a `OCLASS` at the end of getObjectIdentityParts. Nice catch. A comment is not updated. > There is a `ObjectClass` in typedefs.list This is usually taken care of by committers or updated automatically. -- Michael
signature.asc
Description: PGP signature
