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] information schema parameter_default implementation
On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut pete...@gmx.net wrote: Here is an implementation of the information_schema.parameters.parameter_default column. I ended up writing a C function to decode the whole thing from the system catalogs, because it was too complicated in SQL, so I abandoned the approach discussed in [0]. [0]: http://archives.postgresql.org/message-id/1356092400.25658.6.ca...@vanquo.pezone.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers I checked our your patch. There seems to be an issue when we have OUT parameters after the DEFAULT values. For example a simple test case is given below: postgres=# CREATE FUNCTION functest1(a int default 1, out b int) postgres-# RETURNS int postgres-# LANGUAGE SQL postgres-# AS 'SELECT $1'; CREATE FUNCTION postgres=# postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE specific_name LIKE 'functest%' ORDER BY 1; ordinal_position | parameter_name | parameter_default --++--- 1 | a | 1 2 | b | 1 (2 rows) The out parameters gets the same value as the the last default parameter. The patch work only when default values are at the end. Switch the parameters and it starts working(make OUT parameter as first and default one the last one). Below is the example: postgres=# CREATE FUNCTION functest1(out a int, b int default 1) postgres-# RETURNS int postgres-# LANGUAGE SQL postgres-# AS 'SELECT $1'; CREATE FUNCTION postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE specific_name LIKE 'functest%' ORDER BY 1; ordinal_position | parameter_name | parameter_default --++--- 1 | a | 2 | b | 1 (2 rows) Some other minor observations: 1) Some variables are not lined in pg_get_function_arg_default(). 2) I found the following check a bit confusing, maybe you can make it better if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC) 2) inputargn can be assigned in declaration. 3) Function level comment for pg_get_function_arg_default() is missing. 4) You can also add comments inside the function, for example the comment for the line: nth = inputargn - 1 - (proc-pronargs - proc-pronargdefaults); 5) I think the line added in the documentation(informational_schema.sgml) is very long. Consider revising. Maybe change from: The default expression of the parameter, or null if none or if the function is not owned by a currently enabled role. TO The default expression of the parameter, or null if none was specified. It will also be null if the function is not owned by a currently enabled role. I don't know what do you exactly mean by: function is not owned by a currently enabled role? Regards, Ali Dar
Re: [HACKERS] information schema parameter_default implementation
Another thing I forget: The patch does not apply because of the changes in catversion.h Regards, Ali Dar On Thu, Jan 31, 2013 at 6:59 PM, Ali Dar ali.munir@gmail.com wrote: On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut pete...@gmx.net wrote: Here is an implementation of the information_schema.parameters.parameter_default column. I ended up writing a C function to decode the whole thing from the system catalogs, because it was too complicated in SQL, so I abandoned the approach discussed in [0]. [0]: http://archives.postgresql.org/message-id/1356092400.25658.6.ca...@vanquo.pezone.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers I checked our your patch. There seems to be an issue when we have OUT parameters after the DEFAULT values. For example a simple test case is given below: postgres=# CREATE FUNCTION functest1(a int default 1, out b int) postgres-# RETURNS int postgres-# LANGUAGE SQL postgres-# AS 'SELECT $1'; CREATE FUNCTION postgres=# postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE specific_name LIKE 'functest%' ORDER BY 1; ordinal_position | parameter_name | parameter_default --++--- 1 | a | 1 2 | b | 1 (2 rows) The out parameters gets the same value as the the last default parameter. The patch work only when default values are at the end. Switch the parameters and it starts working(make OUT parameter as first and default one the last one). Below is the example: postgres=# CREATE FUNCTION functest1(out a int, b int default 1) postgres-# RETURNS int postgres-# LANGUAGE SQL postgres-# AS 'SELECT $1'; CREATE FUNCTION postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE specific_name LIKE 'functest%' ORDER BY 1; ordinal_position | parameter_name | parameter_default --++--- 1 | a | 2 | b | 1 (2 rows) Some other minor observations: 1) Some variables are not lined in pg_get_function_arg_default(). 2) I found the following check a bit confusing, maybe you can make it better if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC) 2) inputargn can be assigned in declaration. 3) Function level comment for pg_get_function_arg_default() is missing. 4) You can also add comments inside the function, for example the comment for the line: nth = inputargn - 1 - (proc-pronargs - proc-pronargdefaults); 5) I think the line added in the documentation(informational_schema.sgml) is very long. Consider revising. Maybe change from: The default expression of the parameter, or null if none or if the function is not owned by a currently enabled role. TO The default expression of the parameter, or null if none was specified. It will also be null if the function is not owned by a currently enabled role. I don't know what do you exactly mean by: function is not owned by a currently enabled role? Regards, Ali Dar
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 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
[HACKERS] Review: Dumping an Extension's Script
I have done a high level review of the patch, and here are my comments: 1) I don't know if community is really looking for this functionality, I followed the thread a bit and appraently there is a disagreement over it. I will not comments on that. Let the big guys decide if they really want this feature. 2) I think comments are very limited, I would be really great if you can add thorough comments of whatever new that you have added. Couple of examples are given below: i) You have added 'script' variable in 'ExtensionControlFile' structure, but no comments. All other variables have one, you should follow the suite and we can understand what's going on at the first look. ii) What is the need of require option? It would be great if you can go through all the comments and try to add the missing comments and improve the old ones. 3) There are spelling mistakes in the existing comments, for example: i) File:pg_dump.c, line:850 ii) File: pg_dump.h line: 136 It would be good idea to run a spell check on your old and new comments. 4) 'if_no_exits' flag of new grammar rule is set to false, even though the rule states that the extension is created as 'IF NOT EXISTS'. 5) Why 'relocatable' option is added to the new grammar rule of create extension? In a regular 'create extension' statement, when 'schema' option is specified, it means that the extension is relocatable. So why is option needed? If there is a specific reason why 'scheme' option wasn't used, then please explain. 6) I am unable to figure out why new 'require' option is needed(comments would have helped)? Is it because when we will dump the extension with -X flag, it will first dump that referenced extension? I created the following extension(hstore was already created): create extension testex with version '1' requires 'hstore' as $testex$ create type testtype as(a char) $testex$; I then dumped the extension using -X option, only 'testex' extension was dumped and not 'hstore'. Apparently by looking at query being used to fetch extensions in dump, the extensions that it depends on are also being fetched. But it's not working like that. 7) You have removed an old PG comments in pg_dump.c line:7526 above the following check: if (!extinfo-dobj.dump || dataOnly) return; I guess it's been done in mistake? 8) In extension.c the following check is added: /* * If the extension script is not in the command, then the user is not * the extension packager and we want to read about relocatable and * requires in the control file, not in the SQL command. */ if (d_relocatable || d_requires) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg(parameter \%s\ is only expected when using CREATE EXTENSION AS, d_relocatable ? relocatable : requires))); Is the above code really reachable? If the user has not specified a script(old create extension syntax is used), then 'd_relocatable' and 'd_requires' will never be true, because the old 'create extension' syntax doesn't allow it. 9) For every extension being dumped, and then for every object inside that extension. The code traverses all the fetched items of the database to compare if that item belong to that specific extension. Because of that 'DumpableObject **dobjs' is traversed fully, every time a single extension is dumped. This seems to be big big performance hit for a huge database. And considering if many many extensions are dumped in a huge database, dump might become very slow. Can you think of any other scheme? Maybe you should follow PG's design, and fetch the extension objects in dumpExtension() and dump them. 10) pg_dumpall is not handled at all. User can't use -X switch if he wants to dump all the databases with extensions. Regards, Ali Dar
