Re: [HACKERS] [v9.2] DROP statement reworks
2011/10/20 Robert Haas robertmh...@gmail.com:
On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Fri, Oct 21, 2011 at 5:08 AM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Thu, Oct 13, 2011 at 12:46 PM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 19, 2011 at 3:30 PM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 19, 2011 at 11:33 PM, Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
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 robertmh...@gmail.com:
On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai kai...@kaigai.gr.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
2011/10/12 Robert Haas robertmh...@gmail.com:
On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
2011/10/10 Robert Haas robertmh...@gmail.com: On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai kai...@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. 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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Mon, Oct 10, 2011 at 9:39 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/10/10 Robert Haas robertmh...@gmail.com: On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai kai...@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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
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@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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Thanks for your reviewing. 2011/10/4 Robert Haas robertmh...@gmail.com: On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai kai...@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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Sun, Oct 2, 2011 at 4:08 PM, Dimitri Fontaine dimi...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Robert Haas robertmh...@gmail.com writes: On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Excerpts from Dimitri Fontaine's message of lun oct 03 11:54:36 -0300 2011: Robert Haas robertmh...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Mon, Oct 3, 2011 at 11:28 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Hi, Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Dimitri Fontaine dimi...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Hi, Kohei KaiGai kai...@kaigai.gr.jp writes: 2011/8/15 Kohei KaiGai kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
