Re: [HACKERS] Re: Privileges for INFORMATION_SCHEMA.SCHEMATA (was Re: [DOCS] Small clarification in 34.41. schemata)
2013/1/15 Tom Lane t...@sss.pgh.pa.us: Casey Allen Shobe ca...@shobe.info writes: On Wed, Jan 9, 2013 at 8:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: However, it seems to me that this behavior is actually wrong for our purposes, as it represents a too-literal reading of the spec. The SQL standard has no concept of privileges on schemas, only ownership. We do have privileges on schemas, so it seems to me that the consistent thing would be for this view to show any schema that you either own or have some privilege on. IMHO, schemata should follow the standard as it does today. Other platforms have privileges on schemas as well, and this sort of thing seems to fall into the same bucket as other platform compatibilities outside the scope of what the standard thinks about, which means you use pg_catalog to access that information rather than information_schema, which should be expected to work consistently on all platforms that implement it. Meh. To me, standards compliance requires that if you have created a SQL-compliant database, you'd better see spec-compliant output from the information schema. As soon as you do something outside the standard (in this instance, grant some privileges on a schema), it becomes a judgment call whether and how that should affect what you see in the information schema. It may be that the current behavior of this view is actually the best thing, but a standards-compliance argument doesn't do anything to convince me. regards, tom lane My original assumption here was that the documentation [1] was in need of clarification. On the other hand the current output of information_schema.schemata isn't quite I was expecting, which would be as Tom writes: the consistent thing would be for this view to show any schema that you either own or have some privilege on. As it stands, the only way of extracting a list of visible schemas from PostgreSQL's information_schema (i.e. without relying on PostgreSQL-specific system functions) is doing something like this: SELECT DISTINCT(table_schema) FROM information_schema.tables Digging about a bit [2], it seems the only other RDBMSes with a fully-fledged information_schema are Microsoft SQL Server and MySQL. I don't have access to SQL Server; the documentation [3] says Returns one row for each schema in the current database, which also strikes me as incorrect (can someone confirm this behaviour?). For MySQL, the documentation [4] indicates that their implementation shows all schemas (in MySQL: databases) visible to the current user, and I've confirmed this behaviour with MySQL 5.5. Personally I'd support modifying PostgreSQL's information_schema.schemata to show all schemas the current user owns/has privileges on, providing it's not an egregious violation of the SQL standard. It seems I'm not the only user who has been stymied by this issue [5][6][7]; also, resolving it would also make it consistent with MySQL's output [8] [1] http://www.postgresql.org/docs/9.2/static/infoschema-schemata.html [2] http://en.wikipedia.org/wiki/Information_schema [3] http://msdn.microsoft.com/en-us/library/ms182642.aspx [4] http://dev.mysql.com/doc/refman/5.5/en/schemata-table.html [5] http://www.postgresql.org/message-id/CAFjNrYv4MrkbXi-usroCqNiaSyEAzvJ7GjtsEJW2RK7-R=8...@mail.gmail.com [6] http://www.postgresql.org/message-id/200612211146.kblbklqa001...@wwwmaster.postgresql.org [7] http://www.postgresql.org/message-id/50aff3fe.4030...@gmail.com [8] Not that I'm claiming MySQL's implementation is authoritative or anything Regards Ian Lawrence Barwick -- 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: small patch to correct filename formatting error in '\s FILE' output
I've noticed a filename error in feedback messages from psql's '\s' command when saving the command line history to a file specified by an absolute filepath: psql (9.2.2) Type help for help. pgdevel=# \s history.txt Wrote history to file ./history.txt. pgdevel=# \s /tmp/history.txt Wrote history to file .//tmp/history.txt. pgdevel=# \cd /tmp pgdevel=# \s /tmp/history.txt Wrote history to file /tmp//tmp/history.txt. The second and third '\s' commands display incorrect filepaths in the feedback message, despite writing correctly to the specified file. Also, if the specified file could not be written to, the error message displayed formats the filepath differently (i.e. it does not prepend the current working directory), which is potentially confusing, and certainly visually inconsistent: pgdevel=# \cd /tmp pgdevel=# \s foo/history.txt could not save history to file foo/history.txt: No such file or directory pgdevel=# \! mkdir foo pgdevel=# \s foo/history.txt Wrote history to file /tmp/foo/history.txt. The attached patch rectifies these issues by adding a small function 'format_fname()' to psql/stringutils.c which formats the filepath appropriately, depending on whether an absolute filepath was supplied or psql's cwd is set. pgdevel_head=# \s history.txt Wrote history to file ./history.txt. pgdevel_head=# \s /tmp/history.txt Wrote history to file /tmp/history.txt. pgdevel_head=# \cd /tmp pgdevel_head=# \s /tmp/history.txt Wrote history to file /tmp/history.txt. pgdevel_head=# \cd /tmp pgdevel_head=# \s bar/history.txt could not save history to file /tmp/bar/history.txt: No such file or directory pgdevel_head=# \! mkdir bar pgdevel_head=# \s bar/history.txt Wrote history to file /tmp/bar/history.txt. Notes/caveats - The function 'format_fname()' deterimines whether the supplied filepath is absolute by checking for the presence of a '/' as the first character. This strikes me as a bit hacky but I can't think of an alternative. - As far as I can tell, Windows does not support the '\s' command, so there is presumably no need to worry about supporting Windows-style file paths - As far as I can tell, this is the only psql slash command which, after saving data to a file, provides a feedback message containing the filename/path. Regards Ian Lawrence Barwick psql-save-history-2013-01-21.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: small patch to correct filename formatting error in '\s FILE' output
2013/1/23 Tom Lane t...@sss.pgh.pa.us: I wrote: If we did think that this specific backslash command needed to be able to print something other than the filename as-entered, I'd be inclined to just apply make_absolute_path() to the name, instead of relying on inadequate dead-reckoning. However, that would require making make_absolute_path available in src/port/ or someplace, which seems a bit more than this feature is worth. Why should \s, and \s alone, need to remind you where you're cd'd to? It strikes me that a more useful reminder feature could be implemented by having \cd itself print the new current directory, which it could do with a simple call to getcwd(), thus not requiring refactoring of make_absolute_path. Then for instance if you'd forgotten where you were, \cd . would tell you. \! pwd does the trick as well. However personally I prefer to get some kind of feedback from an action, so having \cd confirm the directory would be nice. I'll submit a patch. Related email from the archives on this subject: http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com Does commit 0725065b just need to be reverted, or is an additional patch required to remove the prefixed working directory from \s output? Ian Barwick -- 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] proposal or just idea for psql - show first N rows from relation backslash statement
2013/2/14 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: * Pavel Stehule (pavel.steh...@gmail.com) wrote: SELECT * FROM some_relation LIMIT 10 what do you thinking about creating special statement for this purpose? I'd rather extend TABLE to support a limit clause or something. Can't you pretty much do this already in psql with FETCH_COUNT? I see no good reason to invent more SQL syntax. Doesn't that just split up the retrieval of the result set into blocks of FETCH_COUNT rows, i.e. does not limit the result set? Personally I set commonly-used queries as a psql variable, though what Pavel suggests sounds useful and AFAICT is not additional SQL syntax, just (yet another) psql slash command. Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Contrib module xml2 status
Hi I'm not sure if this is a documentation or hackers issue, but the documentation page for contrib module xml2 refers to PostgreSQL 8.4 in the future tense: It is planned that this module will be removed in PostgreSQL 8.4 in favor of the newer standard API http://www.postgresql.org/docs/devel/static/xml2.html Are there any plans to remove this module by a forseeable date? Regards Ian Barwick
Re: [HACKERS] [DOCS] Contrib module xml2 status
2013/2/22 Andrew Dunstan and...@dunslane.net: On 02/21/2013 12:56 PM, Magnus Hagander wrote: On Thu, Feb 21, 2013 at 6:39 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Feb 20, 2013 at 11:58 AM, Ian Lawrence Barwick barw...@gmail.com wrote: I'm not sure if this is a documentation or hackers issue, but the documentation page for contrib module xml2 refers to PostgreSQL 8.4 in the future tense: It is planned that this module will be removed in PostgreSQL 8.4 in favor of the newer standard API http://www.postgresql.org/docs/devel/static/xml2.html Are there any plans to remove this module by a forseeable date? Nope. I have repeatedly been voted down on removing it, and I've also been repeatedly voted down on removing the deprecation text. Could we at least agree on changing the deprecation text to say This module is deprecated and may be removed in a future release? Not reopening the actual discussion about rmeoving it, but assuming we're not, strong +1 on changing the deprecation message. And don't forget to backpatch the change so it shows up in the old versions of the docs as well. Yes, we should change it to remove the reference to 8.4. Documentation patch attached. The point is we can remove the module when someone fixes and replaces the functionality that's left in there that some people rely on. So far nobody has stepped up to the plate, although now that we have lateral a sane replacement for xpath_table might well be a lot easier to achieve. If someone is interested in working on this I'd be happy to hear about it. Maybe it would be a good Google SOC project. It might be worth adding an explicit entry in the TODO list for removing this and summarising what needs to be done. https://wiki.postgresql.org/wiki/Todo#XML Regards Ian Barwick doc-contrib-xml2-2013-02-22.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] Small patch for CREATE TRIGGER documentation
I found this sentence somewhat confusing: It is possible for a column's value to change even when the trigger is not fired, http://www.postgresql.org/docs/devel/static/sql-createtrigger.html#SQL-CREATETRIGGER-NOTES More precise would be something along the lines It is possible that changes to the column's value will not cause the trigger to be fired. The attached patch hopefully rewords the entire paragraph for clarity. Regards Ian Barwick create-trigger-doc-notes-2013-03-07.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] Minor erratum for 9.2.4 release notes
I guess the 9.2.4 release notes haven't been finalized yet; apologies if this is already addressed, but following sentence: para Also, if you are upgrading from a version earlier than 9.2.2, see the release notes for 9.2.2. /para should read: para Also, if you are upgrading from a version earlier than 9.2.3, see the release notes for 9.2.3. /para Regards Ian Barwick -- 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] Minor erratum for 9.2.4 release notes
2013/4/4 Tom Lane t...@sss.pgh.pa.us: Ian Lawrence Barwick barw...@gmail.com writes: I guess the 9.2.4 release notes haven't been finalized yet; apologies if this is already addressed, but following sentence: para Also, if you are upgrading from a version earlier than 9.2.2, see the release notes for 9.2.2. /para should read: para Also, if you are upgrading from a version earlier than 9.2.3, see the release notes for 9.2.3. /para Um, no, there's no special instructions attached to 9.2.3. The point of that para is to thread back to the last minor release that had something extra for you to do. Got it, sorry for the noise Regards Ian Barwick -- 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] 9.3 release notes suggestions
2013/4/24 Bruce Momjian br...@momjian.us: Thanks for the many suggestions on improving the 9.3 release notes. There were many ideas I would have never thought of. Please keep the suggestions coming. One small suggestion: listitem para Have quotesession id/ (literal%c/) in link linkend=guc-log-line-prefixvarnamelog_line_prefix//link always output four hex digits after the period (Bruce Momjian) /para /listitem This doesn't sound quite right - on OS X at least, PIDs go up to 8, which means %c may output 5 hex digits after the period. The following might be more pedantically accurate: listitem para Have quotesession id/ (literal%c/) in link linkend=guc-log-line-prefixvarnamelog_line_prefix//link always pad the PID value with zeros so at least four hex digits are displayed after the period (Bruce Momjian) /para /listitem if my slightly disengaged brain is grokking the source correctly: src/backend/utils/error/elog.c: appendStringInfo(buf, %lx.%04x, (long) (MyStartTime), MyProcPid); Regards Ian Barwick -- 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] 9.3 release notes suggestions
2013/5/4 Bruce Momjian br...@momjian.us: On Sat, May 4, 2013 at 08:34:18PM +0900, Ian Lawrence Barwick wrote: 2013/4/24 Bruce Momjian br...@momjian.us: Thanks for the many suggestions on improving the 9.3 release notes. There were many ideas I would have never thought of. Please keep the suggestions coming. One small suggestion: listitem para Have quotesession id/ (literal%c/) in link linkend=guc-log-line-prefixvarnamelog_line_prefix//link always output four hex digits after the period (Bruce Momjian) /para /listitem This doesn't sound quite right - on OS X at least, PIDs go up to 8, which means %c may output 5 hex digits after the period. The following might be Oh, I was curious if some OS had larger pid values. I am concerned you aren't going to get session ids of consistent length on that platform. more pedantically accurate: listitem para Have quotesession id/ (literal%c/) in link linkend=guc-log-line-prefixvarnamelog_line_prefix//link always pad the PID value with zeros so at least four hex digits are displayed after the period (Bruce Momjian) /para /listitem OK, changed to: Have quotesession id/ (literal%c/) in link linkend=guc-log-line-prefixvarnamelog_line_prefix//link always output at least four hex digits after the period (Bruce Momjian) This is such a minor change I am trying to keep it short. Just out of curiosity, what was the reason for the change in the first place? (Not that it's something I'm particularly passionate about, I just noticed it when listing changes with potential backwards-compatibilty effects for the wiki). if my slightly disengaged brain is grokking the source correctly: src/backend/utils/error/elog.c: appendStringInfo(buf, %lx.%04x, (long) (MyStartTime), MyProcPid); Yep. In that case maybe the docs need updating as well? http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-LOG-LINE-PREFIX The %c escape prints a quasi-unique session identifier, consisting of two 4-byte hexadecimal numbers (without leading zeros) separated by a dot. Regards Ian Barwick -- 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] 9.3 release notes suggestions
2013/5/5 Bruce Momjian br...@momjian.us: (...) This is such a minor change I am trying to keep it short. Just out of curiosity, what was the reason for the change in the first place? (Not that it's something I'm particularly passionate about, I just noticed it when listing changes with potential backwards-compatibilty effects for the wiki). Well, basically, if you used %c in log_line_prefix, the session id was not a fixed length, so your output shifted around based on the pid, see: http://www.postgresql.org/message-id/20121012185127.gb31...@momjian.us Always showing four digits seems to give greater consistency to the log output. Makes sense as long as your PIDs stay below 0x1, but on OS X it makes it less consistent IMHO, as you still end up with a varying number of digits: 5184ea1f.15ed2 LOG: database system was shut down at 2013-05-04 19:59:41 JST 5184ea1f.15ed1 LOG: database system is ready to accept connections 5184ea1f.15ed6 LOG: autovacuum launcher started 5184ea23.15edb ERROR: column x does not exist at character 8 5184ea23.15edb STATEMENT: select x; 51852890.0a0a ERROR: column x does not exist at character 8 51852890.0a0a STATEMENT: select x; (tested using 9.3 HEAD) if my slightly disengaged brain is grokking the source correctly: src/backend/utils/error/elog.c: appendStringInfo(buf, %lx.%04x, (long) (MyStartTime), MyProcPid); Yep. In that case maybe the docs need updating as well? http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-LOG-LINE-PREFIX The %c escape prints a quasi-unique session identifier, consisting of two 4-byte hexadecimal numbers (without leading zeros) separated by a dot. Uh, that was never right, because the part before the dot is the session start timestamp, and that is 8 hex digits: 50785b3e.7ff9 I understood it as a 4-byte number expressed in hex, which in this case even without zero padding is always going to be 8 hex digits unless your system clock is stuck in the 1970s. I have changed the text to: The literal%c/ escape prints a quasi-unique session identifier, consisting of two hexadecimal numbers separated by a dot. Doc fix backpatched to 9.2.X. Covers all bases :) However it just occurred to me the example following that paragraph is incorrect for 9.3, as the to_hex(pid) output won't be zero-padded. Sorry to be pedantic about this, like I said it's not something I am particularly passionate about and I'd never even taken notice of the %c option, but at least on OS X the documentation didn't match the observed behaviour. Regards Ian Barwick -- 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] \watch stuck on execution of commands returning no tuples
2013/5/8 Robert Haas robertmh...@gmail.com: On Thu, May 2, 2013 at 7:53 PM, Bruce Momjian br...@momjian.us wrote: OK, what other database supports British spelling of commands? Can we call this a compatibility feature. ;-) The feature has been there since 2000: commit ebe0b236909732c75d665c73363bd4ac7a7aa138 Author: Bruce Momjian br...@momjian.us Date: Wed Nov 8 21:28:06 2000 + Add ANALYSE spelling of ANALYZE for vacuum. Maybe we should also support ANALIZAR, ANALYSER, and 分析する. As a British native speaker involved in translating some PostgreSQL-related Japanese text, all I can say is yes please. (Although for true Japanese support, the grammar would have to be pretty much reversed, with the verb being placed last; and WHERE would come before SELECT, which might challenge the parser a little). (I don't know whether I'm joking, so don't ask.) I think I am joking. Regards Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] (trivial patch) remove superfluous semicolons from pg_dump
I noticed an instance of 'appendPQExpBuffer(query, ;);' in pg_dump.c which seems pointless and mildly confusing. There's another seemingly useless one in pg_dumpall.c. Attached patch removes both (if that makes any sense). Regards Ian Barwick pg_dump-cull-semicolons.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] pg_restore: show object schema name in verbose output
I just noticed that pg_restore executing in verbose mode displays the name of the object being restored, but not its schema. I'd like to be able to see the fully-qualified object name because if pg_restore spits out a warning like this: $ pg_restore -d somedb /path/to/dumpfile.pgd pg_restore: WARNING: column session_id has type unknown DETAIL: Proceeding with relation creation anyway. $ verbose mode is useful to identify which object is at issue, e.g.: $ pg_restore -v -d somedb /path/to/dumpfile.pgd pg_restore: connecting to database for restore (...) pg_restore: creating VIEW someview pg_restore: WARNING: column session_id has type unknown DETAIL: Proceeding with relation creation anyway. (...) $ but only shows the bare object name. In the case I recently encountered, objects with the same name existed in multiple schemas, which meant it took longer to track down the offending object than it could have done. The attached patch changes the output to print the schema name too, e.g.: $ pg_restore -v -d somedb /path/to/dumpfile.pgd pg_restore: connecting to database for restore (...) pg_restore: creating VIEW schema94.someview pg_restore: WARNING: column session_id has type unknown DETAIL: Proceeding with relation creation anyway. (...) $ which is more useful, IMHO. Regards Ian Barwick pg-restore-verbose-output-schema-2013-08-03.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_restore: show object schema name in verbose output
2013/8/4 Erik Rijkers e...@xs4all.nl: On Sun, August 4, 2013 04:51, Ian Lawrence Barwick wrote: I just noticed that pg_restore executing in verbose mode displays the name of the object being restored, but not its schema. Good idea. We have many schemata with tables of the same name and reporting the schema name certainly improves the readability and error trackdown during restore. I notice 2 things: 1. pg_restore now outputs reports about COMMENT like this: pg_restore: creating COMMENT restore_verbose_test.TABLE t pg_restore: creating COMMENT restore_verbose_test.COLUMN t.c pg_restore: creating COMMENT restore_verbose_test.COLUMN t.i I assume the .TABLE and .COLUMN here is a bug; it should just be: pg_restore: creating COMMENT restore_verbose_test t as it used to be. 2. Several of the lines that are output by pg_restore now mention the schema, but not the processing line: pg_restore: processing data for table t Could it be added there too? Thanks for the feedback and test case. I'll submit a revised patch. Regards Ian Barwick -- 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_restore: show object schema name in verbose output
2013/8/4 Ian Lawrence Barwick barw...@gmail.com: 2013/8/4 Erik Rijkers e...@xs4all.nl: On Sun, August 4, 2013 04:51, Ian Lawrence Barwick wrote: I just noticed that pg_restore executing in verbose mode displays the name of the object being restored, but not its schema. Good idea. We have many schemata with tables of the same name and reporting the schema name certainly improves the readability and error trackdown during restore. I notice 2 things: 1. pg_restore now outputs reports about COMMENT like this: pg_restore: creating COMMENT restore_verbose_test.TABLE t pg_restore: creating COMMENT restore_verbose_test.COLUMN t.c pg_restore: creating COMMENT restore_verbose_test.COLUMN t.i I assume the .TABLE and .COLUMN here is a bug; it should just be: pg_restore: creating COMMENT restore_verbose_test t as it used to be. Actually the current output would be: pg_restore: creating COMMENT TABLE t Anyway this is a bit trickier than I originally thought, but I understand the inner workings of pg_restore et al better now anyway :) 2. Several of the lines that are output by pg_restore now mention the schema, but not the processing line: pg_restore: processing data for table t Could it be added there too? That looks quite straightforward. Thanks for the feedback and test case. I'll submit a revised patch. The attached patch should work somewhat better, but methinks I'll need to work on it a bit more. Also, for the sake of consistency it would be useful to show the schema (where appropriate) in the owner/privileges setting output, e.g.: pg_restore: setting owner and privileges for TABLE t Regards Ian Barwick pg-restore-verbose-output-schema-2013-08-04.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: small patch to correct filename formatting error in '\s FILE' output
2013/9/10 Bruce Momjian br...@momjian.us: On Tue, Jan 22, 2013 at 07:30:59PM -0500, Tom Lane wrote: Ian Lawrence Barwick barw...@gmail.com writes: Related email from the archives on this subject: http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com I agree with the opinion stated there that \cd with no argument really ought to do what cd with no argument usually does on the platform. So if we're going to fix \cd to print the resulting current directory, wouldn't it work to just set dir to . rather than / for Windows? Does commit 0725065b just need to be reverted, or is an additional patch required to remove the prefixed working directory from \s output? Offhand it looked like reverting the commit would be enough, but I didn't look hard to see if there had been any subsequent related changes. [ pokes around... ] Well, at least there are still no other uses of pset.dirname. I still see that weird behavior in git head: pgdevel=# \s history.txt Wrote history to file ./history.txt. pgdevel=# \s /tmp/history.txt Wrote history to file .//tmp/history.txt. pgdevel=# \cd /tmp pgdevel=# \s /tmp/history.txt Wrote history to file /tmp//tmp/history.txt. Should I revert the suggested patch? IIRC the patch was never applied, the reversion candidate is the existing commit 0725065b. (Sorry for not following up earlier, this one dropped off my radar). Regards Ian Barwick -- 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] Commitfest II, 9.4 edition
2013/9/16 David Fetter da...@fetter.org: has begun! New hackers, this is your chance to contribute by doing the first few steps of review. Seasoned hackers, this is a way to help the whole project along. You know the drill. Too many patches have Nobody today. That's the first thing we'll need to fix, one way or another. Let's make this one great! And for those of us who haven't really participated before, a couple of useful links ;) https://commitfest.postgresql.org/action/commitfest_view/inprogress http://wiki.postgresql.org/wiki/Reviewing_a_Patch It might be worth adding a link on the developer page ( http://www.postgresql.org/developer/ ), as the existence of commitfests is otherwise very well hidden (I ended up googling the link). Regards Ian Barwick -- 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.print_strict_params
2013/9/15 Marko Tiikkaja ma...@joh.to: Attached an updated patch to fix the tabs and to change this to a compile-time option. Now it's not possible to flexibly disable the feature during runtime, but I think that's fine. I'm taking a look at this patch as part of the current commitfest [*], (It's the first time I've formally reviewed a patch for a commitfest so please let me know if I'm missing something.) [*] https://commitfest.postgresql.org/action/patch_view?id=1221 Submission review - * Is the patch in a patch format which has context? Yes. * Does it apply cleanly to the current git master? Yes. No new files are introduced by this patch. * Does it include reasonable tests, necessary doc patches, etc? Yes. However the sample function provided in the documentation throws a runtime error due to a missing FROM-clause entry. Usability review * Does the patch actually implement that? Yes. * Do we want that? It seems like it would be useful; no opposition has been raised on -hackers so far. * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? SQL spec: n/a. I do note that it makes use of the # syntax before the body of a PL/pgSQL function, which is currently only used for #variable_conflict [*]. I can imagine this syntax might be used for other purposes down the road, so it might be worth keeping an eye on it before it becomes a hodge-podge of ad-hoc options. [*] http://www.postgresql.org/docs/current/static/plpgsql-implementation.html * Does it include pg_dump support (if applicable)? n/a * Are there dangers? I don't see any. * Have all the bases been covered? This lineis in exec_get_query_params() and exec_get_dynquery_params(): return (no parameters); Presumably the message will escape translation and this line should be better written as: return _((no parameters)); Also, if the offending query parameter contains a single quote, the output is not escaped: postgres=# select get_userid(E'foo'''); ERROR: query returned more than one row DETAIL: p1 = 'foo'' CONTEXT: PL/pgSQL function get_userid(text) line 9 at SQL statement Not sure if that's a real problem though. Feature test * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I can't see any. * Are there any assertion failures or crashes? No. Performance review -- * Does the patch slow down simple tests? No. * If it claims to improve performance, does it? n/a * Does it slow down other things? No. Coding review - * Does it follow the project coding guidelines? Yes. The functions added in pl_exec.c - exec_get_query_params() and exec_get_dynquery_params() do strike me as potentially misnamed, as they don't actually execute anything but are more utility functions for generating formatted output. Maybe format_query_params() etc. would be better? * Are there portability issues? I don't think so. * Will it work on Windows/BSD etc? Tested on OS X and Linux. I don't see anything, e.g. system calls, which might stop it working on Windows. * Are the comments sufficient and accurate? exec_get_query_params() and exec_get_dynquery_params() could do with some brief comments explaining their purpose. * Does it do what it says, correctly? As far as I can tell, yes. * Does it produce compiler warnings? No. * Can you make it crash? So far, no. Architecture review --- * Is everything done in a way that fits together coherently with other features/modules? Patch affects PL/pgSQL only. * Are there interdependencies that can cause problems? No. Regards Ian Barwick -- 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 for typo in src/bin/psql/command.c
Attached. Regards Ian Barwick psql-command-c-typo.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] plpgsql.print_strict_params
Hi Sorry for the delay on following up on this. 2013/9/18 Marko Tiikkaja ma...@joh.to: Hi, Attached is a patch with the following changes: On 16/09/2013 10:57, I wrote: On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote: However the sample function provided in the documentation throws a runtime error due to a missing FROM-clause entry. Ugh. I'll look into fixing that. Fixed. Confirmed :) This lineis in exec_get_query_params() and exec_get_dynquery_params(): return (no parameters); Presumably the message will escape translation and this line should be better written as: return _((no parameters)); Nice catch. Will look into this. Another option would be to simply omit the DETAIL line if there were no parameters. At this very moment I'm thinking that might be a bit nicer. Decided to remove the DETAIL line in these cases. Yes, on reflection I think that makes most sense. Also, if the offending query parameter contains a single quote, the output is not escaped: postgres=# select get_userid(E'foo'''); Error: query returned more than one row DETAIL: p1 = 'foo'' CONTEXT: PL/pgSQL function get_userid(text) line 9 at SQL statement Not sure if that's a real problem though. Hmm.. I should probably look at what happens when the parameters to a prepared statement are currently logged and imitate that. Yup, they're escaped. Did just that. Also copied the parameters: prefix on the DETAIL line from there. The output looks like this now: postgres=# select get_userid(E'foo'''); ERROR: query returned more than one row DETAIL: parameters: $1 = 'foo''' CONTEXT: PL/pgSQL function get_userid(text) line 6 at SQL statement which looks fine to me. The functions added in pl_exec.c - exec_get_query_params() and exec_get_dynquery_params() do strike me as potentially misnamed, as they don't actually execute anything but are more utility functions for generating formatted output. Maybe format_query_params() etc. would be better? Agreed, they could use some renaming. I went with format_expr_params and format_preparedparamsdata. Thanks. format_preparedparamsdata is a bit hard to scan, but that's just nitpicking on my part ;) * Are the comments sufficient and accurate? exec_get_query_params() and exec_get_dynquery_params() could do with some brief comments explaining their purpose. Agreed. Still agreeing with both of us, added a comment each. Thanks. I also added the new keywords to the unreserved_keywords production, as per the instructions near the beginning of pl_gram.y. Good catch. The patch looks good to me now; does the status need to be changed to Ready for Committer? Regards Ian Barwick -- 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: FORCE_NULL option for copy COPY in CSV mode
Hi, This patch implements the following TODO item: Allow COPY in CSV mode to control whether a quoted zero-length string is treated as NULL Currently this is always treated as a zero-length string, which generates an error when loading into an integer column Re: [PATCHES] allow CSV quote in NULL http://archives.postgresql.org/pgsql-hackers/2007-07/msg00905.php http://wiki.postgresql.org/wiki/Todo#COPY I had a very definite use-case for this functionality recently while importing CSV files generated by Oracle, and was somewhat frustrated by the existence of a FORCE_NOT_NULL option for specific columns, but not one for FORCE_NULL. I'll add this to the November commitfest. Regards Ian Barwick copy-force-null-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] review: psql and pset without any arguments
Hi 2013/9/30 Gilles Darold gilles.dar...@dalibo.com: (...) That's right, here is the patch modified with just a little change with your suggestion: if (popt-topt.numericLocale) printf(_(Locale-adjusted numeric output (%s) is on.\n), param); else printf(_(Locale-adjusted numeric output (%s) is off.\n), param); I'm a bit late to this thread, but I'd just like to say I find this a useful feature which I've missed on the odd occasion. BTW there is a slight typo in the patch, s should be is: Output format (format) s aligned. + printf(_(Output format (%s) s %s.\n), param, + _align2string(popt-topt.format)); Regards Ian Barwick -- 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: FORCE_NULL option for copy COPY in CSV mode
2013/10/9 Andrew Dunstan and...@dunslane.net: On 10/08/2013 12:47 PM, Andrew Dunstan wrote: On 10/08/2013 12:30 PM, Robert Haas wrote: Andrew, are you planning to review commit this? Yes. Incidentally, the patch doesn't seem to add the option to file_fdw, which I really think it should. Patch author here. I'll dig out the use-case I had for this patch and have a look at the file_fdw option, which never occurred to me. (I'm doing some FDW-related stuff at the moment so would be more than happy to handle that too). (Sorry for the delay in following up on this, I kind of assumed the patch would be squirrelled away until the next commitfest ;) ) Regards Ian Barwick -- 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] FDW: possible resjunk columns in AddForeignUpdateTargets
2013/11/6 Albe Laurenz laurenz.a...@wien.gv.at: I have a question concerning the Foreign Data Wrapper API: I find no mention of this in the documentation, but I remember that you can only add a resjunk column that matches an existing attribute of the foreign table and not one with an arbitrary name or definition. Ist that right? My understanding (having recently had a crack at getting an FDW working) is that the name can be arbitrary within reason - at least that's what I get from this bit of the documentation: To do that, add TargetEntry items to parsetree-targetList, containing expressions for the extra values to be fetched. Each such entry must be marked resjunk = true, and must have a distinct resname that will identify it at execution time. Avoid using names matching ctidN or wholerowN, as the core system can generate junk columns of these names. http://www.postgresql.org/docs/9.3/interactive/fdw-callbacks.html#FDW-CALLBACKS-UPDATE Someone more knowledgeable than myself will know better, I hope. Regards Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Review: pset autocomplete add missing options
Review for Pavel Stehule's patch in CF 2013-11:
https://commitfest.postgresql.org/action/patch_view?id=1253
Patch applies cleanly and works as intended; it's a very straightforward
patch so no surprises there.
The patch expands the range of completable items for \pset, putting
them in alphabetical order and syncs them with the list in command.c
introduced by Gilles Darold's earlier patch for \pset without any
options ( https://commitfest.postgresql.org/action/patch_view?id=1202 ).
However double-checking the options available to \pset, I see there
is also 'fieldsep_zero' and 'recordsep_zero', which are special cases
for 'fieldsep' and 'recordsep' respectively and which are therefore not
displayed separately by \pset without-any-options, but should nevertheless
be tab-completable. Modified patch attached to include these.
Regards
Ian Barwick
PS I will endeavour to review a more complex patch
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 021b6c5..24a5c69
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*** psql_completion(char *text, int start, i
*** 3306,3314
else if (strcmp(prev_wd, \\pset) == 0)
{
static const char *const my_list[] =
! {format, border, expanded,
! null, fieldsep, tuples_only, title, tableattr,
! linestyle, pager, recordsep, NULL};
COMPLETE_WITH_LIST_CS(my_list);
}
--- 3306,3315
else if (strcmp(prev_wd, \\pset) == 0)
{
static const char *const my_list[] =
! {border, columns, expanded, fieldsep, fieldsep_zero,
! footer, format, linestyle, null, numericlocale,
! pager, recordsep, recordsep_zero, tableattr, title,
! tuples_only, NULL};
COMPLETE_WITH_LIST_CS(my_list);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Review: pre-commit triggers
Review for Andrew Dunstan's patch in CF 2013-11: https://commitfest.postgresql.org/action/patch_view?id=1312 This review is more from a usage point of view, I would like to spend more time looking at the code but only so many hours in a day, etcetera; I hope this is useful as-is. Submission review - * Is the patch in a patch format which has context? Yes * Does it apply cleanly to the current git master? Yes. No new files are introduced by this patch. * Does it include reasonable tests, necessary doc patches, etc? Documentation patches included, but no tests. (I have a feeling it might be necessary to add a FAQ somewhere as to why there's no transaction rollback trigger). Usability review * Does the patch actually implement that? Yes. * Do we want that? There is an item in the todo-list Add database and transaction-level triggers; the linked thread: http://archives.postgresql.org/pgsql-hackers/2008-05/msg00620.php from 2008 doesn't seem too receptive to the idea, but this time round there don't seem to be any particular objections. Personally I don't have a use-case but it certainly looks like a useful compatibility feature when porting from other databases. Andrew mentions porting from Firebird; for reference this is the relevant Firebird documentation: http://www.firebirdsql.org/refdocs/langrefupd21-ddl-trigger.html * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? Not sure about the SQL spec. The Compatibility section of the CREATE TRIGGER doc page doesn't mention anything along these lines. http://www.postgresql.org/docs/9.3/static/sql-createtrigger.html#SQL-CREATETRIGGER-COMPATIBILITY * Does it include pg_dump support (if applicable)? Yes Feature test * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I'm not sure whether it's intended behaviour, but if the commit trigger itself fails, there's an implicit rollback, e.g.: postgres=# BEGIN ; BEGIN postgres=*# INSERT INTO foo (id) VALUES (1); INSERT 0 1 postgres=*# COMMIT ; NOTICE: Pre-commit trigger called ERROR: relation bar does not exist LINE 1: SELECT foo FROM bar ^ QUERY: SELECT foo FROM bar CONTEXT: PL/pgSQL function pre_commit_trigger() line 4 at EXECUTE statement postgres=# I'd expect this to lead to a failed transaction block, or at least some sort of notice that the transaction itself has been rolled back. Note that 'DROP FUNCTION broken_trigger_function() CASCADE' will remove the broken function without having to resort to single user mode so it doesn't seem like an error in the commit trigger itself will necessarily lead to an intractable situation. * Are there any assertion failures or crashes? No. Performance review -- * Does the patch slow down simple tests? No. * If it claims to improve performance, does it? n/a * Does it slow down other things? No. Coding review - * Does it follow the project coding guidelines? Yes. * Are there portability issues? I don't see any. * Will it work on Windows/BSD etc? Tested on OS X and Linux. I don't see anything, e.g. system calls, which might prevent it working on Windows. * Does it do what it says, correctly? As far as I can tell, yes. * Does it produce compiler warnings? No. * Can you make it crash? So far, no. Architecture review --- * Is everything done in a way that fits together coherently with other features/modules? The changes are not all that complex and I don't see any issues, however I'm not very familiar with that area of the code (but hey, that's why I'm taking a look) so I might be missing something. * Are there interdependencies that can cause problems? I can't see any. Additional notes A sample transaction commit trigger: CREATE OR REPLACE FUNCTION pre_commit_trigger() RETURNS EVENT_TRIGGER LANGUAGE 'plpgsql' AS $$ BEGIN RAISE NOTICE 'Pre-commit trigger called'; END; $$; CREATE EVENT TRIGGER trg_pre_commit ON transaction_commit EXECUTE PROCEDURE pre_commit_trigger(); Some relevant links: http://www.postgresql.org/docs/9.3/interactive/event-triggers.html http://www.postgresql.org/docs/9.3/interactive/sql-createeventtrigger.html http://www.postgresql.org/docs/9.3/interactive/sql-prepare-transaction.html http://en.wikipedia.org/wiki/Database_trigger Regards Ian Barwick -- 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: pre-commit triggers
2013/11/20 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick barw...@gmail.com wrote: I'd expect this to lead to a failed transaction block, or at least some sort of notice that the transaction itself has been rolled back. Ending up in a failed transaction block would be wrong. If the user does a BEGIN, a bunch of stuff, and a COMMIT, they're entitled to assume without checking that they are no longer in a transaction block. Absolutely. There are plenty of ways to fail at COMMIT already, eg deferred foreign key constraints. This shouldn't act any different. Ah OK, I see how that works. Thanks for the explanation. Ian Barwick -- 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] FDW: possible resjunk columns in AddForeignUpdateTargets
2013/11/8 Tom Lane t...@sss.pgh.pa.us: Albe Laurenz laurenz.a...@wien.gv.at writes: What I would like to do is add a custom resjunk column (e.g. a bytea) in AddForeignUpdateTargets that carries a row identifier from the scan state to the modify state. Would that be possible? Can I have anything else than a Var in a resjunk column? [ thinks for awhile... ] Hm. In principle you can put any expression you want into the tlist during AddForeignUpdateTargets. However, if it's not a Var then the planner won't understand that it's something that needs to be supplied by the table scan, so things won't work right in any but the most trivial cases (maybe not even then :-(). What I'd try is creating a Var that has the attno of ctid (ie, SelfItemPointerAttributeNumber) but the datatype you want, ie bytea. This won't match what the catalogs say your table's ctid is, but I think that nothing will care much about that. Apologies for reinvigorating this thread, but I'm running into a similar wall myself and would like to clarify if this approach will work at all. My foreign data source is returning a fixed-length string as a unique row identifier; in AddForeignUpdateTargets() I can create a Var like this: var = makeVar(parsetree-resultRelation, SelfItemPointerAttributeNumber, BPCHAROID, 32, InvalidOid, 0); but is it possible to store something other than a TIDOID here, and if so how? Regards Ian Barwick -- 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] FDW: possible resjunk columns in AddForeignUpdateTargets
2013/12/5 Albe Laurenz laurenz.a...@wien.gv.at: Ian Lawrence Barwick wrote: 2013/11/8 Tom Lane t...@sss.pgh.pa.us: [ thinks for awhile... ] Hm. In principle you can put any expression you want into the tlist during AddForeignUpdateTargets. However, if it's not a Var then the planner won't understand that it's something that needs to be supplied by the table scan, so things won't work right in any but the most trivial cases (maybe not even then :-(). What I'd try is creating a Var that has the attno of ctid (ie, SelfItemPointerAttributeNumber) but the datatype you want, ie bytea. This won't match what the catalogs say your table's ctid is, but I think that nothing will care much about that. Apologies for reinvigorating this thread, but I'm running into a similar wall myself and would like to clarify if this approach will work at all. My foreign data source is returning a fixed-length string as a unique row identifier; in AddForeignUpdateTargets() I can create a Var like this: var = makeVar(parsetree-resultRelation, SelfItemPointerAttributeNumber, BPCHAROID, 32, InvalidOid, 0); but is it possible to store something other than a TIDOID here, and if so how? Subsequent analysis showed that this won't work as you have no way to populate such a resjunk column. resjunk columns seem to get filled with the values from the column of the same name, so currently there is no way to invent your own column, fill it and pass it on. See thread 8b848b463a71b7a905bc5ef18b95528e.squir...@sq.gransy.com What I ended up doing is introduce a column option that identifies a primary key column. I add a resjunk entry for each of those and use them to identify the correct row during an UPDATE or DELETE. That only works for foreign data sources that have a concept of a primary key, but maybe you can do something similar. Thanks for confirming that, I suspected that might be the case. I'll have to go for Plan B (or C or D). Regards Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] comment typo in postgres_fdw/postgres_fdw.c
Presumably classifyConditions, not classifyClauses, is meant. Patch attached. Regards Ian Barwick diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 246a3a9..46ea032 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -723,7 +723,7 @@ postgresGetForeignPlan(PlannerInfo *root, /* * Separate the scan_clauses into those that can be executed remotely and * those that can't. baserestrictinfo clauses that were previously -* determined to be safe or unsafe by classifyClauses are shown in +* determined to be safe or unsafe by classifyConditions are shown in * fpinfo-remote_conds and fpinfo-local_conds. Anything else in the * scan_clauses list should be a join clause that was found safe by * postgresGetForeignPaths. -- 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: Patch FORCE_NULL option for copy COPY in CSV mode
2013-11-01 Payal Singh pa...@omniti.com: The post was made before I subscribed to the mailing list, so posting my review separately. The link to the original patch mail is http://www.postgresql.org/message-id/CAB8KJ=jS-Um4TGwenS5wLUfJK6K4rNOm_V6GRUj+tcKekL2=g...@mail.gmail.com Hi, This patch implements the following TODO item: Allow COPY in CSV mode to control whether a quoted zero-length string is treated as NULL Currently this is always treated as a zero-length string, which generates an error when loading into an integer column Re: [PATCHES] allow CSV quote in NULL http://archives.postgresql.org/pgsql-hackers/2007-07/msg00905.php http://wiki.postgresql.org/wiki/Todo#COPY I had a very definite use-case for this functionality recently while importing CSV files generated by Oracle, and was somewhat frustrated by the existence of a FORCE_NOT_NULL option for specific columns, but not one for FORCE_NULL. I'll add this to the November commitfest. Regards Ian Barwick == Contents Purpose == This patch introduces a new 'FORCE_NULL' option which has the opposite functionality of the already present 'FORCE_NOT_NULL' option for the COPY command. Prior to this option there was no way to convert a zeroed-length quoted value to a NULL value when COPY FROM is used to import data from CSV formatted files. == Submission Review == The patch is in the correct context diff format. It includes changes to the documentation as well as additional regression tests. The description has been discussed and defined in the previous mails leading to this patch. = Functionality Review = CORRECTION NEEDED - Due to a minor error in code (details in 'Code Review' section below), force_null option is not limited to COPY FROM, and works even when COPY TO is used. This should instead give an error message. The updated documentation describes the added functionality clearly. All regression tests passed successfully. Code compilation after including patch was successful. No warnings either. Manually tested COPY FROM with FORCE_NULL for a number of scenarios, all with expected outputs. No issues. Been testing the patch for a few days, no crashes or weird behavior observed. = Code Formatting Review (Needs Improvement) = Looks good, the tabs between variable declaration and accompanying comment can be improved. = Code Review (Needs Improvement) = 1. There is a NOTE: force_not_null option are not applied to the returned fields. before COPY FROM block. Similar note should be added for force_null option too. 2. One of the conditions to check and give an error if force_null is true and copy from is false is wrong, cstate-force_null should be checked instead of cstate-force_notnull: /* Check force_notnull */ if (!cstate-csv_mode cstate-force_notnull != NIL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg(COPY force not null available only in CSV mode))); if (cstate-force_notnull != NIL !is_from) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg(COPY force not null only available using COPY FROM))); /* Check force_null */ if (!cstate-csv_mode cstate-force_null != NIL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg(COPY force null available only in CSV mode))); == if (cstate-force_notnull != NIL !is_from) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg(COPY force null only available using COPY FROM))); === Suggested Changes Conclusion === The Above mentioned error condition should be corrected. Minor comments and tab changes are upto the author. In all, suggested modifications aside, the patch works well and in my opinion, would be a useful addition to the COPY command. Hi Payal Many thanks for the review, and my apologies for not getting back to you earlier. Updated version of the patch attached with suggested corrections. I'm not sure about the tabs in the variable declarations - the whole section seems to be all over the place (regardless of whether tabs are set to 4 or 8 spaces) and could do with tidying up, however I didn't want to expand the scope of the patch too much. Quick recap of the reasons behind this patch - we had a bunch of CSV files (and by bunch I mean
Re: [HACKERS] Review: Patch FORCE_NULL option for copy COPY in CSV mode
2014-01-29 Andrew Dunstan and...@dunslane.net: On 01/28/2014 05:55 AM, Ian Lawrence Barwick wrote: Hi Payal Many thanks for the review, and my apologies for not getting back to you earlier. Updated version of the patch attached with suggested corrections. On a very quick glance, I see that you have still not made adjustments to contrib/file_fdw to accommodate this new option. I don't see why this COPY option should be different in that respect. Hmm, that idea seems to have escaped me completely. I'll get onto it forthwith. Regards Ian Barwick -- 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: Patch FORCE_NULL option for copy COPY in CSV mode
2014/1/29 Ian Lawrence Barwick barw...@gmail.com:
2014-01-29 Andrew Dunstan and...@dunslane.net:
On 01/28/2014 05:55 AM, Ian Lawrence Barwick wrote:
Hi Payal
Many thanks for the review, and my apologies for not getting back to
you earlier.
Updated version of the patch attached with suggested corrections.
On a very quick glance, I see that you have still not made adjustments to
contrib/file_fdw to accommodate this new option. I don't see why this COPY
option should be different in that respect.
Hmm, that idea seems to have escaped me completely. I'll get onto it
forthwith.
Striking while the keyboard is hot... version with contrib/file_fdw
modifications
attached.
Regards
Ian Barwick
diff --git a/contrib/file_fdw/data/text.csv b/contrib/file_fdw/data/text.csv
new file mode 100644
index ed348a9..c7e243c
*** a/contrib/file_fdw/data/text.csv
--- b/contrib/file_fdw/data/text.csv
***
*** 1,4
! AAA,aaa
! XYZ,xyz
! NULL,NULL
! ABC,abc
--- 1,4
! AAA,aaa,123,
! XYZ,xyz,,321
! NULL,NULL,NULL,NULL
! ABC,abc,,
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
new file mode 100644
index 5639f4d..5877512
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
*** struct FileFdwOption
*** 48,56
/*
* Valid options for file_fdw.
! * These options are based on the options for COPY FROM command.
! * But note that force_not_null is handled as a boolean option attached to
! * each column, not as a table option.
*
* Note: If you are adding new option for user mapping, you need to modify
* fileGetOptions(), which currently doesn't bother to look at user mappings.
--- 48,56
/*
* Valid options for file_fdw.
! * These options are based on the options for the COPY FROM command.
! * But note that force_not_null and force_null are handled as boolean options
! * attached to a column, not as a table option.
*
* Note: If you are adding new option for user mapping, you need to modify
* fileGetOptions(), which currently doesn't bother to look at user mappings.
*** static const struct FileFdwOption valid_
*** 69,75
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
{force_not_null, AttributeRelationId},
!
/*
* force_quote is not supported by file_fdw because it's for COPY TO.
*/
--- 69,75
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
{force_not_null, AttributeRelationId},
! {force_null, AttributeRelationId},
/*
* force_quote is not supported by file_fdw because it's for COPY TO.
*/
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 187,192
--- 187,193
Oid catalog = PG_GETARG_OID(1);
char *filename = NULL;
DefElem*force_not_null = NULL;
+ DefElem*force_null = NULL;
List *other_options = NIL;
ListCell *cell;
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 243,252
}
/*
! * Separate out filename and force_not_null, since ProcessCopyOptions
! * won't accept them. (force_not_null only comes in a boolean
! * per-column flavor here.)
*/
if (strcmp(def-defname, filename) == 0)
{
if (filename)
--- 244,253
}
/*
! * Separate out filename and column-specific options, since
! * ProcessCopyOptions won't accept them.
*/
+
if (strcmp(def-defname, filename) == 0)
{
if (filename)
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 255,270
errmsg(conflicting or redundant options)));
filename = defGetString(def);
}
else if (strcmp(def-defname, force_not_null) == 0)
{
if (force_not_null)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
! errmsg(conflicting or redundant options)));
force_not_null = def;
/* Don't care what the value is, as long as it's a legal boolean */
(void) defGetBoolean(def);
}
else
other_options = lappend(other_options, def);
}
--- 256,297
errmsg(conflicting or redundant options)));
filename = defGetString(def);
}
+ /*
+ * force_not_null is a boolean option; after validation we can discard
+ * it - it will be retrieved later in get_file_fdw_attribute_options()
+ */
else if (strcmp(def-defname, force_not_null) == 0)
{
if (force_not_null)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
! errmsg(conflicting or redundant options),
! errhint(option \force_not_null\ supplied more than once for a column)));
! if(force_null)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg(conflicting or redundant options),
! errhint(option \force_not_null\ cannot be used together with \force_null\)));
force_not_null = def;
/* Don't care what the value is, as long as it's a legal boolean */
(void) defGetBoolean(def);
}
+ /* See comments for force_not_null above
Re: [HACKERS] Selecting large tables gets killed
2014-02-20 16:16 GMT+09:00 Ashutosh Bapat ashutosh.ba...@enterprisedb.com: Hi All, Here is a strange behaviour with master branch with head at (...) Looks like a bug in psql to me. Does anybody see that behaviour? It's not a bug, it's your VM's OS killing off a process which is using up too much memory. Check /var/log/messages to see what the kernel has to say about it. Regards Ian Barwick -- 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: Patch FORCE_NULL option for copy COPY in CSV mode
2014-03-02 8:26 GMT+09:00 Andrew Dunstan and...@dunslane.net:
On 01/29/2014 10:59 AM, Ian Lawrence Barwick wrote:
2014/1/29 Ian Lawrence Barwick barw...@gmail.com:
2014-01-29 Andrew Dunstan and...@dunslane.net:
On 01/28/2014 05:55 AM, Ian Lawrence Barwick wrote:
Hi Payal
Many thanks for the review, and my apologies for not getting back to
you earlier.
Updated version of the patch attached with suggested corrections.
On a very quick glance, I see that you have still not made adjustments
to
contrib/file_fdw to accommodate this new option. I don't see why this
COPY
option should be different in that respect.
Hmm, that idea seems to have escaped me completely. I'll get onto it
forthwith.
Striking while the keyboard is hot... version with contrib/file_fdw
modifications
attached.
I have reviewed this. Generally it's good, but the author has made a
significant error - the idea is not to force a quoted empty string to null,
but to force a quoted null string to null, whatever the null string might
be. The default case has these the same, but if you specify a non-empty null
string they aren't.
The author slaps himself on the forehead while regretting he was temporally
constricted when dealing with the patch and never thought to look beyond
the immediate use case.
Thanks for the update, much appreciated.
That difference actually made the file_fdw regression results plain wrong,
in my view, in that they expected a quoted empty string to be turned to null
even when the null string was something else.
I've adjusted this and the docs and propose to apply the attached patch in
the next day or two unless there are any objections.
Unless I'm overlooking something, output from SELECT * FROM text_csv;
in 'output/file_fdw.source' still needs updating?
Regards
Ian Barwick
diff --git a/contrib/file_fdw/data/text.csv b/contrib/file_fdw/data/text.csv
new file mode 100644
index ed348a9..f55d9cf
*** a/contrib/file_fdw/data/text.csv
--- b/contrib/file_fdw/data/text.csv
***
*** 1,4
! AAA,aaa
! XYZ,xyz
! NULL,NULL
! ABC,abc
--- 1,5
! AAA,aaa,123,
! XYZ,xyz,,321
! NULL,NULL,NULL,NULL
! NULL,NULL,NULL,NULL
! ABC,abc,,
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
new file mode 100644
index 5639f4d..7fb1dbc
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
*** struct FileFdwOption
*** 48,56
/*
* Valid options for file_fdw.
! * These options are based on the options for COPY FROM command.
! * But note that force_not_null is handled as a boolean option attached to
! * each column, not as a table option.
*
* Note: If you are adding new option for user mapping, you need to modify
* fileGetOptions(), which currently doesn't bother to look at user mappings.
--- 48,56
/*
* Valid options for file_fdw.
! * These options are based on the options for the COPY FROM command.
! * But note that force_not_null and force_null are handled as boolean options
! * attached to a column, not as table options.
*
* Note: If you are adding new option for user mapping, you need to modify
* fileGetOptions(), which currently doesn't bother to look at user mappings.
*** static const struct FileFdwOption valid_
*** 69,75
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
{force_not_null, AttributeRelationId},
!
/*
* force_quote is not supported by file_fdw because it's for COPY TO.
*/
--- 69,75
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
{force_not_null, AttributeRelationId},
! {force_null, AttributeRelationId},
/*
* force_quote is not supported by file_fdw because it's for COPY TO.
*/
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 187,192
--- 187,193
Oid catalog = PG_GETARG_OID(1);
char *filename = NULL;
DefElem*force_not_null = NULL;
+ DefElem*force_null = NULL;
List *other_options = NIL;
ListCell *cell;
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 243,252
}
/*
! * Separate out filename and force_not_null, since ProcessCopyOptions
! * won't accept them. (force_not_null only comes in a boolean
! * per-column flavor here.)
*/
if (strcmp(def-defname, filename) == 0)
{
if (filename)
--- 244,253
}
/*
! * Separate out filename and column-specific options, since
! * ProcessCopyOptions won't accept them.
*/
+
if (strcmp(def-defname, filename) == 0)
{
if (filename)
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 255,270
errmsg(conflicting or redundant options)));
filename = defGetString(def);
}
else if (strcmp(def-defname, force_not_null) == 0)
{
if (force_not_null)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
! errmsg(conflicting or redundant options)));
force_not_null = def
Re: [HACKERS] Review: Patch FORCE_NULL option for copy COPY in CSV mode
2014-03-05 23:27 GMT+09:00 Andrew Dunstan and...@dunslane.net: On 03/05/2014 09:11 AM, Michael Paquier wrote: After testing this feature, I noticed that FORCE_NULL and FORCE_NOT_NULL can both be specified with COPY on the same column. This does not seem correct. The attached patch adds some more error handling, and a regression test case for that. Strictly they are not actually contradictory, since FORCE NULL relates to quoted null strings and FORCE NOT NULL relates to unquoted null strings. Arguably the docs are slightly loose on this point. Still, applying both FORCE NULL and FORCE NOT NULL to the same column would be rather perverse, since it would result in a quoted null string becoming null and an unquoted null string becoming not null. Too frazzled to recall clearly right now, but I think that was the somewhat counterintuitive conclusion I originally came to. I'd be more inclined just to tighten the docs and maybe expand the regression tests a bit, but I could be persuaded the other way if people think it's worth it. Might be worth doing if it's an essentially useless feature, if only to preempt an unending chain of bug reports. Many thanks for everyone's input on this, and apologies for not giving the patch enough rigorous attention. Regards Ian Barwick -- 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] First draft of update announcement
2014-03-17 13:24 GMT+09:00 Josh Berkus j...@agliodbs.com: ... attached. Please correct! A couple of drive-by corrections: each of their standy databases standy - standby Prevent erroneous operator push-down in pgsql_fdw pgsql_fdw - postgres_fdw Regards Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
