Thanks for your reviewing.
2011/10/4 Robert Haas <robertmhaas@gmail.com>:
> On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> I rebased the patches towards the latest git master, so I believe these
>> are available to apply.
>
> Reviewing away...
>
> I don't see why we need one struct called ObjectProperty and another
> called CatalogProperty. Just fold CatalogProperty into ObjectProperty
> and make it one big flat structure.
>
The reason of this separation is some of ObjectType shares same catalogs;
such as pg_class for OBJECT_(TABLE|VIEW|INDEX|SEQUENCE),
so I thought it is better to reference CatalogProperty by several ObjectProperty
entries to avoid accidental inconsistence.
However, it is just a matter of my preference. I'll modify it.
The big flat structure provides a simple structure on the other hand.
> The get_object_property_waddawadda functions appear to be designed
> under the assumption that each object type will be in the
> ObjectProperty structure at the offset corresponding to (int) objtype.
> Are these functions going to get called from any place that's
> performance-critical enough to justify that optimization? I'd rather
> just have these functions use linear search, so that if someone adds a
> new OBJECT_* constant and doesn't add it to the array it doesn't
> immediately break everything.
>
OK, I'll fix it.
I assume these functions are used in DDL operations; mostly not
a performance critical path.
> get_object_namespace() looks like it ought to live in objectaddress.c,
> not dropcmds.c.
OK, I'll move it.
> check_object_validation() doesn't seem like a very
> good description of what that code actually does -- and that code
> looks like it's copy-and-pasted from elsewhere. Can we avoid that?
>
I noticed that get_object_address() already applied same checks on
pg_type object, and pg_class objects also.
Due to the below reason, it does not invoke get_object_address() for
the pg_class objects, so it seems to me reasonable to move whole
of the logic in check_object_validation() to get_relation_address().
> get_relation_address() is surely a copy of some other bit of code; how
> can we avoid duplication?
>
The get_relation_address() follows the logic in RemoveRelations() to be
eliminated by this patch, so it is not a code duplication.
The reason why we didn't consolidate this routine with get_object_address()
was that remove-index requires locks on the table which owns the index to
be removed, and it was ugly to add an ad-hoc if-block on the routine.
I'll try to work on this. Please wait for a while...
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>