Re: [v9.2] DROP statement reworks

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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Hi,

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2011/8/15 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)2ndQuadrant(dot)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(at)2ndquadrant(dot)fr>:
> Hi,
>
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2011/8/15 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Hi,

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
>> <dimitri(at)2ndquadrant(dot)fr> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)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(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 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(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>

On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/10/10 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.
>
I checked compiler warnings using COPT=-Werror, but it detects warning on
only unrelated files as below. Does it really come from my patches?
(Does it depend on ./configure options?)

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -Werror -I../../../src/include -D_GNU_SOURCE -c -o
execQual.o execQual.c
execQual.c: In function ‘GetAttributeByNum’:
execQual.c:1104:67: error: the comparison will always evaluate as
‘true’ for the address of ‘tmptup’ will never be NULL
[-Werror=address]
execQual.c: In function ‘GetAttributeByName’:
execQual.c:1165:67: error: the comparison will always evaluate as
‘true’ for the address of ‘tmptup’ will never be NULL
[-Werror=address]
execQual.c: In function ‘ExecEvalFieldSelect’:
execQual.c:3914:67: error: the comparison will always evaluate as
‘true’ for the address of ‘tmptup’ will never be NULL
[-Werror=address]
cc1: all warnings being treated as errors

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -Werror -I../../../../src/include -D_GNU_SOURCE -c -o
tuplesort.o tuplesort.c
tuplesort.c: In function ‘comparetup_heap’:
tuplesort.c:2751:66: error: the comparison will always evaluate as
‘true’ for the address of ‘ltup’ will never be NULL [-Werror=address]
tuplesort.c:2752:66: error: the comparison will always evaluate as
‘true’ for the address of ‘rtup’ will never be NULL [-Werror=address]
tuplesort.c: In function ‘copytup_heap’:
tuplesort.c:2783:71: error: the comparison will always evaluate as
‘true’ for the address of ‘htup’ will never be NULL [-Werror=address]
tuplesort.c: In function ‘readtup_heap’:
tuplesort.c:2835:71: error: the comparison will always evaluate as
‘true’ for the address of ‘htup’ will never be NULL [-Werror=address]
cc1: all warnings being treated as errors

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Oct 10, 2011 at 9:39 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/10/10 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.
>>
> I checked compiler warnings using COPT=-Werror, but it detects warning on
> only unrelated files as below. Does it really come from my patches?
> (Does it depend on ./configure options?)

OK, well, I applied pgsql-v9.2-drop-reworks-2.v4.1.patch and tried to
compile, and got this:

In file included from ../../../src/include/catalog/dependency.h:17,
from dependency.c:19:
../../../src/include/catalog/objectaddress.h:21: warning: type
defaults to ‘int’ in declaration of ‘ObjectAddress’
../../../src/include/catalog/objectaddress.h:21: error: expected ‘;’,
‘,’ or ‘)’ before ‘*’ token

The problem here is pretty obvious: you've defined
get_object_namespace, which takes an argument of type ObjectAddress,
before defining the ObjectAddress datatype, which is the next thing in
the same header file. How does that even compile for you?

That's easy enough to fix, but then I get this:

objectaddress.c: In function ‘get_object_namespace’:
objectaddress.c:996: warning: implicit declaration of function
‘get_object_property_attnum_namespace’
objectaddress.c:1000: warning: implicit declaration of function
‘get_object_property_catid_by_oid’
objectaddress.c:1006: warning: implicit declaration of function
‘get_object_property_typetext’
objectaddress.c:1006: warning: format ‘%s’ expects type ‘char *’, but
argument 3 has type ‘int’

Maybe the problem here is that I've only applied the first patch, and
this stuff is cleaned up in the later patches in the series. But what
is the point of separating out the patches if you can't even compile
with only some of them applied?

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

> OK, well, I applied pgsql-v9.2-drop-reworks-2.v4.1.patch and tried to
> compile, and got this:
>
> In file included from ../../../src/include/catalog/dependency.h:17,
>                 from dependency.c:19:
> ../../../src/include/catalog/objectaddress.h:21: warning: type
> defaults to ‘int’ in declaration of ‘ObjectAddress’
> ../../../src/include/catalog/objectaddress.h:21: error: expected ‘;’,
> ‘,’ or ‘)’ before ‘*’ token
>
> The problem here is pretty obvious: you've defined
> get_object_namespace, which takes an argument of type ObjectAddress,
> before defining the ObjectAddress datatype, which is the next thing in
> the same header file.  How does that even compile for you?
>
Sorry, I didn't write out dependency of the patches.
Please apply the patches in order of part-1, part-2 then part-3.

I checked correctness of the part-2 on the tree with the part-1 already.
Both of the part-1 and part-2 patches try to modify objectaddress.h,
and the part-2 tries to add get_object_namespace() definition around
the code added by the part-1, so the patch commands get confused
and moved the hunk in front of the definition of ObjectAddress.

[kaigai(at)iwashi pgsql]$ cat
~/patch/pgsql-v9.2-drop-reworks-2.v4.1.patch | patch -p1
:
patching file src/backend/catalog/objectaddress.c
Hunk #1 succeeded at 976 (offset -429 lines).
:
patching file src/include/catalog/objectaddress.h
Hunk #1 succeeded at 17 with fuzz 2 (offset -18 lines).
:
patching file src/test/regress/expected/drop_if_exists.out

I'm sorry again. I tought it was obvious from the filenames.

* Part-1
pgsql-v9.2-drop-reworks-1.v4.patch
* Part-2 (depends on Part-1)
pgsql-v9.2-drop-reworks-2.v4.1.patch
* Part-3 (depends on Part-1 and -2)
pgsql-v9.2-drop-reworks-3.v4.patch

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I'm sorry again. I tought it was obvious from the filenames.

I guess I got confused because you re-posted part 2 without the other
parts, and I got mixed up and thought you were reposting part one.

I've committed a stripped-down version of the part one patch, which
had several mistakes even in just the part I committed - e.g., you
forgot TABLESPACEOID. I also did some renaming for clarity.

I'm going to throw this back to you for rebasing at this point, which
I realize is going to be somewhat of an unenjoyable task given the way
I cut up your changes to objectaddress.c, but I wasn't very confident
that all of the entries were correct (the one for attributes seemed
clearly wrong to me, for example), and I didn't want to commit a bunch
of stuff that wasn't going to be exercised. I suggest that you merge
the remainder of the part-one changes into part-two. On the flip
side, I think you should take the stuff that deals with dropping
relations OUT of part two. I don't see what good it does us to try to
centralize the drop logic if we still have to have special cases for
relations, so let's just leave that separate for now until we figure
out a better approach, or at least split it off as a separate patch so
that it doesn't hold up all the other changes.

I think get_object_namespace() needs substantial revision. Instead of
passing the object type and the object address, why not just pass the
object address? You should be able to use the classId in the address
to figure out everything you need to know. Since this function is
private to objectaddress.c, there's no reason for it to use those
accessor functions - it can just iterate through the array just as
object_exists() does. That way you also avoid iterating through the
array multiple times. I also think that we probably ought to revise
AlterObjectNamespace() to make use of this new machinery, instead of
making the caller pass in all the same information that
objectaddress.c is now learning how to provide. That would possibly
open the way to a bunch more consolidation of the SET SCHEMA code; in
fact, we might want to clean that up first, before dealing with the
DROP stuff.

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

Robert,

I'm currently trying to revise my patches according to your suggestions,
but I'm facing a trouble about error messages when user tries to drop
a non-exists object.

Because the ObjectProperty array has an entry for each catalogs, it is
unavailable to hold the name of object type (such as "table" or "index")
when multiple object types are associated with a particular system
catalog, such as pg_class, pg_type or pg_proc.

How should I implement the following block?

if (!OidIsValid(address.objectId))
{
ereport(NOTICE,
(errmsg("%s \"%s\" does not exist, skipping",
get_object_property_typetext(stmt->removeType),
NameListToString(objname))));
continue;
}

One idea is to add a separated array to translate from OBJECT_* to
its text representation. (Maybe, it can be used to translattions with
opposite direction.)

Thanks,

2011/10/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I'm sorry again. I tought it was obvious from the filenames.
>
> I guess I got confused because you re-posted part 2 without the other
> parts, and I got mixed up and thought you were reposting part one.
>
> I've committed a stripped-down version of the part one patch, which
> had several mistakes even in just the part I committed - e.g., you
> forgot TABLESPACEOID.  I also did some renaming for clarity.
>
> I'm going to throw this back to you for rebasing at this point, which
> I realize is going to be somewhat of an unenjoyable task given the way
> I cut up your changes to objectaddress.c, but I wasn't very confident
> that all of the entries were correct (the one for attributes seemed
> clearly wrong to me, for example), and I didn't want to commit a bunch
> of stuff that wasn't going to be exercised.  I suggest that you merge
> the remainder of the part-one changes into part-two.  On the flip
> side, I think you should take the stuff that deals with dropping
> relations OUT of part two.  I don't see what good it does us to try to
> centralize the drop logic if we still have to have special cases for
> relations, so let's just leave that separate for now until we figure
> out a better approach, or at least split it off as a separate patch so
> that it doesn't hold up all the other changes.
>
> I think get_object_namespace() needs substantial revision.  Instead of
> passing the object type and the object address, why not just pass the
> object address?  You should be able to use the classId in the address
> to figure out everything you need to know.  Since this function is
> private to objectaddress.c, there's no reason for it to use those
> accessor functions - it can just iterate through the array just as
> object_exists() does.  That way you also avoid iterating through the
> array multiple times.  I also think that we probably ought to revise
> AlterObjectNamespace() to make use of this new machinery, instead of
> making the caller pass in all the same information that
> objectaddress.c is now learning how to provide.  That would possibly
> open the way to a bunch more consolidation of the SET SCHEMA code; in
> fact, we might want to clean that up first, before dealing with the
> DROP stuff.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I'm currently trying to revise my patches according to your suggestions,
> but I'm facing a trouble about error messages when user tries to drop
> a non-exists object.
>
> Because the ObjectProperty array has an entry for each catalogs, it is
> unavailable to hold the name of object type (such as "table" or "index")
> when multiple object types are associated with a particular system
> catalog, such as pg_class, pg_type or pg_proc.
>
> How should I implement the following block?
>
>        if (!OidIsValid(address.objectId))
>        {
>            ereport(NOTICE,
>                    (errmsg("%s \"%s\" does not exist, skipping",
>                            get_object_property_typetext(stmt->removeType),
>                            NameListToString(objname))));
>            continue;
>        }
>
> One idea is to add a separated array to translate from OBJECT_* to
> its text representation. (Maybe, it can be used to translattions with
> opposite direction.)

For reasons of translation, you can't do something like "%s \"%s\"
does not exist, skipping". Instead I think you need an array that
works something like dropmsgstringarray[], but based on the OBJECT_*
constants rather than the RELKIND_* constants. IOW, it maps the
object type to the full error message, not just the name of the object
type.

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

2011/10/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I'm currently trying to revise my patches according to your suggestions,
>> but I'm facing a trouble about error messages when user tries to drop
>> a non-exists object.
>>
>> Because the ObjectProperty array has an entry for each catalogs, it is
>> unavailable to hold the name of object type (such as "table" or "index")
>> when multiple object types are associated with a particular system
>> catalog, such as pg_class, pg_type or pg_proc.
>>
>> How should I implement the following block?
>>
>>        if (!OidIsValid(address.objectId))
>>        {
>>            ereport(NOTICE,
>>                    (errmsg("%s \"%s\" does not exist, skipping",
>>                            get_object_property_typetext(stmt->removeType),
>>                            NameListToString(objname))));
>>            continue;
>>        }
>>
>> One idea is to add a separated array to translate from OBJECT_* to
>> its text representation. (Maybe, it can be used to translattions with
>> opposite direction.)
>
> For reasons of translation, you can't do something like "%s \"%s\"
> does not exist, skipping".  Instead I think you need an array that
> works something like dropmsgstringarray[], but based on the OBJECT_*
> constants rather than the RELKIND_* constants.  IOW, it maps the
> object type to the full error message, not just the name of the object
> type.
>
OK, I'll revise the code based on this idea.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patches are revised one, based on the latest version.

The get_object_namespace() got simplified to take only ObjectAddress,
and attnum_namespace was added to ObjectPropertyType array.

The DropErrorMsgNonExistent() is a function to raise a notice
when the supplied object name was missing and IF EXISTS was
also appended.
Unfortunately, it was not possible to hold all the notification
messages for each object types an array something like
dropmsgstringarray[] in tablecmds.c, because some of object
type requires to generate "%s" part using characteristic way.

Like:
+ case OBJECT_CAST:
+ msgfmt = gettext_noop("cast from type %s to type %s does
not exist, skipping");
+ marg1 = format_type_be(typenameTypeId(NULL,
+ (TypeName *) linitial(objname)));
+ marg2 = format_type_be(typenameTypeId(NULL,
+ (TypeName *) linitial(objargs)));
+ break;

So, I used a big switch statement to handle notification messages
when IF EXISTS was given.

And, also I added regression test cases to detect these code paths,
because some of object types does not cover the case when it was
dropped.

Reworking ALTER xxx SET SCHEMA/RENAME TO/SET OWNER
may be a homework of mine towards the next commit fest?

Thanks,

2011/10/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I'm sorry again. I tought it was obvious from the filenames.
>
> I guess I got confused because you re-posted part 2 without the other
> parts, and I got mixed up and thought you were reposting part one.
>
> I've committed a stripped-down version of the part one patch, which
> had several mistakes even in just the part I committed - e.g., you
> forgot TABLESPACEOID.  I also did some renaming for clarity.
>
> I'm going to throw this back to you for rebasing at this point, which
> I realize is going to be somewhat of an unenjoyable task given the way
> I cut up your changes to objectaddress.c, but I wasn't very confident
> that all of the entries were correct (the one for attributes seemed
> clearly wrong to me, for example), and I didn't want to commit a bunch
> of stuff that wasn't going to be exercised.  I suggest that you merge
> the remainder of the part-one changes into part-two.  On the flip
> side, I think you should take the stuff that deals with dropping
> relations OUT of part two.  I don't see what good it does us to try to
> centralize the drop logic if we still have to have special cases for
> relations, so let's just leave that separate for now until we figure
> out a better approach, or at least split it off as a separate patch so
> that it doesn't hold up all the other changes.
>
> I think get_object_namespace() needs substantial revision.  Instead of
> passing the object type and the object address, why not just pass the
> object address?  You should be able to use the classId in the address
> to figure out everything you need to know.  Since this function is
> private to objectaddress.c, there's no reason for it to use those
> accessor functions - it can just iterate through the array just as
> object_exists() does.  That way you also avoid iterating through the
> array multiple times.  I also think that we probably ought to revise
> AlterObjectNamespace() to make use of this new machinery, instead of
> making the caller pass in all the same information that
> objectaddress.c is now learning how to provide.  That would possibly
> open the way to a bunch more consolidation of the SET SCHEMA code; in
> fact, we might want to clean that up first, before dealing with the
> DROP stuff.
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Oct 13, 2011 at 12:46 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> And, also I added regression test cases to detect these code paths,
> because some of object types does not cover the case when it was
> dropped.

These regression tests seem busted to me. First, I applied the part 2
patch. The regression tests failed. Then, I applied the part 3
patch. Then they passed. So far so good. Then, I took the
regression test portion of the part 2 and part 3 patches and applied
just those. That also fails.

Can we come up with a set of regression tests that:

- passes on unmodified master
- still passes with the part 2 patch applied
- also passes with both the part 2 and part 3 patches applied

AIUI, this patch isn't supposed to be changing any behavior, just
consolidating the code.

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

2011/10/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Oct 13, 2011 at 12:46 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> And, also I added regression test cases to detect these code paths,
>> because some of object types does not cover the case when it was
>> dropped.
>
> These regression tests seem busted to me.  First, I applied the part 2
> patch.  The regression tests failed.  Then, I applied the part 3
> patch.  Then they passed.  So far so good.  Then, I took the
> regression test portion of the part 2 and part 3 patches and applied
> just those.  That also fails.
>
Sorry, it was my misses when I added regression test cases.

> Can we come up with a set of regression tests that:
>
> - passes on unmodified master
> - still passes with the part 2 patch applied
> - also passes with both the part 2 and part 3 patches applied
>
> AIUI, this patch isn't supposed to be changing any behavior, just
> consolidating the code.
>
The attached patches are:

part-1: only regression test of DROP [IF EXISTS] on unmodified master
part-2: drop statement reworks that uses T_DropStmt
part-3: drop statement reworks for other object classes

Unfortunately, the part-3 had to change regression test portion a bit,
because ...
- Unmodified master does not print ", skipping" when we tried to
drop non-existence operator class with IF EXISTS.
- Unmodified master raised an error, not notice, when we tried to
drop non-existence operator family with IF EXISTS.
Is it a bug to be fixed, isn't it?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Oct 19, 2011 at 3:30 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> part-1: only regression test of DROP [IF EXISTS] on unmodified master

Committed. Which I just noticed broke the build farm. Will go fix that now...

> part-2: drop statement reworks that uses T_DropStmt

Committed with some changes.

> part-3: drop statement reworks for other object classes

This is going to need some rebasing.

> Unfortunately, the part-3 had to change regression test portion a bit,
> because ...
>  - Unmodified master does not print ", skipping" when we tried to
>   drop non-existence operator class with IF EXISTS.

OK, we should fix that.

>  - Unmodified master raised an error, not notice, when we tried to
>   drop non-existence operator family with IF EXISTS.
>   Is it a bug to be fixed, isn't it?

Yeah, that seems like a bug.

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

On Wed, Oct 19, 2011 at 11:33 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Committed.  Which I just noticed broke the build farm.  Will go fix that now...

Wow, that was a lot of pretty colors. Or not so pretty colors.

Seems to be turning green again now, so I'm going to bed. Will check
it again in the morning.

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

>> part-3: drop statement reworks for other object classes
>
> This is going to need some rebasing.
>
OK, I rebased it.

This patch includes bug-fix when we tried to drop non-existence
operator family with IF EXISTS.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> part-3: drop statement reworks for other object classes
>>
>> This is going to need some rebasing.
>>
> OK, I rebased it.
>
> This patch includes bug-fix when we tried to drop non-existence
> operator family with IF EXISTS.

I'm thinking we should probably pull that part out and do it
separately. Seems like it should probably be back-patched.

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

2011/10/20 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> part-3: drop statement reworks for other object classes
>>>
>>> This is going to need some rebasing.
>>>
>> OK, I rebased it.
>>
>> This patch includes bug-fix when we tried to drop non-existence
>> operator family with IF EXISTS.
>
> I'm thinking we should probably pull that part out and do it
> separately.  Seems like it should probably be back-patched.
>
I checked REL9_0_STABLE branch:

It seems to me v9.0 implementation is correct. It might be enbugged
when OpFamilyCacheLookup() get missing_ok argument. :-(

/*
* RemoveOpFamily
* Deletes an opfamily.
*/
void
RemoveOpFamily(RemoveOpFamilyStmt *stmt)
{
Oid amID,
opfID;
HeapTuple tuple;
ObjectAddress object;

/*
* Get the access method's OID.
*/
amID = GetSysCacheOid1(AMNAME, CStringGetDatum(stmt->amname));
if (!OidIsValid(amID))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("access method \"%s\" does not exist",
stmt->amname)));

/*
* Look up the opfamily.
*/
tuple = OpFamilyCacheLookup(amID, stmt->opfamilyname);
if (!HeapTupleIsValid(tuple))
{
if (!stmt->missing_ok)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("operator family \"%s\" does not exist for
access method \"%s\"",
NameListToString(stmt->opfamilyname), stmt->amname)));
else
ereport(NOTICE,
(errmsg("operator family \"%s\" does not exist for
access method \"%s\"",
NameListToString(stmt->opfamilyname), stmt->amname)));
return;
}

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Oct 21, 2011 at 5:08 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> It seems to me v9.0 implementation is correct. It might be enbugged
> when OpFamilyCacheLookup() get missing_ok argument. :-(

Yep, looks that way. Will fix.

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