Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch
On Tue, Feb 9, 2010 at 5:49 AM, Leonardo F m_li...@yahoo.it wrote: Not even a comment? As I said, performance results on my system were very good Hi Leonardo, Perhaps you could supply a .sql file containing a testcase illustrating the performance benefits you tested with your patch -- as I understand, the sole purpose of this patch is to speed up CLUSTER -- along with your results? The existing src/test/regress/sql/cluster.sql looks like it only has some basic sanity checks in it, and not performance tests. An idea of what hardware you're testing on and any tweaks to postgresql.conf might be useful as well. I was able to apply your patch cleanly and run CLUSTER with it, and make check passed for me on OS X as well. Hope this helps, and sorry I'm not able to offer more technical advice on your patch. Josh
[HACKERS] patch: Distinguish between unique indexes and unique constraints
Addressing TODO item Distinguish between unique indexes and unique constraints in \d+ for psql, and picking up from thread: http://archives.postgresql.org/message-id/8780.1271187...@sss.pgh.pa.us Attached is a simple patch which clarifies unique constraints with UNIQUE CONSTRAINT in psql's \d+ description of a table. The appearance of unique indexes is left as-is. == Old \d+ display == Indexes: name_uniq_constr UNIQUE, btree (name) == New \d+ display == Indexes: name_uniq_constr UNIQUE CONSTRAINT, btree (name) Josh psql_constraints.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] patch: Distinguish between unique indexes and unique constraints
On Sun, Apr 18, 2010 at 11:41 AM, Robert Haas robertmh...@gmail.com wrote: Josh - you may want to add your patch here: https://commitfest.postgresql.org/action/commitfest_view/open Added, thanks! Josh -- 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] Dry-run mode for pg_archivecleanup
On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini gabriele.bartol...@2ndquadrant.it wrote: Hi guys, I have added the '-n' option to pg_archivecleanup which performs a dry-run and outputs the names of the files to be removed to stdout (making possible to pass the list via pipe to another process). Please find attached the small patch. I submit it to the CommitFest. Hi Gabriele, I have signed on to review this patch for the 2012-01 CF. The patch applies cleanly, includes the necessary documentation, and implements a useful feature. I think the actual debugging line: + fprintf(stdout, %s\n, WALFilePath); might need to be tweaked. First, it's printing to stdout, and I think pg_archivecleanup intentionally sends all its output to stderr, so that it may show up in the postmaster log. (I expect the dry-run mode would often be used to test out an archive_cleanup_command, instead of only in stand-alone mode, where stdout would be fine.) Also, I think the actual message should be something a little more descriptive than just the WALFilePath. In debug mode, pg_archivecleanup prints out a message like: fprintf(stderr, %s: removing file \%s\\n, progname, WALFilePath); I think we'd want to print something similar, i.e. would remove file Oh, and I think the removing file... debug message above should not be printed in dryrun-mode, lest we confuse the admin. Other than that, everything looks good. Josh -- 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] Dry-run mode for pg_archivecleanup
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini gabriele.bartol...@2ndquadrant.it wrote: My actual intention was to have the filename as output of the command, in order to easily pipe it to another script. Hence my first choice was to use the stdout channel, considering also that pg_archivecleanup in dry-run mode is harmless and does not touch the content of the directory. Oh, right - I should have re-read your initial email before diving into the patch. That all makes sense given your intended purpose. I guess your goal of constructing some simple way to pass the files which would be removed on to another script is a little different than what I initially thought the patch would be useful for, namely as a testing/debugging aid for an admin. Perhaps both goals could be met by making use of '--debug' together with '--dry-run'. If they are both on, then an additional message like pg_archivecleanup: would remove file ... would be printed to stderr, along with just the filename printed to stdout you already have. Josh -- 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] disable prompting by default in createuser
On Thu, Dec 22, 2011 at 2:26 PM, Peter Eisentraut pete...@gmx.net wrote: On lör, 2011-11-26 at 01:28 +0200, Peter Eisentraut wrote: I propose that we change createuser so that it does not prompt for anything by default. We can arrange options so that you can get prompts for whatever is missing, but by default, a call to createuser should just run CREATE USER with default options. The fact that createuser issues prompts is always annoying when you create setup scripts, because you have to be careful to specify all the necessary options, and they are inconsistent and different between versions (although the last change about that was a long time ago), and the whole behavior seems contrary to the behavior of all other utilities. I don't think there'd be a real loss by not prompting by default; after all, we don't really want to encourage users to create more superusers, do we? Comments? Patch attached. I'll add it to the next commitfest. I looked at this patch for the 2012-01 CF. I like the idea, the interactive-by-default behavior of createuser has always bugged me as well. I see this patch includes a small change to dropuser, to make the 'username' argument mandatory if --interactive is not set, for symmetry with createuser's new behavior. That's dandy, though IMO we shouldn't have -i be shorthand for --interactive with dropuser, and something different with createuser (i.e. we should just get rid of the i alias for dropuser). Another little inconsistency I see with the behavior when no username to create or drop is given: $ createuser createuser: creation of new role failed: ERROR: role josh already exists $ dropuser dropuser: missing required argument role name Try dropuser --help for more information. i.e. createuser tries taking either $PGUSER or the current username as a default user to create, while dropuser just bails out. Personally, I prefer just bailing out if no create/drop user is specified, but either way I think they should be consistent. Oh, and I think the leading whitespace of this help message: printf(_( --interactive prompt for missing role name and attributes rather\n should be made the same as for other commands with no short-alias, e.g. printf(_( --replication role can initiate replication\n)); Other than those little complaints, everything looks good. Josh -- 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] Psql names completion.
On Mon, Jan 23, 2012 at 5:28 PM, Dominik Bylica dominik2c...@gmail.com wrote: Hello. It's probably not the right place to write, but I guess you are there to take care of it. When I was creating a trigger with command like: create trigger asdf before update on beginninOfTheNameOfMyTable... I hit tab and I got: create trigger asdf before update on SET That was obviously not what I expected to get. Should be fixed in 9.2. Josh -- 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] Dry-run mode for pg_archivecleanup
On Fri, Jan 27, 2012 at 9:47 AM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini gabriele.bartol...@2ndquadrant.it wrote: My actual intention was to have the filename as output of the command, in order to easily pipe it to another script. Hence my first choice was to use the stdout channel, considering also that pg_archivecleanup in dry-run mode is harmless and does not touch the content of the directory. Oh, right - I should have re-read your initial email before diving into the patch. That all makes sense given your intended purpose. I guess your goal of constructing some simple way to pass the files which would be removed on to another script is a little different than what I initially thought the patch would be useful for, namely as a testing/debugging aid for an admin. Perhaps both goals could be met by making use of '--debug' together with '--dry-run'. If they are both on, then an additional message like pg_archivecleanup: would remove file ... would be printed to stderr, along with just the filename printed to stdout you already have. This email thread seems to have trailed off without reaching a conclusion. The patch is marked as Waiting on Author in the CommitFest application, but I'm not sure that's accurate. Can we try to nail this down? Perhaps my last email was a bit wordy. The only real change I am suggesting for Gabriele's patch is that the message printed to stderr when debug + dryrun are activated be changed to would remove file ... from removing file, i.e around line 124: if (debug) fprintf(stderr, %s: %s file \%s\\n, progname, (dryrun ? would remove : removing), WALFilePath); Other than that little quibble, I thought the patch was fine. Josh -- 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] Dry-run mode for pg_archivecleanup
On Tue, Jan 31, 2012 at 10:29 AM, Gabriele Bartolini gabriele.bartol...@2ndquadrant.it wrote: Here is my final version which embeds comments from Josh. I have also added debug information to be printed in case '-d' is given. Looks fine; will mark Ready For Committer. Josh -- 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] disable prompting by default in createuser
On Wed, Feb 1, 2012 at 1:13 PM, Peter Eisentraut pete...@gmx.net wrote: On sön, 2012-01-15 at 18:14 -0500, Josh Kupershmidt wrote: I see this patch includes a small change to dropuser, to make the 'username' argument mandatory if --interactive is not set, for symmetry with createuser's new behavior. That's dandy, though IMO we shouldn't have -i be shorthand for --interactive with dropuser, and something different with createuser (i.e. we should just get rid of the i alias for dropuser). Well, all the other tools also support -i for prompting. Taking a look at the current ./src/bin/scripts executables, I see only 2 out of 9 (`dropdb` and `dropuser`) which have -i mean --interactive, and `reindexdb` has another meaning for -i entirely. So I'm not sure there's such a clear precedent for having -i mean --interactive within our scripts, at least. I'd rather get rid of -i for --inherit, but I fear that will break things as well. I'm not sure what to do. I think breaking backwards compatibility probably won't fly (and should probably be handled by another patch, anyway). I guess it's OK to keep the patch's current behavior, given we are already inconsistent about what -i means. i.e. createuser tries taking either $PGUSER or the current username as a default user to create, while dropuser just bails out. Personally, I prefer just bailing out if no create/drop user is specified, but either way I think they should be consistent. That is intentional long-standing behavior. createdb/dropdb work the same way. OK. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] misleading error message from connectMaintenanceDatabase()
I noticed a misleading error message recently while using createdb. Try: test=# CREATE ROLE dummy NOLOGIN; Now, attempt to use createdb as that role. Here's 9.1.1: $ createdb -Udummy testdb createdb: could not connect to database postgres: FATAL: role dummy is not permitted to log in And here is git head: $ createdb -Udummy testdb createdb: could not connect to databases postgres or template1 Please specify an alternative maintenance database. Try createdb --help for more information. Although I guess you could argue the latter message is technically correct, since could not connect is true, the first error message seems much more helpful. Plus, Please specify an alternative maintenance database is a rather misleading hint to be giving in this situation. This seems to be happening because connectMaintenanceDatabase() is passing fail_ok = true to connectDatabase(), which in turn just returns NULL and doesn't print a PQerrorMessage() for the failed conn. So connectMaintenanceDatabase() has no idea why the connection really failed. A simple fix would be just to pass fail_ok = false for the last connectDatabase() call inside connectMaintenanceDatabase(), and give up on trying to tack on a likely-misleading hint about the maintenance database. Patch attached. This leads to: $ createdb -Udummy testdb createdb: could not connect to database template1: FATAL: role dummy is not permitted to log in which is almost the same as the 9.1.1 output, with the exception that template1 is mentioned by default instead of the postgres database. Josh connectMDB_error.diff 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] vacuumlo issue
On Tue, Mar 20, 2012 at 7:53 AM, Tom Lane t...@sss.pgh.pa.us wrote: I'm not entirely convinced that that was a good idea. However, so far as vacuumlo is concerned, the only reason this is a problem is that vacuumlo goes out of its way to do all the large-object deletions in a single transaction. What's the point of that? It'd be useful to batch them, probably, rather than commit each deletion individually. But the objects being deleted are by assumption unreferenced, so I see no correctness argument why they should need to go away all at once. I think you are asking for this option: -l LIMIT stop after removing LIMIT large objects which was added in b69f2e36402aaa. Josh -- 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] Default mode for shutdown
On Wed, Dec 15, 2010 at 10:11 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: It occurs to me that we may need a new mode, which disconnects sessions that are not in a transaction (or as soon as they are) but leaves in-progress transactions alone; this could be the new default. Of course, this is much more difficult to implement than the current modes. I like this idea, if it's feasible. Might I also suggest that the smart-mode shutdown give a HINT to the user that he can forcibly kill off existing sessions using -m fast. Right now, we show something like this: $ pg_ctl -D PGDATA stop waiting for server to shut down ... failed pg_ctl: server does not shut down And it's not immediately obvious to the user why the server didn't shut down, or how to fix things. Josh -- 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] psql: Add \dL to show languages
On Sat, Jan 15, 2011 at 8:26 PM, Andreas Karlsson andr...@proxel.se wrote: Hi Josh, Here is my review of this patch for the commitfest. Review of https://commitfest.postgresql.org/action/patch_view?id=439 Thanks a lot for the review! Contents and Purpose This patch adds the \dL command in psql to list the procedual languages. To me this seems like a useful addition to the commands in psql and \dL is also a quite sane name for it which follows the established conventions. Documentation of the new command is included and looks good. Runing it = Patch did not apply cleanly against HEAD so fixed it. I tested the comamnds, \dL, \dLS, \dL+, \dLS+ and they wroked as I expected. Support for patterns worked too and while not that useful it was not much code either. The psql completion worked fine too. Yeah, IIRC Fernando included pattern-completion in the original patch, and a reviewer said roughly the same thing -- it's probably overkill, but since it's just a small amount of code and it's already in there, no big deal. Some things I noticed when using it though. I do not like the use of parentheses in the usage description list (procedural) languages. Why not have it simply as list procedural languages? I agree. Should we include a column in \dL+ for the laninline function (DO blocks)? Hrm, I guess that could be useful for the verbose output at least. Performance === Quite irrelevant to this patch. :) Coding == In general the code looks good and follows conventions except for some whitesapce errors that I cleaned up. * Trailing whitespace in src/bin/psql/describe.c. * Incorrect indenation, spaces vs tabs in psql/describe.c and psql/tab-complete.c. * Removed empty line after return in listLanguages to match other functions. The comment for the function is not that descriptive but it is enough and fits with the other functions. Another things is that generated SQL needs its formatting fixed up in my oppinion. I recommend looking at the query built by \dL by using psql -E. Here is an example how the query looks for \dL+ SELECT l.lanname AS Procedural Language, pg_catalog.pg_get_userbyid(l.lanowner) as Owner, l.lanpltrusted AS Trusted, l.lanplcallfoid::regprocedure AS Call Handler, l.lanvalidator::regprocedure AS Validator, NOT l.lanispl AS System Language, pg_catalog.array_to_string(l.lanacl, E'\n') AS Access privileges FROM pg_catalog.pg_language l ORDER BY 1; Sorry, I don't understand.. what's wrong with the formatting of this query? In terms of whitespace, it looks pretty similar to listDomains() directly below listLanguages() in describe.c. Conclusion == The patch looks useful, worked, and there were no bugs obvious to me. The code also looks good and in line with other functions doing similar things. There are just some small issues that I think should be resolved before committing it: laninline, format of the built query and the usage string. Attached is a version that applies to current HEAD and with whitespace fixed. Regards, Andreas Thanks for the cleaned up patch. Josh -- 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] psql: Add \dL to show languages
On Sun, Jan 16, 2011 at 8:52 PM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jan 16, 2011 at 7:04 AM, Magnus Hagander mag...@hagander.net wrote: I do not like the use of parentheses in the usage description list (procedural) languages. Why not have it simply as list procedural languages? Because it lists non-procedural langauges as well? (I didn't check it, that's just a guess) There are many places in our code and documentation where procedural language or language are treated as synonyms. There's no semantic difference; procedural is simply a noise word. [bikeshedding] I agree with Andreas' suggestion that the help string be list procedural languages, even though the \dLS output looks something like this: List of languages Procedural Language | Owner | Trusted -+---+- c | josh | f internal| josh | f plpgsql | josh | t sql | josh | t (4 rows) which, as Magnus points out, includes non-procedural languages (SQL). I think that list languages could be confusing to newcomers -- the very people who might be reading through the help output of psql for the first time -- who might suppose that languages has something to do with the character sets supported by PostgreSQL, and might not even be aware that a variety of procedural languages can be used inside the database. Josh -- 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] psql: Add \dL to show languages
Hi all,
I've updated the patch to address the following points:
* help string now says list procedural languages (no parentheses now)
* the language name column is now titled Name
* added another column in verbose mode for 9.0+ showing whether DO
blocks are possible with the language. I named this column DO
Blocks?, though am open to suggestions
* fixed the whitespace problems Andreas noticed with the SELECT query
Looking at the verbose output, which looks something like this:
List of languages
Name| Owner | Trusted | Call Handler |
Validator| System Language | DO Blocks?
| Access privileges
---+---+-+-++-+---
-+---
plpgsql | josh | t | plpgsql_call_handler() |
plpgsql_validator(oid) | f | t
|
plpythonu | josh | f | plpython_call_handler() | -
| f | t
|
(2 rows)
I have a hard time imagining users who would find Call Handler or
Validator useful. This was in Fernando's original patch, and I just
didn't bother to take it out. If others feel the same way, I'd be
happy to rip those columns out.
Few more comments below:
On Mon, Jan 17, 2011 at 3:51 PM, Andreas Karlsson andr...@proxel.se wrote:
On Sun, 2011-01-16 at 22:32 -0500, Josh Kupershmidt wrote:
On Sat, Jan 15, 2011 at 8:26 PM, Andreas Karlsson andr...@proxel.se wrote:
Should we include a column in \dL+ for the laninline function (DO
blocks)?
Hrm, I guess that could be useful for the verbose output at least.
Magnus Hagander agreed with that idea and added that for that we need to
check the version. The column was added in 9.0 if I recall.
Added.
[snip]
* Missing indentation before ACL column, the other functions have it.
* One space before FROM instead of one newline like the other queries.
* The space before ORDER BY.
These should be fixed now.
That's enough nitpickery for now. :)
I spend enough of my time nitpicking others. Turnabout is fair play :)
Thanks for all the review and feedback from everyone.
Josh
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5f61561..30d4507 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=gt;
*** 1249,1254
--- 1249,1269
/listitem
/varlistentry
+ varlistentry
+ termliteral\dL[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
+ listitem
+ para
+ Lists all procedural languages. If replaceable
+ class=parameterpattern/replaceable
+ is specified, only languages whose names match the pattern are listed.
+ By default, only user-created languages
+ are shown; supply the literalS/literal modifier to include system
+ objects. If literal+/literal is appended to the command name, each
+ language is listed with its call handler, validator, access privileges,
+ and whether it is a system object.
+ /para
+ /listitem
+ /varlistentry
varlistentry
termliteral\dn[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 962c13c..301dc11 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 416,421
--- 416,424
case 'l':
success = do_lo_list();
break;
+ case 'L':
+ success = listLanguages(pattern, show_verbose, show_system);
+ break;
case 'n':
success = listSchemas(pattern, show_verbose, show_system);
break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 205190f..5984748 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** listTables(const char *tabtypes, const c
*** 2566,2571
--- 2566,2638
}
+ /* \dL
+ *
+ * Describes Languages.
+ */
+ bool
+ listLanguages(const char *pattern, bool verbose, bool showSystem)
+ {
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(buf);
+
+ printfPQExpBuffer(buf,
+ SELECT l.lanname AS \%s\,\n,
+ gettext_noop(Name));
+ if (pset.sversion = 80300)
+ appendPQExpBuffer(buf,
+ pg_catalog.pg_get_userbyid(l.lanowner) as \%s\,\n,
+ gettext_noop(Owner));
+
+ appendPQExpBuffer(buf,
+ l.lanpltrusted AS \%s\,
+ gettext_noop(Trusted));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(buf,
+ ,\n l.lanplcallfoid::regprocedure AS \%s\,\n
+ l.lanvalidator::regprocedure AS \%s\,\n
+ NOT l.lanispl AS \%s\,\n ,
+ gettext_noop(Call Handler),
+ gettext_noop(Validator
Re: [HACKERS] psql: Add \dL to show languages
On Tue, Jan 18, 2011 at 1:35 PM, Andreas Karlsson andr...@proxel.se wrote:
Hi Josh,
Nope, I do not have any better ideas than DO Blocks?.
Everything looks good with the exception one bug now.
\dL foo
* QUERY **
SELECT l.lanname AS Name,
pg_catalog.pg_get_userbyid(l.lanowner) as \Owner,
l.lanpltrusted AS Trusted
FROM pg_catalog.pg_language lWHERE l.lanname ~ '^(foo)$'
ORDER BY 1;
**
ERROR: syntax error at or near l
LINE 4: FROM pg_catalog.pg_language lWHERE l.lanname ~ '^(foo)$'
I believe the fix is to move \n from before the WHERE clause to after
the FROM, and from before ORDER BY to after WHERE.
Whoops, good you caught that. Should be fixed now.
Fix this bug and I believe this patch is ready for a committer.
PS. You added some trailing withspace after printACLColumn, A
recommendation if you want to avoid it is to either have a git commit
hook which checks for that and/or have colouring of git diffs so you can
see it marked in red. I use both. :)
Got that now too. I lost my ~/.emacs file recently, which is mostly
why I'm making whitespace mistakes. Rebuilding slowly though;
(setq-default show-trailing-whitespace t) is what I needed.
I left the Call Handler and Validator columns in the verbose
output since I haven't heard otherwise.
Josh
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5f61561..30d4507 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=gt;
*** 1249,1254
--- 1249,1269
/listitem
/varlistentry
+ varlistentry
+ termliteral\dL[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
+ listitem
+ para
+ Lists all procedural languages. If replaceable
+ class=parameterpattern/replaceable
+ is specified, only languages whose names match the pattern are listed.
+ By default, only user-created languages
+ are shown; supply the literalS/literal modifier to include system
+ objects. If literal+/literal is appended to the command name, each
+ language is listed with its call handler, validator, access privileges,
+ and whether it is a system object.
+ /para
+ /listitem
+ /varlistentry
varlistentry
termliteral\dn[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 962c13c..301dc11 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 416,421
--- 416,424
case 'l':
success = do_lo_list();
break;
+ case 'L':
+ success = listLanguages(pattern, show_verbose, show_system);
+ break;
case 'n':
success = listSchemas(pattern, show_verbose, show_system);
break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 205190f..abfd854 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** listTables(const char *tabtypes, const c
*** 2566,2571
--- 2566,2638
}
+ /* \dL
+ *
+ * Describes Languages.
+ */
+ bool
+ listLanguages(const char *pattern, bool verbose, bool showSystem)
+ {
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(buf);
+
+ printfPQExpBuffer(buf,
+ SELECT l.lanname AS \%s\,\n,
+ gettext_noop(Name));
+ if (pset.sversion = 80300)
+ appendPQExpBuffer(buf,
+ pg_catalog.pg_get_userbyid(l.lanowner) as \%s\,\n,
+ gettext_noop(Owner));
+
+ appendPQExpBuffer(buf,
+ l.lanpltrusted AS \%s\,
+ gettext_noop(Trusted));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(buf,
+ ,\n l.lanplcallfoid::regprocedure AS \%s\,\n
+ l.lanvalidator::regprocedure AS \%s\,\n
+ NOT l.lanispl AS \%s\,\n ,
+ gettext_noop(Call Handler),
+ gettext_noop(Validator),
+ gettext_noop(System Language));
+ if (pset.sversion = 9)
+ appendPQExpBuffer(buf, l.laninline != 0 AS \%s\,\n ,
+ gettext_noop(DO Blocks?));
+ printACLColumn(buf, l.lanacl);
+ }
+
+ appendPQExpBuffer(buf,
+ \nFROM pg_catalog.pg_language l\n);
+
+ processSQLNamePattern(pset.db, buf, pattern, false, false,
+ NULL, l.lanname, NULL, NULL);
+
+ if (!showSystem !pattern)
+ appendPQExpBuffer(buf, WHERE lanplcallfoid != 0\n);
+
+ appendPQExpBuffer(buf, ORDER BY 1;);
+
+ res = PSQLexec(buf.data, false);
+ termPQExpBuffer(buf);
+ if (!res)
+ return false;
+
+ myopt.nullPrint = NULL;
+ myopt.title = _(List of languages);
+ myopt.translate_header = true;
+
+ printQuery(res, myopt, pset.queryFout, pset.logfile);
+
+ PQclear(res);
+ return true;
+ }
+
+
/*
Re: [HACKERS] psql: Add \dL to show languages
On Wed, Jan 19, 2011 at 9:09 PM, Robert Haas robertmh...@gmail.com wrote:
This patch doesn't seem terribly consistent to me - we show the name
of the call handler and the name of the validator, but for the inline
handler we just indicate whether there is one or not. That seems like
something that we should make consistent, and my vote is to show the
name in all cases.
OK, changed. I've shuffled the columns slightly so that handlers and
Validator columns are next to each other.
Also, I'm wondering whether the System Language column be renamed to
Internal Language, for consistency with the terminology used here:
http://www.postgresql.org/docs/current/static/catalog-pg-language.html
Fixed, updated patch attached. Though, reading the description of
lanispl on that page, I wonder if user-defined language correctly
describes plpgsql these days, which comes installed by default.
Josh
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5f61561..30d4507 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=gt;
*** 1249,1254
--- 1249,1269
/listitem
/varlistentry
+ varlistentry
+ termliteral\dL[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
+ listitem
+ para
+ Lists all procedural languages. If replaceable
+ class=parameterpattern/replaceable
+ is specified, only languages whose names match the pattern are listed.
+ By default, only user-created languages
+ are shown; supply the literalS/literal modifier to include system
+ objects. If literal+/literal is appended to the command name, each
+ language is listed with its call handler, validator, access privileges,
+ and whether it is a system object.
+ /para
+ /listitem
+ /varlistentry
varlistentry
termliteral\dn[S+] [ link linkend=APP-PSQL-patternsreplaceable class=parameterpattern/replaceable/link ]/literal/term
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 962c13c..301dc11 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 416,421
--- 416,424
case 'l':
success = do_lo_list();
break;
+ case 'L':
+ success = listLanguages(pattern, show_verbose, show_system);
+ break;
case 'n':
success = listSchemas(pattern, show_verbose, show_system);
break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 205190f..e1db4c0 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** listTables(const char *tabtypes, const c
*** 2566,2571
--- 2566,2638
}
+ /* \dL
+ *
+ * Describes Languages.
+ */
+ bool
+ listLanguages(const char *pattern, bool verbose, bool showSystem)
+ {
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(buf);
+
+ printfPQExpBuffer(buf,
+ SELECT l.lanname AS \%s\,\n,
+ gettext_noop(Name));
+ if (pset.sversion = 80300)
+ appendPQExpBuffer(buf,
+ pg_catalog.pg_get_userbyid(l.lanowner) as \%s\,\n,
+ gettext_noop(Owner));
+
+ appendPQExpBuffer(buf,
+ l.lanpltrusted AS \%s\,
+ gettext_noop(Trusted));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(buf,
+ ,\n NOT l.lanispl AS \%s\,\n
+ l.lanplcallfoid::regprocedure AS \%s\,\n
+ l.lanvalidator::regprocedure AS \%s\,\n ,
+ gettext_noop(Internal Language),
+ gettext_noop(Call Handler),
+ gettext_noop(Validator));
+ if (pset.sversion = 9)
+ appendPQExpBuffer(buf, l.laninline::regprocedure AS \%s\,\n ,
+ gettext_noop(Inline Handler));
+ printACLColumn(buf, l.lanacl);
+ }
+
+ appendPQExpBuffer(buf,
+ \nFROM pg_catalog.pg_language l\n);
+
+ processSQLNamePattern(pset.db, buf, pattern, false, false,
+ NULL, l.lanname, NULL, NULL);
+
+ if (!showSystem !pattern)
+ appendPQExpBuffer(buf, WHERE lanplcallfoid != 0\n);
+
+ appendPQExpBuffer(buf, ORDER BY 1;);
+
+ res = PSQLexec(buf.data, false);
+ termPQExpBuffer(buf);
+ if (!res)
+ return false;
+
+ myopt.nullPrint = NULL;
+ myopt.title = _(List of languages);
+ myopt.translate_header = true;
+
+ printQuery(res, myopt, pset.queryFout, pset.logfile);
+
+ PQclear(res);
+ return true;
+ }
+
+
/*
* \dD
*
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 2029ef8..4e80bcf 100644
*** a/src/bin/psql/describe.h
--- b/src/bin/psql/describe.h
*** extern bool listUserMappings(const char
*** 84,88
--- 84,90
/* \det */
extern bool listForeignTables(const char *pattern, bool verbose);
+ /* \dL */
+ extern bool listLanguages(const char *pattern, bool verbose, bool
Re: [HACKERS] Suggestion for --truncate-tables to pg_restore
Hi Karl, I signed on to review this patch for the current CF. Most of the background for the patch seems to be in the message below, so I'm going to respond to this one first. On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc k...@meme.com wrote: On 09/20/2012 12:24:49 PM, Karl O. Pinc wrote: I've had problems using pg_restore --data-only when restoring individual schemas (which contain data which has had bad things done to it). --clean does not work well because of dependent objects in other schemas. OK. First, the problem: Begin with the following structure: CREATE TABLE schemaA.foo (id PRIMARY KEY, data INT); CREATE VIEW schemaB.bar AS SELECT * FROM schemaA.foo; Then, by accident, somebody does: UPDATE schemaA.foo SET data = data + (RANDOM() * 1000)::INT; So, you want to restore the data into schemaA.foo. But schemaA.foo has (bad) data in it that must first be removed. It would seem that using pg_restore --clean -n schemaA -t foo my_pg_dump_backup would solve the problem, it would drop schemaA.foo, recreate it, and then restore the data. But this does not work. schemaA.foo does not drop because it's got a dependent database object, schemaB.bar. Right. Of course there are manual work-arounds. One of these is truncating schemaA.foo and then doing a pg_restore with --data-only. Simply doing TRUNCATE manually was the first thought that occurred to me when I saw your example. The manual work-arounds become increasingly burdensome as you need to restore more tables. The case that motivated me was an attempt to restore the data in an entire schema, one which contained a significant number of tables. TBH, I didn't find the example above particularly compelling for demonstrating the need for this feature. If you've just got one table with dependent views which needs to be restored, it's pretty easy to manually TRUNCATE and have pg_restore --data-only reload the table. (And easy enough to combine the truncate and restore into a single transaction in case anything goes wrong, if need be.) But I'm willing to grant that this proposed feature is potentially as useful as existing restore-jiggering options like --disable-triggers. And I guess I could see that if you're really stuck having to perform a --data-only restore of many tables, this feature could come in handy. So, the idea here is to be able to do a data-only restore, first truncating the data in the tables being restored to remove the existing corrupted data. The proposal is to add a --truncate-tables option to pg_restore. There were some comments on syntax. I proposed to use -u as a short option. This was thought confusing, given it's use in other Unix command line programs (mysql). Since there's no obvious short option, forget it. Just have a long option. Agreed. Another choice is to avoid introducing yet another option and instead overload --clean so that when doing a --data-only restore --clean truncates tables and otherwise --clean retains the existing behavior of dropping and re-creating the restored objects. I like the --truncate-tables flag idea better than overloading --clean, since it makes the purpose immediately apparent. (I tested pg_restore with 9.1 and when --data-only is used --clean is ignored, it does not even produce a warning. This is arguably a bug.) +1 for having pg_restore bail out with an error if the user specifies --data-only --clean. By the same token, --clean and --truncate-tables together should also raise a not allowed error. More serious objections were raised regarding semantics. What if, instead, the initial structure looked like: CREATE TABLE schemaA.foo (id PRIMARY KEY, data INT); CREATE TABLE schemaB.bar (id INT CONSTRAINT bar_on_foo REFERENCES foo , moredata INT); With a case like this, in most real-world situations, you'd have to use pg_restore with --disable-triggers if you wanted to use --data-only and --truncate-tables. The possibility of foreign key referential integrity corruption is obvious. Why would --disable-triggers be used in this example? I don't think you could use --truncate-tables to restore only table foo, because you would get: ERROR: cannot truncate a table referenced in a foreign key constraint (At least, I think so, not having tried with the actual patch.) You could use --disable-triggers when restoring bar. Though if you're just enabling that option for performance purposes, and are unable to guarantee that the restore will leave the tables in a consistent state, well then it seems like you shouldn't use the option. Aside: Unless you're restoring databases in their entirety the pg_restore --disable-triggers option makes it easy to introduce foreign key referential integrity corruption. Yup, and I think the docs could do more to warn users about --disable-triggers in particular. And I see you've submitted a separate patch along those lines. In fact, since pg_restore
Re: [HACKERS] Suggestion for --truncate-tables to pg_restore
On Wed, Nov 21, 2012 at 5:48 AM, Karl O. Pinc k...@meme.com wrote: On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc k...@meme.com wrote: On 09/20/2012 12:24:49 PM, Karl O. Pinc wrote: OT: After looking at the code I found a number of conflicting option combinations are not tested for or rejected. I can't recall what they are now. The right way to fix these would be to send in a separate patch for each conflict all attached to one email/commitfest item? Or one patch that just gets adjusted until it's right? Typically I think it's easiest for the reviewer+committer to consider a bunch of such similar changes altogether in one patch, rather than in a handful of separate patches, though that's just my own preference. More serious objections were raised regarding semantics. What if, instead, the initial structure looked like: CREATE TABLE schemaA.foo (id PRIMARY KEY, data INT); CREATE TABLE schemaB.bar (id INT CONSTRAINT bar_on_foo REFERENCES foo , moredata INT); With a case like this, in most real-world situations, you'd have to use pg_restore with --disable-triggers if you wanted to use --data-only and --truncate-tables. The possibility of foreign key referential integrity corruption is obvious. Why would --disable-triggers be used in this example? I don't think you could use --truncate-tables to restore only table foo, because you would get: ERROR: cannot truncate a table referenced in a foreign key constraint (At least, I think so, not having tried with the actual patch.) You could use --disable-triggers when restoring bar. I tried it and you're quite right. (I thought I'd tried this before, but clearly I did not -- proving the utility of the review process. :-) My assumption was that since triggers were turned off that constraints, being triggers, would be off as well and therefore tables with foreign keys could be truncated. Obviously not, since the I get the above error. I just tried it. --disable-triggers does not disable constraints. Just to be clear, I believe the problem in this example is that --disable-triggers does not disable any foreign key constraints of other tables when you are restoring a single table. So with: pg_restore -1 --truncate-tables --disable-triggers --table=foo \ --data-only my.dump ... you would get the complaint ERROR: cannot truncate a table referenced in a foreign key constraint which is talking about bar's referencing foo, and there was no ALTER TABLE bar DISABLE TRIGGER ALL done, since bar isn't being restored. I don't have a quibble with this existing behavior -- you are instructing pg_restore to only mess with bar, after all. See below, though, for a comparison of --clean and --truncate-tables when restoring foo and bar together. For your first example, --truncate-tables seems to have some use, so that the admin isn't forced to recreate various views which may be dependent on the table. (Though it's not too difficult to work around this case today.) As an aside: I never have an issue with this, having planned ahead. I'm curious what the not-too-difficult work-around is that you have in mind. I'm not coming up with a tidy way to, e.g, pull a lot of views out of a dump. Well, for the first example, there was one table and only one view which depended on the table, so manually editing the list file like so: pg_restore --list --file=my.dump output.list # manually edit file output.list, select only entries pertaining # to the table and dependent view pg_restore -1 --clean --use-list=output.list ... isn't too arduous, though it would become so if you had more dependent views to worry about. I'm willing to count this use-case as a usability win for --truncate-tables, since with that option the restore can be boiled down to a short and sweet: pg_restore -1 --data-only --truncate-tables --table=mytable my.dump ... Though note this may not prove practical for large tables, since --data-only leaves constraints and indexes intact during the restore, and can be massively slower compared to adding the constraints only after the data has been COPYed in, as pg_restore otherwise does. For the second example involving FKs, I'm a little bit more hesitant about endorsing the use of --truncate-tables combined with --disable-triggers to potentially allow bogus FKs. I know this is possible to some extent today using the --disable-triggers option, but it makes me nervous to be adding a mode to pg_restore if it's primarily designed to work together with --disable-triggers as a larger foot-gun. This is the crux of the issue, and where I was thinking of taking this patch. I very often am of the mindset that foreign keys are no more or less special than other more complex data integrity rules enforced with triggers. (I suppose others might say the same regards to integrity enforced at the application level.) This naturally inclines me to think that
Re: [HACKERS] Suggestion for --truncate-tables to pg_restore
On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc k...@meme.com wrote: P.S. An outstanding question regards --truncate-tables is whether it should drop indexes before truncate and re-create them after restore. Sounds like it should. Well, that would improve performance, but it also makes the behavior of object significantly different from what one might expect from the name. One of the problems here is that there seem to be a number of slightly-different things that one might want to do, and it's not exactly clear what all of them are, or whether a reasonable number of options can cater to all of them. Another problem: attempting to drop a unique constraint or primary key (if we're counting these as indexes to be dropped and recreated, which they should be if the goal is reasonable restore performance) which is referenced by another table's foreign key will cause: ERROR: cannot drop constraint xxx on table yyy because other objects depend on it and as discussed upthread, it would be impolite for pg_restore to presume it should monkey with dropping+recreating other tables' constraints to work around this problem, not to mention impossible when pg_restore is not connected to the target database. It is a common administrative task to selectively restore some existing tables' contents from a backup, and IIRC was the impetus for this patch. Instead of adding a bunch of options to pg_restore, perhaps a separate tool specific to this task would be the way to go. It could handle the minutiae of truncating, dropping and recreating constraints and indexes of the target tables, and dealing with FKs sensibly, without worrying about conflicts with existing pg_restore options and behavior. Josh -- 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] Suggestion for --truncate-tables to pg_restore
Sorry for the delay in following up here. On Mon, Nov 26, 2012 at 8:30 PM, Karl O. Pinc k...@meme.com wrote: On 11/26/2012 08:45:08 PM, Josh Kupershmidt wrote: On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc k...@meme.com wrote: P.S. An outstanding question regards --truncate-tables is whether it should drop indexes before truncate and re-create them after restore. Sounds like it should. Well, that would improve performance, but it also makes the behavior of object significantly different from what one might expect from the name. One of the problems here is that there seem to be a number of slightly-different things that one might want to do, and it's not exactly clear what all of them are, or whether a reasonable number of options can cater to all of them. Another problem: attempting to drop a unique constraint or primary key (if we're counting these as indexes to be dropped and recreated, which they should be if the goal is reasonable restore performance) which is referenced by another table's foreign key will cause: ERROR: cannot drop constraint xxx on table yyy because other objects depend on it and as discussed upthread, it would be impolite for pg_restore to presume it should monkey with dropping+recreating other tables' constraints to work around this problem, not to mention impossible when pg_restore is not connected to the target database. I'm thinking impossible because it's impossible to know what the existing FKs are without a db connection. Impossible is a problem. You may have another reason why it's impossible. Yes, that's what I meant. Meanwhile it sounds like the --truncate-tables patch is looking less and less desirable. I'm ready for rejection, but will soldier on in the interest of not wasting other people work on this, if given direction to move forward. Well, as far as I was able to tell, the use-case where this patch worked without trouble was limited to restoring a table, or schema with table(s), that: a.) has some view(s) dependent on it b.) has no other tables with FK references to it, so that we don't run into: ERROR: cannot truncate a table referenced in a foreign key constraint c.) is not so large that it takes forever for data to be restored with indexes and constraints left intact d.) and whose admin does not want to use --clean plus a list-file which limits pg_restore to the table and its views I was initially hoping that the patch would be more useful for restoring a table with FKs pointing to it, but it seems the only reliable way to do this kind of selective restore with pg_restore is with --clean and editing the list-file. Editing the list-file is certainly tedious and prone to manual error, but I'm not sure this particular patch has a wide enough use-case to alleviate that pain significantly. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] allowing multiple PQclear() calls
The documentation for PQclear() doesn't say whether it is safe to call PQclear() more than once on the same PGresult pointer. In fact, it is not safe, but apparently only because of this last step: /* Free the PGresult structure itself */ free(res); The other members of PGresult which may be freed by PQclear are set to NULL or otherwise handled so as not to not be affected by a subsequent PQclear(). I find that accounting for whether I've already PQclear'ed a given PGresult can be quite tedious in some cases. For example, in the cleanup code at the end of a function where control may goto in case of a problem, it would be much simpler to unconditionally call PQclear() without worrying about whether this was already done. One can see an admittedly small illustration of this headache in pqSetenvPoll() in our own codebase, where several times PQclear(res); is called immediately before a goto error_return; Would it be crazy to add an already_freed flag to the pg_result struct which PQclear() would set, or some equivalent safety mechanism, to avoid this hassle for users? Josh -- 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] allowing multiple PQclear() calls
On Tue, Dec 11, 2012 at 5:18 AM, Boszormenyi Zoltan z...@cybertec.at wrote:
2012-12-11 12:45 keltezéssel, Simon Riggs írta:
On 11 December 2012 10:39, Marko Kreen mark...@gmail.com wrote:
On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt schmi...@gmail.com
wrote:
Would it be crazy to add an already_freed flag to the pg_result
struct which PQclear() would set, or some equivalent safety mechanism,
to avoid this hassle for users?
Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().
So why doesn't PQclear() do that?
Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?
Same can be said for e.g. PQfinish(). Calling it again crashes your client,
as I have recently discovered when I added atexit() functions that
does if (conn) PQfinish(conn); and the normal flow didn't do conn = NULL;
after it was done.
Ah, well. I guess using a macro like:
#define SafeClear(res) do {PQclear(res); res = NULL;} while (0);
will suffice for me.
Josh
--
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] Strange errors from 9.2.1 and 9.2.2 (I hope I'm missing something obvious)
On Tue, Dec 11, 2012 at 6:01 PM, David Gould da...@sonic.net wrote: I'm sure I've had a stroke or something in the middle of the night and just didn't notice, but I'm able to reproduce the following on three different hosts on both 9.2.1 and 9.2.2. As far as I know the only difference between these queries is whitespace since I just up-arrowed them in psql and deleted a space or lf. And as far as I can tell none of these errors are correct. Complete transcript, freshly started 9.2.2. dg@jekyl:~$ psql psql (9.2.2) Type help for help. dg=# CREATE TABLE t ( i INTEGER, PRIMARY KEY (i) ); ERROR: type key does not exist LINE 3: PRIMARY KEY (i) Hrm, although I didn't see such characters in your above text, perhaps you have some odd Unicode characters in your input. For example, the attached superficially similar input file will generate the same error message for me. (The odd character in my input is U+2060, 'Word Joiner', encoded 0xE2 0x81 0xA0.) Josh test.sql 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] Multiple --table options for other commands
On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com wrote: Hi Josh, I've signed up to review this patch. Thanks! I configured with assertions, built, and tested using the attached script. It seems to do what it's supposed to and the code looks ok to me. The docs build. The text is reasonable. I also diffed the output of the attached script with the output of an unpatched head and got what I expected. Cool test script. Yes, the current pg_restore silently ignores multiple --table arguments, and seems to use the last one. You are introducing a backwards incompatible change here. I don't know what to do about it, other than perhaps to have the patch go into 10.0 (!?) and introduce a patch now that complains about multiple --table arguments. On the other hand, perhaps it's simply undocumented what pg_restore does when given repeated, conflicting, arguments and we're free to change this. Any thoughts? Agreed with Robert that this change should be reasonable in a major version (i.e. 9.3). On 12/10/2012 09:23:03 PM, Karl O. Pinc wrote: On 10/30/2012 10:14:19 PM, Josh Kupershmidt wrote: I went ahead and cooked up a patch to allow pg_restore, clusterdb, vacuumdb, and reindexdb to accept multiple --table switches. Hope I didn't overlook a similar tool, but these were the only other commands I found taking a --table argument. I believe you need ellipses behind --table in the syntax summaries of the command reference docs. Hrm, I was following pg_dump's lead here for the .sgml docs, and didn't see anywhere that pg_dump makes the multiple --table syntax explicit other than in the explanatory text underneath the option. I also note that the pg_dump --help output says table(s) so you probably want to have pg_restore say the same now that it takes multiple tables. Good catch, will fix, and ditto reindexdb's --index help output. (It is possible that the --help output for pg_dump was worded to say table(s) because one can use a pattern --table specification with pg_dump, though IMO it's helpful to mention table(s) in the --help output for the rest of these programs as well, as a little reminder to the user.) Josh multiple_tables.v2.diff 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] Multiple --table options for other commands
On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc k...@meme.com wrote: On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote: On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com wrote: I believe you need ellipses behind --table in the syntax summaries of the command reference docs. Hrm, I was following pg_dump's lead here for the .sgml docs, and didn't see anywhere that pg_dump makes the multiple --table syntax explicit other than in the explanatory text underneath the option. Yes. I see. I didn't look at all the command's reference pages but did happen to look at clusterdb, which does have --table in the syntax summary. I just checked and you need to fix clusterdb, reindexdb, and vacuumdb. OK, I made some changes to the command synopses for these three commands. For some reason, reindexdb.sgml was slightly different from the other two, in that the --table and --index bits were underneath a group node instead of arg. I've changed that one to be like the other two, and made them all have: arg choice=opt rep=repeat to include the ellipses after the table -- I hope that matches what you had in mind. Josh multiple_tables.v3.diff 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] Multiple --table options for other commands
On Thu, Dec 13, 2012 at 6:05 AM, Karl O. Pinc k...@meme.com wrote: On 12/13/2012 12:35:14 AM, Karl O. Pinc wrote: On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote: On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc k...@meme.com wrote: On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote: On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com wrote: I believe you need ellipses behind --table in the syntax summaries of the command reference docs. Yes. I see. I didn't look at all the command's reference pages but did happen to look at clusterdb, which does have --table in the syntax summary. I just checked and you need to fix clusterdb, reindexdb, and vacuumdb. OK, I made some changes to the command synopses for these three commands. I want the ... outside the square braces, because the --table (or -t) must repeat for each table specified. Like: vacuumdb [connection-option...] [option...] [ --table | -t table [( column [,...] )] ]... [dbname] reindexdb [connection-option...] [ --table | -t table ]... [ --index | -i index ]... [dbname] Have you had trouble getting this to work? Perhaps arg choice=optarg rep=repeat ... would work? Well, I fooled around with the SGML for a bit and came up with something which has the ellipses in brackets: [ --table | -t table ] [...] which I like a little better than not having the second set of brackets. New version attached. Josh multiple_tables.v04.diff 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] Multiple --table options for other commands
On Thu, Dec 13, 2012 at 7:24 PM, Karl O. Pinc k...@meme.com wrote: The problem is that the pg man pages would then have a syntax different from all the other man pages in the system, which all have ... outside of square braces. See man cat, e.g. FWIW, `man cat` on my OS X machine has synopsis: cat [-benstuv] [file ...] though I see: cat [OPTION] [FILE]... on Debian. (I wonder if the problem is because the style sheets have been tweaked to work well with sql?) Because I don't see the traditional man page ellipsis syntax anywhere in the pg docs, and because all the pg client command line programs that use repeating arguments all have a simplified syntax summary with just [options ...] or some such, I suspect that there may be a problem putting the ellipsis outside of the square braces. Yeah, I tried a few different ways and didn't manage to get: [ --table | -t table ] ... It would be nice if we could get some guidance from someone more familiar with the pg docbook stylesheets. As a fallback I'd do to the clusterdb, reindexdb, and vacuumdb syntax summaries what was done to the pg_dump and pg_restore syntax summaries. Remove all the detail. This is sucky, and _still_ leaves the reference pages with a syntax summary syntax that differs from regular man pages, but because the text is relatively information free nobody notices. That should be how the v2 patch has it. My inclination, since you can't make it work and we don't seem to be getting any help here, is to remove all the detail in the syntax summaries, push it through to a committer, and get some feedback that way. If someone out there feels that the formatting of these commands' synopses should look like: [ --table | -t table ] ... and knows how to massage the SGML to get that, I'm happy to accommodate the change. Otherwise, I think either the v4 or v2 patch should be acceptable. Josh -- 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] Multiple --table options for other commands
On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc k...@meme.com wrote: My brain seems to have turned itself on. I went and (re)read the docbook manual and was able to come up with this, which works: arg choice=plain rep=repeatarg choice=opt group choice=plain arg choice=plainoption--table/option/arg arg choice=plainoption-t/option/arg /group replaceabletable/replaceable /arg/arg Yay! (indentation et-al aside) That does seem to work, thanks for figuring out the syntax. Sorry to be so persnickety, and unhelpful until now. It seemed like it should be doable, but something was going wrong between keyboard and chair. I guess I should be doing this when better rested. No problem, here is v5 with changed synopses. Josh multiple_tables.v5.diff 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] Patch for checking file parameters to psql before password prompt
On Sun, Dec 2, 2012 at 4:37 AM, Alastair Turner b...@ctrlf5.co.za wrote: Patch for the changes discussed in http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php attached (eventually ...) In summary: If the input file (-f) doesn't exist or the ouput or log files (-o and -l) can't be created psql exits before prompting for a password. I assume you meant -L instead of -l here for specifying psql's log file. I don't think that the inability to write to psql's log file should be treated as a fatal error, especially since it is not treated as such by the current code: $ psql test -L /tmp/not_allowed psql: could not open log file /tmp/not_allowed: Permission denied [... proceeds to psql prompt from here ...] and the user (or script) may still usefully perform his work. Whereas with your patch: $ psql test -L /tmp/not_allowed psql: could not open log file /tmp/not_allowed: Permission denied $ And IMO the same concern applies to the query results file, -o. Although +1 for the part about having psql exit early if the input filename does not exist, since psql already bails out in this case, and there is nothing else to be done in such case. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] discarding duplicate indexes
I recently came across a scenario like this (tested on git head): CREATE TABLE test (id int); CREATE INDEX test_idx1 ON test (id); CREATE INDEX test_idx2 ON test (id); CREATE TABLE test_copycat (LIKE test INCLUDING ALL); \d test_copycat Why do we end up with only one index on test_copycat? The culprit seems to be transformIndexConstraints(), which explains: * Scan the index list and remove any redundant index specifications. This * can happen if, for instance, the user writes UNIQUE PRIMARY KEY. A * strict reading of SQL92 would suggest raising an error instead, but * that strikes me as too anal-retentive. - tgl 2001-02-14 and this code happily throws out the second index statement in this example, since its properties are identical to the first. (Side note: some index properties, such as tablespace specification and comment, are ignored when determining duplicates). This behavior does seem like a minor POLA violation to me -- if we do not forbid duplicate indexes on the original table, it seems surprising to do so silently with INCLUDING INDEXES. There was consideration of similar behavior when this patch was proposed[1], so perhaps the behavior is as-designed, and I guess no one else has complained. IMO this behavior should at least be documented under the LIKE source_table section of CREATE TABLE's doc page. Josh [1] http://archives.postgresql.org/pgsql-patches/2007-07/msg00173.php -- 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] discarding duplicate indexes
On Thu, Dec 20, 2012 at 1:26 AM, Gavin Flower gavinflo...@archidevsys.co.nz wrote: On 20/12/12 14:57, Josh Kupershmidt wrote: CREATE TABLE test (id int); CREATE INDEX test_idx1 ON test (id); CREATE INDEX test_idx2 ON test (id); I initially misread your example code, but after I realised my mistake, I thought of an alternative scenario that might be worth considering. CREATE TABLE test (id int, int sub, text payload); CREATE INDEX test_idx1 ON test (id, sub); CREATE INDEX test_idx2 ON test (id); Now test_idx2 is logically included in test_idx1, but if the majority of transactions only query on id, then test_idx2 would be more better as it ties up less RAM Well, this situation works without any LIKE ... INCLUDING INDEXES surprises. If you CREATE TABLE test_copycat (LIKE test INCLUDING INDEXES); you should see test_copycat created with both indexes, since indexParams is considered for this deduplicating. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] bad examples in pg_dump README
The ./src/bin/pg_dump README contains several inoperable examples. First, this suggestion: | or to list tables: | | pg_restore backup-file --table | less seems bogus, i.e. the --table option requires an argument specifing which table to restore, and does not list tables. Next, this suggested command: | pg_restore backup-file -l --oid --rearrange | less hasn't worked since 7.4 or thereabouts, since we don't have --rearrange or --oid anymore. (I'm not sure we ever had --oid, maybe it was supposed to be --oid-order?). Then, three examples claim we support a --use flag, e.g. | pg_restore backup.bck --use=toc.lis script.sql where presumably --use-list was meant. Further little gripes with this README include mixed use of tabs and spaces for the examples, and lines running over the 80-column limit which at least some of our other READMEs seem to honor. I started out attempting to fix up the README, keeping the original example commands intact, but upon further reflection I think the examples of dumping, restoring, and selective-restoring in that REAMDE don't belong there anyway. There are already better examples of such usage in the pg_dump/pg_restore documentation pages, and IMO that's where such generally-useful usage information belongs. I propose slimming down the pg_dump README, keeping intact the introductory notes and details of the tar format. Josh pg_dump_README.diff 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] bad examples in pg_dump README
On Sat, Jan 5, 2013 at 7:34 AM, Magnus Hagander mag...@hagander.net wrote: On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt schmi...@gmail.com wrote: Do we need to keep it at all, really? Certainly the introductory part is covered in the main documentation already... Pretty much. (I did find note #2 mildly interesting.) I believe the tar notes are also out of date. For example, they claim that you can't expect pg_restore to work with an uncompressed tar format - yet the header in pg_backup_tar.c says that an uncompressed tar format is compatible with a directory format dump, and thus you *can* use pg_restore. (And fwiw,the note about the user should probably go in src/port/ now that we moved the tar header creation there a few days ago) Hrm yeah, so the second paragraph under the tar section can certainly be axed. I would suggest we just drop the README file completely. I don't think it adds any value at all. Any objections to that path? :) I think that's OK, since there's not much left in that README after removing the bogus examples, introductory text that's covered elsewhere, and obsolete second paragraph about the tar format. Perhaps we could keep the other paragraphs about the tar format, either in the header comments for pg_backup_tar.c or in src/port/, though? Oh, and for this comment in pg_backup_tar.c: | * See the headers to pg_backup_files pg_restore for more details. there is no longer a pg_backup_files.c. Josh -- 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] bad examples in pg_dump README
On Mon, Jan 7, 2013 at 8:12 PM, Peter Eisentraut pete...@gmx.net wrote: On Sat, 2013-01-05 at 15:34 +0100, Magnus Hagander wrote: On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt schmi...@gmail.com wrote: I propose slimming down the pg_dump README, keeping intact the introductory notes and details of the tar format. Do we need to keep it at all, really? Certainly the introductory part is covered in the main documentation already... I'd remove it and distribute the remaining information, if any, between the source code and the man page. Here's a patch to do so. After removing the discussed bogus information, there were only two notes which I still found relevant, so I stuck those in the appropriate header comments. I didn't preserve the comment about blank username group for tar'ed files, since there are already some comments along these lines in tarCreateHeader(), and these are postgres not blank nowadays. The pg_dump/pg_restore man pages seemed to already do a good enough job of showing usage examples that I didn't find anything worth adding there. Josh pg_dump_README.v2.diff 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] string escaping in tutorial/syscat.source
On Tue, Jan 15, 2013 at 6:35 PM, Jeff Janes jeff.ja...@gmail.com wrote: Do you propose back-patching this? You could argue that this is a bug in 9.1 and 9.2. Before that, they generate deprecation warnings, but do not give the wrong answer. I think that backpatching to 9.1 would be reasonable, though I won't complain if the fix is only applied to HEAD. If it is only to be applied to HEAD, or only to 9.1, 9.2, and HEAD, then this part seems to be unnecessary and I think should be removed (setting a value to its default is more likely to cause confusion than remove confusion): SET standard_conforming_strings TO on; and the corresponding reset as well. Well, it may be unnecessary for people who use the modern default standard_conforming_strings. But some people have kept standard_conforming_strings=off in recent versions because they have old code which depends on this. And besides, it seems prudent to make the dependency explicit, rather than just hoping the user has the correct setting, and silently giving wrong results if not. Plus being able to work with old server versions. Other than that, it does what it says (fix a broken example), does it cleanly, we want this, and I have no other concerns. I guess the src/tutorial directory could participate in regression tests, in which case this problem would have been detected when introduced, but I don't think I can demand that you invent regression tests for a feature you are just fixing rather than creating. Yeah, I don't think tying into the regression tests is warranted here. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] fixing pg_ctl with relative paths
There have been some complaints[1][2] in the past about pg_ctl not playing nice with relative path specifications for the datadir. Here's a concise illustration: $ mkdir /tmp/mydata/ initdb /tmp/mydata/ $ cd /tmp/ $ pg_ctl -D ./mydata/ start $ cd / $ pg_ctl -D /tmp/mydata/ restart IMO it's pretty hard to defend the behavior of the last step, where pg_ctl knows exactly which datadir the user has specified, and succeeds in stopping the server but not starting it. Digging into this gripe, a related problem I noticed is that `pg_ctl ... restart` doesn't always preserve the -D $DATADIR argument as the following comment suggests it should[4]: * We could pass PGDATA just in an environment * variable but we do -D too for clearer postmaster * 'ps' display Specifically, if one passes in additional -o options, e.g. $ pg_ctl -D /tmp/mydata/ -o -N 10 restart after which postmaster.opts will be missing the -D ... argument which is otherwise recorded, and the `ps` output is similarly abridged. Anyway, Tom suggested[3] two possible ways of fixing the original gripe, and I went with his latter suggestion, | for pg_ctl restart to override that | option with its freshly-derived idea of where the data directory is mainly so we don't need to worry about the impact of changing the appearance of postmaster.opts, plus it seems like this code fits better in pg_ctl.c rather than the postmaster (where the postmaster.opts file is actually written). The fix seems to be pretty simple, namely stripping post_opts of the old -D ... portion and having the new specification, if any, be used instead. I believe the attached patch should fix these two infelicities. Full disclosure: the strip_datadirs() function makes no attempt to properly handle pathnames containing quotes. There seems to be some, uh, confusion in the existing code about how these should be handled already. For instance, $ mkdir /tmp/here's a \ quote $ initdb /tmp/here's a \ quote How to successfully start, restart, and stop the server with pg_ctl is left as an exercise for the reader. So I tried to avoid that can of worms with this patch, though I'm happy to robustify strip_datadirs() if we'd like to properly support such pathnames, and there's consensus on how to standardize the escaping. Josh [1] http://www.postgresql.org/message-id/caa-alv72o+negjaiphormbqsmztkzayatxrd+c9v60ybmmm...@mail.gmail.com [2] http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g...@mail.gmail.com [3] http://www.postgresql.org/message-id/27233.1350234...@sss.pgh.pa.us [4] Note, ps output and postmaster.opts will not include the datadir specification if you rely solely on the PGDATA environment variable for pg_ctl pgctl_paths.v01.diff 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] autocomplete - SELECT fx
On Mon, Jul 2, 2012 at 1:13 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I tested Peter's patch and it works well. I liked it as well. But I'm not sure what should happen with the patch now. It seems like it'd be commit-ready with just a tweak or two to the query as I noted in my last mail, but Tom did seem opposed[1] to the idea in the first thread, and no one else has spoken up recently in favor of the idea, so maybe it should just be marked Rejected or Returned with Feedback? Josh [1] http://archives.postgresql.org/message-id/13612.1328887227%40sss.pgh.pa.us -- 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] autocomplete - SELECT fx
On Sat, Jul 7, 2012 at 5:43 PM, Noah Misch n...@leadboat.com wrote: I like the patch, as far as it goes. It's the natural addition to the completions we already offer; compare the simplistic completion after WHERE. Like Pavel and Robert, I think a delightful implementation of tab completion for SELECT statements would require radical change. Thanks for the feedback, Noah. Peter, are you interested in posting an updated version of your patch? (The only problems I remember are checking attisdropped and function visibility.) Josh -- 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] [PATCH] psql \n shortcut for set search_path =
On Tue, Jul 10, 2012 at 2:09 AM, Colin 't Hart co...@sharpheart.org wrote: Attached please find a trivial patch for psql which adds a \n meta command as a shortcut for typing set search_path =. I think the use-case is a bit narrow: saving a few characters typing on a command not everyone uses very often (I don't), at the expense of adding yet another command to remember. Plus it opens the floodgates to people wanting yet more separate commands for other possibly commonly-used SET commands. There are a lot of GUCs, after all, even counting only those changeable at runtime. Josh -- 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] pg_reorg in core?
On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier michael.paqu...@gmail.com wrote: Hi all, During the last PGCon, I heard that some community members would be interested in having pg_reorg directly in core. I'm actually not crazy about this idea, at least not given the current state of pg_reorg. Right now, there are a quite a few fixes and features which remain to be merged in to cvs head, but at least we can develop pg_reorg on a schedule independent of Postgres itself, i.e. we can release new features more often than once a year. Perhaps when pg_reorg is more stable, and the known bugs and missing features have been ironed out, we could think about integrating into core. Granted, a nice thing about integrating with core is we'd probably have more of an early warning when reshuffling of PG breaks pg_reorg (e.g. the recent splitting of the htup headers), but such changes have been quick and easy to fix so far. Just to recall, pg_reorg is a functionality developped by NTT that allows to redistribute a table without taking locks on it. The technique it uses to reorganize the table is to create a temporary copy of the table to be redistributed with a CREATE TABLE AS whose definition changes if table is redistributed with a VACUUM FULL or CLUSTER. Then it follows this mechanism: - triggers are created to redirect all the DMLs that occur on the table to an intermediate log table. N.B. CREATE TRIGGER takes an AccessExclusiveLock on the table, see below. - creation of indexes on the temporary table based on what the user wishes - Apply the logs registered during the index creation - Swap the names of freshly created table and old table - Drop the useless objects The code is hosted by pg_foundry here: http://pgfoundry.org/projects/reorg/. I am also maintaining a fork in github in sync with pgfoundry here: https://github.com/michaelpq/pg_reorg. Just, do you guys think it is worth adding a functionality like pg_reorg in core or not? If yes, well I think the code of pg_reorg is going to need some modifications to make it more compatible with contrib modules using only EXTENSION. For the time being pg_reorg is divided into 2 parts, binary and library. The library part is the SQL portion of pg_reorg, containing a set of C functions that are called by the binary part. This has been extended to support CREATE EXTENSION recently. The binary part creates a command pg_reorg in charge of calling the set of functions created by the lib part, being just a wrapper of the library part to control the creation and deletion of the objects. It is also in charge of deleting the temporary objects by callback if an error occurs. By using the binary command, it is possible to reorganize a single table or a database, in this case reorganizing a database launches only a loop on each table of this database. My idea is to remove the binary part and to rely only on the library part to make pg_reorg a single extension with only system functions like other contrib modules. In order to do that what is missing is a function that could be used as an entry point for table reorganization, a function of the type pg_reorg_table(tableoid) and pg_reorg_table(tableoid, text). All the functionalities of pg_reorg could be reproducible: - pg_reorg_table(tableoid) for a VACUUM FULL reorganization - pg_reorg_table(tableoid, NULL) for a CLUSTER reorganization if table has a CLUSTER key - pg_reorg_table(tableoid, columnname) for a CLUSTER reorganization based on a wanted column. Is it worth the shot? I haven't seen this documented as such, but AFAICT the reason that pg_reorg is split into a binary and set of backend functions which are called by the binary is that pg_reorg needs to be able to control its steps in several transactions so as to avoid holding locks excessively. The reorg_one_table() function uses four or five transactions per table, in fact. If all the logic currently in the pg_reorg binary were moved into backend functions, calling pg_reorg_table() would have to be a single transaction, and there would be no advantage to using such a function vs. CLUSTER or VACUUM FULL. Also, having a separate binary we should be able to perform some neat tricks such as parallel index builds using multiple connections (I'm messing around with this idea now). AFAIK this would also not be possible if pg_reorg were contained solely in the library functions. Josh -- 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] pg_reorg in core?
On Thu, Sep 20, 2012 at 8:33 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Sep 21, 2012 at 12:07 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier michael.paqu...@gmail.com wrote: What could be also great is to move the project directly into github to facilitate its maintenance and development. No argument from me there, especially as I have my own fork in github, but that's up to the current maintainers. Granted, a nice thing about integrating with core is we'd probably have more of an early warning when reshuffling of PG breaks pg_reorg (e.g. the recent splitting of the htup headers), but such changes have been quick and easy to fix so far. Yes, that is also why I am proposing to integrate it into core. Its maintenance pace would be faster and easier than it is now in pgfoundry. If the argument for moving pg_reorg into core is faster and easier development, well I don't really buy that. Yes, there would presumably be more eyeballs on the project, but you could make the same argument about any auxiliary Postgres project which wants more attention, and we can't have everything in core. And I fail to see how being in-core makes development easier; I think everyone here would agree that the bar to commit things to core is pretty darn high. If you're concerned about the [lack of] development on pg_reorg, there are plenty of things to fix without moving the project. I recently posted an issues roundup to the reorg list, if you are interested in pitching in. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] string escaping in tutorial/syscat.source
Hi all, It seems the queries in ./src/tutorial/syscat.source use string escaping with the assumption that standard_conforming_strings is off, and thus give wrong results with modern versions. A simple fix is attached. Josh syscat.source_escaping.diff 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] Multiple --table options for other commands
Hi all, I see there's already a TODO for allowing pg_restore to accept multiple --table arguments[1], but would folks support adding this capability to various other commands which currently accept only a single --table argument, such as clusterdb, vacuumdb, and reindexdb? Looking at pg_dump, it seems like these other commands would want to use the SimpleStringList / SimpleStringCell machinery which currently lives only in pg_dump. I'm guessing that ./src/bin/pg_dump/dumputils.c would be the right place for such common code? Josh [1] http://archives.postgresql.org/pgsql-hackers/2010-08/msg00689.php -- 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] Multiple --table options for other commands
On Sun, Oct 28, 2012 at 4:30 PM, Josh Kupershmidt schmi...@gmail.com wrote:
I see there's already a TODO for allowing pg_restore to accept
multiple --table arguments[1], but would folks support adding this
capability to various other commands which currently accept only a
single --table argument, such as clusterdb, vacuumdb, and reindexdb?
I went ahead and cooked up a patch to allow pg_restore, clusterdb,
vacuumdb, and reindexdb to accept multiple --table switches. Hope I
didn't overlook a similar tool, but these were the only other commands
I found taking a --table argument.
If you run, say:
pg_dump -Fc --table=tab1 --table=tab2 ... |
pg_restore --table=tab1 --table=tab2 ...
without the patch, only tab2 will be restored and pg_restore will
make no mention of this omission to the user. With the patch, both
tables should be restored.
I also added support for multiple --index options for reindexdb, since
that use-case seemed symmetrical to --table. As suggested previously,
I moved the SimpleStringList types and functions out of pg_dump.h and
common.c into dumputils.{h,c}, so that the code in ./src/bin/scripts/
could use it.
If there are no objections, I can add the patch to the next CF.
Josh
multiple_tables.v1.diff
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] psql: tab completions for 'WITH'
Hi all, I noticed psql's tab-completion for 'WITH' is a bit overeager. If you try to tab-complete commands like: ALTER ROLE jsmith WITH [TAB] COPY tbl FROM 'filename' WITH [TAB] you'll get 'RECURSIVE' unhelpfully filled in. I think 'RECURSIVE' should only be suggested if 'WITH' is the first and only word of the line. On a related note, I found it annoying that after fixing the above problem, trying: ALTER ROLE jsmith WITH [TAB] CREATE ROLE jsmith WITH [TAB] didn't suggest any tab-completions -- it only works if you leave off the 'WITH' noise word, which I happen to use. Attached are fixes for both of these gripes. I'll add to the next CF. Josh overeager_with.diff Description: Binary data create_alter_role_tab_complete.diff 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] psql: tab completions for 'WITH'
On Tue, Apr 10, 2012 at 10:38 AM, Peter Eisentraut pete...@gmx.net wrote: On tis, 2012-04-03 at 22:34 -0700, Josh Kupershmidt wrote: I noticed psql's tab-completion for 'WITH' is a bit overeager. If you try to tab-complete commands like: ALTER ROLE jsmith WITH [TAB] COPY tbl FROM 'filename' WITH [TAB] you'll get 'RECURSIVE' unhelpfully filled in. I think 'RECURSIVE' should only be suggested if 'WITH' is the first and only word of the line. Committed that. Thanks! On a related note, I found it annoying that after fixing the above problem, trying: ALTER ROLE jsmith WITH [TAB] CREATE ROLE jsmith WITH [TAB] didn't suggest any tab-completions -- it only works if you leave off the 'WITH' noise word, which I happen to use. Hmm, but now you've set it up so that you can complete ALTER ROLE foo WITH WITH. Were you aware of that? D'oh, I overlooked that. Attached is v2: the diff is a tad lengthier now, but that should fix it. Josh create_alter_role_tab_complete.v2.diff 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] Last gasp
On Wed, Apr 11, 2012 at 8:59 AM, Peter Geoghegan pe...@2ndquadrant.com wrote: On 11 April 2012 15:35, Magnus Hagander mag...@hagander.net wrote: For example, Thom (and others) could collect a number of typo fixes in their own repo and then just ask for a merge.The advantage over just staging multiple commits and then submitting a patch would be that multiple people could work on it... This is hardly a radical idea at all - it's basically how git was really intended to be used at scale. Of course, unless some committer is going to make it their responsibility to merge those commits say every 3 months, there's no point in bothering. This could consolidate the number of typo commits to boot, as they could be rebased. TBH, I find it slightly embarrassing to have to ask a committer to fix a minor typo, and it's hardly reasonable to expect me to save my typos up. Big +1 from me. Particularly for the docs, it'd be nice to have more committer bandwidth available, if there's a reasonable way to do so without causing needless merge work for existing committers. Like Peter, I hate to bother busy committers with trivial typofixes, and sometimes I just don't bother sending such changes in, and they get lost :-( Maybe keeping doc/ as a 'git submodule' could work? Or, as Tom suggests, adding a global committer who could focus on docs changes would effectively solve the problem as well. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql: server version check for \dO
Hi all,
I think psql's \dO command is missing the server version check which
similar commands such as \dx use. Right now \dO errors out with:
test=# \dO
ERROR: relation pg_catalog.pg_collation does not exist
when talking to servers older than 9.1, which don't have pg_collation.
Simple patch for listCollations() attached.
Josh
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index 8dfb570..2cfacd3
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** listCollations(const char *pattern, bool
*** 3061,3066
--- 3061,3073
printQueryOpt myopt = pset.popt;
static const bool translate_columns[] = {false, false, false, false, false};
+ if (pset.sversion 90100)
+ {
+ fprintf(stderr, _(The server (version %d.%d) does not support collations.\n),
+ pset.sversion / 1, (pset.sversion / 100) % 100);
+ return true;
+ }
+
initPQExpBuffer(buf);
printfPQExpBuffer(buf,
--
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] Draft release notes complete
On Wed, May 9, 2012 at 8:11 PM, Bruce Momjian br...@momjian.us wrote: I have completed my draft of the 9.2 release notes, and committed it to git. I am waiting for our development docs to build, but after 40 minutes, I am still waiting: This bit: Previously supplied years and year masks of less than four digits wrapped inconsistently. I first read as Previously-supplied years... instead of Previously, years and year masks with In line with what Robert said, IMO he should be credited on pg_opfamily_is_visible(), particularly since it was his idea and code originally IIRC. And a few more I'm familiar with with, such as psql's \ir which he substantially revised, not to mention his much-appreciated assistance with all the psql comment-displaying under 'E.1.3.9.2.1. Comments'. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_restore logging inconsistency
Hi all, Bosco Rama recently complained[1] about not seeing a message printed by pg_restore for each LO to be restored. The culprit seems to be the different level passed to ahlog() for this status message: pg_backup_archiver.c: ahlog(AH, 2, restoring large object with OID %u\n, oid); pg_backup_tar.c:ahlog(AH, 1, restoring large object OID %u\n, oid); depending on whether one is restoring a tar-format or custom-format dump. I think these messages should be logged at the same level, to avoid this inconsistency. The attached patch logs them both with level=1, and makes the message texts identical. Note, as of 9.0 there is already a line like this printed for each LO: pg_restore: executing BLOB 135004 so I could see the argument for instead wanting to hide the restoring large object messages. However, the OP was interested in seeing something like a status indicator for the lo_write() calls which may take a long time, and the above message isn't really helpful for that purpose as it is printed earlier in the restore process. Plus it seems reasonable to make verbose mode, well, verbose. Josh [1] http://archives.postgresql.org/pgsql-general/2012-05/msg00456.php pg_restore_lo_message.diff 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] [BUGS] Tab completion of function arguments not working in all cases
[Hope it's OK if I move this thread to -hackers, as part of CF review.] On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: Hi, I noticed this while testing 9.2, but it seems to go back to at least 8.3. Tab completion of function arguments doesn't work if the function is schema-qualified or double-quoted. So for example, DROP FUNCTION my_function ( TAB completes the functions arguments, but DROP FUNCTION my_schema.my_function ( TAB doesn't offer any completions, and nor does DROP FUNCTION my function ( TAB +1 for the idea. I find the existing behavior rather confusing, particularly the fact that a schema-qualified function name will be tab-completed, i.e. this works. DROP FUNCTION my_schema.myTAB but then, as your second example above shows, no completions are subsequently offered for the function arguments. As a side note unrelated to this patch, I also dislike how function name tab-completions will not fill in the opening parenthesis, which makes for unnecessary work for the user, as one then has to type the parenthesis and hit tab again to get possible completions for the function arguments. The current behavior causes: DROP FUNCTION my_fTAB which completes to: DROP FUNCTION my_function enter parenthesis, and hit tab: DROP FUNCTION my_function(TAB which, if there is only one match, could complete to: DROP FUNCTION my_function(integer) when the last three steps could have been consolidated with better tab-completion. Perhaps this could be a TODO. The attached patch fixes these problems by introducing a new macro COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which seems to be the nearest analogous code that covers all the edge cases. Anyway, on to the review of the patch itself: * Submission * Patch applies cleanly to git head, and regression tests are not expected for tab-completion enhancements. * Features Usability * I've verified that tab-completing of the first argument to functions for DROP FUNCTION and ALTER FUNCTION commands for the most part works as expected. The one catch I noticed was that Query_for_list_of_arguments wasn't restricting its results to currently-visible functions, so with a default search_path, if you have these two functions defined: public.doppelganger(text) my_schema.doppelganger(bytea) and then try: DROP FUNCTION doppelganger(TAB you get tab-completions for both text) and bytea(, when you probably expected only the former. That's easy to fix though, please see attached v2 patch. * Coding * The new macro COMPLETE_WITH_ARG seems fine. The existing code used malloc() directly for DROP FUNCTION and ALTER FUNCTION (tab-complete.c, around lines 867 and 2190), which AIUI is frowned upon in favor of pg_malloc(). The patch avoids this ugliness by using the new COMPLETE_WITH_ARG macro, so that's a nice fixup. Overall, a nice fix for an overlooked piece of the tab-completion machinery. Josh tab-complete.funcargs.v2.diff 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] [BUGS] Tab completion of function arguments not working in all cases
On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: On 18 June 2012 04:21, Josh Kupershmidt schmi...@gmail.com wrote: As a side note unrelated to this patch, I also dislike how function name tab-completions will not fill in the opening parenthesis, which makes for unnecessary work for the user, as one then has to type the parenthesis and hit tab again to get possible completions for the function arguments. The current behavior causes: DROP FUNCTION my_fTAB which completes to: DROP FUNCTION my_function enter parenthesis, and hit tab: DROP FUNCTION my_function(TAB which, if there is only one match, could complete to: DROP FUNCTION my_function(integer) when the last three steps could have been consolidated with better tab-completion. Perhaps this could be a TODO. Hmm, I find that it does automatically fill in the opening parenthesis, but only once there is a space after the completed function name. So DROP FUNCTION my_fTAB completes to DROP FUNCTION my_function (note the space at the end). Then pressing TAB one more time gives DROP FUNCTION my_function ( , and then pressing TAB again gives the function arguments. Alternatively DROP FUNCTION my_functionTAB (no space after function name) first completes to DROP FUNCTION my_function (adding the space), and then completes with the opening parenthesis, and then with the function arguments. It's a bit clunky, but I find that repeatedly pressing TAB is easier than typing the opening bracket. Interesting, I see the behavior you describe on Linux, using psql + libreadline, and the behavior I showed (i.e. repeatedly pressing tab won't automatically fill in the function arguments after the function name is completed, seemingly because no space is deposited after the completed function name) is with OS X 10.6, using psql + libedit. [snip] you get tab-completions for both text) and bytea(, when you probably expected only the former. That's easy to fix though, please see attached v2 patch. Good catch. I think that's a useful additional test, and is also consistent with the existing code in Query_for_list_of_attributes. OK, I'll mark Ready for Committer in the CF. Josh -- 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] patch: autocomplete for functions
On Sun, Feb 19, 2012 at 12:10 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
Hello
I found so this extremely simple patch should be useful.
It helps for pattern SELECT fx();
There was thread about it.
Hi Pavel,
I signed up to be reviewer for this patch, and finally got around to
taking a look. This thread, and the thread about Peter's earlier patch
along the same lines have gotten a bit muddled, so allow me some recap
for my own sanity.
The first point to be addressed, is that Pavel's patch is basically a
subset of Peter's earlier[1] patch. Pavel's patch autocompletes
SELECT TAB
with possible function names. Peter's patch autocompletes both
possible column names and possible function names. So, which version,
if any, would we want? Both Tom[2] and depesz[3] have asked for column
names to be autocompleted if we're going to go down this road, and I
personally would find completion of column names handy. Others [5][6]
have asked for function names to be (also?) autocompleted here, so it
seems reasonable that we'd want to include both.
I counted two general objections to Peter's patch in these threads, namely:
1.) Complaints about the tab-completion not covering all cases,
possibly leading to user frustration at our inconsistency. [2] [4]
2.) Concerns that the tab-completion wouldn't be useful given how
many results we'd have from system columns and functions [7]
I do agree these are two legitimate concerns. However, for #1, our
tab-completion is and has always been incomplete. I take the basic
goal of the tab-completion machinery to be offer a suggestion when
we're pretty sure we know what the user wants, otherwise stay quiet.
There are all sorts of places where our reliance on inspecting back
only a few words into the current line and not having a true command
parser prevents us from being able to make tab-completion guesses, but
that's been accepted so far, and I don't think it's fair to raise the
bar for this patch.
Re: concern #2, Tom complained about there being a bunch of possible
column and function completions in the regression test database. That
may be true, but if you look at this slightly-modified version of the
query Peter's patch proposes:
SELECT substring(name, 1, 3) AS sub, COUNT(*)
FROM (
SELECT attname FROM pg_attribute WHERE NOT attisdropped
UNION
SELECT proname || '(' FROM pg_proc p WHERE
pg_catalog.pg_function_is_visible(p.oid)) t (name)
GROUP BY sub ORDER BY COUNT(*) DESC;
I count only 384 distinct 3-length prefixes in an empty database,
thanks to many built-in columns and functions sharing the same prefix
(e.g. int or pg_). Obviously, there is plenty of room left for
3-length prefixes, out of the 27^3 or more possibilities. In some
casual mucking around in my own databases, I found the
column-completion rather useful, and typing 3 characters of a
column-name to be sufficient to give matches which were at least
non-builtin attributes, and often a single unique match.
There were some ideas down-thread about how we might filter out some
of the noise in these completions, which would be interesting. I'd be
happy with the patch as-is though, modulo the attisdropped and
pg_function_is_visible() tweaks to the query.
Josh
[1]
http://archives.postgresql.org/message-id/1328820579.11241.4.camel%40vanquo.pezone.net
[2] http://archives.postgresql.org/message-id/7745.1328855069%40sss.pgh.pa.us
[3] http://www.depesz.com/2011/07/08/wish-list-for-psql/
[4] http://archives.postgresql.org/message-id/13612.1328887227%40sss.pgh.pa.us
[5]
http://archives.postgresql.org/message-id/CA%2BTgmoY7wRGgBbFhKwfASqrNOPwZwCjfm1AhL82769Xx-SyduA%40mail.gmail.com
[6]
http://archives.postgresql.org/message-id/20120210140637.GB19783%40ldn-qws-004.delacy.com
[7] http://archives.postgresql.org/message-id/9966.1331920074%40sss.pgh.pa.us
--
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] patch: autocomplete for functions
On Mon, Mar 19, 2012 at 1:01 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I'm rather of the contrary opinion -- surely if we're going to complete function names, we should only complete those that are in schemas in the path; similarly for column names. I think it makes sense to only include currently-visible functions, but *not* only columns from currently visible tables, since we won't know yet whether the user intends to schema-qualify the table name. (BTW I didn't check but does this completion work if I schema-qualify a column name?) Peter's proposed tab-completion only kicks in for the column-name itself. Keep in mind, the user might be trying to enter: SELECT schema.table.column ... SELECT table.column ... SELECT table_alias.column ... SELECT column ... and presumably want to tab-complete the second token somehow. I'm a bit leery about trying to tab-complete those first two, and the third is right out. Just having the fourth would make me happy. Josh -- 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] return values of backend sub-main functions
On Tue, Jun 19, 2012 at 4:31 AM, Peter Eisentraut pete...@gmx.net wrote: On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote: On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(), WalSenderMain(), and WalSndLoop() to return void as well. I agree this code is not very consistent or useful, but one question: what should the callers do if one of these functions *does* return? I was thinking of a two-pronged approach: First, add __attribute__((noreturn)) to the functions. This will cause a suitable compiler to verify on a source-code level that nothing actually returns from the function. And second, at the call site, put an abort(); /* not reached */. Together, this will make the code cleaner and more consistent, and will also help the compiler out a bit about the control flow. Patch for 9.3 attached. +1. Should this call around line 4114 of postmaster.c not bother with proc_exit() anymore: /* And run the backend */ proc_exit(BackendRun(port)); I was hoping that some of the clang static analyzer complaints would go away with these changes, though it looks like only one[1] did. I would be interested to see the similar elog/ereport patch you mentioned previously, perhaps that will eliminate a bunch of warnings. Josh [1] this one goes away with the patch: http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_signal_backend() asymmetry
Hi all, I have one nitpick related to the recent changes for pg_cancel_backend() and pg_terminate_backend(). If you use these functions as an unprivileged user, and try to signal a nonexistent PID, you get: ERROR: must be superuser or have the same role to cancel queries running in other server processes Whereas if you do the same thing as a superuser, you get: WARNING: PID 123 is not a PostgreSQL server process pg_cancel_backend --- f (1 row) The comment above the WARNING generated for the latter case says: /* * This is just a warning so a loop-through-resultset will not abort * if one backend terminated on its own during the run */ which is nice, but wouldn't unprivileged users want the same behavior? Not to mention, the ERROR above is misleading, as it claims the nonexistent PID really belongs to another user. A simple fix is attached. The existing code called both BackendPidGetProc(pid) and IsBackendPid(pid) while checking a non-superuser's permissions, which really means two separate calls to BackendPidGetProc(pid). I simplified that to down to a single BackendPidGetProc(pid) call. Josh pg_signal_backend_asymmetry.diff 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] pg_signal_backend() asymmetry
On Thu, Jun 28, 2012 at 6:48 AM, Noah Misch n...@leadboat.com wrote: On Thu, Jun 28, 2012 at 01:36:49AM -0700, Daniel Farina wrote: On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote: I have one nitpick related to the recent changes for pg_cancel_backend() and pg_terminate_backend(). If you use these functions as an unprivileged user, and try to signal a nonexistent PID, you get: I think the goal there is to avoid leakage of the knowledge or non-knowledge of a given PID existing once it is deemed out of Postgres' control. Although I don't have a specific attack vector in mind for when one knows a PID exists a-priori, it does seem like an unnecessary admission on the behalf of other programs. I think it was just an oversight. I agree that these functions have no business helping users probe for live non-PostgreSQL PIDs on the server, but they don't do so and Josh's patch won't change that. I recommend committing the patch. Users will be able to probe for live PostgreSQL PIDs, but pg_stat_activity already provides those. Yeah, I figured that since pg_stat_activity already shows users PIDs, there should be no need to have pg_signal_backend() lie about whether a PID was a valid backend or not. And if for some reason we did want to lie, we should give an accurate message like not valid backend, or must be superuser or have the same role I noticed that I neglected to update the comment which was in the non-superuser case: updated version attached. On Thu, Jun 28, 2012 at 5:22 AM, Magnus Hagander mag...@hagander.net wrote: Well, there are two sides to it - one is the message, the other is if it should be ERROR or WARNING. My reading is that Josh is suggesting we make them WARNING in both cases, right? Maybe I should have said I had a few nitpicks instead of just one :-) A more detailed summary of the little issues I'm aiming to fix: 1a.) SIGNAL_BACKEND_NOPERMISSION being used for both invalid PID and role mismatch, causing the must be superuser or have ... message to be printed in both cases for non-superusers 1b.) Since SIGNAL_BACKEND_NOPERMISSION is used for both duties, you'll get an ERROR instead of just a WARNING during plausibly-normal usage. For example, if you're running SELECT pg_cancel_backend(pid) FROM pg_stat_activity WHERE usename = CURRENT_USER AND pid != pg_backend_pid(); you might be peeved if it ERROR'ed out with the bogus message claiming the process was owned by someone else, when the backend has just exited on its own. So even if we thought it was worth hiding to non-superusers whether the PID is invalid or belongs to someone else, I think the message should just be at WARNING level. 2a.) Existing code uses two calls to BackendPidGetProc() for non-superusers in the error-free case. 2b.) I think the comment the check for whether it's a backend process is below is a little misleading, since IsBackendPid() is just checking whether the return of BackendPidGetProc() is NULL, which has already been done. 3.) grammar fix for comment (on it's own - on its own) Josh pg_signal_backend_asymmetry.v2.diff 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] Posix Shared Mem patch
On Tue, Jul 3, 2012 at 6:57 AM, Robert Haas robertmh...@gmail.com wrote: Here's a patch that attempts to begin the work of adjusting the documentation for this brave new world. I am guessing that there may be other places in the documentation that also require updating, and this page probably needs more work, but it's a start. I think the boilerplate warnings in config.sgml about needing to raise the SysV parameters can go away; patch attached. Josh config.sgml.diff 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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Fri, Mar 11, 2011 at 8:54 AM, Bruce Momjian br...@momjian.us wrote:
I have added it to the next commit fest.
Hi Torello,
I have volunteered (more accurately, Greg Smith volunteered me :-)
to be a reviewer for this patch. I know you're a bit new here, so I
thought I'd outline where this patch stands and what's expected if
you'd like to move it along.
We organize patch reviews via commitfests lasting a month or so.
Some more information about this process:
http://wiki.postgresql.org/wiki/CommitFest
Each commitfest is a period wherein you can expect to receive some
feedback on your patch and advice on things which might need to be
improved (in this case, it's my job to provide you this feedback).
Your patch is in the upcoming commitfest, scheduled to run from June
15 to July 14.
So if you're interested in being responsible for this patch, or some
variant of it, eventually making its way into PostgreSQL 9.2, you
should be willing to update your patch based on feedback, request
advice, etc. during this period. If you're not interested in getting
sucked into this process that's OK -- just please advise us if that's
the case, and maybe someone else will be willing to take charge of the
patch.
Anssi and I posted some initial feedback on the patch's goals earlier.
I would like to ultimately see users have the capability to
pg_cancel_backend() their own queries. But I could at least conceive
of others not wanting this behavior enabled by default. So perhaps
this patch's approach of granting extra privs to the database owner
could work as a first attempt. And maybe a later version could
introduce a GUC allowing the DBA to control whether users can
cancel/terminate their backends, or we could instead have an option
flag to CREATE/ALTER ROLE, allowing per-user configuration.
It would be helpful to hear from others whether this patch's goals
would work as a first pass at this problem, so that Torello doesn't
waste time on a doomed approach. Also, it might be helpful to add an
entry on the Todo list for 'allow non-superusers to use
pg_cancel_backend()', in case this patch gets sunk.
Now, a few technical comments about the patch:
1.) This bit looks dangerous:
+backend = pgstat_fetch_stat_beentry(i);
+if (backend-st_procpid == pid) {
Since pgstat_fetch_stat_beentry() might return NULL.
I'm a bit suspicious about whether looping through
pgstat_fetch_stat_beentry() is the best way to determine the database
owner for a given backend PID, but I haven't dug in enough yet to
suggest a better alternative.
2.) The way the code inside pg_signal_backend() is structured, doing:
select pg_cancel_backend(12345);
as non-superuser, where '12345' is a fictitious PID, can now give you
the incorrect error message:
ERROR: must be superuser or target database owner to signal other
server processes
3.) No documentation adjustments, and the comments need some cleaup.
Torello: I'll be happy to handle comments/documentation for you as a
native English speaker, so you don't have to worry about this part.
That's it for now. Torello, I look forward to hearing back from you,
and hope that you have some time to work on this patch further.
Josh
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql: missing tab completions for COMMENT ON
Hi all,
psql's auto-complete support for COMMENT ON was missing support for a
few object types:
1.) EXTENSION and PROCEDURAL LANGUAGE are now auto-complete candidates
for COMMENT ON [TAB]. Lists of extensions and procedural languages
should also be filled in when a user types
COMMENT ON EXTENSION [TAB]
COMMENT ON PROCEDURAL LANGUAGE [TAB]
2.) This part of tab-complete.c looked like a spurious leftover:
*** psql_completion(char *text, int start, i
*** 1580,1592
COMPLETE_WITH_LIST(list_TRANS2);
}
else if ((pg_strcasecmp(prev4_wd, COMMENT) == 0
pg_strcasecmp(prev3_wd, ON) == 0) ||
(pg_strcasecmp(prev6_wd, COMMENT) == 0
! pg_strcasecmp(prev5_wd, ON) == 0) ||
!(pg_strcasecmp(prev5_wd, ON) == 0
! pg_strcasecmp(prev4_wd, TEXT) == 0
! pg_strcasecmp(prev3_wd, SEARCH) == 0))
COMPLETE_WITH_CONST(IS);
Since we want these choices to be filled in for COMMENT ON TEXT SEARCH [TAB]:
{CONFIGURATION, DICTIONARY, PARSER, TEMPLATE, NULL};
which were already being handled correctly in an above block.
One piece that I gave up on trying to fix is the auto-completion for
{OPERATOR, OPERATOR CLASS, OPERATOR FAMILY}, since getting it working
correctly would be a real hassle. There's the trouble of whether to
auto-complete operators for OPERATOR [TAB], or whether to fill in
{CLASS, FAMILY} instead. Plus the auto-completes for 'USING
index_method'.
While wasting time on OPERATOR [TAB], I realized we're being a bit
overeager with this bit:
else if ((pg_strcasecmp(prev4_wd, COMMENT) == 0
pg_strcasecmp(prev3_wd, ON) == 0) ||
(pg_strcasecmp(prev6_wd, COMMENT) == 0
pg_strcasecmp(prev5_wd, ON) == 0))
COMPLETE_WITH_CONST(IS);
which will auto-complete e.g.
COMMENT ON AGGREGATE avg [TAB]
with 'IS', when instead we'd want the possible argument types to avg,
or nothing at all. Same deal with a few other object types, but it's
probably not worth worrying about (at least, I'm not worrying about it
at the moment).
Barring objections, I can add this patch to the CF.
Josh
tab_complete.v1.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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Sun, May 29, 2011 at 5:04 AM, Noah Misch n...@leadboat.com wrote:
What risks arise from unconditionally allowing these calls for the same user's
backends? `pg_cancel_backend' ought to be safe enough; the user always has
access to the standard cancellation protocol, making the SQL interface a mere
convenience (albeit a compelling one). `pg_terminate_backend' does open up
access to a new behavior, but no concrete risks come to mind.
Looking around, I see there were real problems[1] with sending SIGTERM
to individual backends back in 2005 or so, and pg_terminate_backend()
was only deemed safe enough to put in for 8.4 [2]. So expanding
pg_terminate_backend() privileges does make me a tad nervous.
Reading through those old threads made me realize this patch would
give database owners the ability to kill off autovacuum workers. Seems
like we'd want to restrict that power to superusers.
On the other hand, this *would* be substantial new authority for database
owners. Seems like a reasonable authority to grant, though.
And I also realized that this patch's approach might force us to
maintain a permissions wart if we ever want to implement fine-grained
control for this stuff, e.g. a per-role setting enabling self-kills.
It would be a bit lame to have to document Use this CREATE/ALTER ROLE
flag. Or be the database owner. Or be a superuser.
Now, a few technical comments about the patch:
1.) This bit looks dangerous:
+ backend = pgstat_fetch_stat_beentry(i);
+ if (backend-st_procpid == pid) {
Since pgstat_fetch_stat_beentry() might return NULL.
I think you want BackendPidGetProc().
Ah, thanks for the pointer.
Josh
--
[1]
http://postgresql.1045698.n5.nabble.com/pg-terminate-backend-idea-td1930120.html
[2]
http://postgresql.1045698.n5.nabble.com/Re-COMMITTERS-pgsql-Add-pg-terminate-backend-to-allow-terminating-only-a-single-td1983763i20.html
--
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] Any idea for serializing INSERTING SERIAL column?
On Tue, May 31, 2011 at 8:08 PM, Tatsuo Ishii is...@postgresql.org wrote: [snip] In summary, 1) LOCK table foo cannot be used because of conflict with autovacuum 2) LOCK sequence just doesn't work 3) SELECT 1 FROM LOCK sequece fails after XID wraparound If you have other idea to serialize concurrent INSERT to a table, I would like to hear from you. Sorry, I'm not real familiar with pgpool, but have you thought about using an advisory lock on the target table, instead of a real lock (SELECT ... FOR UPDATE / LOCK table)? An advisory lock should not interfere with autovacuum. Obviously, this would only work if all the INSERTs in your example were coming from a single application (i.e. pgpool) which would honor the advisory lock. Josh -- 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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch n...@leadboat.com wrote: On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote: Looking around, I see there were real problems[1] with sending SIGTERM to individual backends back in 2005 or so, and pg_terminate_backend() was only deemed safe enough to put in for 8.4 [2]. So expanding pg_terminate_backend() privileges does make me a tad nervous. The documentation for the CREATE USER flag would boil down to omit this flag only if you're worried about undiscovered PostgreSQL bugs in this area. I'd echo Tom's sentiment from the first thread, In any case I think we have to solve it, not create new mechanisms to try to ignore it. I do agree with Tom's sentiment from that thread. But, if we are confident that pg_terminate_backend() is safe enough to relax permissions on, then I take it you agree we should plan to extend this power to all users? And if so, is this patch a good first step on that path? Reading through those old threads made me realize this patch would give database owners the ability to kill off autovacuum workers. Seems like we'd want to restrict that power to superusers. Would we? Any old user can already stifle VACUUM by holding a transaction open. This is true, though it's possible we might at some point want a backend process which really shouldn't be killable by non-superusers (if vacuum/autovacuum isn't one already.) Actually, I could easily imagine a superuser running an important query on a database getting peeved if a non-superuser were allowed to cancel/terminate his queries. Josh -- 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] Review: psql include file using relative path
On Sun, Jun 5, 2011 at 10:21 AM, Gurjeet Singh singh.gurj...@gmail.com wrote: On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt schmi...@gmail.com wrote: Tweaks applied, but omitted the C variable names as I don't think that adds much value. Your rewordings are fine, but the the article the is missing in a few spots, e.g. * uses \ir command - uses the \ir command * to currently processing file - to the currently processing file * same as \i command - same as the \i command I think processing is better (and consistent with the rest of the comments) than processed here: + * the file from where the currently processed file (if any) is located. New version of the patch attached. Thanks for the review. I think the patch is in pretty good shape now. The memory leak is gone AFAICT, and the comments and documentation updates look good. Josh -- 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] Review: psql include file using relative path
On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh singh.gurj...@gmail.com wrote: Attached an updated patch. If you find it ready for committer, please mark it so in the commitfest app. I can't find anything further to nitpick with this patch, and have marked it Ready For Committer in the CF. Thanks for your work on this, I am looking forward to the feature. Josh -- 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] smallserial / serial2
On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening b...@gmx.de wrote:
I tried to look at everything and cover everthing but please consider that
this is my first review so please have a second look at it!
I took a look at this as well, and I didn't encounter any problems
either. The patch is surprisingly small, but it looks like all the
documentation spots got covered, and I didn't see any missing pieces;
pg_dump copes fine, I didn't try pg_upgrade but I wouldn't expect
problems there either.
Actually, the one piece I think could be added is a regression test. I
didn't see any existing tests that covered bigserial/serial8, either,
so I went ahead and added a few tests for smallerial and bigserial. I
didn't see the testcases Brar posted at first, or I might have adopted
a few from there, but this should cover basic use of smallserial.
Josh
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 13e1565..968fcce 100644
*** a/src/test/regress/expected/sequence.out
--- b/src/test/regress/expected/sequence.out
*** SELECT * FROM serialTest;
*** 16,21
--- 16,99
force | 100
(3 rows)
+ -- test smallserial / bigserial
+ CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
+ f5 bigserial, f6 serial8);
+ NOTICE: CREATE TABLE will create implicit sequence serialtest2_f2_seq for serial column serialtest2.f2
+ NOTICE: CREATE TABLE will create implicit sequence serialtest2_f3_seq for serial column serialtest2.f3
+ NOTICE: CREATE TABLE will create implicit sequence serialtest2_f4_seq for serial column serialtest2.f4
+ NOTICE: CREATE TABLE will create implicit sequence serialtest2_f5_seq for serial column serialtest2.f5
+ NOTICE: CREATE TABLE will create implicit sequence serialtest2_f6_seq for serial column serialtest2.f6
+ INSERT INTO serialTest2 (f1)
+ VALUES ('test_defaults');
+ INSERT INTO serialTest2 (f1, f2, f3, f4, f5, f6)
+ VALUES ('test_max_vals', 2147483647, 32767, 32767, 9223372036854775807,
+ 9223372036854775807),
+ ('test_min_vals', -2147483648, -32768, -32768, -9223372036854775808,
+ -9223372036854775808);
+ -- All these INSERTs should fail:
+ INSERT INTO serialTest2 (f1, f3)
+ VALUES ('bogus', -32769);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f4)
+ VALUES ('bogus', -32769);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f3)
+ VALUES ('bogus', 32768);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f4)
+ VALUES ('bogus', 32768);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f5)
+ VALUES ('bogus', -9223372036854775809);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f6)
+ VALUES ('bogus', -9223372036854775809);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f5)
+ VALUES ('bogus', 9223372036854775808);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f6)
+ VALUES ('bogus', 9223372036854775808);
+ ERROR: bigint out of range
+ SELECT * FROM serialTest2 ORDER BY f2 ASC;
+ f1 | f2 | f3 | f4 | f5 | f6
+ ---+-+++--+--
+ test_min_vals | -2147483648 | -32768 | -32768 | -9223372036854775808 | -9223372036854775808
+ test_defaults | 1 | 1 | 1 |1 |1
+ test_max_vals | 2147483647 | 32767 | 32767 | 9223372036854775807 | 9223372036854775807
+ (3 rows)
+
+ SELECT nextval('serialTest2_f2_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f3_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f4_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f5_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f6_seq');
+ nextval
+ -
+2
+ (1 row)
+
-- basic sequence operations using both text and oid references
CREATE SEQUENCE sequence_test;
SELECT nextval('sequence_test'::text);
*** SELECT nextval('sequence_test2');
*** 221,231
(1 row)
-- Information schema
! SELECT * FROM information_schema.sequences WHERE sequence_name IN ('sequence_test2');
! sequence_catalog | sequence_schema | sequence_name | data_type | numeric_precision | numeric_precision_radix | numeric_scale | start_value | minimum_value | maximum_value | increment | cycle_option
! --+-++---+---+-+---+-+---+---+---+--
! regression | public | sequence_test2 | bigint|64 | 2 | 0 | 32 | 5 | 36| 4 | YES
! (1 row)
-- Test comments
COMMENT ON SEQUENCE asdf IS
Re: [HACKERS] psql describe.c cleanup
On Tue, Jun 14, 2011 at 12:15 PM, Merlin Moncure mmonc...@gmail.com wrote: I did a quick review and test of your patch. It didn't quite apply cleanly due to recent non-related describe.c changes -- updated patch attached. Thanks for looking at this. Your updated patch looks good to me. First, I'll give you a thumbs up on the original inspiration for the patch. The output should be standardized, and I see no reason not to append a semicolon on usability basis. Beyond that, the changes are mostly cosmetic and I can't see how it will break things outside of terminating a query early by accident (I didn't see any). Yeah, I really didn't want to break any queries, so I did my best to test every query I changed. What I do wonder though is if the ; appending should really be happening in printQuery() instead of in each query -- the idea being that formatting for external consumption should be happening in one place. Maybe that's over-thinking it though. That's a fair point, and hacking up printQuery() would indeed solve my original gripe about copy-pasting queries out of psql -E. But more generally, I think that standardizing the style of the code is a Good Thing, particularly where style issues impact usability (like here). I would actually like to see a movement towards having all these queries use whitespace/formatting consistently. For instance, when I do a \d sometable I see some queries with lines bleeding out maybe 200 columns, some wrapped at 80 columns, etc. This sort of style issue is not something that a simple kludge in printQuery() could solve (and even if we put in a sophisticated formatting tool inside printQuery(), we'd still be left with an ugly describe.c). For the record, I find that having SQL queries consistently formatted makes them much more readable, allowing a human to roughly parse a query on sight once you're used to the formatting. So I wouldn't be opposed to putting the kludge in printQuery(), but I would like to see us standardize the queries regardless. (And in that case, I wouldn't mind if we dropped all the semicolons in describe.c, just that we're consistent.) Josh -- 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] psql describe.c cleanup
On Tue, Jun 14, 2011 at 3:25 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: pgindent moves strings back to the left when it thinks they fit within 80 columns. Yes, that seems pretty screwy. I am losing track of the ways in which pgindent has managed to mangle our source code :-/ Josh -- 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] patch: Allow \dd to show constraint comments
On Sun, Jun 19, 2011 at 3:25 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/6/18 Josh Kupershmidt schmi...@gmail.com: I think the v5 patch should be marked as 'Ready for Committer' I think we still need to handle my Still TODO concerns noted upthread. I don't have a lot of time this weekend due to a family event, but I was mulling over putting in a is_system boolean column into pg_comments and then fixing psql's \dd[S] to use pg_comments. But I am of course open to other suggestions. Josh -- 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] smallserial / serial2
On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas robertmh...@gmail.com wrote: Committed the main patch, and your regression tests. Hmph, looks like buildfarm members koi and jaguar are failing make check now: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koidt=2011-06-22%2008%3A06%3A00 due to a difference in sequence.out. I didn't muck with the part about SELECT * FROM foo_seq_new; which is causing the diff, but it looks like the only difference is in the log_cnt column, which seems pretty fragile to rely on in a regression test. Maybe those SELECTS just shouldn't include log_cnt. Josh -- 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] patch: Allow \dd to show constraint comments
On Wed, Jun 29, 2011 at 12:15 PM, Merlin Moncure mmonc...@gmail.com wrote: Patch applies clean, does what it is supposed to do, and matches other conventions in describe.c Passing to committer. pg_comments may be a better way to go, but that is a problem for another day... Thanks for the review, and sorry for my tardiness in getting back into this thread. Since the describe.c patch is already written and reviewed, I agree it's worthwhile to commit, even though the pg_comments patch aims to stomp on this approach. (pg_comments might need some further baking time..) Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] plpgsql extension install nitpick
Hi all,
I noticed that the plpgsql extension gets installed with an extension
comment of 'PL/pgSQL procedural language', which comes from
plpgsql.control. That seems fine and dandy, but take a look at the
following query (modified from psql's \dL query):
SELECT l.lanname AS Name,
pg_catalog.pg_get_userbyid(l.lanowner) as Owner,
l.lanpltrusted AS Trusted,
d.description
FROM pg_catalog.pg_language l
LEFT JOIN pg_description d
ON d.classoid = l.tableoid AND d.objoid = l.oid;
You should see plpgsql has no comment. The comment does show up if
you're looking for extension comments, like in this query (lifted from
the pg_comments patch):
SELECT
d.objoid, d.classoid, d.objsubid,
'extension'::text AS objtype,
ext.extnamespace AS objnamespace,
ext.extname AS objname,
d.description,
nsp.nspname IN ('pg_catalog', 'information_schema') AS is_system
FROM
pg_description d
JOIN pg_extension ext ON d.classoid = ext.tableoid AND d.objoid =
ext.oid
JOIN pg_namespace nsp ON ext.extnamespace = nsp.oid
WHERE
d.objsubid = 0;
So, basically, I would like to have that comment show up for the first
query. I imagine this could be fixed quite easily by adding:
COMMENT ON PROCEDURAL LANGUAGE plpgsql IS 'PL/pgSQL procedural language';
somewhere like plpgsql--1.0.sql. And if you're wondering why I care
about any of this, it's because I'd like to fix up psql's \dL command
to display the comments attached to procedural languages, and I'd
rather not have to special-case plpgsql.
Josh
--
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] plpgsql extension install nitpick
On Sat, Jul 2, 2011 at 11:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Josh Kupershmidt schmi...@gmail.com writes: [ plpgsql's comment is now attached to the extension, not the PL itself ] So, basically, I would like to have that comment show up for the first query. I imagine this could be fixed quite easily by adding: COMMENT ON PROCEDURAL LANGUAGE plpgsql IS 'PL/pgSQL procedural language'; I can't get excited about adding duplicative comments at different semantic levels. I admit I don't like the idea of duplicating comments either :-( Though maybe the extension comment could be tweaked to 'Extension supplying PL/pgSQL procedural language', or something like that. We just went through an exercise to suppress comments on functions that are meant to be accessed through operators, and this seems like much the same kind of situation. I think it will not be long before COMMENT ON PROCEDURAL LANGUAGE is a historical curiosity, because everybody will ship their PLs as extensions and the comment on the extension will be the thing to look at. IOW, the fact that there even is a database object type procedural language will soon be an implementation detail of interest only to PL authors. Or, perhaps more concretely: if we do as you suggest, won't \dd be putting out two identical comments on two different kinds of objects, and won't people find that confusing and inconvenient? Well, currently I believe \dd shows comments for neither languages nor extensions. And IMO it doesn't need to, either: these should properly be handled by \dx and \dL, respectively. (The proposed pg_comments view would show 'plpgsql' twice, once as objtype = language, and once as objtype = extension -- I agree that's not ideal). ... And if you're wondering why I care about any of this, it's because I'd like to fix up psql's \dL command to display the comments attached to procedural languages, and I'd rather not have to special-case plpgsql. Well, all four PLs supplied with core work the same way here, so a special case targeted at only plpgsql would be quite wrong anyway. Not sure I follow you here... the COMMENT for plpgsql is in a different place than for 'c', 'internal', and 'sql', which is what I'm on about. Anyways, if you're not keen on adding in another comment for plpgsql, do you have a suggestion on how to make \dL display the comments for all languages? Josh -- 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] patch: Allow \dd to show constraint comments
On Thu, Jul 7, 2011 at 10:00 PM, Robert Haas robertmh...@gmail.com wrote: [review of original, small patch to add another type to \dd's output] I am inclined to say that we should reject this patch as it stands. That's totally OK - that original patch was of marginal use given the larger brokenness of \dd. With or without pg_comments, I think we need a plan for \dd, and adding one object type is not a plan. The closest thing I've seen to a plan is this comment from Josh: -- ISTM that \dd is best suited as a command to show the comments for objects for which we don't have an individual backslash command, or objects for which it's not practical to show the comment in the backslash command. -- If someone wants to implement that, I'm OK with it, though I think we should also consider the alternative of abolishing \dd and just always display the comments via the per-object type commands (e.g. \d+ would display the table, constraint, trigger, and rule comments). And I'm quite happy you said that: I'm just about to post part 1 of a patch to start fixing up the individual backslash commands which clearly should be showing comments, but aren't. I'm thinking that clarifying which backslash commands should show object comments, and which types must be left for \dd to clean up can be handled separately from pg_comments. And my preference is to whittle down \dd to as little as necessary (if it could be gotten rid of entirely, even better, but I doubt that's going to fly). I don't want to, as Josh says, let the perfect be the enemy of the good, but if we change this as proposed we're just going to end up changing it again. That's going to be more work than just doing it once, and if it happens over the course of multiple releases, then it creates more annoyance for our users, too. I don't really think this is such a large project that we can't get it right in one try. Fair enough. If the pg_comments patch does go down in flames, I can circle back and patch up the rest of the holes in \dd. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] patch: pg_comments system view
On Fri, Jul 15, 2011 at 4:48 PM, Josh Berkus j...@agliodbs.com wrote: I am unable to figure out the status of the pg_comments patch from this thread. What's going on with it? I don't blame you :-) I think this thread got so confusing because two separate topics were intertwined. (I'm going to try to change this thread subject to reflect the fact that we're really talking about pg_comments here now.) First, the original post, with a small patch to fix one of the many problems with psql's \dd command. That patch was rejected because it only plugged one of the many problems with \dd. I have since started another thread[1] with a plausible fix for \dd and several other backslash commands, so that we will have working displays of all comments with minimal duplication. Second, we have the pg_comments view (latest version is the v10.WIP). Despite its WIP tag, I think it is actually pretty close to being complete at this point. The first concern which I raised a concern in that thread[2]: 1.) For now, I'm just ignoring the issue of visibility checks; is the only big issue I see as still outstanding. At first, I was assuming that \dd should naturally read from pg_comments to fetch the object comments it is interested in. But that would mean that we'd need some way to duplicate those visibility checks \dd was doing, either in \dd or in another is_visible column in pg_comments. I haven't tried either of those options out yet, but I was worried they'd both be tricky/ugly. Which leads me to think, maybe it's not so bad if \dd stays the way I've suggested in thread[1], i.e. just querying pg_[sh]description for the five object types it needs directly. After all, \dd will IMO be close to useless/deprecated once we have pg_comments; it'll be much easier to just query pg_comments for what you're looking for directly, and \dd will only display five funky object types, anyway. How do folks feel about this issue? The second concern I raised with the last pg_comments patch, I think now might be a good time to re-examine what types of objects are displayed by \dd. should be handled by thread[1], and the third concern is just about whitespace. Oh, and docs need some adjusting too, and it'd be nice if someone sanity checked my guesses for the is_system column (or if it's not needed, that's OK too). So that's where the pg_comments patch stands, at least AIUI. Clear as mud yet? :) Josh [1] http://archives.postgresql.org/pgsql-hackers/2011-07/msg00459.php [2] http://archives.postgresql.org/message-id/CAK3UJRGNwKq0c2VsSYV-Mg55Y_kvZE=8fmr_xt8rzp__1lo...@mail.gmail.com -- 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] patch: pg_comments system view
On Fri, Jul 15, 2011 at 7:01 PM, Josh Berkus j...@agliodbs.com wrote: On 7/15/11 3:54 PM, Josh Kupershmidt wrote: So that's where the pg_comments patch stands, at least AIUI. Clear as mud yet? :) Sounds like returned with feedback to me. Yeah, that's fine for this CF (though I do welcome any suggestions/feedback about the problems which need to be fixed). I'll try to put together another version for the next CF soon. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql: bogus descriptions displayed by \d+
Hi all, The psql output for \d+ on indexes, sequences, and views is rather bogus. Examples below from the SQL at bottom. So, if you look at \d+ newtbl, the right-most column named Description should display any comments attached to newtbl's columns. You should see bcol column comment as the Description for column bcol. That works OK. Now, try this: test=# \d+ newtbl_idx_bcol Index public.newtbl_idx_bcol Column | Type | Definition | Storage | Description +-++-+- bcol | integer | bcol | plain | What's the Description displayed in that table? Well, right now it's totally broken (not displaying anything). I'm not sure if we should try to display the comment attached to column bcol in this case: if so, what would we do for e.g. functional indexes? A similar situation exists for sequences and views displayed by \d+. I'd be OK with just ripping out Description entirely in these cases; if you want to see the comment attached to the index or sequence itself, you can use \di+ or \ds+. Although now might also be a good time to think about properly displaying sequence or index comments via \d+, and how they should be displayed. Thoughts? Josh -- Example SQL creating a few objects with comments. -- CREATE TABLE newtbl (acol serial PRIMARY KEY, bcol int NOT NULL, ccol text DEFAULT NULL, dcol text NOT NULL); COMMENT ON TABLE newtbl IS 'newtbl table comment'; COMMENT ON COLUMN newtbl.bcol IS 'bcol column comment'; COMMENT ON SEQUENCE newtbl_acol_seq IS 'sequence comment'; CREATE INDEX newtbl_idx_bcol ON newtbl (bcol); COMMENT ON INDEX newtbl_idx_bcol IS 'single-column index on newtbl'; CREATE VIEW myview AS SELECT * FROM newtbl; COMMENT ON VIEW myview IS 'view comment'; -- 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] psql: bogus descriptions displayed by \d+
On Sat, Jul 16, 2011 at 12:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: After a bit of review of the archives, the somebody was me: http://git.postgresql.org/gitweb/?p=postgresql.gita=commitdiffh=b7d67954456f15762c04e5269b64adc88dcd0860 and this thread was the discussion about it: http://archives.postgresql.org/pgsql-hackers/2009-12/msg01982.php It looks like we thought about pg_dump, but did not think about psql. Ah, interesting. I didn't even know this functionality existed. And I think there is some documentation lacking; in the 8.4 doc page: http://www.postgresql.org/docs/8.4/static/sql-comment.html I don't see any mention of comments on an index's columns. And the docs also neglect to mention comments on a view's columns as well, which is why I thought \d+ view_name was producing bogus output as well (it's really looking for those column comments). I think it might be reasonable to remove the Description column from \d+ output for indexes and sequences, on the grounds that (1) it's useless against 9.x servers, and (2) for those relkinds we add other columns and so the horizontal space is precious. AFAICT the extra Description column for \d+ sequence_name is bogus in both 8.4 and 9.0, so there should be no objections to ripping that out. We could also consider showing Description only when talking to a pre-9.0 server; but that's going to render the code even more spaghetti-ish, and the value seems pretty limited. And as for \d+ index_name, I agree with Robert's sentiments here, doesn't seem worth the bother. Josh -- 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] patch: Allow \dd to show constraint comments
On Sun, Jul 17, 2011 at 11:25 PM, Robert Haas robertmh...@gmail.com wrote: On Sat, Jul 2, 2011 at 8:37 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Sun, Jun 19, 2011 at 11:59 AM, Josh Kupershmidt schmi...@gmail.com wrote: It seems funny to have is_system = true unconditionally for any object type. Why'd you do it that way? Or maybe I should ask, why true rather than false? Thanks for the review! Well, I was hoping to go by the existing psql backslash commands' notions about what qualifies as system and what doesn't; that worked OK for commands which supported the 'S' modifier, but not all do. For objects like tablespaces, where you must be a superuser to create one, it seems like they should all be considered is_system = true, on the vague premise that superuser = system. Other objects like roles are admittedly murkier, and I didn't find any precedent inside describe.c to help make such distinctions. But this is probably all a moot point, see below. I had to tweak the pg_proc.h entry for pg_opfamily_is_visible to play nice with the recent transform function change to that file. It seems like we ought to have this function for symmetry regardless of what happens to the rest of this, so I extracted and committed this part. Good idea. Issues still to be resolved: 1.) For now, I'm just ignoring the issue of visibility checks; I didn't see a simple way to support these checks \dd was doing: processSQLNamePattern(pset.db, buf, pattern, true, false, n.nspname, p.proname, NULL, pg_catalog.pg_function_is_visible(p.oid)); I'm a bit leery of adding an is_visible column into pg_comments, but I'm not sure I have a feasible workaround if we do want to keep this visibility check. Maybe a big CASE statement for the last argument of processSQLNamePattern() would work... Yeah... or we could add a function pg_object_is_visible(classoid, objectoid) that would dispatch to the relevant visibility testing function based on object type. Not sure if that's too much of a kludge, but it wouldn't be useful only here: you could probably use it in combination with pg_locks, for example. Something like that might work, though an easy escape is just leaving the query style of \dd as it was originally (i.e. querying the various catalogs directly, and not using pg_comments): discussed a bit in the recent pg_comments thread w/ Josh Berkus. The real problem with the is_system column is that it seems to be entirely targeted around what psql happens to feel like doing. I'm pretty sure we'll regret it if we choose to go that route. Yeah, it did turn out more messy than I had hoped, and I'm not sure it'd be possible to iron out the semantics of is_system in a way that leaves everyone happy. Probably best to just rip it out if \dd won't need it. I'll try to send an updated patch by this weekend. Josh -- 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] patch: Allow \dd to show constraint comments
[Resending with gzip'ed patch this time, I think the last attempt got eaten.]
On Mon, Jul 18, 2011 at 11:15 PM, Robert Haas robertmh...@gmail.com wrote:
On Mon, Jul 18, 2011 at 10:57 PM, Josh Kupershmidt schmi...@gmail.com wrote:
1.) For now, I'm just ignoring the issue of visibility checks; I
didn't see a simple way to support these checks \dd was doing:
processSQLNamePattern(pset.db, buf, pattern, true, false,
n.nspname, p.proname, NULL,
pg_catalog.pg_function_is_visible(p.oid));
I'm a bit leery of adding an is_visible column into pg_comments, but
I'm not sure I have a feasible workaround if we do want to keep this
visibility check. Maybe a big CASE statement for the last argument of
processSQLNamePattern() would work...
Yeah... or we could add a function pg_object_is_visible(classoid,
objectoid) that would dispatch to the relevant visibility testing
function based on object type. Not sure if that's too much of a
kludge, but it wouldn't be useful only here: you could probably use it
in combination with pg_locks, for example.
Something like that might work, though an easy escape is just leaving
the query style of \dd as it was originally (i.e. querying the various
catalogs directly, and not using pg_comments): discussed a bit in the
recent pg_comments thread w/ Josh Berkus.
That's another approach. It seems a bit lame, but...
I went ahead and patched up \dd to display its five object types with
its old-style rooting around in catalogs. I played around again with
the idea of having \dd query pg_comments, but gave up when I realized:
1. We might not be saving much complexity in \dd's query
2. Taking the is_system column out was probably good for pg_comments,
but exacerbates point 1.), not to mention the visibility testing that
would have to be done somehow.
3. The objname column of pg_comments is intentionally different
than the Name column displayed by \dd; the justification for this
was that \dd's Name display wasn't actually useful to recreate the
call to COMMENT ON, but this difference in pg_comments would make it
pretty tricky to keep \dd's Name the same
4. I still would like to get rid of \dd entirely, thus it seems less
important whether it uses pg_comments. It's down to five object types
now; I think that triggers, constraints, and rules could feasibly be
incorporated into \d+ output as Robert suggested upthread, and perhaps
operator class operator family could get their own backslash
commands.
Some fixes:
* shuffled the query components in describe.c's objectDescription()
so they're alphabetized by object type
* untabified pg_comments in system_views.sql to match its surroundings
* the WHERE d.objsubid = 0 was being omitted in one or two spots,
* the objects with descriptions coming from pg_shdescription, which
does not have the objsubid column, were using NULL::integer instead of
0, as all the other non-column object types should have. This seemed
undesirable, and counter to what the doc page claimed.
* fixed up psql's documentation and help string for \dd
Updated patch attached, along with a revised SQL script to make
testing easier. I can add this to the next CF.
Note, there is a separate thread[1] with just the psql changes broken
out, if it's helpful to consider the psql changes separately from
pg_comments. I do need to update the patch posted there with this
latest set of changes.
Josh
--
[1] http://archives.postgresql.org/pgsql-hackers/2011-07/msg00459.php
CREATE SCHEMA myschema;
COMMENT ON SCHEMA myschema IS 'schema comment';
CREATE DOMAIN us_postal_code AS TEXT
CHECK(
VALUE ~ '^\\d{5}$'
OR VALUE ~ '^\\d{5}-\\d{4}$'
);
COMMENT ON DOMAIN us_postal_code IS 'domain comment';
CREATE DOMAIN uncommented_domain AS TEXT CHECK(true);
COMMENT ON TABLESPACE pg_default IS 'default tablespace';
CREATE TABLE mytbl (a serial PRIMARY KEY, b int);
COMMENT ON TABLE mytbl IS 'example table';
COMMENT ON SEQUENCE mytbl_a_seq IS 'serial sequence';
COMMENT ON COLUMN mytbl.a IS 'column comment';
CREATE TABLE myschema.another_tbl (a int);
ALTER TABLE myschema.another_tbl ADD CONSTRAINT a_chk_con CHECK(a != 0);
COMMENT ON TABLE myschema.another_tbl IS 'another_tbl comment';
COMMENT ON CONSTRAINT a_chk_con ON myschema.another_tbl IS 'constraint comment';
CREATE INDEX myidx ON mytbl (a);
COMMENT ON INDEX myidx IS 'example index';
ALTER TABLE mytbl ADD CONSTRAINT mycon CHECK (b 100);
COMMENT ON CONSTRAINT mycon ON mytbl IS 'constraint comment';
CREATE VIEW myview AS SELECT * FROM mytbl;
COMMENT ON VIEW myview IS 'view comment';
CREATE TABLE dummy_tbl (a int);
CREATE RULE myrule AS
ON INSERT TO dummy_tbl
DO INSTEAD NOTHING;
COMMENT ON RULE myrule ON dummy_tbl IS 'bogus rule';
CREATE FUNCTION ex_trg_func() RETURNS trigger AS $$
BEGIN
RETURN NEW;
END;
$$ LANGUAGE plpgsql;
COMMENT ON FUNCTION ex_trg_func() IS 'function comment';
create trigger ex_trg BEFORE INSERT OR UPDATE ON mytbl
Re: [HACKERS] psql: bogus descriptions displayed by \d+
On Sun, Jul 17, 2011 at 10:54 AM, Josh Kupershmidt schmi...@gmail.com wrote: On Sat, Jul 16, 2011 at 12:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: After a bit of review of the archives, the somebody was me: http://git.postgresql.org/gitweb/?p=postgresql.gita=commitdiffh=b7d67954456f15762c04e5269b64adc88dcd0860 and this thread was the discussion about it: http://archives.postgresql.org/pgsql-hackers/2009-12/msg01982.php It looks like we thought about pg_dump, but did not think about psql. Ah, interesting. I didn't even know this functionality existed. And I think there is some documentation lacking; in the 8.4 doc page: Here's a small patch against branch 8.4 to mention support for COMMENT ON index_name.column_name. Also, a patch against master to: * get rid of the bogus Description outputs for \d+ sequence_name and \d+ index_name * clarify in the COMMENT ON doc page that a table _or view_ name may be used for comments on columns, rules, and triggers. If we allowed constraints on views, we could have just put in a note explaining that table_name.column_name applies to tables and views, but constraints are the odd man out. * slightly reordered the listing of the first bunch of Parameters on that page so that agg_name comes first, as it does in the Synopsis section I noticed that the synopsis for CREATE RULE: http://www.postgresql.org/docs/9.1/static/sql-createrule.html uses the term table, which could be a similar omission. However, on that page the first sentence of the description specifies table or view so it might be fine as-is. And while I'm messing with this, some further nitpicks about psql not addressed by these patches: * The Storage column for \d+ sequence_name is correct, I suppose, but repetitive * The Type column for \dv+ view_name, \di+ index_name, \ds+ sequence_name , etc. seems borderline useless.. shouldn't you know what type you're looking at based on the backslash command you're using? Plus the table heading could be more specific than List of relations, e.g. List of views. Josh diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml index ab12614..58a2f02 100644 *** a/doc/src/sgml/ref/comment.sgml --- b/doc/src/sgml/ref/comment.sgml *** COMMENT ON *** 26,32 AGGREGATE replaceable class=PARAMETERagg_name/replaceable (replaceable class=PARAMETERagg_type/replaceable [, ...] ) | CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable) | COLLATION replaceable class=PARAMETERobject_name/replaceable | ! COLUMN replaceable class=PARAMETERtable_name/replaceable.replaceable class=PARAMETERcolumn_name/replaceable | CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable ON replaceable class=PARAMETERtable_name/replaceable | CONVERSION replaceable class=PARAMETERobject_name/replaceable | DATABASE replaceable class=PARAMETERobject_name/replaceable | --- 26,32 AGGREGATE replaceable class=PARAMETERagg_name/replaceable (replaceable class=PARAMETERagg_type/replaceable [, ...] ) | CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable) | COLLATION replaceable class=PARAMETERobject_name/replaceable | ! COLUMN replaceable class=PARAMETERtable_or_view_name/replaceable.replaceable class=PARAMETERcolumn_name/replaceable | CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable ON replaceable class=PARAMETERtable_name/replaceable | CONVERSION replaceable class=PARAMETERobject_name/replaceable | DATABASE replaceable class=PARAMETERobject_name/replaceable | *** COMMENT ON *** 42,48 OPERATOR FAMILY replaceable class=PARAMETERobject_name/replaceable USING replaceable class=parameterindex_method/replaceable | [ PROCEDURAL ] LANGUAGE replaceable class=PARAMETERobject_name/replaceable | ROLE replaceable class=PARAMETERobject_name/replaceable | ! RULE replaceable class=PARAMETERrule_name/replaceable ON replaceable class=PARAMETERtable_name/replaceable | SCHEMA replaceable class=PARAMETERobject_name/replaceable | SEQUENCE replaceable class=PARAMETERobject_name/replaceable | SERVER replaceable class=PARAMETERobject_name/replaceable | --- 42,48 OPERATOR FAMILY replaceable class=PARAMETERobject_name/replaceable USING replaceable class=parameterindex_method/replaceable | [ PROCEDURAL ] LANGUAGE replaceable class=PARAMETERobject_name/replaceable | ROLE replaceable class=PARAMETERobject_name/replaceable | ! RULE replaceable class=PARAMETERrule_name/replaceable ON replaceable class=PARAMETERtable_or_view_name/replaceable | SCHEMA replaceable class=PARAMETERobject_name/replaceable | SEQUENCE replaceable class=PARAMETERobject_name/replaceable | SERVER replaceable class=PARAMETERobject_name/replaceable | *** COMMENT ON *** 52,58 TEXT SEARCH DICTIONARY replaceable class=PARAMETERobject_name/replaceable | TEXT SEARCH PARSER replaceable class
Re: [HACKERS] psql: bogus descriptions displayed by \d+
On Fri, Jul 22, 2011 at 12:44 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jul 21, 2011 at 9:17 PM, Josh Kupershmidt schmi...@gmail.com wrote: Here's a small patch against branch 8.4 to mention support for COMMENT ON index_name.column_name. I am not in favor of this - because we'd also need to mention every other relkind that can support comments. I think if we want to do something here we should change it to say relation_name, and then clarify what that means further down. Similarly with the patch for master. Also, if we're going to make a change here, we probably should make sure it matches the actual behavior. In master, that's to allow comments on columns of tables, views, composite types, and foreign tables. That seems like a good way to document this; patch for master updated. I avoided mucking with the documentation for COMMENT ON RULE and COMMENT ON TRIGGER this time; they both say table when they really mean table or view, but maybe trying to differentiate between table, table_or_view, and relation will make things overly complicated. Also, a patch against master to: * get rid of the bogus Description outputs for \d+ sequence_name and \d+ index_name This part looks OK, but instead of doing a negative test (not-index, not-sequence) let's have it do a positive test, for the same types comment.c allows. Right, fixed. And while I'm messing with this, some further nitpicks about psql not addressed by these patches: * The Storage column for \d+ sequence_name is correct, I suppose, but repetitive I'm OK with removing that. Hrm, would it be better to keep that Storage bit around in some non-repetitive form, maybe on its own line below the table output? * The Type column for \dv+ view_name, \di+ index_name, \ds+ sequence_name , etc. seems borderline useless.. shouldn't you know what type you're looking at based on the backslash command you're using? Not really. You can do something like this, for example: \dti+ ...to show both indexes and tables. I see. Didn't know about that trick. Josh diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml index ab12614..736907e 100644 *** a/doc/src/sgml/ref/comment.sgml --- b/doc/src/sgml/ref/comment.sgml *** COMMENT ON *** 26,32 AGGREGATE replaceable class=PARAMETERagg_name/replaceable (replaceable class=PARAMETERagg_type/replaceable [, ...] ) | CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable) | COLLATION replaceable class=PARAMETERobject_name/replaceable | ! COLUMN replaceable class=PARAMETERtable_name/replaceable.replaceable class=PARAMETERcolumn_name/replaceable | CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable ON replaceable class=PARAMETERtable_name/replaceable | CONVERSION replaceable class=PARAMETERobject_name/replaceable | DATABASE replaceable class=PARAMETERobject_name/replaceable | --- 26,32 AGGREGATE replaceable class=PARAMETERagg_name/replaceable (replaceable class=PARAMETERagg_type/replaceable [, ...] ) | CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable) | COLLATION replaceable class=PARAMETERobject_name/replaceable | ! COLUMN replaceable class=PARAMETERrelation_name/replaceable.replaceable class=PARAMETERcolumn_name/replaceable | CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable ON replaceable class=PARAMETERtable_name/replaceable | CONVERSION replaceable class=PARAMETERobject_name/replaceable | DATABASE replaceable class=PARAMETERobject_name/replaceable | *** COMMENT ON *** 97,105 variablelist varlistentry - termreplaceable class=parameterobject_name/replaceable/term - termreplaceable class=parametertable_name.column_name/replaceable/term termreplaceable class=parameteragg_name/replaceable/term termreplaceable class=parameterconstraint_name/replaceable/term termreplaceable class=parameterfunction_name/replaceable/term termreplaceable class=parameteroperator_name/replaceable/term --- 97,104 variablelist varlistentry termreplaceable class=parameteragg_name/replaceable/term + termreplaceable class=parameterobject_name/replaceable/term termreplaceable class=parameterconstraint_name/replaceable/term termreplaceable class=parameterfunction_name/replaceable/term termreplaceable class=parameteroperator_name/replaceable/term *** COMMENT ON *** 143,148 --- 142,158 /para /listitem /varlistentry + + varlistentry + termreplaceablerelation_name.column_name/replaceable/term + listitem + para +For comments on columns, the name of the relation and column. Column +comments may be used with tables, views, composite types, and +foreign tables. + /para + /listitem + /varlistentry varlistentry termreplaceable
Re: [HACKERS] psql: bogus descriptions displayed by \d+
On Tue, Jul 26, 2011 at 9:53 AM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jul 25, 2011 at 10:29 PM, Josh Kupershmidt schmi...@gmail.com wrote: I think this is basically the right approach but I found what you did here a bit wordy, so I simplified it, committed it, and back-patched to 9.0 with suitable adjustment. Hopefully I didn't simplify it into a form you don't like. That looks fine. Minor grammar quibble about: + When commenting on a column, + replaceable class=parameterrelation_name/replaceable must refer + to a table, view, composite types, or foreign table. types should probably be the singular type. * get rid of the bogus Description outputs for \d+ sequence_name and \d+ index_name Committed this part to head with minor tweaks. Thanks for the commit. * The Storage column for \d+ sequence_name is correct, I suppose, but repetitive I'm OK with removing that. Hrm, would it be better to keep that Storage bit around in some non-repetitive form, maybe on its own line below the table output? Well, I don't really see that it has any value. I'd probably just leave it the way it is, but if we're going to change something, I would favor removing it over relocating it. I notice the Storage information is also repeated for multi-column indexes. I don't mind leaving this wart as-is for now, since single-column indexes are probably the norm, and we would presumably want to fix both types in one go. Josh -- 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] psql: bogus descriptions displayed by \d+
On Wed, Jul 27, 2011 at 5:19 PM, Peter Eisentraut pete...@gmx.net wrote:
On tis, 2011-07-26 at 09:53 -0400, Robert Haas wrote:
On Mon, Jul 25, 2011 at 10:29 PM, Josh Kupershmidt
schmi...@gmail.com wrote:
That seems like a good way to document this; patch for master
updated.
I avoided mucking with the documentation for COMMENT ON RULE and
COMMENT ON TRIGGER this time; they both say table when they really
mean table or view, but maybe trying to differentiate between
table, table_or_view, and relation will make things overly
complicated.
I think this is basically the right approach but I found what you did
here a bit wordy, so I simplified it, committed it, and back-patched
to 9.0 with suitable adjustment. Hopefully I didn't simplify it into
a form you don't like.
I would like to argue for reverting this. If you want to word-smith
details like this, relation doesn't carry any additional meaning. PG
hackers know that internally, a relation is a table, view, index,
sequence, etc., but for the user, it doesn't mean anything.
The original page used table_name in the synopsis in three places:
COMMENT ON {COLUMN, CONSTRAINT, and RULE}. If you're suggesting that
it's intuitively obvious what's meant by table in each of those
three cases, I respectfully disagree: I only think I know now because
I bothered to test all of these recently, and read a bit of comment.c.
(Hint: table means different things in all three cases).
I'll also note that you included index in your list of what a
relation is, and omitted composite type -- this is exactly the
confusion I was trying to avoid. COMMENT ON COLUMN no longer supports
indexes, and does support composite types. Plus, I don't think we
should be designing docs so that only PG hackers know what's really
meant. That hasn't seemed to be the modus operandi of maintaining the
rest of the docs.
Note that we don't use relation_name anywhere else in SQL command
synopses. So far, no one has complained that we don't mention that
views are allowed in the SELECT command or the GRANT command.
I actually complained upthread about CREATE RULE using the term
table in its synopsis, when it really means table or view, but I
thought that was OK because there was an immediate clarification right
below the synopsis.
I think table_name is fine, and if you are very worried, add below that
a table_name also includes views (or whatever).
It includes tables, views, composite types, and foreign tables. Is
table really an appropriate description for all those objects?
As a side note, backpatching this creates additional translation work in
backbranches. So please keep it to a minimum if it's not outright
wrong.
That's a legitimate concern; I don't have a strong opinion about
whether stuff like this ought to be backpatched.
Josh
--
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] psql: display of object comments
On Thu, Aug 4, 2011 at 12:26 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jul 26, 2011 at 8:38 PM, Josh Kupershmidt schmi...@gmail.com wrote: [new patch] I've committed the portion of this that displays comments on languages and casts. Thanks! For domains and conversions, I am wondering if we should display the comments only when + is specified, since the output is fairly wide already. I wasn't sure whether there was some sort of precedent for whether comments should be displayed only in verbose mode, but looking through the existing backslash commands, it seems reasonable to make it verbose-only if the output is already pushing 80 characters for typical usage (object names and other column outputs of lengths typically encountered). A few existing backslash commands, such as \dn and maybe \db, don't exactly follow this precedent. Not sure if we want to bother adjusting the existing commands to be consistent in this regard. Defining typical usage is pretty wishy-washy, so I'm not real inclined to try messing with the existing commands. For foreign data wrappers, foreign servers, and foreign tables, I am wondering if there is any particular rule we should adhere to in terms of where the description shows up in the output column list. It doesn't seem entirely consistent the way you've done it here, but maybe you've put more thought into it than I have. Hrm, what wasn't consistent? I intended to just put the Description column at the end of the outputs for \dew, \des, and \det, which seems to be the way other commands handle this. Though now that I double check, I notice that the verbose modes of these commands include extra columns which come after Description, and it might be better to force Description to stay at the end in those cases, the way that \dT[+] and \dFt[+] do. Though perhaps you're complaining about something different -- \dL isn't forcing Description to the end in verbose mode. Josh -- 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] vacuumlo patch
On Tue, Jul 26, 2011 at 12:18 PM, Timothy D. F. Lewis
elatl...@gmail.com wrote:
I'm not sure what David did for me so as per Roberts suggestion I have added
this patch to the commit fest.
I'm hoping I have not put this patch in more than one workflow.
Hi Tim,
I would be willing to review this patch for the next CommitFest. I'd
like to request that you send an updated version of your patch *as an
attachment* to avoid the problems with long lines getting
automatically wrapped, as Alvaro mentioned. I had trouble getting the
existing patches to apply.
A few preliminary comments about the patch:
1. It wasn't clear to me whether you're OK with Aron's suggested
tweak, please include it in your patch if so.
2. I think it might be better to use INT_MAX instead of hardcoding 2147483647.
3. Your patch has some minor code style differences wrt. the existing
code, e.g.
+if(param-transaction_limit!=0
deleted=param-transaction_limit)
should have a space before the first '(' and around the '!=' and '='
4. The rest of the usage strings spell out 'large object(s)' instead
of abbreviating 'LO'
+printf( -l LIMIT stop after removing LIMIT LOs\n);
5. transaction_limit is an int, yet you're using strtol() which
returns long. Maybe you want to use atoi() or make transaction_limit a
long?
Thanks
Josh
--
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] vacuumlo patch
On Sun, Aug 7, 2011 at 12:41 AM, Tim elatl...@gmail.com wrote: Hi Josh, Thanks for help. Attached is a patch including changes suggested in your comments. Excerpts from Josh's message On Sat, Aug 6, 2011 at 9:57 PM: 1. It wasn't clear to me whether you're OK with Aron's suggested tweak, please include it in your patch if so. I decided to and included Aron's tweak. I'm not sure if the PostgreSQL project prefers simplified code over faster run time (if only under rare conditions). Who knows maybe the compiler will re-optimize it anyway. Thanks for the quick update. It was pretty hard for me to compare your initial versions with Aron's, since I had trouble with those patches. But if it's just a minor change to how transaction_limit is handled, I wouldn't worry about it; the vast majority of vacuumlo's time is going to be spent in the database AFAICT. Incidentally, if someone wants to optimize vacuumlo, I see some low-hanging fruit there, such as maybe avoiding that expensive loop of DELETEs out of vacuum_l entirely with a bit smarter queries. But that's a problem for another day. 2. I think it might be better to use INT_MAX instead of hardcoding 2147483647. Fixed, though I'm not sure if I put the include in the preferred order. Hrm yeah.. maybe keep it so that all the system headers are together (i.e. put limits.h after fcntl.h). At least, that's how similar header files like pg_upgrade.h seem to be structured. 5. transaction_limit is an int, yet you're using strtol() which returns long. Maybe you want to use atoi() or make transaction_limit a long? The other INT in the code is using strtol() so I also used strtol instead of changing more code. I'm not sure if there are any advantages or disadvantages. maybe it's easier to prevent/detect/report overflow wraparound. Ugh, yeah you're right. I think this vacuumlo.c code is not such a great role model for clean code :-) Probably not a big deal, since you are checking for param.transaction_limit 0 which would detect overflow. I'm not sure if the other half of that check, (param.transaction_limit INT_MAX) has any meaning, i.e. how can an int ever be INT_MAX? I can't think of a good reason anyone would want to limit transaction to something more than INT_MAX. Implementing that would best be done in separate and large patch as PQntuples returns an int and is used on 271 lines across PostgreSQL. Right, I wasn't suggesting that would actually be useful - I just thought the return type of strtol() or atoi() should agree with its lvalue. I've only spent a little bit of time with this patch and LOs so far, but one general question/idea I have is whether the -l LIMIT option could be made smarter in some way. Say, instead of making the user figure out how many LOs he can feasibly delete in a single transaction (how is he supposed to know anyway, trial and error?) could we figure out what that limit should be based on max_locks_per_transaction? and handle deleting all the ophan LOs in several transactions for the user automatically? Josh -- 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] vacuumlo patch
On Sun, Aug 7, 2011 at 3:54 AM, Tim elatl...@gmail.com wrote: Excerpts from Josh's message On Sun, Aug 7, 2011 at 2:36 AM: could we figure out what that limit should be based on max_locks_per_transaction? It would be nice to implement via -l max instead of making users do it manually or something like this -l $(grep max_locks_per_transaction.*= postgresql.conf | perl -p -e 's/#.*//g;s/^.*?([0-9]+).*?$/$1/g' | grep . |head -1 ). For this patch I just want to enable the limit functionality, leaving higher level options like max to the user for now. handle deleting all the ophan LOs in several transactions for the user automatically? I addressed this option before and basically said it is an undesirable alternative because It is a less flexible option that is easily implemented in a shell script. Again it would be a welcome extra option but it can be left to the user for now. As a preface, I appreciate the work you're putting into this module, and I am all for keeping the scope of this change as small as possible so that we actually get somewhere. Having said that, it's a bit unfortunate that this module seems to be rather neglected, and has some significant usability problems. For instance, if you do blow out the max_locks_per_transaction limit, you get a very reasonable error message and hint like: Failed to remove lo 44459: ERROR: out of shared memory HINT: You might need to increase max_locks_per_transaction. Unfortunately, the code doesn't 'break;' after that, it just keeps plowing through the lo_unlink() calls, generating a ton of rather unhelpful messages like: Failed to remove lo 47087: ERROR: current transaction is aborted, commands ignored until end of transaction block which clog up stderr, and make it easy to miss the one helpful error message at the beginning. Now, here's where your patch might really help things with a minor adjustment. How about structuring that lo_unlink() call so that users will see only a reasonably helpful error message if they happen to come across this problem, like this in non-verbose mode: WARNING: out of shared memory Failed to remove lo 46304: ERROR: out of shared memory HINT: You might need to increase max_locks_per_transaction. Bailing out. Try using -l LIMIT flag, with a LIMIT of 1845 (Side note: I was asking upthread about how to figure out what LIMIT value to use, because I don't understand how max_locks_per_transaction relates to the number of lo_unlink() calls one can make in a transaction... I seem to be able use a limit of 1845, but 1846 will error out, with max_locks_per_transaction = 64. Anyone know why this is?) A related problem I noticed that's not really introduced by your script, but which might easily be fixed, is the return value from vacuumlo(). The connection and query failures at the top of the function all return -1 upon failure, but if an lo_unlink() call fails and the entire transaction gets rolled back, vacuumlo() happily returns 0. I've put together an updated version of your patch (based off your latest version downstream with help output alphabetized) showing how I envision these two problems being fixed. Also, a small adjustment to your SGML changes to blend in better. Let me know if that all looks OK or if you'd rather handle things differently. The new error messages might well need some massaging. I didn't edit the INT_MAX stuff either, will leave that for you. Josh vacuumlo.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] psql: display of object comments
On Mon, Aug 8, 2011 at 4:34 PM, Robert Haas robertmh...@gmail.com wrote: OK, I've now committed most of this, with some additions to the documentation. Remaining bits attached. Looks good, thanks. I am a bit confused as to why we have both \det and \dE. They seem redundant. Shouldn't we rip one of those out? IMHO, \det should be the one to go, as it could be useful to do, e.g. \dtvE, which isn't going to work with the \det syntax. Yeah, I was wondering that myself. At first I thought maybe someone added in one without being aware of the other, but it looks like both \dE and \det got added in by commit 0d692a0dc9f0e532c67c577187fe5d7d323cb95b. They are using different queries internally (pg_class vs. pg_foreign_table), but I doubt end users would care about that. Or perhaps the author just wanted a command name similar to \dew and \des. +1 for getting rid of one of them; I don't really care which one gets the axe. Though we should make sure to be able to show all possible columns in whichever command we keep (i.e. add Options column to \dE+ if we keep that one). BTW, I haven't bothered setting up functioning foreign tables yet, but is Size always going to be 0 bytes in \dE+? Josh -- 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] psql: display of object comments
On Mon, Aug 8, 2011 at 6:01 PM, Josh Kupershmidt schmi...@gmail.com wrote: (i.e. add Options column to \dE+ if we keep that one). Oh nevermind, Options is displayed by \d+ foreign_table_name. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] mb_regress.sh gripes
Hi all, A few gripes about mb_regress.sh: 1. No exit code is specified, so even if there are differences between results/ and expected/ the script will still return 0. 2. The 'dropdb' command is used to wipe out the utf8 database before the run. This generates an error message like: dropdb: database removal failed: ERROR: database utf8 does not exist the first time you run the script. IMO it would be less startling to just print a NOTICE here. 3. No error checking for whether createdb succeeds. The attached patch fixes these problems. Josh mb_regress_gripes.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] dropdb and dropuser: IF EXISTS
I noticed a few places where it would be handy if dropdb took a flag
like --if-exists which would basically just add in the 'IF EXISTS'
clause to the DROP DATABASE statement. For example, scripts like
find_static or mbregress.sh use dropdb createdb, but they generate
noisy errors from dropdb when run for the first time since there's no
--if-exists flag. (They could just pipe 'DROP DATABASE IF EXISTS ...'
to psql, but what's the point of having dropdb if it's not used?)
Attached is a very quick patch implementing the --if-exists or -X
option for dropdb and dropuser. I didn't bother adding in a check to
make sure the server version was 8.2+ since we're not even supporting
8.1 nowadays, though that'd be easy enough to add in.
Josh
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index e20bcdb..2092bb6 100644
*** a/doc/src/sgml/ref/dropdb.sgml
--- b/doc/src/sgml/ref/dropdb.sgml
*** PostgreSQL documentation
*** 87,92
--- 87,102
/varlistentry
varlistentry
+ termoption-X//term
+ termoption--if-exists//term
+ listitem
+para
+Don't report an error if the specified database does not exist.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-V//term
termoption--version//term
listitem
diff --git a/doc/src/sgml/ref/dropuser.sgml b/doc/src/sgml/ref/dropuser.sgml
index c158103..22580a4 100644
*** a/doc/src/sgml/ref/dropuser.sgml
--- b/doc/src/sgml/ref/dropuser.sgml
*** PostgreSQL documentation
*** 89,94
--- 89,104
/varlistentry
varlistentry
+ termoption-X//term
+ termoption--if-exists//term
+ listitem
+para
+ Don't report an error if the specified user does not exist.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-V//term
termoption--version//term
listitem
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index 4cec63e..187bf6c 100644
*** a/src/bin/scripts/dropdb.c
--- b/src/bin/scripts/dropdb.c
*** main(int argc, char *argv[])
*** 29,34
--- 29,35
{password, no_argument, NULL, 'W'},
{echo, no_argument, NULL, 'e'},
{interactive, no_argument, NULL, 'i'},
+ {if-exists, no_argument, NULL, 'X'},
{NULL, 0, NULL, 0}
};
*** main(int argc, char *argv[])
*** 43,48
--- 44,50
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
+ bool if_exists = false;
PQExpBufferData sql;
*** main(int argc, char *argv[])
*** 54,60
handle_help_version_opts(argc, argv, dropdb, help);
! while ((c = getopt_long(argc, argv, h:p:U:wWei, long_options, optindex)) != -1)
{
switch (c)
{
--- 56,62
handle_help_version_opts(argc, argv, dropdb, help);
! while ((c = getopt_long(argc, argv, h:p:U:wWeiX, long_options, optindex)) != -1)
{
switch (c)
{
*** main(int argc, char *argv[])
*** 79,84
--- 81,89
case 'i':
interactive = true;
break;
+ case 'X':
+ if_exists = true;
+ break;
default:
fprintf(stderr, _(Try \%s --help\ for more information.\n), progname);
exit(1);
*** main(int argc, char *argv[])
*** 110,117
initPQExpBuffer(sql);
! appendPQExpBuffer(sql, DROP DATABASE %s;\n,
! fmtId(dbname));
/*
* Connect to the 'postgres' database by default, except have the
--- 115,122
initPQExpBuffer(sql);
! appendPQExpBuffer(sql, DROP DATABASE %s%s;\n,
! (if_exists ? IF EXISTS : ), fmtId(dbname));
/*
* Connect to the 'postgres' database by default, except have the
*** help(const char *progname)
*** 146,151
--- 151,157
printf(_(\nOptions:\n));
printf(_( -e, --echoshow the commands being sent to the server\n));
printf(_( -i, --interactive prompt before deleting anything\n));
+ printf(_( -X, --if-exists don't report error if database doesn't exist\n));
printf(_( --helpshow this help, then exit\n));
printf(_( --version output version information, then exit\n));
printf(_(\nConnection options:\n));
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 0949a5e..bf5196f 100644
*** a/src/bin/scripts/dropuser.c
--- b/src/bin/scripts/dropuser.c
*** main(int argc, char *argv[])
*** 29,34
--- 29,35
{password, no_argument, NULL, 'W'},
{echo, no_argument, NULL, 'e'},
{interactive, no_argument, NULL, 'i'},
+ {if-exists, no_argument, NULL, 'X'},
{NULL, 0, NULL, 0}
};
*** main(int argc, char *argv[])
*** 43,48
--- 44,50
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
+ bool if_exists = false;
Re: [HACKERS] dropdb and dropuser: IF EXISTS
On Fri, Aug 26, 2011 at 10:42 PM, Robert Haas robertmh...@gmail.com wrote:
+1 for --if-exists, but -X isn't doing a lot for me, especially since
we've used -X for other purposes in other commands. I'd just skip
having a short form for this one.
Fine by me. Updated patch attached.
Josh
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index e20bcdb..509b41e 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -87,6 +87,15 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists//term
+ listitem
+ para
+ Don't report an error if the specified database does not exist.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-V//term
termoption--version//term
listitem
diff --git a/doc/src/sgml/ref/dropuser.sgml b/doc/src/sgml/ref/dropuser.sgml
index c158103..0fb917d 100644
--- a/doc/src/sgml/ref/dropuser.sgml
+++ b/doc/src/sgml/ref/dropuser.sgml
@@ -89,6 +89,15 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists//term
+ listitem
+ para
+Don't report an error if the specified user does not exist.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-V//term
termoption--version//term
listitem
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index 4cec63e..b4b10b8 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -29,6 +29,7 @@ main(int argc, char *argv[])
{password, no_argument, NULL, 'W'},
{echo, no_argument, NULL, 'e'},
{interactive, no_argument, NULL, 'i'},
+ {if-exists, no_argument, NULL, 'X'},
{NULL, 0, NULL, 0}
};
@@ -43,6 +44,7 @@ main(int argc, char *argv[])
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
+ bool if_exists = false;
PQExpBufferData sql;
@@ -79,6 +81,9 @@ main(int argc, char *argv[])
case 'i':
interactive = true;
break;
+ case 'X':
+if_exists = true;
+break;
default:
fprintf(stderr, _(Try \%s --help\ for more information.\n), progname);
exit(1);
@@ -110,8 +115,8 @@ main(int argc, char *argv[])
initPQExpBuffer(sql);
- appendPQExpBuffer(sql, DROP DATABASE %s;\n,
- fmtId(dbname));
+ appendPQExpBuffer(sql, DROP DATABASE %s%s;\n,
+ (if_exists ? IF EXISTS : ), fmtId(dbname));
/*
* Connect to the 'postgres' database by default, except have the
@@ -146,6 +151,7 @@ help(const char *progname)
printf(_(\nOptions:\n));
printf(_( -e, --echoshow the commands being sent to the server\n));
printf(_( -i, --interactive prompt before deleting anything\n));
+ printf(_( --if-exists don't report error if database doesn't exist\n));
printf(_( --helpshow this help, then exit\n));
printf(_( --version output version information, then exit\n));
printf(_(\nConnection options:\n));
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 0949a5e..13abb54 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -29,6 +29,7 @@ main(int argc, char *argv[])
{password, no_argument, NULL, 'W'},
{echo, no_argument, NULL, 'e'},
{interactive, no_argument, NULL, 'i'},
+ {if-exists, no_argument, NULL, 'X'},
{NULL, 0, NULL, 0}
};
@@ -43,6 +44,7 @@ main(int argc, char *argv[])
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
+ bool if_exists = false;
PQExpBufferData sql;
@@ -79,6 +81,9 @@ main(int argc, char *argv[])
case 'i':
interactive = true;
break;
+ case 'X':
+if_exists = true;
+break;
default:
fprintf(stderr, _(Try \%s --help\ for more information.\n), progname);
exit(1);
@@ -110,7 +115,8 @@ main(int argc, char *argv[])
}
initPQExpBuffer(sql);
- appendPQExpBuffer(sql, DROP ROLE %s;\n, fmtId(dropuser));
+ appendPQExpBuffer(sql, DROP ROLE %s%s;\n,
+ (if_exists ? IF EXISTS : ), fmtId(dropuser));
conn = connectDatabase(postgres, host, port, username, prompt_password, progname);
@@ -141,6 +147,7 @@ help(const char *progname)
printf(_(\nOptions:\n));
printf(_( -e, --echoshow the commands being sent to the server\n));
printf(_( -i, --interactive prompt before deleting anything\n));
+ printf(_( --if-exists don't report error if user doesn't exist\n));
printf(_( --helpshow this help, then exit\n));
printf(_( --version output version information, then exit\n));
printf(_(\nConnection options:\n));
--
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] dropdb and dropuser: IF EXISTS
On Tue, Aug 30, 2011 at 11:14 AM, Robert Haas robertmh...@gmail.com wrote: Committed with some edits. I stole the documentation language from the DROP DATABASE and DROP USER pages and just copied it over, instead of saying the same thing in different words. And I rearranged the options handling to be more consistent with what we do elsewhere. Thanks. I think you accidentally copied the DROP DATABASE snippet into dropuser.sgml as well. Josh -- 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] bugfix: --echo-hidden is not supported by \sf statements
On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/14 Tom Lane t...@sss.pgh.pa.us: Well, fine, but then it should fix both of them and remove minimal_error_message altogether. I would however suggest eyeballing what happens when you try \ef nosuchfunction (with or without -E). I'm pretty sure that the reason for the special error handling in these functions is that the default error reporting was unpleasant for this use-case. so I rewrote patch still is simple On the end I didn't use PSQLexec - just write hidden queries directly from related functions - it is less invasive Sorry for the delay, but I finally had a chance to review this patch. I think the patch does a good job of bringing the behavior of \sf and \ef in-line with the other meta-commands as far as --echo-hidden is concerned. About the code changes: The bulk of the code change comes from factoring TraceQuery() out of PSQLexec(), so that \ef and \sf may make use of this query-printing without going through PSQLexec(). And not using PSQLexec() lets us avoid a somewhat uglier error message like: ERROR: function nonexistent does not exist LINE 1: SELECT 'nonexistent'::pg_catalog.regproc::pg_catalog.oid Tom suggested removing minimal_error_message() entirely, which would be nice if possible. It is a shame that \sf nonexistent produces a scary-looking ERROR: message (and would cause any in-progress transaction to need a rollback) while the other informational metacommands do not. Actually, the other metacommands are not entirely consistent with each other; compare \dt nonexistent and \df nonexistent. It would be nice if the case of a matching function not found by \sf or \ef could be handled in the same fashion as: # \d nonexistent Did not find any relation named nonexistent. though I realize that's not trivial due to the implementation of lookup_function_oid(). At any rate, this nitpicking about the error handling in the case that the function is not found is quibbling about behavior unchanged by the patch. I don't have any complaints about the patch itself, so I think it can be applied as-is, and we can attack the error handling issue separately. Josh -- 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] bugfix: --echo-hidden is not supported by \sf statements
On Wed, Feb 27, 2013 at 12:09 PM, Stephen Frost sfr...@snowman.net wrote: * Pavel Stehule (pavel.steh...@gmail.com) wrote: I don't agree so it works well - you cannot use short type names is significant issue This is for psql. In what use-case do you see that being a serious limitation? I might support having psql be able to fall-back to checking if the function name is unique (or perhaps doing that first before going on to look at the function arguments) but I don't think this should all be punted to the backend where only 9.3+ would have any real support for a capability which already exists in other places and should be trivially added to these. Since time is running short for discussion of 9.3: I still think this patch is an improvement over the status quo, and is committable as-is. Yes, the patch doesn't address the existing ugliness with minimal_error_message() and sidestepping PSQLexec(), but at least it fixes the --echo-hidden behavior, and the various other issues may be handled separately. Josh -- 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] bugfix: --echo-hidden is not supported by \sf statements
On Mon, Mar 4, 2013 at 11:39 AM, Stephen Frost sfr...@snowman.net wrote: Josh, * Josh Kupershmidt (schmi...@gmail.com) wrote: I still think this patch is an improvement over the status quo, and is committable as-is. Yes, the patch doesn't address the existing ugliness with minimal_error_message() and sidestepping PSQLexec(), but at least it fixes the --echo-hidden behavior, and the various other issues may be handled separately. Which patch, exactly, are you referring to wrt this..? I'm less than convinced that it's in a committable state, but I'd like to make sure that we're talking about the same thing here... Sorry, this second version posted by Pavel: http://www.postgresql.org/message-id/cafj8prb3-tov5s2dcgshp+vedyk9s97d7hn7rdmmw9ztrj-...@mail.gmail.com Josh -- 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] bugfix: --echo-hidden is not supported by \sf statements
On Mon, Mar 4, 2013 at 11:54 AM, Stephen Frost sfr...@snowman.net wrote: Yeah, no, I don't think we should go in this direction. The whole TraceQuery thing is entirely redundant to what's already there and which should have been used from the beginning. This would be adding on to that mistake instead of fixing it. If we're going to fix it, let's fix it correctly. Fair enough. I thought the little extra ugliness was stomachable, but I'm willing to call this patch Returned with Feedback for now. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
