[v9.2] DROP statement reworks

I updated the patches that I send a month before.

These are rebased to the latest tree, and the part-3 portion also consolidates
DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
previous patch.

Rest of portions are not changed.

Thanks,

2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached three patches try to consolidate code path of DROP
statement on various kind of object classes.

The first part provides a set of internal api to reference properties
of object types; as we had a discussion before.
It adds a big static array indexed by ObjectType; that holds OID of
corresponding catalog, catcache id, attribute number of name,
namespace, owner and so on. Every field of this array is accessable
via get_object_property_*() interfaces.
As a proof-of-concept, I reworked object_exists() with this array to
lookup a heaptuple by the supplied ObjectAddress.

The second part is a revised one from what I submitted in the commit-fest July.
It consolidate existing code paths corresponding to DropStmt (schema,
relations, text searches, type, domain, extension, conversion,
collation).

The third part is more reworks for other object classes (aggregate,
function, language, foreign data wrapper, foreign server, cast,
operator, trigger, rule).

I don't touch on shared database objects (database, tablespace and
role) because its behavior on deletion is not compatible with any
other object classes. So, it is unclear for me whether it is a
suitable case to be consolidated.
And, I also don't touch code corresponding to user-mapping because it
is not supported by get_object_address() yet.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Hi,

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached three patches try to consolidate code path of DROP
statement on various kind of object classes.

These are rebased to the latest tree, and the part-3 portion also consolidates
DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
previous patch.

I've been reviewing those patches, that are all about code refactoring.
I like what it's doing, generalizing ad-hoc code by adding some more
knowledge about the source tree into some data structures. Typically,
what catcache to use for a given object's class-id.

The patches are not in git am format nor in patch format, so I could
only read them, I didn't install them nor compiled the code, didn't run
the regression tests.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

The patches are not in git am format nor in patch format, so I could
only read them, I didn't install them nor compiled the code, didn't run
the regression tests.

I've marked the patch as Waiting on Author and referred to my previous
message. Thanks,

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

2011/9/25 Dimitri Fontaine <dimitri@2ndquadrant.fr>:

Hi,

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached three patches try to consolidate code path of DROP
statement on various kind of object classes.

These are rebased to the latest tree, and the part-3 portion also consolidates
DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
previous patch.

I've been reviewing those patches, that are all about code refactoring.
I like what it's doing, generalizing ad-hoc code by adding some more
knowledge about the source tree into some data structures.  Typically,
what catcache to use for a given object's class-id.

The patches are not in git am format nor in patch format, so I could
only read them, I didn't install them nor compiled the code, didn't run
the regression tests.

Thanks for your efforts.

I rebased the patches towards the latest git master, so I believe these
are available to apply.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Hi,

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

I've been reviewing those patches, that are all about code refactoring.
I like what it's doing, generalizing ad-hoc code by adding some more
knowledge about the source tree into some data structures.  Typically,
what catcache to use for a given object's class-id.

I rebased the patches towards the latest git master, so I believe these
are available to apply.

Ok I needed `git apply' to apply the patches, now that I used that I can
confirm that the 3 patches apply, compile, pass tests, and that I could
play with them a little. I think I'm going to mark that ready for
commiter. I don't have enough time for a more deep review but at the
same time patch reading and testing both passed :)

You might need to post a version that patch will be happy with, though,
using e.g.

git diff |filterdiff --format=context > /tmp/foo.patch

Then diffstat reports:
35 files changed, 932 insertions(+), 1913 deletions(-), 345 modifications(!)

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Sun, Oct 2, 2011 at 4:08 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Ok I needed `git apply' to apply the patches, now that I used that I can
confirm that the 3 patches apply, compile, pass tests, and that I could
play with them a little.  I think I'm going to mark that ready for
commiter.  I don't have enough time for a more deep review but at the
same time patch reading and testing both passed :)

You might need to post a version that patch will be happy with, though,
using e.g.

 git diff |filterdiff --format=context > /tmp/foo.patch

Then diffstat reports:
 35 files changed, 932 insertions(+), 1913 deletions(-), 345 modifications(!)

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them. My Mac has 2.5.8 and
handles unidiffs no problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them. My Mac has 2.5.8 and
handles unidiffs no problem.

Even containing git headers?

Here's what I'm talking about here:

src/backend/catalog/objectaddress.c | 653 ++++++++++++++++++++++++++++++-----
src/include/catalog/objectaddress.h | 13 +
src/include/nodes/parsenodes.h | 2 +-
3 files changed, 575 insertions(+), 93 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 8feb601..6094146 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -82,6 +82,463 @@ static ObjectAddress get_object_address_opcf(ObjectType objtype, List *objname,
 						List *objargs, bool missing_ok);
 static bool object_exists(ObjectAddress address);

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them.  My Mac has 2.5.8 and
handles unidiffs no problem.

Even containing git headers?

Yeah, it just skips right over them. I've never had even a minor
problem on that account, which is why I was surprised to see it giving
you so much trouble.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them. My Mac has 2.5.8 and
handles unidiffs no problem.

Even containing git headers?

Yeah, it just skips right over them. I've never had even a minor
problem on that account, which is why I was surprised to see it giving
you so much trouble.

I haven't observed any such problems even with the rather ancient copy
of GNU patch on my HPUX box (seems to be 2.5.4, released in 1999).
I vaguely recall having had to replace the even older vendor-supplied
patch because that one didn't do unidiffs ...

regards, tom lane

Excerpts from Dimitri Fontaine's message of lun oct 03 11:54:36 -0300 2011:

Robert Haas <robertmhaas@gmail.com> writes:

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them. My Mac has 2.5.8 and
handles unidiffs no problem.

Even containing git headers?

I remember redirecting whole emails to "patch", and it worked just fine.
And this wasn't recently.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas@gmail.com> writes:

Yeah, it just skips right over them. I've never had even a minor
problem on that account, which is why I was surprised to see it giving
you so much trouble.

Ok then, I'll try some more next time. Been trying not to spend too
much time here… on the other hand git apply was happy to apply it…

Sorry for the noise,

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Oct 3, 2011 at 11:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I think that new versions of patch can handle unified diffs without a
problem, but older versions choke on them.  My Mac has 2.5.8 and
handles unidiffs no problem.

Even containing git headers?

Yeah, it just skips right over them.  I've never had even a minor
problem on that account, which is why I was surprised to see it giving
you so much trouble.

I haven't observed any such problems even with the rather ancient copy
of GNU patch on my HPUX box (seems to be 2.5.4, released in 1999).
I vaguely recall having had to replace the even older vendor-supplied
patch because that one didn't do unidiffs ...

I have seen unified diffs blow up when using patch on a fairly new
HP-UX box, but I'm not sure whether I'm using an OS-supplied copy of
patch or something someone installed along the line somewhere.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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 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.

get_object_namespace() looks like it ought to live in objectaddress.c,
not dropcmds.c. 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?
get_relation_address() is surely a copy of some other bit of code; how
can we avoid duplication?

Setting status to "Waiting on Author".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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>

On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

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.

Yeah, that's a problem that's been in the back of my mind for a bit
now, but I haven't come up with a good solution. I don't think
RemoveRelations() is the only place we have this problem, either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

The attached patches are revised ones.

* The array of ObjectProperty became a large flat data structure.
* get_object_property_xxx() put off the assumption the array of
ObjectProperty is indexed by OBJECT_* begin from 0.
* get_object_namespace() was relocated to objectaddress.c from
the dropcmds.c
* The logic of check_object_validation() got included within
get_relation_address(), and rewritten more smartly, as:

+   relkind = RelationGetForm(relation)->relkind;
+   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
+       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
+       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
+       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
+       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
+       ereport(ERROR,
+               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                errmsg("\"%s\" is not a %s",
+                       NameListToString(objname),
+                       get_object_property_typetext(objtype))));
+

Thanks,

2011/10/5 Robert Haas <robertmhaas@gmail.com>:

On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

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.

Yeah, that's a problem that's been in the back of my mind for a bit
now, but I haven't come up with a good solution.  I don't think
RemoveRelations() is the only place we have this problem, either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

* The logic of check_object_validation() got included within
 get_relation_address(), and rewritten more smartly, as:

+   relkind = RelationGetForm(relation)->relkind;
+   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
+       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
+       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
+       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
+       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
+       ereport(ERROR,
+               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                errmsg("\"%s\" is not a %s",
+                       NameListToString(objname),
+                       get_object_property_typetext(objtype))));
+

That's no good. We've discussed it before. It breaks translatability.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/10/5 Robert Haas <robertmhaas@gmail.com>:

On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

* The logic of check_object_validation() got included within
 get_relation_address(), and rewritten more smartly, as:

+   relkind = RelationGetForm(relation)->relkind;
+   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
+       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
+       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
+       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
+       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
+       ereport(ERROR,
+               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                errmsg("\"%s\" is not a %s",
+                       NameListToString(objname),
+                       get_object_property_typetext(objtype))));
+

That's no good.  We've discussed it before.  It breaks translatability.

Hmm. It indeed makes translation hard.
I reverted this portion of the part-2 patch, as attached.
Please review the newer one, instead of the previous revision.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Hmm. It indeed makes translation hard.
I reverted this portion of the part-2 patch, as attached.
Please review the newer one, instead of the previous revision.

Please fix the compiler warnings.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company