Re: [HACKERS] missing rename support
Dean Rasheed dean.a.rash...@gmail.com writes: [ alter-rule-rename_complete.v2.patch ] Committed with assorted editorialization. Aside from cosmetic issues, the main changes were: * use RangeVarGetRelidExtended with a callback to perform the lookup and locking of the target relation. This is a new API that the original version of RenameRewriteRule couldn't have known about. I borrowed the code pretty much verbatim from renametrig(), and am now wondering whether there shouldn't be some attempt to unify the callbacks for this. * call CacheInvalidateRelcache to ensure that other sessions notice the rule tuple update. It may be that this isn't necessary because nothing looks at the rule name fields in relcache entries ... but I wouldn't bet on that, and in any case it seems like bad practice to let stale cache entries hang around. 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] missing rename support
The tweaks made by you seems fine. I'm good with it. Regards, Ali Dar On Sun, Feb 3, 2013 at 8:04 PM, Dean Rasheed dean.a.rash...@gmail.comwrote: On 29 January 2013 15:34, Ali Dar ali.munir@gmail.com wrote: Please find attached the complete patch for alter rename rule. I have followed all the suggestions. This looks good. I've tested it, and it appears to work as intended. I'm happy with the code, and the new docs and regression tests look OK. I have a couple of minor tweaks (see attached): * On the new manual page, I replaced table with table or view. * In the new tab-completion code, I modified the query so that it completes with tables as well as views, and limited the results to just those relations that have a rule with the name specified, otherwise the list of completions could be very long. If you're happy with these changes, I think this is ready for committer review. Regards, Dean
Re: [HACKERS] missing rename support
On 29 January 2013 15:34, Ali Dar ali.munir@gmail.com wrote: Please find attached the complete patch for alter rename rule. I have followed all the suggestions. This looks good. I've tested it, and it appears to work as intended. I'm happy with the code, and the new docs and regression tests look OK. I have a couple of minor tweaks (see attached): * On the new manual page, I replaced table with table or view. * In the new tab-completion code, I modified the query so that it completes with tables as well as views, and limited the results to just those relations that have a rule with the name specified, otherwise the list of completions could be very long. If you're happy with these changes, I think this is ready for committer review. Regards, Dean alter-rule-rename_complete.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] missing rename support
Please find attached the complete patch for alter rename rule. I have followed all the suggestions. Followings things are added in this updated patch: 1) Disallow alter rename of ON SELECT rules. 2) Remove warning. 3) Varibles are lined up. 4) Used qualified_name instead of makeRangeVarFromAnyName. 5) Throw appropriate error if user tries to alter rename rule on irrelavent object(e.g index). 6) Psql tab support added 7) Regression test cases added. 8) Documentation added. Regards, Ali Dar On Mon, Jan 21, 2013 at 12:34 AM, Dean Rasheed dean.a.rash...@gmail.comwrote: On 3 January 2013 13:49, Ali Dar ali.munir@gmail.com wrote: Find attached an initial patch for ALTER RENAME RULE feature. Please note that it does not have any documentation yet. Hi, I just got round to looking at this. All-in-all it looks OK. I just have a few more review comments, in addition to Stephen's comment about renaming SELECT rules... This compiler warning should be fixed with another #include: alter.c:107:4: warning: implicit declaration of function ‘RenameRewriteRule’ In gram.y, I think you can just use qualified_name instead of makeRangeVarFromAnyName(). In RenameRewriteRule(), I think it's worth doing a check to ensure that the relation actually is a table or a view (you might have some other relation kind at that point in the code). If the user accidentally types the name of an index, say, instead of a table, then it is better to throw an error saying xxx is not a table or a view rather than reporting that the rule doesn't exist. I think this could probably use some simple regression tests to test both the success and failure cases. It would be nice to extend psql tab completion to support this too, although perhaps that could be done as a separate patch. Don't forget the docs! Regards, Dean alter-rule-rename_complete.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] missing rename support
On 3 January 2013 13:49, Ali Dar ali.munir@gmail.com wrote: Find attached an initial patch for ALTER RENAME RULE feature. Please note that it does not have any documentation yet. Hi, I just got round to looking at this. All-in-all it looks OK. I just have a few more review comments, in addition to Stephen's comment about renaming SELECT rules... This compiler warning should be fixed with another #include: alter.c:107:4: warning: implicit declaration of function ‘RenameRewriteRule’ In gram.y, I think you can just use qualified_name instead of makeRangeVarFromAnyName(). In RenameRewriteRule(), I think it's worth doing a check to ensure that the relation actually is a table or a view (you might have some other relation kind at that point in the code). If the user accidentally types the name of an index, say, instead of a table, then it is better to throw an error saying xxx is not a table or a view rather than reporting that the rule doesn't exist. I think this could probably use some simple regression tests to test both the success and failure cases. It would be nice to extend psql tab completion to support this too, although perhaps that could be done as a separate patch. Don't forget the docs! Regards, Dean -- 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] missing rename support
* Ali Dar (ali.munir@gmail.com) wrote: Find attached an initial patch for ALTER RENAME RULE feature. Please note that it does not have any documentation yet. Just took a quick look through this. Seems to be alright, but why do we allow renaming ON SELECT rules at all, given that they must be named _RETURN? My thinking would be to check for that case and error out if someone tries it. You should try to keep variables lined up: Relationpg_rewrite_desc; HeapTuple ruletup; + Oid owningRel; should be: Relationpg_rewrite_desc; HeapTuple ruletup; + Oid owningRel; I'd also strongly recommend looking through that entire function very carefully. Code that's been #ifdef'd out tends to rot. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] missing rename support
Stephen Frost sfr...@snowman.net writes: * Ali Dar (ali.munir@gmail.com) wrote: Find attached an initial patch for ALTER RENAME RULE feature. Please note that it does not have any documentation yet. Just took a quick look through this. Seems to be alright, but why do we allow renaming ON SELECT rules at all, given that they must be named _RETURN? My thinking would be to check for that case and error out if someone tries it. Agreed, we should exclude ON SELECT rules. You should try to keep variables lined up: pgindent is probably a better answer than trying to get this right manually. 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] missing rename support
On Sat, Dec 3, 2011 at 4:46 PM, Peter Eisentraut peter_e(at)gmx(dot)net wrote: I noticed the following object types don't have support for an ALTER ... RENAME command: DOMAIN (but ALTER TYPE works) FOREIGN DATA WRAPPER OPERATOR RULE SERVER Are there any restrictions why these couldn't be added? I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Find attached an initial patch for ALTER RENAME RULE feature. Please note that it does not have any documentation yet. Alter_Rule_Rename.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] missing rename support
On Sat, Dec 3, 2011 at 4:46 PM, Peter Eisentraut pete...@gmx.net wrote: I noticed the following object types don't have support for an ALTER ... RENAME command: DOMAIN (but ALTER TYPE works) FOREIGN DATA WRAPPER OPERATOR RULE SERVER Are there any restrictions why these couldn't be added? I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. -- 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] missing rename support
Robert Haas robertmh...@gmail.com writes: I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. Note that renaming an index-based constraint should also rename the index. I believe the other direction works already. 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
