On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote:
> On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <peter@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
