Re: [HACKERS] Backup throttling
On Aug 19, 2013, at 9:11 PM, Andres Freund wrote: On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote: 2013-08-19 19:20 keltezéssel, Andres Freund írta: Hi, On 2013-07-24 09:20:52 +0200, Antonin Houska wrote: Hello, the purpose of this patch is to limit impact of pg_backup on running server. Feedback is appreciated. Based on a quick look it seems like you're throttling on the receiving side. Is that a good idea? Especially over longer latency links, TCP buffering will reduce the effect on the sender side considerably. Throttling on the sender side requires extending the syntax of BASE_BACKUP and maybe START_REPLICATION so both can be throttled but throttling is still initiated by the receiver side. Seems fine to me. Under the premise that the idea is decided to be worthwile to be integrated. Which I am not yet convinced of. i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple: just assume a weak server - maybe just one disk or two - and a slave. master and slave are connected via a 1 GB network. pg_basebackup will fetch data full speed basically putting those lonely disks out of business. we actually had a case where a client asked if PostgreSQL is locked during base backup. of course it was just disk wait caused by a full speed pg_basebackup. regarding the client side implementation: we have chosen this way because it is less invasive. i cannot see a reason to do this on the server side because we won't have 10 pg_basebackup-style tools making use of this feature anyway. of course, if you got 20 disk and a 1 gbit network this is useless - but many people don't have that. regards, hans-jürgen schönig -- Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de -- 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] Backup throttling
On 2013-08-21 08:10:42 +0200, PostgreSQL - Hans-Jürgen Schönig wrote: On Aug 19, 2013, at 9:11 PM, Andres Freund wrote: On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote: 2013-08-19 19:20 keltezéssel, Andres Freund írta: Hi, On 2013-07-24 09:20:52 +0200, Antonin Houska wrote: Hello, the purpose of this patch is to limit impact of pg_backup on running server. Feedback is appreciated. Based on a quick look it seems like you're throttling on the receiving side. Is that a good idea? Especially over longer latency links, TCP buffering will reduce the effect on the sender side considerably. Throttling on the sender side requires extending the syntax of BASE_BACKUP and maybe START_REPLICATION so both can be throttled but throttling is still initiated by the receiver side. Seems fine to me. Under the premise that the idea is decided to be worthwile to be integrated. Which I am not yet convinced of. i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple: just assume a weak server - maybe just one disk or two - and a slave. master and slave are connected via a 1 GB network. pg_basebackup will fetch data full speed basically putting those lonely disks out of business. we actually had a case where a client asked if PostgreSQL is locked during base backup. of course it was just disk wait caused by a full speed pg_basebackup. regarding the client side implementation: we have chosen this way because it is less invasive. i cannot see a reason to do this on the server side because we won't have 10 pg_basebackup-style tools making use of this feature anyway. The problem is that receiver side throttling over TCP doesn't always work all that nicely unless you have a low rate of transfer and/or very low latency . Quite often you will have OS buffers/the TCP Window being filled in bursts where the sender sends at max capacity and then a period where nothing happens on the sender. That's often not what you want when you need to throttle. Besides, I can see some value in e.g. normal streaming replication also being rate limited... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] BUG #8335: trim() un-document behaviour
On 08/14/2013 11:27 PM, Bruce Momjian wrote: On Mon, Aug 12, 2013 at 11:31:38PM -0400, Bruce Momjian wrote: On Mon, Aug 12, 2013 at 05:19:30PM -0400, Bruce Momjian wrote: Attached are docs that add the new syntax, and mention it is non-standard; you can see the output here: http://momjian.us/tmp/pgsql/functions-string.html#FUNCTIONS-STRING-SQL We do document three syntaxes for substring() in the same table, one row for each, so there is precedent for doing this. Attached is an updated patch with a proper example. I could move the extra syntax into the description of the existing trim entry instead. Patch applied to head. I did not apply this to 9.3 in case we change our minds about documenting this. This commit introduced the following: doc$ make -s html Processing HTML.index... 2409 entries loaded... collateindex.pl: duplicated index entry found: TRIM 1 entries ignored... Done. -- Vik -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] R: [pgsql-zh-general] (solved - 谢谢) Chinese in Postgres
Eureka. The problem is solved. Bambo: I guess you forget to convert you string to UTF-8 before insert. No, too many conversions :) The fact is that in the source code the query was casted to UnicodeString, because of previous requirements on the project: //tmp is a char* and contains the query uniQueryStr = UnicodeString(tmp); //useless executeMyQuery(uniQueryStr); //this is wrong!! executeMyQuery(tmp); //that's right :). TODO never use UnicodeString anymore... I already tried to avoid that cast, but when the target was compiled on the Linux server (that's why i used putty) the executable was not overwritten, for some weird reason. So the executable was not 100% according to the source code; I was always used to compile code with Visual studio or Eclipse so i wasn't aware of such possibility. To make it working, I just removed all the old files and made a long, full and fresh build. Sorry for making you loose time. XieXie / 谢谢, Francesco Invita i tuoi amici e Tiscali ti premia! Il consiglio di un amico vale più di uno spot in TV. Per ogni nuovo abbonato 30 € di premio per te e per lui! Un amico al mese e parli e navighi sempre gratis: http://freelosophy.tiscali.it/ -- 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] Back-patch change in hashed DISTINCT estimation?
On 2013-08-20 17:24:18 -0400, Tom Lane wrote: In a thread over in pgsql-performance, Tomas Vondra pointed out that choose_hashed_distinct was sometimes making different choices than choose_hashed_grouping, so that queries like these: select distinct x from ... ; select x from ... group by 1; might get different plans even though they should be equivalent. After some debugging it turns out that I omitted hash_agg_entry_size() from the hash table size estimate in choose_hashed_distinct --- I'm pretty sure I momentarily thought that this function must yield zero if there are no aggregates, but that's wrong. So we need a patch like this: What I'm wondering is whether to back-patch this or leave well enough alone. The risk of back-patching is that it might destabilize plan choices that people like. [...] A possible compromise is to back-patch into 9.3 (where the argument about destabilizing plan choices doesn't have much force yet), but not further. +1 for 9.3 only. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] BUG #8335: trim() un-document behaviour
On Wed, Aug 21, 2013 at 12:32:20PM +0200, Vik Fearing wrote: On 08/14/2013 11:27 PM, Bruce Momjian wrote: On Mon, Aug 12, 2013 at 11:31:38PM -0400, Bruce Momjian wrote: On Mon, Aug 12, 2013 at 05:19:30PM -0400, Bruce Momjian wrote: Attached are docs that add the new syntax, and mention it is non-standard; you can see the output here: http://momjian.us/tmp/pgsql/functions-string.html#FUNCTIONS-STRING-SQL We do document three syntaxes for substring() in the same table, one row for each, so there is precedent for doing this. Attached is an updated patch with a proper example. I could move the extra syntax into the description of the existing trim entry instead. Patch applied to head. I did not apply this to 9.3 in case we change our minds about documenting this. This commit introduced the following: doc$ make -s html Processing HTML.index... 2409 entries loaded... collateindex.pl: duplicated index entry found: TRIM 1 entries ignored... Done. Interesting. I didn't realize HTML shows errors that 'make check' doesn't. Anyway, I have removed the second index reference, so the error should be gone now. Thanks for the report. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Bison 3.0 updates
On 2013-07-29 08:02:49 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-07-29 07:11:13 -0400, Stephen Frost wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: The bottom line was: It looks like our choices are (1) teach configure to enable -fno-aggressive-loop-optimizations if the compiler recognizes it, or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9. I am in favor of fixing the back branches via (1), because it's less work and much less likely to break third-party extensions. Some other people argued for (2), but I've not seen any patch emerge from them, and you can bet I'm not going to do it. Yea, just passing -fno-aggressive-loop-optimizations seems like the safest and best option to me also.. I think we need to do both. There very well might be other optimizations made based on the unreachability information. If we turn off the optimization, that will fix any other cases as well, no? So why would we risk breaking third-party code by back-porting the struct declaration changes? This seems to be the agreed upon course of action, so I've prepared a patch including a preliminary commit message. I confirmed that it fixes the issue with gcc 4.8 and 9.1 for me. The patch needs to be applied to 9.1, 9.0 and 8.4. Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From 1d9f13fd9245c66de02478875c9f6c3eea3bc978 Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Wed, 21 Aug 2013 13:10:23 +0200 Subject: [PATCH] Amend CFLAGS for gcc 4.8 to prevent optimization problems due to variable length structs gcc 4.8 concludes it can terminate some loops prematurely, causing e.g. initdb to fail in optimized builds, because we embed variable length structs inside catalog structs. If those embedded variable length structs are not the trailing field gcc concludes that they an area of memory has to be of a certain size and uses that information to infer that some loops can only iterate a limited number of times. In reality, the actual struct layout isn't used for any of the elements beyond the first variable length element, it's just there for the benefit genbki.pl. Later fields are only accessed indirectly via heap_getattr() and friends. But the compiler doesn't know that. Commits 8137f2c3 / 8a3f745f, which are only included in 9.2 and onwards, thus hide all variable length fields after the first one via #ifdefs. For the benefit of earlier releases, let configure check for -fno-aggressive-loop-optimizations, which disables this kind of optimization. It was concluded that this way of fixing problems arising from the optimization is less likely to cause problems than backporting the aforementioned commits. Per discussion on the hackers mailinglist. --- configure.in | 3 +++ 1 file changed, 3 insertions(+) diff --git a/configure.in b/configure.in index cf1afeb..76cd5eb 100644 --- a/configure.in +++ b/configure.in @@ -438,6 +438,9 @@ if test $GCC = yes -a $ICC = no; then PGAC_PROG_CC_CFLAGS_OPT([-fwrapv]) # Disable FP optimizations that cause various errors on gcc 4.5+ or maybe 4.6+ PGAC_PROG_CC_CFLAGS_OPT([-fexcess-precision=standard]) + # Disable some loop optimizations, causes error due to variable length structs + # needed for 9.2 and gcc 4.8+ + PGAC_PROG_CC_CFLAGS_OPT([-fno-aggressive-loop-optimizations]) elif test $ICC = yes; then # Intel's compiler has a bug/misoptimization in checking for # division by NAN (NaN == 0), -mp1 fixes it, so add it to the CFLAGS. -- 1.8.2.rc2.4.g7799588.dirty -- 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] CAST Within EXCLUSION constraint
On Tue, Aug 20, 2013 at 8:53 PM, David E. Wheeler da...@justatheory.comwrote: On Aug 20, 2013, at 6:50 PM, Tom Lane t...@sss.pgh.pa.us wrote: You need more parentheses -- (source::text) would've worked. Alas, no, same problem as for CAST(): ERROR: functions in index expression must be marked IMMUTABLE No problem, I can use CAST(), right? So I try: EXCLUDE USING gist (CAST(source AS text) WITH =, within WITH ) Not so much: try.sql:13: ERROR: functions in index expression must be marked IMMUTABLE I guess it's because locale settings might change, and therefore change the text representation? Seems unlikely, though. Not locale, just renaming one of the values would be enough to break that. Admittedly we don't provide an official way to do that ATM, but you can do an UPDATE on pg_enum. Ah, right. Maybe if there was a way to get at some immutable numeric value… It seems reasonable to me to cast enum to oid. However, creating casts without function isn't allowed for enums. test=# create cast (source as oid) without function; ERROR: enum data types are not binary-compatible However, this restriction can be avoided either by writing dummy C-function or touching catalog directly: test=# insert into pg_cast values ((select oid from pg_type where typname = 'source'), (select oid from pg_type where typname = 'oid'), 0, 'e', 'b'); INSERT 341001 1 Then you can define desired restriction. CREATE TABLE things ( source source NOT NULL, within tstzrange NOT NULL, EXCLUDE USING gist ((source::oid) WITH =, within WITH ) ); Probably, I'm missing something and casting enum to oid is somehow unsafe? -- With best regards, Alexander Korotkov.
[HACKERS] PL/pgSQL, RAISE and error context
Hi, By default, PL/pgSQL does not print the error context of a RAISE statement, for example: =# create function foof() returns void as $$ begin raise exception 'foo'; end $$ language plpgsql; CREATE FUNCTION =# create function bar() returns void as $$ begin perform foof(); end $$ language plpgsql; CREATE FUNCTION =# select bar(); ERROR: foo CONTEXT: SQL statement SELECT foof() PL/pgSQL function bar line 1 at PERFORM I find this extremely surprising, since if you raise the same exception (or a DEBUG/NOTICE message) in multiple places, the error context is missing valuable information. With a trivial change the last error could be: =# select bar(); ERROR: foo CONTEXT: PL/pgSQL function foof line 1 RAISE SQL statement SELECT foof() PL/pgSQL function bar line 1 at PERFORM which I find a lot better. Thoughts? Regards, Marko Tiikkaja -- 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] Back-patch change in hashed DISTINCT estimation?
* Andres Freund (and...@2ndquadrant.com) wrote: On 2013-08-20 17:24:18 -0400, Tom Lane wrote: In a thread over in pgsql-performance, Tomas Vondra pointed out that choose_hashed_distinct was sometimes making different choices than choose_hashed_grouping, so that queries like these: select distinct x from ... ; select x from ... group by 1; might get different plans even though they should be equivalent. After some debugging it turns out that I omitted hash_agg_entry_size() from the hash table size estimate in choose_hashed_distinct --- I'm pretty sure I momentarily thought that this function must yield zero if there are no aggregates, but that's wrong. So we need a patch like this: What I'm wondering is whether to back-patch this or leave well enough alone. The risk of back-patching is that it might destabilize plan choices that people like. [...] A possible compromise is to back-patch into 9.3 (where the argument about destabilizing plan choices doesn't have much force yet), but not further. +1 for 9.3 only. Yeah, I've been thinking about this a bit also and agree that 9.3 is fine but not farther back. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] PL/pgSQL, RAISE and error context
2013/8/21 Marko Tiikkaja ma...@joh.to Hi, By default, PL/pgSQL does not print the error context of a RAISE statement, for example: =# create function foof() returns void as $$ begin raise exception 'foo'; end $$ language plpgsql; CREATE FUNCTION =# create function bar() returns void as $$ begin perform foof(); end $$ language plpgsql; CREATE FUNCTION =# select bar(); ERROR: foo CONTEXT: SQL statement SELECT foof() PL/pgSQL function bar line 1 at PERFORM I find this extremely surprising, since if you raise the same exception (or a DEBUG/NOTICE message) in multiple places, the error context is missing valuable information. With a trivial change the last error could be: =# select bar(); ERROR: foo CONTEXT: PL/pgSQL function foof line 1 RAISE SQL statement SELECT foof() PL/pgSQL function bar line 1 at PERFORM which I find a lot better. +1 Pavel Thoughts? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/**mailpref/pgsql-hackershttp://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/pgSQL, RAISE and error context
On 8/21/13 2:28 PM, I wrote: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: An even worse example: =# create function foof() returns void as $$ begin raise exception 'foo'; end $$ language plpgsql; CREATE FUNCTION =# create function barf() returns void as $$ declare _ record; begin for _ in execute 'select foof()' loop end loop; end $$ language plpgsql; CREATE FUNCTION =# select barf(); ERROR: foo CONTEXT: PL/pgSQL function barf line 1 at FOR over EXECUTE statement Notice how there's no mention at all about the function the error came from, and compare that to: =# select barf(); ERROR: foo CONTEXT: PL/pgSQL function foof line 1 RAISE PL/pgSQL function barf line 1 at FOR over EXECUTE statement Regards, Marko Tiikkaja -- 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] PL/pgSQL, RAISE and error context
Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CAST Within EXCLUSION constraint
Alexander Korotkov aekorot...@gmail.com writes: It seems reasonable to me to cast enum to oid. However, creating casts without function isn't allowed for enums. test=# create cast (source as oid) without function; ERROR: enum data types are not binary-compatible The reason for that is you'd get randomly different results on another installation. In this particular application, I think David doesn't really care about what values he gets as long as they're distinct, so this might be an OK workaround for him. But that's the reasoning for the general prohibition. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/pgSQL, RAISE and error context
On 8/21/13 4:22 PM, Tom Lane wrote: Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. They have an option: they can reduce verbosity in their client. I currently don't have any real options. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [9.3 doc fix] clarification of Solaris versions
Hello, One of my colleagues, who is relatively new to PostgreSQL, asked me if PostgreSQL supports Solaris 11. The reason why he had this question is that the following page says Solaris 10 instead of Solaris 10 and later. http://www.postgresql.org/docs/devel/static/kernel-resources.html So, I suggest this tiny modification to avoid misunderstanding. In addition, I suggest removing references to OpenSolaris because OpenSolaris is already discontinued. I'm attaching one patch file. Could you commit this change? Regards MauMau doc_solaris_version.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] Back-patch change in hashed DISTINCT estimation?
Stephen Frost sfr...@snowman.net wrote: Yeah, I've been thinking about this a bit also and agree that 9.3 is fine but not farther back. +1 to 9.3 but no farther back. I would be in favor of going farther back if there were not fairly obvious workarounds for the OOM problems that lack of back-patch could cause. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Martijn, * Martijn van Oosterhout (klep...@svana.org) wrote: ISTM you want some kind of hybrid setting like: #include_system auto.conf which simultaneously does three things: 1. Sets the enable_alter_system flag 2. Indicates the file to use 3. Indicates the priority of the setting re other settings. Comment it out, ALTER SYSTEM stop working. Put it back and it's immediately clear what it means. And the user can control where the settings go. Yeah, that's certainly an interesting idea. I might call it 'auto_conf_file auto.conf' to avoid the '#include' concern and to perhaps clarify that it's more than just a regular 'include'. Syntax is a bit fugly though. Agreed. Thanks, Stephen (who is still unhappy about the GUC-specific handling for relative paths in postgresql.conf) signature.asc Description: Digital signature
Re: [HACKERS] PL/pgSQL, RAISE and error context
On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. It *is* (apologies for the hijack) too verbose but whatever context suppressing we added doesn't work in pretty much any interesting case. What is basically needed is for the console to honor log_error_verbosity (which I would prefer) or a separate GUC in manage the console logging verbosity: set log_error_verbosity = 'terse'; SET CREATE OR REPLACE FUNCTION Notice(_msg TEXT) RETURNS VOID AS $$ BEGIN RAISE NOTICE '[%] %', clock_timestamp()::timestamp(0)::text, _msg; END; $$ LANGUAGE PLPGSQL; CREATE OR REPLACE FUNCTION foo() RETURNS VOID AS $$ BEGIN PERFORM Notice('test'); END; $$ LANGUAGE PLPGSQL; -- context will print postgres=# select foo(); NOTICE: [2013-08-21 09:52:08] test CONTEXT: SQL statement SELECT Notice('test') PL/pgSQL function foo() line 4 at PERFORM CREATE OR REPLACE FUNCTION bar() RETURNS VOID AS $$ SELECT Notice('test'); $$ LANGUAGE SQL; -- context will not print postgres=# select bar(); NOTICE: [2013-08-21 09:54:55] test -- context will print CREATE OR REPLACE FUNCTION baz() RETURNS VOID AS $$ select 0; SELECT Notice('test'); $$ LANGUAGE SQL; postgres=# select baz(); NOTICE: [2013-08-21 09:55:26] test CONTEXT: SQL function baz statement 2 merlin -- 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] PL/pgSQL, RAISE and error context
On 8/21/13 5:05 PM, Merlin Moncure wrote: On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. It *is* (apologies for the hijack) too verbose but whatever context suppressing we added doesn't work in pretty much any interesting case. What is basically needed is for the console to honor log_error_verbosity (which I would prefer) or a separate GUC in manage the console logging verbosity: Why does \set VERBOSITY 'terse' not work for you? Regards, Marko Tiikkaja -- 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] PL/pgSQL, RAISE and error context
On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote: On 8/21/13 5:05 PM, Merlin Moncure wrote: On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. It *is* (apologies for the hijack) too verbose but whatever context suppressing we added doesn't work in pretty much any interesting case. What is basically needed is for the console to honor log_error_verbosity (which I would prefer) or a separate GUC in manage the console logging verbosity: Why does \set VERBOSITY 'terse' not work for you? Because it can't be controlled mid-function...that would suppress all context of errors as well as messages and so it's useless. Also psql directives for this purpose is a hack anyways -- what if I'm using a non-psql client? what I really want is: SET LOCAL log_console_verbosity = 'x' merlin -- 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] CAST Within EXCLUSION constraint
On Aug 21, 2013, at 4:13 PM, Tom Lane t...@sss.pgh.pa.us wrote: test=# create cast (source as oid) without function; ERROR: enum data types are not binary-compatible The reason for that is you'd get randomly different results on another installation. In this particular application, I think David doesn't really care about what values he gets as long as they're distinct, so this might be an OK workaround for him. But that's the reasoning for the general prohibition. I’m okay with my function that casts to text, at least for now. An integer would be nicer, likely smaller for my index, but not a big deal. David -- 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] PL/pgSQL, RAISE and error context
2013/8/21 Merlin Moncure mmonc...@gmail.com On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote: On 8/21/13 5:05 PM, Merlin Moncure wrote: On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja ma...@joh.to writes: By default, PL/pgSQL does not print the error context of a RAISE statement, for example: It used to do so, in the beginning when we first added context-printing. There were complaints that the result was too verbose; for instance if you had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd get two lines for every one you wanted. I think if we undid this we'd get the same complaints again. I agree that in complicated nests of functions the location info is more interesting than it is in trivial cases, but that doesn't mean you're not going to hear such complaints from people with trivial functions. It *is* (apologies for the hijack) too verbose but whatever context suppressing we added doesn't work in pretty much any interesting case. What is basically needed is for the console to honor log_error_verbosity (which I would prefer) or a separate GUC in manage the console logging verbosity: Why does \set VERBOSITY 'terse' not work for you? Because it can't be controlled mid-function...that would suppress all context of errors as well as messages and so it's useless. Also psql directives for this purpose is a hack anyways -- what if I'm using a non-psql client? what I really want is: SET LOCAL log_console_verbosity = 'x' it is not bad idea Pavel merlin -- 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] PL/pgSQL, RAISE and error context
On 2013-08-21 17:18, Merlin Moncure wrote: On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote: Why does \set VERBOSITY 'terse' not work for you? Because it can't be controlled mid-function...that would suppress all context of errors as well as messages and so it's useless. Fair enough. what I really want is: SET LOCAL log_console_verbosity = 'x' log_min_messages vs. client_min_messages, so client_error_verbosity sounds more appropriate IMO. Regards, Marko Tiikkaja -- 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] PL/pgSQL, RAISE and error context
Merlin Moncure mmonc...@gmail.com writes: On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote: Why does \set VERBOSITY 'terse' not work for you? Because it can't be controlled mid-function...that would suppress all context of errors as well as messages and so it's useless. Also psql directives for this purpose is a hack anyways -- what if I'm using a non-psql client? what I really want is: SET LOCAL log_console_verbosity = 'x' There was a protocol design decision a long time ago that verbosity ought to be controlled on the client side. If we start suppressing fields server-side I think we're going to have problems. In particular, I'm going to throw the what if I'm not using psql argument right back at you: what's the reason for thinking that a different client/application would have the identical desires about what fields to see? It seems unlikely that a Java application, say, would want the server to be selective about what information it sends. I'm entirely prepared to believe that psql's VERBOSITY behavior could use more options, though. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
On Wed, Aug 21, 2013 at 4:05 AM, Andres Freund and...@2ndquadrant.com wrote: On 2013-08-20 17:24:18 -0400, Tom Lane wrote: In a thread over in pgsql-performance, Tomas Vondra pointed out that choose_hashed_distinct was sometimes making different choices than choose_hashed_grouping, so that queries like these: select distinct x from ... ; select x from ... group by 1; might get different plans even though they should be equivalent. After some debugging it turns out that I omitted hash_agg_entry_size() from the hash table size estimate in choose_hashed_distinct --- I'm pretty sure I momentarily thought that this function must yield zero if there are no aggregates, but that's wrong. So we need a patch like this: What I'm wondering is whether to back-patch this or leave well enough alone. The risk of back-patching is that it might destabilize plan choices that people like. [...] A possible compromise is to back-patch into 9.3 (where the argument about destabilizing plan choices doesn't have much force yet), but not further. +1 for 9.3 only. I agree. work_mem is hard to tune with any great precision analytically. If it is carefully tuned, it was probably done empirically, so changing the behavior in back branches seems bad. Cheers, Jeff -- 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] GSOC13 proposal - extend RETURNING syntax
Hi, 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta: W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze: Here's a new one, for v7: setrefs.c: In function ‘set_plan_refs’: setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function [-Wmaybe-uninitialized] bind_returning_variables(rlist, before_index, after_index); ^ setrefs.c:1957:21: note: ‘before_index’ was declared here int after_index=0, before_index; ^ Right, my mistake. Sorry and thanks. Fixed. Regards, Karol Trzcionka With this fixed, a more complete review: * Is the patch in a patch format which has context? (eg: context diff format) Yes. * Does it apply cleanly to the current git master? Yes. * Does it include reasonable tests, necessary doc patches, etc? There is a new regression test (returning_before_after.sql) covering this feature. However, I think it should be added to the group where returning.sql resides currently. There is a value in running it in parallel to other tests. Sometimes a subtle bug is uncovered because of this and your v2 patch fixed such a bug already. doc/src/sgml/ref/update.sgml describes this feature. doc/src/sgml/dml.sgml should also be touched to cover this feature. * Does the patch actually implement what it's supposed to do? Yes. * Do we want that? Yes. * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? RETURNING is a PostgreSQL extension, so the SQL-spec part of the question isn't applicable. It implements the community-agreed behaviour, according to the new regression test coverage. * Does it include pg_dump support (if applicable)? n/a * Are there dangers? I don't think so. * Have all the bases been covered? It seems so. I have also tried mixing before/after columns in different orders and the query didn't fail: zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text); CREATE TABLE zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c'); INSERT 0 1 zozo=# select * from t1; id | i1 | i2 | t1 | t2 ++++ 1 | 1 | 1 | a | a 2 | 2 | 2 | b | b 3 | 3 | 3 | c | c (3 rows) zozo=# begin; BEGIN zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2; i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2 +++++++- 2 | 2 | 2 | 4 | b | b | b | bx2 (1 row) UPDATE 1 zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2 ++++++-+- 3 | 3 | 9 | 6 | c | c | cx3 | cx2 (1 row) UPDATE 1 * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I don't know. * Are there any assertion failures or crashes? No. * 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. * Does it follow the project coding guidelines? Mostly. In the src/test/regress/parallel_schedule contains an extra new line at the end, it shouldn't. In b/src/backend/optimizer/plan/setrefs.c: +static void bind_returning_variables(List *rlist, int bef, int aft); but later it becomes not public: + */ +void bind_returning_variables(List *rlist, int bef, int aft) +{ Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward declaration is not needed, the function is called only from later functions. Similar for parse_clause.c: +extern void addAliases(ParseState *pstate); +void addAliases(ParseState *pstate) This external declaration is not needed since it is already in src/include/parser/parse_clause.h In setrefs.c, bind_returning_variables() I would also rename the function arguments, so before and after are spelled out. These are not C keywords. Some assignments, like: + var=(Var*)tle; and + int index_rel=1; in setrefs.c need spaces. if() statements need a space before the ( and not after. Add spaces in the {} list in addAliases(): + char*aliases[] = {before,after}; like this: { before, after } Spaces are needed here, too: + for(i=0 ; inoal; i++) This noal should be naliases or n_aliases if you really want a variable. I would simply use the constant 2 for the two for() loops in addAliases() instead, its purpose is obvious enough. In setrefs.c, bind_returning_variables(): + Var *var = NULL; + foreach(temp, rlist){ Add an empty line after the declaration block. * Are there portability issues? No. * Will it work on Windows/BSD etc? Yes. * Are the comments sufficient
Re: [HACKERS] PL/pgSQL, RAISE and error context
On Wed, Aug 21, 2013 at 10:47 AM, Tom Lane t...@sss.pgh.pa.us wrote: Merlin Moncure mmonc...@gmail.com writes: On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja ma...@joh.to wrote: Why does \set VERBOSITY 'terse' not work for you? Because it can't be controlled mid-function...that would suppress all context of errors as well as messages and so it's useless. Also psql directives for this purpose is a hack anyways -- what if I'm using a non-psql client? what I really want is: SET LOCAL log_console_verbosity = 'x' There was a protocol design decision a long time ago that verbosity ought to be controlled on the client side. If we start suppressing fields server-side I think we're going to have problems. In particular, I'm going to throw the what if I'm not using psql argument right back at you: what's the reason for thinking that a different client/application would have the identical desires about what fields to see? It seems unlikely that a Java application, say, would want the server to be selective about what information it sends. I didn't like that decision then and I don't like it now. Why should the protocol mandate that error context always be sent? Why does this have anything to do with the protocol at all? Just because we can't imagine a case where a java application would not want context to be sent in some or all contexts doesn't mean that operators should not have control over it being emitted. What harm could providing an option possibly cause? I'm entirely prepared to believe that psql's VERBOSITY behavior could use more options, though. How would that be structured... \set VERBOSITY 'NOTICE:terse'? merlin -- 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] GSOC13 proposal - extend RETURNING syntax
2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta: Hi, 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta: W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze: Here's a new one, for v7: setrefs.c: In function ‘set_plan_refs’: setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function [-Wmaybe-uninitialized] bind_returning_variables(rlist, before_index, after_index); ^ setrefs.c:1957:21: note: ‘before_index’ was declared here int after_index=0, before_index; ^ Right, my mistake. Sorry and thanks. Fixed. Regards, Karol Trzcionka With this fixed, a more complete review: * Is the patch in a patch format which has context? (eg: context diff format) Yes. * Does it apply cleanly to the current git master? Yes. * Does it include reasonable tests, necessary doc patches, etc? There is a new regression test (returning_before_after.sql) covering this feature. However, I think it should be added to the group where returning.sql resides currently. There is a value in running it in parallel to other tests. Sometimes a subtle bug is uncovered because of this and your v2 patch fixed such a bug already. doc/src/sgml/ref/update.sgml describes this feature. doc/src/sgml/dml.sgml should also be touched to cover this feature. * Does the patch actually implement what it's supposed to do? Yes. * Do we want that? Yes. * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? RETURNING is a PostgreSQL extension, so the SQL-spec part of the question isn't applicable. It implements the community-agreed behaviour, according to the new regression test coverage. * Does it include pg_dump support (if applicable)? n/a * Are there dangers? I don't think so. * Have all the bases been covered? It seems so. I have also tried mixing before/after columns in different orders and the query didn't fail: zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text); CREATE TABLE zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c'); INSERT 0 1 zozo=# select * from t1; id | i1 | i2 | t1 | t2 ++++ 1 | 1 | 1 | a | a 2 | 2 | 2 | b | b 3 | 3 | 3 | c | c (3 rows) zozo=# begin; BEGIN zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2; i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2 +++++++- 2 | 2 | 2 | 4 | b | b | b | bx2 (1 row) UPDATE 1 zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2 ++++++-+- 3 | 3 | 9 | 6 | c | c | cx3 | cx2 (1 row) UPDATE 1 * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I don't know. * Are there any assertion failures or crashes? No. * 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. * Does it follow the project coding guidelines? Mostly. In the src/test/regress/parallel_schedule contains an extra new line at the end, it shouldn't. In b/src/backend/optimizer/plan/setrefs.c: +static void bind_returning_variables(List *rlist, int bef, int aft); but later it becomes not public: + */ +void bind_returning_variables(List *rlist, int bef, int aft) +{ Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward declaration is not needed, the function is called only from later functions. Similar for parse_clause.c: +extern void addAliases(ParseState *pstate); +void addAliases(ParseState *pstate) This external declaration is not needed since it is already in src/include/parser/parse_clause.h In setrefs.c, bind_returning_variables() I would also rename the function arguments, so before and after are spelled out. These are not C keywords. Some assignments, like: + var=(Var*)tle; and + int index_rel=1; in setrefs.c need spaces. if() statements need a space before the ( and not after. Add spaces in the {} list in addAliases(): + char*aliases[] = {before,after}; like this: { before, after } Spaces are needed here, too: + for(i=0 ; inoal; i++) This noal should be naliases or n_aliases if you really want a variable. I would simply use the constant 2 for the two for() loops in addAliases() instead, its purpose is obvious enough. In setrefs.c, bind_returning_variables(): + Var *var = NULL; + foreach(temp, rlist){ Add an empty line after the declaration block. * Are there portability issues? No. * Will it work
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013-08-21 20:00 keltezéssel, Boszormenyi Zoltan írta: 2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta: Hi, 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta: W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze: Here's a new one, for v7: setrefs.c: In function ‘set_plan_refs’: setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function [-Wmaybe-uninitialized] bind_returning_variables(rlist, before_index, after_index); ^ setrefs.c:1957:21: note: ‘before_index’ was declared here int after_index=0, before_index; ^ Right, my mistake. Sorry and thanks. Fixed. Regards, Karol Trzcionka With this fixed, a more complete review: * Is the patch in a patch format which has context? (eg: context diff format) Yes. * Does it apply cleanly to the current git master? Yes. * Does it include reasonable tests, necessary doc patches, etc? There is a new regression test (returning_before_after.sql) covering this feature. However, I think it should be added to the group where returning.sql resides currently. There is a value in running it in parallel to other tests. Sometimes a subtle bug is uncovered because of this and your v2 patch fixed such a bug already. doc/src/sgml/ref/update.sgml describes this feature. doc/src/sgml/dml.sgml should also be touched to cover this feature. * Does the patch actually implement what it's supposed to do? Yes. * Do we want that? Yes. * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? RETURNING is a PostgreSQL extension, so the SQL-spec part of the question isn't applicable. It implements the community-agreed behaviour, according to the new regression test coverage. * Does it include pg_dump support (if applicable)? n/a * Are there dangers? I don't think so. * Have all the bases been covered? It seems so. I have also tried mixing before/after columns in different orders and the query didn't fail: zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text); CREATE TABLE zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b'); INSERT 0 1 zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c'); INSERT 0 1 zozo=# select * from t1; id | i1 | i2 | t1 | t2 ++++ 1 | 1 | 1 | a | a 2 | 2 | 2 | b | b 3 | 3 | 3 | c | c (3 rows) zozo=# begin; BEGIN zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1, after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2; i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2 +++++++- 2 | 2 | 2 | 4 | b | b | b | bx2 (1 row) UPDATE 1 zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1, after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2 ++++++-+- 3 | 3 | 9 | 6 | c | c | cx3 | cx2 (1 row) UPDATE 1 * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I don't know. * Are there any assertion failures or crashes? No. * 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. * Does it follow the project coding guidelines? Mostly. In the src/test/regress/parallel_schedule contains an extra new line at the end, it shouldn't. In b/src/backend/optimizer/plan/setrefs.c: +static void bind_returning_variables(List *rlist, int bef, int aft); but later it becomes not public: + */ +void bind_returning_variables(List *rlist, int bef, int aft) +{ Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward declaration is not needed, the function is called only from later functions. Similar for parse_clause.c: +extern void addAliases(ParseState *pstate); +void addAliases(ParseState *pstate) This external declaration is not needed since it is already in src/include/parser/parse_clause.h In setrefs.c, bind_returning_variables() I would also rename the function arguments, so before and after are spelled out. These are not C keywords. Some assignments, like: + var=(Var*)tle; and + int index_rel=1; in setrefs.c need spaces. if() statements need a space before the ( and not after. Add spaces in the {} list in addAliases(): + char*aliases[] = {before,after}; like this: { before, after } Spaces are needed here, too: + for(i=0 ; inoal; i++) This noal should be naliases or n_aliases if you really want a variable. I would simply use the constant 2 for the two for() loops in addAliases() instead, its purpose is obvious enough. In setrefs.c, bind_returning_variables(): + Var *var = NULL; + foreach(temp, rlist){ Add an empty line after the declaration block.
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze: With this fixed, a more complete review: Thanks. There is a new regression test (returning_before_after.sql) covering this feature. However, I think it should be added to the group where returning.sql resides currently. There is a value in running it in parallel to other tests. Sometimes a subtle bug is uncovered because of this and your v2 patch fixed such a bug already. Modified to work correct in parallel testing doc/src/sgml/ref/update.sgml describes this feature. doc/src/sgml/dml.sgml should also be touched to cover this feature. I don't think so, there is not any info about returning feature, I think it shouldn't be part of my patch. In the src/test/regress/parallel_schedule contains an extra new line at the end, it shouldn't. Fixed In b/src/backend/optimizer/plan/setrefs.c: +static void bind_returning_variables(List *rlist, int bef, int aft); but later it becomes not public: + */ +void bind_returning_variables(List *rlist, int bef, int aft) +{ I've change to static in the definition. +extern void addAliases(ParseState *pstate); +void addAliases(ParseState *pstate) This external declaration is not needed since it is already in src/include/parser/parse_clause.h Removed. In setrefs.c, bind_returning_variables() I would also rename the function arguments, so before and after are spelled out. These are not C keywords. Changed. Some assignments, like: + var=(Var*)tle; and + int index_rel=1; in setrefs.c need spaces. if() statements need a space before the ( and not after. Add spaces in the {} list in addAliases(): + char*aliases[] = {before,after}; like this: { before, after } Spaces are needed here, too: + for(i=0 ; inoal; i++) This noal should be naliases or n_aliases if you really want a variable. I would simply use the constant 2 for the two for() loops in addAliases() instead, its purpose is obvious enough. Added some whitespaces. In setrefs.c, bind_returning_variables(): + Var *var = NULL; + foreach(temp, rlist){ Add an empty line after the declaration block. Added. Other comments: This #define in pg_class: diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 49c4f6f..1b09994 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -154,6 +154,7 @@ DESCR(); #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */ #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */ #define RELKIND_MATVIEW 'm' /* materialized view */ +#define RELKIND_BEFORE 'b' /* virtual table for before/after statements */ #define RELPERSISTENCE_PERMANENT 'p' /* regular table */ #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */ RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because RTE_BEFORE wasn't available (not included?). Speaking of which, RTE_BEFORE is more properly named RTE_RETURNING_ALIAS or something like that because it covers both before and after. Someone may have a better idea for naming this symbol. Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases) One question, though, about this part: @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root, int rtoffset) { indexed_tlist *itlist; + int after_index=0, before_index=0; + Query *parse = root-parse; + ListCell *rt; + RangeTblEntry *bef; + + int index_rel=1; + + foreach(rt,parse-rtable) + { + bef = (RangeTblEntry *)lfirst(rt); + if(strcmp(bef-eref-aliasname,after) == 0 bef-rtekind == RTE_BEFORE ) + { + after_index = index_rel; + } + if(strcmp(bef-eref-aliasname,before) == 0 bef-rtekind == RTE_BEFORE ) + { + before_index = index_rel; + } + index_rel++; + } /* * We can perform the desired Var fixup by abusing the fix_join_expr * machinery that formerly handled inner indexscan fixup. We search the @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root, resultRelation, rtoffset); + bind_returning_variables(rlist, before_index, after_index); pfree(itlist); return rlist; Why is it enough to keep the last before_index and after_index values? What if
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi, 2013-08-21 20:52 keltezéssel, Karol Trzcionka írta: W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze: With this fixed, a more complete review: Thanks. There is a new regression test (returning_before_after.sql) covering this feature. However, I think it should be added to the group where returning.sql resides currently. There is a value in running it in parallel to other tests. Sometimes a subtle bug is uncovered because of this and your v2 patch fixed such a bug already. Modified to work correct in parallel testing doc/src/sgml/ref/update.sgml describes this feature. doc/src/sgml/dml.sgml should also be touched to cover this feature. I don't think so, there is not any info about returning feature, I think it shouldn't be part of my patch. In the src/test/regress/parallel_schedule contains an extra new line at the end, it shouldn't. Fixed In b/src/backend/optimizer/plan/setrefs.c: +static void bind_returning_variables(List *rlist, int bef, int aft); but later it becomes not public: + */ +void bind_returning_variables(List *rlist, int bef, int aft) +{ I've change to static in the definition. +extern void addAliases(ParseState *pstate); +void addAliases(ParseState *pstate) This external declaration is not needed since it is already in src/include/parser/parse_clause.h Removed. In setrefs.c, bind_returning_variables() I would also rename the function arguments, so before and after are spelled out. These are not C keywords. Changed. Some assignments, like: + var=(Var*)tle; and + int index_rel=1; in setrefs.c need spaces. if() statements need a space before the ( and not after. Add spaces in the {} list in addAliases(): + char*aliases[] = {before,after}; like this: { before, after } Spaces are needed here, too: + for(i=0 ; inoal; i++) This noal should be naliases or n_aliases if you really want a variable. I would simply use the constant 2 for the two for() loops in addAliases() instead, its purpose is obvious enough. Added some whitespaces. In setrefs.c, bind_returning_variables(): + Var *var = NULL; + foreach(temp, rlist){ Add an empty line after the declaration block. Added. Other comments: This #define in pg_class: diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 49c4f6f..1b09994 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -154,6 +154,7 @@ DESCR(); #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */ #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */ #define RELKIND_MATVIEW 'm' /* materialized view */ +#define RELKIND_BEFORE 'b' /* virtual table for before/after statements */ #define RELPERSISTENCE_PERMANENT 'p' /* regular table */ #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */ RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because RTE_BEFORE wasn't available (not included?). Speaking of which, RTE_BEFORE is more properly named RTE_RETURNING_ALIAS or something like that because it covers both before and after. Someone may have a better idea for naming this symbol. Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases) Others may have also a word in this topic, but consider that this is *not* a regular alias and for those, RTE_ALIAS is not used, like in UPDATE table AS alias SET ... Maybe RTE_RETURNING_ALIAS takes a little more typing, but it becomes obvious when reading and new code won't confuse it with regular aliases. One question, though, about this part: @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root, int rtoffset) { indexed_tlist *itlist; + int after_index=0, before_index=0; + Query *parse = root-parse; + ListCell *rt; + RangeTblEntry *bef; + + int index_rel=1; + + foreach(rt,parse-rtable) + { + bef = (RangeTblEntry *)lfirst(rt); + if(strcmp(bef-eref-aliasname,after) == 0 bef-rtekind == RTE_BEFORE ) + { + after_index = index_rel; + } + if(strcmp(bef-eref-aliasname,before) == 0 bef-rtekind == RTE_BEFORE ) + { + before_index = index_rel; + } + index_rel++; + } /* * We can perform the desired Var fixup by abusing the fix_join_expr * machinery that formerly handled inner indexscan fixup. We search the @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root, resultRelation, rtoffset); + bind_returning_variables(rlist, before_index, after_index); pfree(itlist); return rlist;
[HACKERS] pg_system_identifier()
After someone in IRC asked if there was an equivalent to MySQL's server_id, it was noted that we do have a system identifier but it's not very accessible. The attached patch implements a pg_system_identifier() function that exposes it. Shall I add this to the next commitfest? -- Vik *** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml *** *** 13473,13478 SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n); --- 13473,13484 /row row +entryliteralfunctionpg_system_identifier()/function/literal/entry +entrytypetext/type/entry +entrysystem identifier for the database cluster/entry + /row + + row entryliteralfunctionpg_trigger_depth()/function/literal/entry entrytypeint/type/entry entrycurrent nesting level of productnamePostgreSQL/ triggers *** a/src/backend/access/transam/xlogfuncs.c --- b/src/backend/access/transam/xlogfuncs.c *** *** 35,40 --- 35,50 static void validate_xlog_location(char *str); + /* + * pg_system_identifier + */ + Datum + pg_system_identifier(PG_FUNCTION_ARGS) + { + char ret[32]; + sprintf(ret, UINT64_FORMAT, GetSystemIdentifier()); + PG_RETURN_TEXT_P(cstring_to_text(ret)); + } /* * pg_start_backup: set up for taking an on-line backup dump *** a/src/include/access/xlog_fn.h --- b/src/include/access/xlog_fn.h *** *** 13,18 --- 13,19 #include fmgr.h + extern Datum pg_system_identifier(PG_FUNCTION_ARGS); extern Datum pg_start_backup(PG_FUNCTION_ARGS); extern Datum pg_stop_backup(PG_FUNCTION_ARGS); extern Datum pg_switch_xlog(PG_FUNCTION_ARGS); *** a/src/include/catalog/catversion.h --- b/src/include/catalog/catversion.h *** *** 53,58 --- 53,59 */ /* mmddN */ + /* needs bump */ #define CATALOG_VERSION_NO 201307221 #endif *** a/src/include/catalog/pg_proc.h --- b/src/include/catalog/pg_proc.h *** *** 2954,2959 DATA(insert OID = 2171 ( pg_cancel_backend PGNSP PGUID 12 1 0 0 0 f f f f t f v --- 2954,2961 DESCR(cancel a server process' current query); DATA(insert OID = 2096 ( pg_terminate_backend PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 16 23 _null_ _null_ _null_ _null_ pg_terminate_backend _null_ _null_ _null_ )); DESCR(terminate a server process); + DATA(insert OID = 3179 ( pg_system_identifier PGNSP PGUID 12 1 0 0 0 f f f f f f i 0 0 25 _null_ _null_ _null_ _null_ pg_system_identifier _null_ _null_ _null_ )); + DESCR(database system identifier); DATA(insert OID = 2172 ( pg_start_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v 2 0 25 25 16 _null_ _null_ _null_ _null_ pg_start_backup _null_ _null_ _null_ )); DESCR(prepare for taking an online backup); DATA(insert OID = 2173 ( pg_stop_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v 0 0 25 _null_ _null_ _null_ _null_ pg_stop_backup _null_ _null_ _null_ )); -- 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] Bison 3.0 updates
Andres Freund and...@2ndquadrant.com writes: On 2013-07-29 08:02:49 -0400, Tom Lane wrote: It looks like our choices are (1) teach configure to enable -fno-aggressive-loop-optimizations if the compiler recognizes it, or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9. I am in favor of fixing the back branches via (1), because it's less work and much less likely to break third-party extensions. This seems to be the agreed upon course of action, so I've prepared a patch including a preliminary commit message. I confirmed that it fixes the issue with gcc 4.8 and 9.1 for me. Committed --- thanks for doing the legwork to check it fixes the problem. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql_check_function - rebase for 9.3
On Wed, 2013-03-27 at 23:25 +0100, Pavel Stehule wrote: I redesigned output from plpgsql_check_function. Now, it returns table everytime. Litlle bit code simplification. This patch is in the 2013-09 commitfest but needs a rebase. -- 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] Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
This patch is in the 2013-09 commitfest but needs a rebase. -- 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] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
On Thu, 2013-07-11 at 10:51 -0400, Nicholas White wrote: I've attached a revised version that fixes the issues above: This patch is in the 2013-09 commitfest but needs a rebase. -- 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] Updatable view columns
On Mon, 2013-08-12 at 15:27 +0100, Dean Rasheed wrote: Attached is a work-in-progress patch to extend auto-updatable views to support views containing a mix of updatable and non-updatable columns. This is basically the columns part of SQL Feature T111, Updatable joins, unions, and columns. This patch needs to be rebased. -- 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] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
but needs a rebase. See attached - thanks! lead-lag-ignore-nulls.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] docbook-xsl version for release builds
On Fri, 2013-07-12 at 12:30 +0200, Magnus Hagander wrote: Given that, I'm fine with just bumping the version on borka to that version. Any objections? This was not done for 9.3rc1, AFAICT. Let's please do it for the next release builds. -- 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] Fix Windows socket error checking for MinGW
On Mon, Aug 19, 2013 at 08:46:11AM -0500, Michael Cronenworth wrote: On 08/17/2013 12:16 AM, Noah Misch wrote: 1. Redefine those constants for more (all?) compilers. 2. Remove that block and put #ifdef around all usage of such constants in frontend code, as you have done. 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, so frontend code can treat errno like backend code treats errno. What do you recommend? Option 1 is dangerous. I'd rather let the environments keep their constants. I concur, but our field experience doing it this way lessens my concern. Option 2 is the least dangerous but it adds lines of code. More than that, as Robert explained, we seek to _isolate_ Windows-specific code. That's true even when a better-isolated approach takes more lines. Option 3: The errno variable is not set in Windows so relying on it is not possible. Look at src/backend/port/win32/socket.c; it emulates POSIX socket interfaces on Windows, and that emulation includes setting errno. I'd favor option 3 if we weren't already successfully using option 1 for Visual Studio 2010, the compiler of official 9.2 and 9.3 binaries. Since we have been doing that, I figure changing to option 3 now is riskier than staying the course. nm -- Noah Misch EnterpriseDB http://www.enterprisedb.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] CAST Within EXCLUSION constraint
On Wed, Aug 21, 2013 at 10:13:15AM -0400, Tom Lane wrote: Alexander Korotkov aekorot...@gmail.com writes: It seems reasonable to me to cast enum to oid. However, creating casts without function isn't allowed for enums. test=# create cast (source as oid) without function; ERROR: enum data types are not binary-compatible The reason for that is you'd get randomly different results on another installation. In this particular application, I think David doesn't really care about what values he gets as long as they're distinct, so this might be an OK workaround for him. But that's the reasoning for the general prohibition. While a WITHOUT FUNCTION cast does *guarantee* that flaw, working around the restriction with a cast function is all too likely to create the same flaw. Here's the comment about the restriction: * Theoretically you could build a user-defined base type that is * binary-compatible with a composite, enum, or array type. But we * disallow that too, as in practice such a cast is surely a mistake. * You can always work around that by writing a cast function. That's reasonable enough, but we could reduce this to a WARNING. Alexander shows a credible use case. A superuser can easily introduce breakage through careless addition of WITHOUT FUNCTION casts. Permitting borderline cases seems more consistent with the level of user care already expected in this vicinity. -- Noah Misch EnterpriseDB http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Wed, Aug 21, 2013 at 8:22 PM, Stephen Frost sfr...@snowman.net wrote: Martijn, * Martijn van Oosterhout (klep...@svana.org) wrote: ISTM you want some kind of hybrid setting like: #include_system auto.conf which simultaneously does three things: 1. Sets the enable_alter_system flag 2. Indicates the file to use 3. Indicates the priority of the setting re other settings. Comment it out, ALTER SYSTEM stop working. Put it back and it's immediately clear what it means. And the user can control where the settings go. Yeah, that's certainly an interesting idea. I might call it 'auto_conf_file auto.conf' to avoid the '#include' concern and to perhaps clarify that it's more than just a regular 'include'. This can resolve the problem of whether to read auto file rather cleanly, so the idea is: Enable/Disable reading of auto file - a. Have a new include in postresql.conf #include_auto_conf_filepostgresql.auto.conf as it is a special include, we can read this file relative to data directory. Enable/Disable Alter System command --- This can be achieved in 3 ways: a. Check before executing Alter System if include directive is disabled, then just issue a warning to user and proceed with command. b. Check before executing Alter System if include directive is disabled, then just issue an error and stop. c. Have a new guc enable_alter_system which will behave as described in my previous mail and below: 1. enable_alter_system a new GUC whose default value =off. 2. Alter System will check this variable and return error (not allowed), if this parameter is off. 3. Now if user enables include directive in postgresql.conf, it will enable Alter System as value of enable_alter_system is on. 4. User can run Alter System command to disable Alter System enable_alter_system = off. Now even though include directive is enabled, but new Alter System commands will not work, however existing parameter's take into effect on restart/sighup. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.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] Backup throttling
On Aug 21, 2013, at 10:57 AM, Andres Freund wrote: On 2013-08-21 08:10:42 +0200, PostgreSQL - Hans-Jürgen Schönig wrote: On Aug 19, 2013, at 9:11 PM, Andres Freund wrote: On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote: 2013-08-19 19:20 keltezéssel, Andres Freund írta: Hi, On 2013-07-24 09:20:52 +0200, Antonin Houska wrote: Hello, the purpose of this patch is to limit impact of pg_backup on running server. Feedback is appreciated. Based on a quick look it seems like you're throttling on the receiving side. Is that a good idea? Especially over longer latency links, TCP buffering will reduce the effect on the sender side considerably. Throttling on the sender side requires extending the syntax of BASE_BACKUP and maybe START_REPLICATION so both can be throttled but throttling is still initiated by the receiver side. Seems fine to me. Under the premise that the idea is decided to be worthwile to be integrated. Which I am not yet convinced of. i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple: just assume a weak server - maybe just one disk or two - and a slave. master and slave are connected via a 1 GB network. pg_basebackup will fetch data full speed basically putting those lonely disks out of business. we actually had a case where a client asked if PostgreSQL is locked during base backup. of course it was just disk wait caused by a full speed pg_basebackup. regarding the client side implementation: we have chosen this way because it is less invasive. i cannot see a reason to do this on the server side because we won't have 10 pg_basebackup-style tools making use of this feature anyway. The problem is that receiver side throttling over TCP doesn't always work all that nicely unless you have a low rate of transfer and/or very low latency . Quite often you will have OS buffers/the TCP Window being filled in bursts where the sender sends at max capacity and then a period where nothing happens on the sender. That's often not what you want when you need to throttle. Besides, I can see some value in e.g. normal streaming replication also being rate limited... what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest. regards, hans -- Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers