On 27.02.24 08:57, Alvaro Herrera wrote:
> On 2024-Feb-27, Michael Paquier wrote:
>
>> These would cause compilation failures. Saying that, this is a very
>> nice cleanup, so I've fixed these and applied the patch after checking
>> that the one-one replacements were correct.
>
> Oh, I thought we were going to get rid of ObjectClass altogether -- I
> mean, have getObjectClass() return ObjectAddress->classId, and then
> define the OCLASS values for each catalog OID [... tries to ...] But
> this(*) doesn't work for two reasons:
I have long wondered what the point of ObjectClass is. I find the extra
layer of redirection, which is used only in small parts of the code, and
the similarity to ObjectType confusing. I happened to have a draft
patch for its removal lying around, so I'll show it here, rebased over
what has already been done in this thread.
> 1. some switches processing the OCLASS enum don't have "default:" cases.
> This is so that the compiler tells us when we fail to add support for
> some new object class (and it's been helpful). If we were to
I think you can also handle that with some assertions and proper test
coverage. It's not even clear how strong this benefit is. For example,
in AlterObjectNamespace_oid(), you could still put a new OCLASS into the
"ignore object types that don't have schema-qualified names" case, and
it might or might not be wrong. Also, there are already various OCLASS
switches that do have a default case, so it's not even clear what the
preferred coding style should be.
> 2. all users of getObjectClass would have to include the catalog header
> specific to every catalog it wants to handle; so tablecmds.c and
> dependency.c would have to include almost all catalog includes, for
> example.
This doesn't seem to be a problem in practice; see patch.