Re: [HACKERS] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. I agree with you. If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. On further review, my gut is having second thoughts. This patch is an awful lot smaller and easier to verify correctness if I just mess with the try calls and not the regular ones; and it avoids both back-patching hazards for us and hoops for third-party loadable modules that are using the non-try versions of those functions to jump through. Third try attached... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company there-is-no-try-v3.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote: On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: I agree with you. ?If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. On further review, my gut is having second thoughts. This patch is an awful lot smaller and easier to verify correctness if I just mess with the try calls and not the regular ones; and it avoids both back-patching hazards for us and hoops for third-party loadable modules that are using the non-try versions of those functions to jump through. +1. (Note that the function header comments need a few more updates.) -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Mon, Jun 27, 2011 at 2:59 PM, Noah Misch n...@leadboat.com wrote: On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote: On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: I agree with you. ?If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. On further review, my gut is having second thoughts. This patch is an awful lot smaller and easier to verify correctness if I just mess with the try calls and not the regular ones; and it avoids both back-patching hazards for us and hoops for third-party loadable modules that are using the non-try versions of those functions to jump through. +1. (Note that the function header comments need a few more updates.) Oh, good catch, thanks. Committed with some further comment changes. -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
The attached patch is rebased one towards the latest tree, using relation_openrv_extended(). Although it is not a matter in this patch itself, I found a problem on the upcoming patch that consolidate routines associated with DropStmt. Existing RemoveRelations() acquires a lock on the table owning an index to be removed in the case when OBJECT_INDEX is supplied. However, the revised get_object_address() opens the supplied relation (= index) in same time with lookup of its name. So, we may break down the relation_openrv_extended() into a pair of RangeVarGetRelid() and relation_open(). Any good idea? 2011/6/27 Robert Haas robertmh...@gmail.com: On Mon, Jun 27, 2011 at 2:59 PM, Noah Misch n...@leadboat.com wrote: On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote: On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: I agree with you. ?If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. On further review, my gut is having second thoughts. This patch is an awful lot smaller and easier to verify correctness if I just mess with the try calls and not the regular ones; and it avoids both back-patching hazards for us and hoops for third-party loadable modules that are using the non-try versions of those functions to jump through. +1. (Note that the function header comments need a few more updates.) Oh, good catch, thanks. Committed with some further comment changes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-drop-reworks-part-0.v5.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Mon, Jun 27, 2011 at 4:40 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is rebased one towards the latest tree, using relation_openrv_extended(). Committed. Although it is not a matter in this patch itself, I found a problem on the upcoming patch that consolidate routines associated with DropStmt. Existing RemoveRelations() acquires a lock on the table owning an index to be removed in the case when OBJECT_INDEX is supplied. However, the revised get_object_address() opens the supplied relation (= index) in same time with lookup of its name. So, we may break down the relation_openrv_extended() into a pair of RangeVarGetRelid() and relation_open(). Not without looking at the patch. I will respond on that thread when I've read through it more thoroughly. -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
I revised my patch based on your there-is-no-try-v2.patch. It enabled to implement 'missing_ok' support without modification of orders to solve the object name and relation locking. Thanks, 2011/6/22 Robert Haas robertmh...@gmail.com: On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. I agree with you. If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-drop-reworks-part-0.v4.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Wed, Jun 22, 2011 at 6:18 AM, Noah Misch n...@leadboat.com wrote: On Tue, Jun 21, 2011 at 11:11:41PM -0400, Robert Haas wrote: On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. ?It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. ?But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). ?If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? ?Comments? ?Objections? At least the last hunk (in pltcl.c) seems to have the flag backwards. Ah, nuts. Sorry. I think that and parse_relation.c are the only places where the try variants are used; nobody else is willing to fail, and everyone else is passing false. Revised patch attached. All existing call sites updated by this patch hardcode the flag, and only 3? proposed call sites would take advantage of the ability to not do so. The try_relation_openrv() case is fairly rare and likely to remain that way. Given that, I mildly prefer the code as it is, even if that means doing missing_ok ? try_relation_openrv() : relation_openrv() in a few places. Could always wrap that in a static function of objectaddress.c. I don't really like the idea of having a static function in objectaddress.c, because I think there will eventually be more callers who sometimes want to pass missing_ok = true and other times pass missing_ok = false. Plus, it seems a little nutty to have a function that, depending on whether its argument is true or false, calls one of two other functions which are virtually cut-and-paste copies of each other, except that one internally has true and the other false. I just work here, though. Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
Robert Haas robertmh...@gmail.com writes: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). +1 for that, although the try_ prefix might be inappropriate naming now; how about cond_relation_openrv? 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 Reworks Part.0 - 'missing_ok' support of get_object_address
Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. -- Á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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. I agree with you. If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Sun, Jun 19, 2011 at 7:40 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Sorry, the previous revision did not update regression test part towards the latest one. Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? Comments? Objections? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company there-is-no-try.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
Robert Haas robertmh...@gmail.com writes: Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? Comments? Objections? At least the last hunk (in pltcl.c) seems to have the flag backwards. 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? Comments? Objections? At least the last hunk (in pltcl.c) seems to have the flag backwards. Ah, nuts. Sorry. I think that and parse_relation.c are the only places where the try variants are used; nobody else is willing to fail, and everyone else is passing false. Revised patch attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company there-is-no-try-v2.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
Thanks for your review. 2011/6/19 Robert Haas robertmh...@gmail.com: On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is a preparation to rework implementation of DROP statement using a common code. That intends to apply get_object_address() to resolve name of objects to be removed, and eventually minimizes the number of places to put permission checks. Its first step is an enhancement of get_object_address; to accept 'missing_ok' argument to handle cases when IF EXISTS clause is supplied. If 'missing_ok' was true and the supplied name was not found, the patched get_object_address() returns an ObjectAddress with InvalidOid as objectId. If 'missing_ok' was false, its behavior is not changed. Let's consistently make missing_ok the last argument to all of the functions to which we're adding it. OK. I revised position of the 'missing_ok' argument. I don't think it's a good idea for get_relation_by_qualified_name() to be second-guessing the error message that RangeVarGetRelid() feels like throwing. OK. I revised the patch to provide 'true' on RangeVarGetRelid(). Its side effect is on error messages when user gives undefined object name. I think that attempting to fetch the column foo.bar when foo doesn't exist should be an error even if missing_ok is true. I believe that's consistent with what we do elsewhere. (If it *were* necessary to open the relation without failing if it's not there, you could use try_relation_openrv instead of doing as you've done here.) It was fixed. AlterTable() already open the relation (without missing_ok) in the case when we drop a column, so I don't think we need to acquire relation locks if the supplied column was missing. There is certainly a more compact way of writing the logic in get_object_address_typeobj. Also, I think that function should be called get_object_address_type(); the obj on the end seems redundant. I renamed the function name to get_object_address_type(), and consolidate initialization of ObjectAddress variables. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-drop-reworks-part-0.2.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
Sorry, the previous revision did not update regression test part towards the latest one. 2011/6/19 Kohei KaiGai kai...@kaigai.gr.jp: Thanks for your review. 2011/6/19 Robert Haas robertmh...@gmail.com: On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is a preparation to rework implementation of DROP statement using a common code. That intends to apply get_object_address() to resolve name of objects to be removed, and eventually minimizes the number of places to put permission checks. Its first step is an enhancement of get_object_address; to accept 'missing_ok' argument to handle cases when IF EXISTS clause is supplied. If 'missing_ok' was true and the supplied name was not found, the patched get_object_address() returns an ObjectAddress with InvalidOid as objectId. If 'missing_ok' was false, its behavior is not changed. Let's consistently make missing_ok the last argument to all of the functions to which we're adding it. OK. I revised position of the 'missing_ok' argument. I don't think it's a good idea for get_relation_by_qualified_name() to be second-guessing the error message that RangeVarGetRelid() feels like throwing. OK. I revised the patch to provide 'true' on RangeVarGetRelid(). Its side effect is on error messages when user gives undefined object name. I think that attempting to fetch the column foo.bar when foo doesn't exist should be an error even if missing_ok is true. I believe that's consistent with what we do elsewhere. (If it *were* necessary to open the relation without failing if it's not there, you could use try_relation_openrv instead of doing as you've done here.) It was fixed. AlterTable() already open the relation (without missing_ok) in the case when we drop a column, so I don't think we need to acquire relation locks if the supplied column was missing. There is certainly a more compact way of writing the logic in get_object_address_typeobj. Also, I think that function should be called get_object_address_type(); the obj on the end seems redundant. I renamed the function name to get_object_address_type(), and consolidate initialization of ObjectAddress variables. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-drop-reworks-part-0.3.patch Description: Binary data -- 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 Reworks Part.0 - 'missing_ok' support of get_object_address
On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is a preparation to rework implementation of DROP statement using a common code. That intends to apply get_object_address() to resolve name of objects to be removed, and eventually minimizes the number of places to put permission checks. Its first step is an enhancement of get_object_address; to accept 'missing_ok' argument to handle cases when IF EXISTS clause is supplied. If 'missing_ok' was true and the supplied name was not found, the patched get_object_address() returns an ObjectAddress with InvalidOid as objectId. If 'missing_ok' was false, its behavior is not changed. Let's consistently make missing_ok the last argument to all of the functions to which we're adding it. I don't think it's a good idea for get_relation_by_qualified_name() to be second-guessing the error message that RangeVarGetRelid() feels like throwing. I think that attempting to fetch the column foo.bar when foo doesn't exist should be an error even if missing_ok is true. I believe that's consistent with what we do elsewhere. (If it *were* necessary to open the relation without failing if it's not there, you could use try_relation_openrv instead of doing as you've done here.) There is certainly a more compact way of writing the logic in get_object_address_typeobj. Also, I think that function should be called get_object_address_type(); the obj on the end seems redundant. Apart from those comments this looks pretty sensible. -- 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
[HACKERS] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
The attached patch is a preparation to rework implementation of DROP statement using a common code. That intends to apply get_object_address() to resolve name of objects to be removed, and eventually minimizes the number of places to put permission checks. Its first step is an enhancement of get_object_address; to accept 'missing_ok' argument to handle cases when IF EXISTS clause is supplied. If 'missing_ok' was true and the supplied name was not found, the patched get_object_address() returns an ObjectAddress with InvalidOid as objectId. If 'missing_ok' was false, its behavior is not changed. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-drop-reworks-part-0.1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
