Re: [HACKERS] Backup throttling

2013-08-21 Thread PostgreSQL - Hans-Jürgen Schönig

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

2013-08-21 Thread Andres Freund
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

2013-08-21 Thread Vik Fearing
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

2013-08-21 Thread ciifrance...@tiscali.it

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?

2013-08-21 Thread Andres Freund
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

2013-08-21 Thread Bruce Momjian
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

2013-08-21 Thread Andres Freund
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

2013-08-21 Thread Alexander Korotkov
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

2013-08-21 Thread Marko Tiikkaja

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?

2013-08-21 Thread Stephen Frost
* 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-08-21 Thread Pavel Stehule
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

2013-08-21 Thread Marko Tiikkaja

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

2013-08-21 Thread Tom Lane
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

2013-08-21 Thread Tom Lane
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

2013-08-21 Thread Marko Tiikkaja

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

2013-08-21 Thread MauMau

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?

2013-08-21 Thread Kevin Grittner
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])

2013-08-21 Thread Stephen Frost
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

2013-08-21 Thread Merlin Moncure
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

2013-08-21 Thread Marko Tiikkaja

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

2013-08-21 Thread Merlin Moncure
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

2013-08-21 Thread David E. Wheeler
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-08-21 Thread Pavel Stehule
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

2013-08-21 Thread Marko Tiikkaja

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

2013-08-21 Thread Tom Lane
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?

2013-08-21 Thread Jeff Janes
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

2013-08-21 Thread Boszormenyi Zoltan

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

2013-08-21 Thread Merlin Moncure
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 Thread Boszormenyi Zoltan

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 Thread Boszormenyi Zoltan

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

2013-08-21 Thread Karol Trzcionka
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

2013-08-21 Thread Boszormenyi Zoltan

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()

2013-08-21 Thread Vik Fearing
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

2013-08-21 Thread Tom Lane
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

2013-08-21 Thread Peter Eisentraut
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]

2013-08-21 Thread Peter Eisentraut
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

2013-08-21 Thread Peter Eisentraut
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

2013-08-21 Thread Peter Eisentraut
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

2013-08-21 Thread Nicholas White
 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

2013-08-21 Thread Peter Eisentraut
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

2013-08-21 Thread Noah Misch
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

2013-08-21 Thread Noah Misch
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])

2013-08-21 Thread Amit Kapila
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

2013-08-21 Thread PostgreSQL - Hans-Jürgen Schönig

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