Re: [v9.2] DROP statement reworks

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [v9.2] DROP statement reworks
Date: 2011-10-05 08:47:08
Message-ID: CADyhKSXm5DD0zCRusD4HhTkhTKN4eNfnQSx+u_8vAH9S-+17Pw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thanks for your reviewing.

2011/10/4 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hamza Bin Sohail 2011-10-05 09:12:43 Query regarding postgres lock contention
Previous Message Peter Eisentraut 2011-10-05 08:26:26 Re: Bug with pg_ctl -w/wait and config-only directories