[HACKERS] Why does analyze_new_cluster.sh use sleep?

2012-08-23 Thread Peter Eisentraut
The script analyze_new_cluster.sh output by pg_upgrade contains several
sleep calls (see contrib/pg_upgrade/check.c).  What is the point of
this?  If the purpose of this script is to get the database operational
again as soon as possible, waiting a few seconds doing nothing surely
isn't helping.

I could maybe see the point of waiting a bit between the different
vacuumdb calls, to catch some breath, but the one before the first call
to vacuumdb is highly dubious to me.




-- 
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] xlog file naming

2012-08-23 Thread Peter Eisentraut
On Tue, 2012-08-21 at 12:01 -0400, Robert Haas wrote:
 On Wed, Aug 15, 2012 at 8:43 PM, Bruce Momjian br...@momjian.us wrote:
  Are there any TODO items here?
 
 It's possible there's something we want to change here, but it's far
 from obvious what that thing is.  Our WAL file handling is
 ridiculously hard to understand, but the problem with changing it is
 that there will then be two things people have to understand, and a
 lot of tools that have to be revamped.  It isn't clear that it's worth
 going through that kind of pain for a minor improvement in clarity.

The idea was that since we already broke some tools, possibly silently
(...FF files that they previously skipped), a more radical renaming
might break those tools more obviously, and make some other things
simpler/easier down the road.




-- 
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] new --maintenance-db options

2012-08-23 Thread Peter Eisentraut
On Sun, 2012-06-24 at 01:26 +0300, Peter Eisentraut wrote:
 About the new --maintenance-db options:
 
 Why was this option not added to createuser and dropuser?  In the
 original discussion[0] they were mentioned, but it apparently never
 made it into the code. 

What should we do with this?  Add the option to createuser and dropuser
now, and think about a more permanent/useful/complete solution in 9.3?




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] to_timestamp() too loose?

2012-08-23 Thread Magnus Hagander
postgres=# select to_timestamp('2012-08-01', '-mm-dd');
  to_timestamp

 2012-08-01 00:00:00+02

postgres=# select to_timestamp('2012-08-00', '-mm-dd');
  to_timestamp

 2012-08-01 00:00:00+02

postgres=# select to_timestamp('2012-00-00', '-mm-dd');
  to_timestamp

 2012-01-01 00:00:00+01



Should we really convert 00 to 01?

We also do things like:
postgres=# select to_timestamp('2012-00-99', '-mm-dd');
  to_timestamp

 2012-04-08 00:00:00+02


And while I guess there's some logic in that, it's not documented
AFAICT. Or am I just not finding the docs?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

2012-08-23 Thread Andres Freund
Hi,

While debugging an instance of this bug I noticed that plperlu always removes 
the SIGFPE handler and sets it to ignore:


andres@awork2:~$ psql -p 5435 -U postgres -h /var/run/postgresql test
Timing is on.
psql (9.1devel, server 9.1.5)
Type help for help.

test=# SELECT pg_backend_pid();
 pg_backend_pid 

   9287

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/9287/status|awk 
'{print $2}'
01301800
000180006287

test=# DO LANGUAGE plperlu ;

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/9287/status|awk 
'{print $2}'
01301880
000180006207

Note the 8'th bit being unset in SigCgt and set in SigIgn. Thats SIGFPE...

Not sure how relevant this really is, but it could cause errors to be 
ignored...

Greetings,

Andres
-- 
 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] pg_stat_replication vs StandbyReplyMessage

2012-08-23 Thread Magnus Hagander
On Wed, Aug 15, 2012 at 5:39 PM, Bruce Momjian br...@momjian.us wrote:
 On Mon, Aug 15, 2011 at 01:03:35PM +0200, Magnus Hagander wrote:
 The pg_stat_replication view exposes all the fields in
 StandbyReplyMessage *except* for the timestamp when the message was
 generated. On an active system this is not all that interesting, but
 on a mostly idle system that allows the monitoring to react faster
 than the timeout that actually kicks the other end off - and could be
 useful in manual debugging scenarios. Any particular reason why this
 was not exposed as it's own column?

 Did this ever get done?  I don't think so, though everyone wanted it.

Nope, it wasn't done. Should probably do that for 9.3 (since adding a
field to pg_stat_replication will cause initdb, so we can't really do
it for 9.2 unless it was really critical - and it's not).

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] alter enum add value if not exists

2012-08-23 Thread Magnus Hagander
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan and...@dunslane.net wrote:
 Here is a patch for this feature, which should alleviate some of the woes
 caused by adding labels not being transactional (and thus not allowing for
 the catching of errors).

I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?

I don't recall the exact discussion, but was there something about
enum labels that made it impossible to make them transactional, or was
it just lots of work, let's do that later instead to get the feature
in? If the second, does anyone have plans to fix it? It is a quite
annoying limitation :(

That said, this functionality would be useful even *if* the enum label
addition was made transactional...

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] to_timestamp() too loose?

2012-08-23 Thread Amit Kapila
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Magnus Hagander
Sent: Thursday, August 23, 2012 2:08 PM

 postgres=# select to_timestamp('2012-08-01', '-mm-dd');
  to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-08-00', '-mm-dd');
   to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-00-00', '-mm-dd');
   to_timestamp
 
  2012-01-01 00:00:00+01

For the above different databases have different behaviour 
Oracle - return error for 2 and 3 stating invalid day, invalid month
respectively. 
MySQL  - return output as follows 
select to_timestamp('2012-08-00', '-mm-dd'); 
2012-07-31 00:00:00 
select to_timestamp('2012-00-00', '-mm-dd'); 
2011-11-30 00:00:00


 Should we really convert 00 to 01?
I believe for invalid dates, behavior is database dependent, so the behavior
of PG should be okay.


 We also do things like:
 postgres=# select to_timestamp('2012-00-99', '-mm-dd');
  to_timestamp
 
  2012-04-08 00:00:00+02

For the above different databases have different behaviour 
Oracle - returns error stating invalid month. 
MySQL  - NULL 
PG - as it converts to julian date, so the output is based on that
calculation. 

In this, it should actually throw error because user might not be able to
makeout any relation of output. 
However that will create behavior inconsistency.


With Regards,
Amit Kapila.  




-- 
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] alter enum add value if not exists

2012-08-23 Thread Andrew Dunstan


On 08/23/2012 06:47 AM, Magnus Hagander wrote:

On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan and...@dunslane.net wrote:

Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing for
the catching of errors).

I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?



Well, you can't remove a label, and if the test succeeds it results in 
your doing nothing, so my possibly naive thinking was that that wasn't 
necessary. But I could easily be wrong :-)





I don't recall the exact discussion, but was there something about
enum labels that made it impossible to make them transactional, or was
it just lots of work, let's do that later instead to get the feature
in? If the second, does anyone have plans to fix it? It is a quite
annoying limitation :(



I don't know of any plans to fix it.



That said, this functionality would be useful even *if* the enum label
addition was made transactional...



Right.

cheers

andrew



--
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] alter enum add value if not exists

2012-08-23 Thread Magnus Hagander
On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan and...@dunslane.net wrote:

 On 08/23/2012 06:47 AM, Magnus Hagander wrote:

 On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan and...@dunslane.net
 wrote:

 Here is a patch for this feature, which should alleviate some of the woes
 caused by adding labels not being transactional (and thus not allowing
 for
 the catching of errors).

 I haven't actually checked the code in detail, but if it's not
 transactional, how does it actually prevent race conditions? Doesn't
 it at least have to do it's check *after* the enum is locked?



 Well, you can't remove a label, and if the test succeeds it results in your
 doing nothing, so my possibly naive thinking was that that wasn't necessary.
 But I could easily be wrong :-)

Ah, good point.
But still: what if:

Session A checks if the label is present, it's not.
Session B checks if the label is present, it's not.
Session A locks the enum, and adds the label, then releases lock.
Session B locks the enum, and tries to add it -- and you still get a failure.

It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?

Or am I missing something? (I probably am :D - I still haven't looked
at it in detail)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] alter enum add value if not exists

2012-08-23 Thread Andrew Dunstan


On 08/23/2012 07:39 AM, Magnus Hagander wrote:

On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan and...@dunslane.net wrote:

On 08/23/2012 06:47 AM, Magnus Hagander wrote:

On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan and...@dunslane.net
wrote:

Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing
for
the catching of errors).

I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?



Well, you can't remove a label, and if the test succeeds it results in your
doing nothing, so my possibly naive thinking was that that wasn't necessary.
But I could easily be wrong :-)

Ah, good point.
But still: what if:

Session A checks if the label is present, it's not.
Session B checks if the label is present, it's not.
Session A locks the enum, and adds the label, then releases lock.
Session B locks the enum, and tries to add it -- and you still get a failure.

It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?

Or am I missing something? (I probably am :D - I still haven't looked
at it in detail)



Yeah, looking further this was probably a thinko on my part. Thanks for 
noticing. I've moved the test down so it's done right after the lock is 
acquired. Revised patch attached.


cheers

andrew


*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
***
*** 28,34  ALTER TYPE replaceable class=PARAMETERname/replaceable OWNER TO replaceab
  ALTER TYPE replaceable class=PARAMETERname/replaceable RENAME ATTRIBUTE replaceable class=PARAMETERattribute_name/replaceable TO replaceable class=PARAMETERnew_attribute_name/replaceable
  ALTER TYPE replaceable class=PARAMETERname/replaceable RENAME TO replaceable class=PARAMETERnew_name/replaceable [ CASCADE | RESTRICT ]
  ALTER TYPE replaceable class=PARAMETERname/replaceable SET SCHEMA replaceable class=PARAMETERnew_schema/replaceable
! ALTER TYPE replaceable class=PARAMETERname/replaceable ADD VALUE replaceable class=PARAMETERnew_enum_value/replaceable [ { BEFORE | AFTER } replaceable class=PARAMETERexisting_enum_value/replaceable ]
  
  phrasewhere replaceable class=PARAMETERaction/replaceable is one of:/phrase
  
--- 28,34 
  ALTER TYPE replaceable class=PARAMETERname/replaceable RENAME ATTRIBUTE replaceable class=PARAMETERattribute_name/replaceable TO replaceable class=PARAMETERnew_attribute_name/replaceable
  ALTER TYPE replaceable class=PARAMETERname/replaceable RENAME TO replaceable class=PARAMETERnew_name/replaceable [ CASCADE | RESTRICT ]
  ALTER TYPE replaceable class=PARAMETERname/replaceable SET SCHEMA replaceable class=PARAMETERnew_schema/replaceable
! ALTER TYPE replaceable class=PARAMETERname/replaceable ADD VALUE [ IF NOT EXISTS ] replaceable class=PARAMETERnew_enum_value/replaceable [ { BEFORE | AFTER } replaceable class=PARAMETERexisting_enum_value/replaceable ]
  
  phrasewhere replaceable class=PARAMETERaction/replaceable is one of:/phrase
  
***
*** 106,112  ALTER TYPE replaceable class=PARAMETERname/replaceable ADD VALUE replacea
 /varlistentry
  
 varlistentry
! termliteralADD VALUE [ BEFORE | AFTER ]/literal/term
  listitem
   para
This form adds a new value to an enum type. If the new value's place in
--- 106,112 
 /varlistentry
  
 varlistentry
! termliteralADD VALUE [ IF NOT EXISTS ] [ BEFORE | AFTER ]/literal/term
  listitem
   para
This form adds a new value to an enum type. If the new value's place in
***
*** 114,119  ALTER TYPE replaceable class=PARAMETERname/replaceable ADD VALUE replacea
--- 114,124 
literalAFTER/literal, then the new item is placed at the end of the
list of values.
   /para
+  para
+   If literalIF NOT EXISTS/literal is used, it is not an error if the
+   type already contains the new value, and no action  is taken. Otherwise,
+   an error will occur if the new value is already present.
+  /para
  /listitem
 /varlistentry
  
*** a/src/backend/catalog/pg_enum.c
--- b/src/backend/catalog/pg_enum.c
***
*** 177,183  void
  AddEnumLabel(Oid enumTypeOid,
  			 const char *newVal,
  			 const char *neighbor,
! 			 bool newValIsAfter)
  {
  	Relation	pg_enum;
  	Oid			newOid;
--- 177,184 
  AddEnumLabel(Oid enumTypeOid,
  			 const char *newVal,
  			 const char *neighbor,
! 			 bool newValIsAfter,
! 	 bool skipIfExists)
  {
  	Relation	pg_enum;
  	Oid			newOid;
***
*** 209,214  AddEnumLabel(Oid enumTypeOid,
--- 210,230 
  	 */
  	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
  
+ 	/* Do the IF NOT EXISTS test if specified */
+ 	if (skipIfExists)
+ 	{
+ 		

Re: [HACKERS] xlog file naming

2012-08-23 Thread Tom Lane
Peter Eisentraut pete...@gmx.net writes:
 On Tue, 2012-08-21 at 12:01 -0400, Robert Haas wrote:
 It's possible there's something we want to change here, but it's far
 from obvious what that thing is.  Our WAL file handling is
 ridiculously hard to understand, but the problem with changing it is
 that there will then be two things people have to understand, and a
 lot of tools that have to be revamped.  It isn't clear that it's worth
 going through that kind of pain for a minor improvement in clarity.

 The idea was that since we already broke some tools, possibly silently
 (...FF files that they previously skipped), a more radical renaming
 might break those tools more obviously, and make some other things
 simpler/easier down the road.

I think we already had that discussion, and the consensus was that
we did not want to break WAL-related tools unnecessarily.  If there
were a high probability that the FF change will actually break tools in
practice, the conclusion might have been different; but nobody believes
that there is much risk there.

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] new --maintenance-db options

2012-08-23 Thread Tom Lane
Peter Eisentraut pete...@gmx.net writes:
 On Sun, 2012-06-24 at 01:26 +0300, Peter Eisentraut wrote:
 About the new --maintenance-db options:
 
 Why was this option not added to createuser and dropuser?  In the
 original discussion[0] they were mentioned, but it apparently never
 made it into the code. 

 What should we do with this?  Add the option to createuser and dropuser
 now, and think about a more permanent/useful/complete solution in 9.3?

IMO it is now too late for 9.2 ... especially if you're of the opinion
that the current design is bad.  Propagating a wrong choice into even
more places doesn't seem like a step forward.

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] to_timestamp() too loose?

2012-08-23 Thread Tom Lane
Magnus Hagander mag...@hagander.net writes:
 postgres=# select to_timestamp('2012-08-01', '-mm-dd');
   to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-08-00', '-mm-dd');
   to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-00-00', '-mm-dd');
   to_timestamp
 
  2012-01-01 00:00:00+01

 Should we really convert 00 to 01?

to_timestamp is intentionally pretty loose.  Personally, if I wanted
sanity checking on a date string in any common format, I would just
cast the string to timestamp(tz), and *not* use to_timestamp.

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] to_timestamp() too loose?

2012-08-23 Thread Magnus Hagander
On Thu, Aug 23, 2012 at 3:42 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Magnus Hagander mag...@hagander.net writes:
 postgres=# select to_timestamp('2012-08-01', '-mm-dd');
   to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-08-00', '-mm-dd');
   to_timestamp
 
  2012-08-01 00:00:00+02

 postgres=# select to_timestamp('2012-00-00', '-mm-dd');
   to_timestamp
 
  2012-01-01 00:00:00+01

 Should we really convert 00 to 01?

 to_timestamp is intentionally pretty loose.  Personally, if I wanted
 sanity checking on a date string in any common format, I would just
 cast the string to timestamp(tz), and *not* use to_timestamp.

Shouldn't we put at least a note, and IMO even a *warning* in the docs
saying that it is like this? (or am I missing one we have) It's not
really consistent with how most of postgres works :)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] to_timestamp() too loose?

2012-08-23 Thread Tom Lane
Magnus Hagander mag...@hagander.net writes:
 On Thu, Aug 23, 2012 at 3:42 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 to_timestamp is intentionally pretty loose.  Personally, if I wanted
 sanity checking on a date string in any common format, I would just
 cast the string to timestamp(tz), and *not* use to_timestamp.

 Shouldn't we put at least a note, and IMO even a *warning* in the docs
 saying that it is like this? (or am I missing one we have) It's not
 really consistent with how most of postgres works :)

I have no objection to a note and even a couple of examples, but
try to keep down the dudgeon --- the reason it's like this is that
people have found it useful for the conversion to be forgiving about
field ranges.  An example is that you can compute next week by
adding 7 to the day field, without worrying about whether you need
to wrap that at 28, 29, 30, or 31 days.  This behavior corresponds
directly to Unix mktime(3), which is required by POSIX spec to be
lax about field ranges.

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] TRUE/FALSE vs true/false

2012-08-23 Thread Bruce Momjian
On Mon, Aug 20, 2012 at 03:09:08PM -0400, Robert Haas wrote:
 I have difficult believing that a change of this type, if implemented
 judiciously, is really going to create that much difficulty in
 back-patching.  I don't do as much back-patching as Tom either (no one
 does), but most of the patches I do back-patch can be cherry-picked
 all the way back without a problem.  Some require adjustment, but even
 then this kind of thing is pretty trivial to handle, as it's pretty
 obvious what happened when you look through it.  The really nasty
 problems tend to come from places where the code has been rearranged,
 rather than simple A-for-B substitutions.
 
 I think the thing we need to look at is what percentage of our code
 churn is coming from stuff like this, versus what percentage of it is
 coming from other factors.  If we change 250,000 lines of code per
 release cycle and of that this kind of thing accounts for 5,000 lines
 of deltas, then IMHO it's not really material.  If it accounts for
 50,000 lines of deltas out of the same base, that's probably more than
 can really be justified by the benefit we're going to get out of it.

The true/false capitalization patch changes 1.2k lines.

-- 
  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] GetSnapshotData() comments

2012-08-23 Thread Bruce Momjian
On Tue, Aug 21, 2012 at 11:48:19AM -0400, Robert Haas wrote:
 On Tue, Aug 14, 2012 at 5:41 PM, Bruce Momjian br...@momjian.us wrote:
  Did these comment updates ever get addressed?
 
 Partially.
 
 I just made a commit to clean up the rest of it.

Thanks.

-- 
  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] B-tree parent pointer and checkpoints

2012-08-23 Thread Bruce Momjian
On Tue, Aug 21, 2012 at 11:55:20AM -0400, Robert Haas wrote:
 On Wed, Aug 15, 2012 at 6:23 PM, Bruce Momjian br...@momjian.us wrote:
  Has this been addressed?  A TODO?
 
 I don't think anything's been done about it.  According to your email
 of October 11, 2011, you already did add a TODO for this.

Ah, I see that now.  Thanks.

-- 
  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] TRUE/FALSE vs true/false

2012-08-23 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 On Mon, Aug 20, 2012 at 03:09:08PM -0400, Robert Haas wrote:
 I think the thing we need to look at is what percentage of our code
 churn is coming from stuff like this, versus what percentage of it is
 coming from other factors.  If we change 250,000 lines of code per
 release cycle and of that this kind of thing accounts for 5,000 lines
 of deltas, then IMHO it's not really material.  If it accounts for
 50,000 lines of deltas out of the same base, that's probably more than
 can really be justified by the benefit we're going to get out of it.

 The true/false capitalization patch changes 1.2k lines.

I did a quick look at git diff --stat between recent branches:

$ git diff --shortstat REL9_0_9 REL9_1_5
 3186 files changed, 314847 insertions(+), 210452 deletions(-)
$ git diff --shortstat REL9_1_5 REL9_2_BETA4
 2037 files changed, 290919 insertions(+), 189487 deletions(-)

However, when you look at things a bit closer, these numbers are
misleading because they include the .po files, which seem to have huge
inter-branch churn --- well in excess of 10 lines changed per
release, at least in git's simpleminded view.  Excluding those, as well
as src/test/isolation/expected/prepared-transactions.out which added
34843 lines all by itself, I get
173080 insertions, 70300 deletions for 9.0.9 - 9.1.5
130706 insertions, 55714 deletions for 9.1.5 - 9.2beta4.
So it looks like we touch order-of-magnitude of 100K lines per release;
which still seems astonishingly high, but then this includes docs and
regression tests not just code.  If I restrict the stat to *.[chyl]
files it's about half that:

$ git diff --numstat REL9_0_9 REL9_1_5 | grep '\.[chyl]$' | awk '{a += $1; b += 
$2}
END{print a,b}'
90234 33902
$ git diff --numstat REL9_1_5 REL9_2_BETA4 | grep '\.[chyl]$' | awk '{a += $1; 
b += $2}
END{print a,b}'
90200 42218

So a patch of 1K lines would by itself represent about 2% of the typical
inter-branch delta.  Maybe that's below our threshold of pain, or maybe
it isn't.  I'd be happier about it if there were a more compelling
argument for it, but to me it looks like extremely trivial neatnik-ism.

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] temporal support patch

2012-08-23 Thread Kevin Grittner
Jeff Davis pg...@j-davis.com wrote:
 On Tue, 2012-08-21 at 17:07 -0500, Kevin Grittner wrote:
 
 The fact that it has an unknown sequence number or timestamp for
 purposes of ordering visibility of transactions doesn't mean you
 can't show that it completed in an audit log.  In other words, I
 think the needs for a temporal database are significantly
 different from the needs of an auditing system.
 
 ...
  
 I would assume an audit log would have very different needs from
 tracking changes for a temporal database view.  It even seems
 possible that you might want to see what people *looked* at,
 versus just changes.  You might want to see transactions which
 were rolled back, which are irrelevant for a temporal view.  If
 we're talking about an auditing system, we're talking about an
 almost completely different animal from a temporal view of the
 database.
 
 OK, I think I see what you're saying now. Basically, an audit log
 means different things to different people, so I think it confused
 the issue.
 
Probably.  When I think of an audit log, I tend to think of viewing
who did what when, without that necessarily caring a lot about
viewing interim visible database states.
 
 But temporal is fairly vague, too.
 
Yeah, but in this context I have taken it to mean that someone wants
to run a query such that it sees the database state as of some
previous point in time.  Even with a read-only transaction, if you
want to avoid seeing states of the database which are inconsistent
with business rules enforced through serializable transactions, you
need to deal with some tricky problems.
 
 I am most interested in the topic you brought up about
 serializability and system time (transaction time), because it
 would be a fundamental piece upon which we can build a lot of
 these other things (including what could be called an audit log).
 
[brain dump follows -- remember, you *said* you were interested]
 
If you think it matters for what you are calling an audit log, then
I probably have an incomplete or inaccurate understanding of what
you mean by audit log.  Perhaps you could sketch that out a bit? 
(Or point back to where it was described, if I've missed or
forgotten something that went before.)
 
The reason it's tricky is that while SSI fully complies with the
requirement that the behavior of a set of concurrent serializable
transactions running in a database is consistent with some serial
(one-at-a-time) execution of those transactions, it does not share
certain properties with other types of serializable implementations,
so people may be assuming those additional properties where they
don't actually exist.
 
The two most common alternatives to SSI are S2PL and OCC.  Under
both of these techniques, the apparent order of execution (the order
in which the transactions could have run to produce the same results
as if they were run one-at-a-time) is the commit order.  In S2PL
this is accomplished by having reads block writes until commit time
and writes block everything until commit time.  In OCC this is
accomplished by checking the read set of a transaction at commit
time and rolling back the transaction if there is a single write by
another transaction which conflicts with the predicate locks of the
read set (i.e., there is a single read-write conflict out from the
transaction being committed).
 
SSI dodges the blocking and the high rollback rate, but the
technique has these characteristics which may be surprising:
 
 - The apparent order of execution is not always the commit order. 
If two transactions are concurrent, and T1 reads something which
would look different if it could see the work of T2 (but it *can't*
because the transactions are concurrent), then T1 *appears* to have
executed before T2.  T2 might actually *start* first and *commit*
first, but if there was overlap and the rw-conflict, then T1 ran
first *logically*.  SSI prevents cycles in this ordering, by
canceling a transaction when a possible cycle is detected.
 
 - A read-only transactions can cause an anomaly where there would
otherwise not be one.  This is because a transaction which
appeared to commit after another transaction based on rw-conflicts
may have actually committed first, and would be visible to the
read-only transaction while the work of the earlier transaction
would not show up for it; if no transaction observes that state,
then the problem goes away when the logically earlier transaction
later commits.  If the state is observed, even by a read-only
transaction, then the earlier transaction logically can't have
happened -- so it must be rolled back with a serialization failure.
Within one database, this is tracked and handled by SSI.  My
concern is that the transactions might both commit, then a time
traveler goes back and sees the state that never happened.

One of the features added with SSI was DEFERRABLE transactions.  The
point of this is that when a snapshot is generated, it can often be
determined (either immediately or 

[HACKERS] size of .po changesets

2012-08-23 Thread Alvaro Herrera
Excerpts from Tom Lane's message of jue ago 23 11:01:05 -0400 2012:

 
 $ git diff --shortstat REL9_0_9 REL9_1_5
  3186 files changed, 314847 insertions(+), 210452 deletions(-)
 $ git diff --shortstat REL9_1_5 REL9_2_BETA4
  2037 files changed, 290919 insertions(+), 189487 deletions(-)
 
 However, when you look at things a bit closer, these numbers are
 misleading because they include the .po files, which seem to have huge
 inter-branch churn --- well in excess of 10 lines changed per
 release, at least in git's simpleminded view.

Yeah, IMHO .po files are handled pretty badly by SCMs.  I wonder if we
could reduce the amount of git churn caused by those files by simply
removing all comment lines from these files as they are exported from
pgtranslation into postgresql.git?  Since they are not source for
postgresql.git anyway, the other one being the canonical repository,
there doesn't seem to be any point to those lines ... or am I mistaken?

-- 
Álvaro Herrerahttp://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] size of .po changesets

2012-08-23 Thread Roger Leigh
On Thu, Aug 23, 2012 at 11:21:35AM -0400, Alvaro Herrera wrote:
 Excerpts from Tom Lane's message of jue ago 23 11:01:05 -0400 2012:
 
  
  $ git diff --shortstat REL9_0_9 REL9_1_5
   3186 files changed, 314847 insertions(+), 210452 deletions(-)
  $ git diff --shortstat REL9_1_5 REL9_2_BETA4
   2037 files changed, 290919 insertions(+), 189487 deletions(-)
  
  However, when you look at things a bit closer, these numbers are
  misleading because they include the .po files, which seem to have huge
  inter-branch churn --- well in excess of 10 lines changed per
  release, at least in git's simpleminded view.
 
 Yeah, IMHO .po files are handled pretty badly by SCMs.  I wonder if we
 could reduce the amount of git churn caused by those files by simply
 removing all comment lines from these files as they are exported from
 pgtranslation into postgresql.git?  Since they are not source for
 postgresql.git anyway, the other one being the canonical repository,
 there doesn't seem to be any point to those lines ... or am I mistaken?

Have you considered adding --no-location to XGETTEXT_OPTIONS in
po/Makevars?  This stops the massive churn through line renumbering
changes.


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linuxhttp://people.debian.org/~rleigh/
 `. `'   schroot and sbuild  http://alioth.debian.org/projects/buildd-tools
   `-GPG Public Key  F33D 281D 470A B443 6756 147C 07B3 C8BC 4083 E800


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] default_isolation_level='serializable' crashes on Windows

2012-08-23 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes:
 How about we fix the serializable versus HS  Windows bugs in one
 patch, and then look at the other as a separate patch? If that's OK,
 I think this is ready, unless my message text can be improved. (And
 I will have a shot at my first back-patching)

I poked around this area a bit.  I notice that
check_transaction_read_only has got the same fundamental error: it
thinks it can safely consult RecoveryInProgress(), which in general
it cannot.

The particular cases we have discussed so far cannot lead to a crash
there, because in startup scenarios XactReadOnly wouldn't be on already.
However I think that a background process not connected to shared memory
could be crashed if somebody changed transaction_read_only from true to
false in postgresql.conf.  That's sufficiently far-fetched that maybe we
shouldn't worry about it, but still it seems ugly.

On reflection I think maybe the best solution is for
check_transaction_read_only to test IsTransactionState(), and just
allow the change always if outside a transaction.  That would make its
IsSubTransaction test a bit saner/safer as well.  We could do the same
thing in check_XactIsoLevel.  We still need a test in
GetSerializableTransactionSnapshot, or someplace else in the vicinity
of that, to cover the default_transaction_isolation = serializable case;
but if we leave the error check in place in check_XactIsoLevel, you'll
get an immediate rather than delayed error from trying to crank the
level up manually in hot standby, which seems more user-friendly.

Will send an updated patch as soon as I'm done testing this idea.

One other point: I notice that Kevin used ERRCODE_FEATURE_NOT_SUPPORTED
in the error messages he added, which seems sane in isolation.
However, the GUC-based code is (by default) throwing
ERRCODE_INVALID_PARAMETER_VALUE when it rejects due to
RecoveryInProgress.  I'm not totally sure whether that was thought about
or just fell out of not thinking.  Which one do we want here?

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] size of .po changesets

2012-08-23 Thread Tom Lane
Roger Leigh rle...@codelibre.net writes:
 On Thu, Aug 23, 2012 at 11:21:35AM -0400, Alvaro Herrera wrote:
 Yeah, IMHO .po files are handled pretty badly by SCMs.  I wonder if we
 could reduce the amount of git churn caused by those files by simply
 removing all comment lines from these files as they are exported from
 pgtranslation into postgresql.git?

 Have you considered adding --no-location to XGETTEXT_OPTIONS in
 po/Makevars?  This stops the massive churn through line renumbering
 changes.

I think the line numbers are actually useful to the translators --- at
least, the theory behind having them is to make it easy to look at the
message in context.  Alvaro's point is that the copies of the .po files
in our SCM are pretty much write-only data, and could be stripped
relative to what the translators work with.

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] 9.2RC1 wraps this Thursday ...

2012-08-23 Thread Andrew Dunstan


On 08/23/2012 12:40 AM, Tom Lane wrote:

I wrote:

... I really can't take responsibility for any of this since
I don't have a Windows development environment.  One of the Windows-
hacking committers needs to pick this issue up.  Anyone?

[ crickets ]

I guess everybody who might take an interest in this is out sailing...

After further reflection I've realized that, while this is a new bug in
9.2, it is not really a regression from 9.1.  The failure only occurs if
pg_ctl is pointed at a configuration-only directory (one that contains
postgresql.conf but is not the real data directory).  But that is a case
that did not work at all in any previous release, so no users will be
relying on it.

Accordingly, I don't think this is a release-blocker, so I'm going to
move it to the non-blocker section of the open-items page.

Anybody who wants to fix it is surely welcome to, but I'm not going
to consider this item as a reason to postpone RC1.






I'm not sure what you want done. I can test Amit's patch in a couple of 
Windows environments (say XP+mingw and W7+MSVC) if that's what you need.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] 9.2RC1 wraps this Thursday ...

2012-08-23 Thread Tom Lane
Andrew Dunstan and...@dunslane.net writes:
 On 08/23/2012 12:40 AM, Tom Lane wrote:
 Anybody who wants to fix it is surely welcome to, but I'm not going
 to consider this item as a reason to postpone RC1.

 I'm not sure what you want done. I can test Amit's patch in a couple of 
 Windows environments (say XP+mingw and W7+MSVC) if that's what you need.

Well, the patch as-is just adds another copy of code that really needs
to be refactored into some new file in src/port/ or some such.  That's
not work I care to do while being unable to test the result ...

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] sha1, sha2 functions into core?

2012-08-23 Thread Bruce Momjian
On Mon, Aug 20, 2012 at 07:08:12PM -0400, Tom Lane wrote:
 The only reason I can see for pushing more crypto into core is
 if we needed to stop using MD5 for the core password authentication
 functionality.  While that might come to pass eventually, I am aware of
 no evidence whatever that SHAn, per se, is an improvement over MD5 for
 password auth purposes.  Moreover, as Josh just mentioned, anybody who
 thinks it might be insufficiently secure for their purposes has got
 plenty of alternatives available today (SSL certificates, PAM backed
 by whatever-you-want, etc).
 
 TBH, I think if we do anything at all about this in the near future,
 it'll be window dressing to shut up the people who heard once that MD5
 was insecure and know nothing about it beyond that --- but if Postgres
 uses MD5 for passwords, it must be insecure.  So I tend to agree with
 Andrew that we should wait till the NIST competition dust settles; but
 what I'll be looking for afterwards is which algorithm has the most
 street cred with the average slashdotter.
 
 Also, as I mentioned upthread, we need to do more than just drop in
 a new hashing algorithm.  MD5 is far from being the weakest link
 in what we're doing today.

If anyone believe Tom is inaccurate in MD5 is far from being the
weakest link, see this 2004 email from Greg Stark explaining the odds
of salt reuse and password packet replay:

http://archives.postgresql.org/pgsql-hackers/2004-08/msg01540.php

-- 
  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] 9.2RC1 wraps this Thursday ...

2012-08-23 Thread Andrew Dunstan


On 08/23/2012 01:58 PM, Tom Lane wrote:

Andrew Dunstan and...@dunslane.net writes:

On 08/23/2012 12:40 AM, Tom Lane wrote:

Anybody who wants to fix it is surely welcome to, but I'm not going
to consider this item as a reason to postpone RC1.

I'm not sure what you want done. I can test Amit's patch in a couple of
Windows environments (say XP+mingw and W7+MSVC) if that's what you need.

Well, the patch as-is just adds another copy of code that really needs
to be refactored into some new file in src/port/ or some such.  That's
not work I care to do while being unable to test the result ...





OK, I'll see if I can carve out a bit of time.

cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] default_isolation_level='serializable' crashes on Windows

2012-08-23 Thread Tom Lane
I wrote:
 I poked around this area a bit.  I notice that
 check_transaction_read_only has got the same fundamental error: it
 thinks it can safely consult RecoveryInProgress(), which in general
 it cannot.

After rereading the whole thread I saw that Heikki had already pointed
this out, and come to the same conclusion about how to fix it:

 On reflection I think maybe the best solution is for
 check_transaction_read_only to test IsTransactionState(), and just
 allow the change always if outside a transaction.

Attached is a version of the patch that does it like that.  I've checked
that this fixes the problem (as well as Robert's earlier report) in an
EXEC_BACKEND build, which is as close as I can get to the Windows
environment.

I tweaked Kevin's error message to keep the same capitalization as the
existing text for the message in check_XactIsoLevel --- if we change
that it will cause work for the translators, and I don't think it's
enough of an improvement to justify that.

I also back-propagated the use of ERRCODE_FEATURE_NOT_SUPPORTED into
the GUC check hooks.  On reflection this seems more appropriate than
ERRCODE_INVALID_PARAMETER_VALUE.

Lastly, I simplified the added code in InitPostgres down to just a
bare assignment to XactIsoLevel.  It doesn't seem worthwhile to add
all the cycles involved in SetConfigOption(), when we have no desire
to change the GUC permanently.  (I think Kevin's code was wrong anyway
in that it was using PGC_S_OVERRIDE, which would impact the reset
state for the GUC.)

I think this is ready to go.  Kevin, do you want to apply it?  You
had mentioned wanting some practice with back-patches.

regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 0e9eb3a64f5db66dd2fca68252a597e68defdbf8..5d894ba77f74d02637008deedd4ca2919d8af6b8 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
*** show_log_timezone(void)
*** 533,543 
   * read-only may be changed to read-write only when in a top-level transaction
   * that has not yet taken an initial snapshot.	Can't do it in a hot standby
   * slave, either.
   */
  bool
  check_transaction_read_only(bool *newval, void **extra, GucSource source)
  {
! 	if (*newval == false  XactReadOnly)
  	{
  		/* Can't go to r/w mode inside a r/o transaction */
  		if (IsSubTransaction())
--- 533,548 
   * read-only may be changed to read-write only when in a top-level transaction
   * that has not yet taken an initial snapshot.	Can't do it in a hot standby
   * slave, either.
+  *
+  * If we are not in a transaction at all, just allow the change; it means
+  * nothing since XactReadOnly will be reset by the next StartTransaction().
+  * The IsTransactionState() test protects us against trying to check
+  * RecoveryInProgress() in contexts where shared memory is not accessible.
   */
  bool
  check_transaction_read_only(bool *newval, void **extra, GucSource source)
  {
! 	if (*newval == false  XactReadOnly  IsTransactionState())
  	{
  		/* Can't go to r/w mode inside a r/o transaction */
  		if (IsSubTransaction())
*** check_transaction_read_only(bool *newval
*** 556,561 
--- 561,567 
  		/* Can't go to r/w mode while recovery is still active */
  		if (RecoveryInProgress())
  		{
+ 			GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
  			GUC_check_errmsg(cannot set transaction read-write mode during recovery);
  			return false;
  		}
*** check_transaction_read_only(bool *newval
*** 569,574 
--- 575,582 
   *
   * We allow idempotent changes at any time, but otherwise this can only be
   * changed in a toplevel transaction that has not yet taken a snapshot.
+  *
+  * As in check_transaction_read_only, allow it if not inside a transaction.
   */
  bool
  check_XactIsoLevel(char **newval, void **extra, GucSource source)
*** check_XactIsoLevel(char **newval, void *
*** 598,604 
  	else
  		return false;
  
! 	if (newXactIsoLevel != XactIsoLevel)
  	{
  		if (FirstSnapshotSet)
  		{
--- 606,612 
  	else
  		return false;
  
! 	if (newXactIsoLevel != XactIsoLevel  IsTransactionState())
  	{
  		if (FirstSnapshotSet)
  		{
*** check_XactIsoLevel(char **newval, void *
*** 616,621 
--- 624,630 
  		/* Can't go to serializable mode while recovery is still active */
  		if (newXactIsoLevel == XACT_SERIALIZABLE  RecoveryInProgress())
  		{
+ 			GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
  			GUC_check_errmsg(cannot use serializable mode in a hot standby);
  			GUC_check_errhint(You can use REPEATABLE READ instead.);
  			return false;
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index b22faf9607d87f1a2bc7ce306e923dc12b8cfcc7..8b5a95ceaa0ff460d34231dadc2d3562fa7b0c63 100644
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
*** GetSerializableTransactionSnapshot(Snaps

Re: [HACKERS] size of .po changesets

2012-08-23 Thread Alvaro Herrera
Excerpts from Tom Lane's message of jue ago 23 13:33:46 -0400 2012:
 Roger Leigh rle...@codelibre.net writes:
  On Thu, Aug 23, 2012 at 11:21:35AM -0400, Alvaro Herrera wrote:
  Yeah, IMHO .po files are handled pretty badly by SCMs.  I wonder if we
  could reduce the amount of git churn caused by those files by simply
  removing all comment lines from these files as they are exported from
  pgtranslation into postgresql.git?
 
  Have you considered adding --no-location to XGETTEXT_OPTIONS in
  po/Makevars?  This stops the massive churn through line renumbering
  changes.
 
 I think the line numbers are actually useful to the translators --- at
 least, the theory behind having them is to make it easy to look at the
 message in context.

Yeah, and I, for one, do use them quite a bit to look up the code
context when the message is unclear.

 Alvaro's point is that the copies of the .po files
 in our SCM are pretty much write-only data, and could be stripped
 relative to what the translators work with.

Right.

-- 
Álvaro Herrerahttp://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] Large number of open(2) calls with bulk INSERT into empty table

2012-08-23 Thread Bruce Momjian
On Tue, Aug 21, 2012 at 09:52:02AM -0400, Robert Haas wrote:
 On Mon, Aug 20, 2012 at 6:44 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Robert Haas robertmh...@gmail.com writes:
  On Mon, Aug 20, 2012 at 4:27 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Surely we could just prevent creation of the FSM until the table has
  reached at least, say, 10 blocks.
 
  Any threshold beyond one block would mean potential space wastage,
  but it's hard to get excited about that until you're into the dozens
  of pages.
 
  I dunno, I think one-row tables are pretty common.
 
  Sure, and for that you don't need an FSM, because any row allocation
  attempt will default to trying the last existing block before it extends
  (see RelationGetBufferForTuple).  It's only once you've got more than
  one block in the table that it becomes interesting.
 
  If we had a convention that FSM is only created for rels of more than
  N blocks, perhaps it'd be worthwhile to teach RelationGetBufferForTuple
  to try all existing blocks when relation size = N.  Or equivalently,
  hack the FSM code to return all N pages when it has no info.
 
 Now that's an idea I could get behind.  I'd pick a smaller value of N
 than what you suggested (10), perhaps 5.  But I like it otherwise.

TODO added:

Avoid creation of the free space map for small tables

http://archives.postgresql.org/pgsql-hackers/2011-11/msg01751.php
http://archives.postgresql.org/pgsql-hackers/2012-08/msg00552.php 

-- 
  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] TODO

2012-08-23 Thread Bruce Momjian
On Tue, Aug 21, 2012 at 12:16:11PM +0900, Tatsuo Ishii wrote:
 I found this in https://wiki.postgresql.org/wiki/Todo :
 
   Improve ability to display optimizer analysis using OPTIMIZER_DEBUG 
 
 What does this actually mean?
 
 Add GUC switch to enable optimizer debug on/off?
 More fancy/useful info should be printed?
 If so, what kind of information is required?

Well, right now, OPTIMIZER_DEBUG lets you see what plans were considered
and removed.  I was thinking that information should be available
without a special compiled binary.

-- 
  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] default_isolation_level='serializable' crashes on Windows

2012-08-23 Thread Kevin Grittner
Tom Lane t...@sss.pgh.pa.us wrote:
 
 I tweaked Kevin's error message to keep the same capitalization as
 the existing text for the message in check_XactIsoLevel --- if we
 change that it will cause work for the translators, and I don't
 think it's enough of an improvement to justify that.
 
That's one of the reasons I agonized over it -- I think the way I
left it is a little better and more consistent with other messages,
but didn't know whether the difference was worth translator effort. 
I'm happy to trust your judgment on that.
 
 Lastly, I simplified the added code in InitPostgres down to just a
 bare assignment to XactIsoLevel.  It doesn't seem worthwhile to
 add all the cycles involved in SetConfigOption(), when we have no
 desire to change the GUC permanently.  (I think Kevin's code was
 wrong anyway in that it was using PGC_S_OVERRIDE, which would
 impact the reset state for the GUC.)
 
Point taken on PGC_S_OVERRIDE.  And that probably fixes the issue
that caused me to hold up when I was about ready to pull the trigger
this past weekend.  A final round of testing showed a SET line on
psql start, which is clearly wrong.  I suspected that I needed to go
to a lower level in setting that, but hadn't had a chance to sort
out just what the right path was.  In retrospect, just directly
assigning the value seems pretty obvious.
 
 I think this is ready to go.
 
With your changes, I agree.
 
 Kevin, do you want to apply it?  You had mentioned wanting some
 practice with back-patches.
 
I'm getting on a plane to Istanbul in less than 48 hours for the
VLDB conference, and scrambling to tie up loose ends.  I don't want
to be under that kind of time-pressure when I back-patch for the
first time, for fear of making a mess of things and not being around
to clean up the mess; so my first back-patch is probably best left
for another time.
 
I'll run through my tests again tonight, against your patch, not
that I expect any problems with it.  Unfortunately I can't test
Windows, as I don't have a build environment for that.
 
Thanks for going over this.
 
-Kevin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] default_isolation_level='serializable' crashes on Windows

2012-08-23 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes:
 I'll run through my tests again tonight, against your patch, not
 that I expect any problems with it.  Unfortunately I can't test
 Windows, as I don't have a build environment for that.

FWIW, you can approximate Windows close enough for this type of problem
by building with EXEC_BACKEND defined.  I usually add the #define to
pg_config.h after configuring, though of course there's more than one
way to do it.

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


[HACKERS] pg_upgrade's exec_prog() coding improvement

2012-08-23 Thread Alvaro Herrera
Hi,

I've been bitten twice by exec_prog()s API, so here's a patch to try to
make it a bit harder to misuse.

There are two main changes here; one is to put the logfile option as the
first argument; then comes a bool, then the format string.  This means
you get a warning if you pass the wrong number of arguments before the
format; previously I mis-merged in the KEY SHARE patch so that I was
passing the log file as format, and the compiler didn't complain at all.

The other interesting change I did was move the responsibility of adding
SYSTEMQUOTE and the  %s 21 redirections to exec_prog itself,
reducing clutter in the caller.  This makes the callers a lot easier to
read.

One other minor change I did was split it in two versions: a simpler one
with less frammishes, that doesn't let you supply an alternative log
file and doesn't return a status value.  This is used for all but one of
the callers; only that one caller was interested in the result value
anyway.  And that caller is also the only one that passes an
opt_log_file other than NULL, so I removed that from the simple version.

Lastly, I removed the is_priv boolean, which seems pretty pointless;
just made all calls set the umask inconditionally.  The only caller
doing this was the cp/xcopy call, and I don't see any reason for this to
be different from other callers.

This makes pg_upgrade a bit more readable.

If anyone can test this on Windows I would be appreciate it.

One problem with this is that I get this warning:

/pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’:
/pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be 
possible candidate for ‘gnu_printf’ format attribute 
[-Wmissing-format-attribute]
/pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be 
possible candidate for ‘gnu_printf’ format attribute 
[-Wmissing-format-attribute]

I have no idea how to silence that.  Ideas?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


upgrade_execprog.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] [WIP] Performance Improvement by reducing WAL for Update Operation

2012-08-23 Thread Bruce Momjian
On Wed, Aug 22, 2012 at 07:38:33PM +0530, Amit Kapila wrote:
 I had made sure no full_page_write happens by making checkpoint interval and
 checkpoints segments  longer.
 
  
 
 Original code - 1.8GModified code - 1.1G  Diff - 63% reduction, incase of
 fill factor 100.
 Original code - 1.6GModified code - 1.1G  Diff - 45% reduction, incase of
 fill factor 80.
 
  
 
 I am still in process of collecting synchronous commit mode on data.

Wow, that sounds promising.

-- 
  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] Is this an appropriate item?

2012-08-23 Thread Bruce Momjian
On Thu, Aug 23, 2012 at 11:46:38AM +0900, Tatsuo Ishii wrote:
  On Wed, Aug 22, 2012 at 9:32 PM, Tatsuo Ishii is...@postgresql.org wrote:
  Hi,
 
  I found following item in the Developer FAQ.
  I don't see why this is related to developers.
  
   Why aren't there more compression options when dumping tables?
 
  
  it looks more like a TODO, or we think we are not interested on this?
  if the latter, then it probably is part of the things we don't want
  of the dev faq
  
  btw, the previous item is this:
  Why don't you use threads, raw devices, async-I/O, insert your
  favorite wizz-bang feature here?
  
  what's the difference about the async-I/O mentioned here and
  synchronous_commit=off, if there is none maybe we should remove that
  part
 
 BTW, this:
 
   Why aren't there more compression options when dumping tables?
 
 is a subsection of this:
  Why don't you use threads, raw devices, async-I/O, insert your favorite 
  wizz-bang feature here?
 
 I don't see any relationship between former and latter.

Compression item removed from English FAQ.

-- 
  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] Why does analyze_new_cluster.sh use sleep?

2012-08-23 Thread Bruce Momjian
On Thu, Aug 23, 2012 at 02:17:44AM -0400, Peter Eisentraut wrote:
 The script analyze_new_cluster.sh output by pg_upgrade contains several
 sleep calls (see contrib/pg_upgrade/check.c).  What is the point of
 this?  If the purpose of this script is to get the database operational
 again as soon as possible, waiting a few seconds doing nothing surely
 isn't helping.
 
 I could maybe see the point of waiting a bit between the different
 vacuumdb calls, to catch some breath, but the one before the first call
 to vacuumdb is highly dubious to me.

The sleep is there so the user can read the status message, in case it
scrolls off the screen once the next stage starts.

-- 
  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] 9.2RC1 wraps this Thursday ...

2012-08-23 Thread Andrew Dunstan


On 08/23/2012 02:44 PM, Andrew Dunstan wrote:


On 08/23/2012 01:58 PM, Tom Lane wrote:

Andrew Dunstan and...@dunslane.net writes:

On 08/23/2012 12:40 AM, Tom Lane wrote:

Anybody who wants to fix it is surely welcome to, but I'm not going
to consider this item as a reason to postpone RC1.

I'm not sure what you want done. I can test Amit's patch in a couple of
Windows environments (say XP+mingw and W7+MSVC) if that's what you 
need.

Well, the patch as-is just adds another copy of code that really needs
to be refactored into some new file in src/port/ or some such. That's
not work I care to do while being unable to test the result ...





OK, I'll see if I can carve out a bit of time.




I have spent a couple of hours on this, and I'm sufficiently nervous 
about it that I'm not going to do anything in a hurry. I will see what 
can be done over the weekend, possibly, but no promises.


TBH I'd rather stick with the less invasive approach of the original 
patch at this stage, and see about refactoring for 9.3.


cheers

andrew




--
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] TODO

2012-08-23 Thread Josh Berkus

 Well, right now, OPTIMIZER_DEBUG lets you see what plans were considered
 and removed.  I was thinking that information should be available
 without a special compiled binary.

+1.  It would also be popular with our academic users.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] TODO

2012-08-23 Thread Peter Geoghegan
On 23 August 2012 20:21, Bruce Momjian br...@momjian.us wrote:
 On Tue, Aug 21, 2012 at 12:16:11PM +0900, Tatsuo Ishii wrote:
 I found this in https://wiki.postgresql.org/wiki/Todo :

   Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

 What does this actually mean?

 Add GUC switch to enable optimizer debug on/off?
 More fancy/useful info should be printed?
 If so, what kind of information is required?

 Well, right now, OPTIMIZER_DEBUG lets you see what plans were considered
 and removed.  I was thinking that information should be available
 without a special compiled binary.

That seems like an unreasonable burden on what OPTIMIZER_DEBUG can do.
If you can't build Postgres from source, something that is relatively
low barrier, you have little hope of sensibly interpreting low-level
output from the planner. However, I could get behind having it as a
configure option, which it is currently not. It would probably also
make sense to expose SELECTIVITY_DEBUG and GEQO_DEBUG.

-- 
Peter Geoghegan   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Recently noticed documentation issues

2012-08-23 Thread Craig Ringer

Hi all

I've recently noticed two oversights in the docs that I'd like to fix.


First, in sql-fetch, there's no hint that the cursor name can be the 
quoted value of a refcursor, eg:


FETCH ALL FROM unnamed portal 1;

This *is* shown in an example in plpgsql-cursors, but only in some 
sample code. If you set out with the question how do I fetch the 
contents of a refcursor returned from a function it's a lot harder to 
find the results than it could be.


I'd like to add a short discussion of refcursors and an example to 
sql-fetch, and refer to that from plpgsql-cursors to make it clearer how 
you work with plpgsql cursors from SQL.




Second, in functions-datetime, I wasn't able to find any mention of the 
behaviour of CASTing a DATE to a TIMESTAMP or to a TIMESTAMPTZ. I'd like 
to explicitly state that casting date to timestamp or timestamptz 
produces a date in local time, and show that if you want to cast a date 
to another time zone you can use:


thedate::timestamp AT TIME ZONE 'UTC'

to produce a timestamptz containing that date at midnight UTC.

Opinions?

--
Criaig Ringer


--
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] Outdated Japanse developers FAQ

2012-08-23 Thread Tatsuo Ishii
 On Tue, Aug 21, 2012 at 7:49 AM, Tatsuo Ishii is...@postgresql.org wrote:
 Please let me know if this is not the right place to ask this kind of
 queston.

 PostgreSQL Developers FAQ in Japanese:

 http://wiki.postgresql.org/wiki/Developer_FAQ/ja

 looks pretty outdated. It was last updated on 7 November 2010 (English
 FAQ was last updated on 27 September 2011). Even it says PostgreSQL's
 repository is CVS, not git. Does anybody know who is the mainter for
 this?
 
 Itagaki-san according to the history page:
 http://wiki.postgresql.org/index.php?title=Developer_FAQ/jaaction=history
 
 If there's no particular maintainer for this, I would like to
 volunteer to update the page.
 
 Please feel free to update the page.
 
 Ok, I will do it with my colleagues. BTW, user's FAQ is also outdated
 (last update was 16 May 2010). Unfortunately I don't have time to work
 on it.

Done.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] default_isolation_level='serializable' crashes on Windows

2012-08-23 Thread Kevin Grittner
 Tom Lane  wrote:
 Kevin Grittner  writes:
 
 I'll run through my tests again tonight, against your patch, not
 that I expect any problems with it. Unfortunately I can't test
 Windows, as I don't have a build environment for that.
 
 FWIW, you can approximate Windows close enough for this type of
 problem by building with EXEC_BACKEND defined. I usually add the
 #define to pg_config.h after configuring, though of course there's
 more than one way to do it.
 
It tested out fine both ways.
 
For anybody else wanting to use EXEC_BACKEND for testing -- don't
count on --enable-depend to rebuild everything correctly.  I used
maintainer-clean and it worked.
 
-Kevin


-- 
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] Why does analyze_new_cluster.sh use sleep?

2012-08-23 Thread Peter Eisentraut
On Thu, 2012-08-23 at 17:05 -0400, Bruce Momjian wrote:
 On Thu, Aug 23, 2012 at 02:17:44AM -0400, Peter Eisentraut wrote:
  The script analyze_new_cluster.sh output by pg_upgrade contains several
  sleep calls (see contrib/pg_upgrade/check.c).  What is the point of
  this?  If the purpose of this script is to get the database operational
  again as soon as possible, waiting a few seconds doing nothing surely
  isn't helping.
  
  I could maybe see the point of waiting a bit between the different
  vacuumdb calls, to catch some breath, but the one before the first call
  to vacuumdb is highly dubious to me.
 
 The sleep is there so the user can read the status message, in case it
 scrolls off the screen once the next stage starts.

That seems completely arbitrary and contrary to the point of the script.
The pg_upgrade output already explains what the script is for.  If we
really wanted the user to confirm what is going to happen, we should
wait for a key press or something.  I also don't think that 2 seconds is
enough to read and react to the written text.  Also, by that logic, we
need to put a delay between each database processed by vacuumdb as well.




-- 
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] size of .po changesets

2012-08-23 Thread Peter Eisentraut
On Thu, 2012-08-23 at 11:21 -0400, Alvaro Herrera wrote:
 Yeah, IMHO .po files are handled pretty badly by SCMs.

By SCMs that store diffs internally, perhaps, but Git doesn't, so I
don't think it matters much for storage whether .po files diff well.

 I wonder if we
 could reduce the amount of git churn caused by those files by simply
 removing all comment lines from these files as they are exported from
 pgtranslation into postgresql.git?  Since they are not source for
 postgresql.git anyway, the other one being the canonical repository,
 there doesn't seem to be any point to those lines ... or am I mistaken?

I don't see this being worth the trouble.  It would just make it more
difficult to track where the files are coming from.  There could also be
problems with downstream distributors if we are not shipping files in
source form.



-- 
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] [WIP] Performance Improvement by reducing WAL for Update Operation

2012-08-23 Thread Amit Kapila
From: Bruce Momjian [mailto:br...@momjian.us] 
Sent: Friday, August 24, 2012 2:12 AM
On Wed, Aug 22, 2012 at 07:38:33PM +0530, Amit Kapila wrote:
 I had made sure no full_page_write happens by making checkpoint interval
and
 checkpoints segments  longer.
 
  
 
 Original code - 1.8GModified code - 1.1G  Diff - 63% reduction,
incase of
 fill factor 100.
 Original code - 1.6GModified code - 1.1G  Diff - 45% reduction,
incase of
 fill factor 80.
 
  
 
 I am still in process of collecting synchronous commit mode on data.

 Wow, that sounds promising.
  Thanks you.

Right now I am collecting the data for Synchronous_commit =on mode; My
initial observation is that
incase fsync is off, the results are good(around 50% perf improvement). 
However if fsync is on, the performance results fall down to 3~5%. I am not
sure even if the data for I/O is reduced, 
Still why there is no big performance gain as in case of Synchronous_commit
= off or when fsync is off.

I am trying with different methods of wal_sync_method parameter and by
setting some value of commit_delay as suggested by Peter Geoghegan in one of
his mails.

Please suggest me if anyone has any thoughts on what kind of parameter's are
best for such a use case or let me know if I am missing anything and such
kind of performance improvement can only improve performance for fsync =off
case.

With Regards,
Amit Kapila.



-- 
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] Recently noticed documentation issues

2012-08-23 Thread Amit Kapila
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
Sent: Friday, August 24, 2012 7:17 AM

 I've recently noticed two oversights in the docs that I'd like to fix.


 First, in sql-fetch, there's no hint that the cursor name can be the 
 quoted value of a refcursor, eg:

 FETCH ALL FROM unnamed portal 1;

 This *is* shown in an example in plpgsql-cursors, but only in some 
 sample code. If you set out with the question how do I fetch the 
 contents of a refcursor returned from a function it's a lot harder to 
 find the results than it could be.

Isn't what you are telling explained in Returning Cursors section on below
link:
http://www.postgresql.org/docs/9.1/static/plpgsql-cursors.html



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] plperl sigfpe reset can crash the server

2012-08-23 Thread Andres Freund
On Thursday, August 23, 2012 12:17:22 PM Andres Freund wrote:
 Hi,
 
 While debugging an instance of this bug I noticed that plperlu always
 removes the SIGFPE handler and sets it to ignore:
 
 
 andres@awork2:~$ psql -p 5435 -U postgres -h /var/run/postgresql test
 Timing is on.
 psql (9.1devel, server 9.1.5)
 Type help for help.
 
 test=# SELECT pg_backend_pid();
  pg_backend_pid
 
9287
 
 root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/9287/status|awk
 '{print $2}'
 01301800
 000180006287
 
 test=# DO LANGUAGE plperlu ;
 
 root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/9287/status|awk
 '{print $2}'
 01301880
 000180006207
 
 Note the 8'th bit being unset in SigCgt and set in SigIgn. Thats SIGFPE...
 
 Not sure how relevant this really is, but it could cause errors to be
 ignored...

In fact it can be used to crash the server:
test=# SELECT (-2^31)::int/-1;
ERROR:  floating-point exception
DETAIL:  An invalid floating-point operation was signaled. This probably means 
an out-of-range result or an invalid operation, such as division by zero.
test=# DO LANGUAGE plperl ;
DO
Time: 172.235 ms
test=# SELECT (-2^31)::int/-1;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Greetings,

Andres
-- 
 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] plperl sigfpe reset can crash the server

2012-08-23 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On Thursday, August 23, 2012 12:17:22 PM Andres Freund wrote:
 While debugging an instance of this bug I noticed that plperlu always
 removes the SIGFPE handler and sets it to ignore:

 In fact it can be used to crash the server:

Um ... how exactly can that happen, if the signal is now ignored?

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] plperl sigfpe reset can crash the server

2012-08-23 Thread Andres Freund
On Friday, August 24, 2012 06:55:04 AM Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On Thursday, August 23, 2012 12:17:22 PM Andres Freund wrote:
  While debugging an instance of this bug I noticed that plperlu always
  
  removes the SIGFPE handler and sets it to ignore:
  In fact it can be used to crash the server:
 Um ... how exactly can that happen, if the signal is now ignored?

Don't ask me the hard questions at 7 in the morning. I have no clue yet.

I don't see where but something resets SIGFPE before the server crashes. If I 
catch the sigfpe with gdb I see:

test=# SELECT pg_backend_pid();
 pg_backend_pid 

  18084

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/18084/status
SigIgn: 01301800
SigCgt: 000180006287

test=# SELECT (-2^31)::int/-1;
ERROR:  floating-point exception
DETAIL:  An invalid floating-point operation was signaled. This probably means 
an out-of-range result or an invalid operation, such as division by zero.

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/18084/status
SigIgn: 01301800
SigCgt: 000180006287

test=# DO LANGUAGE plperl ;

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/18084/status
SigIgn: 01301880
SigCgt: 000180006207

test=# SELECT (-2^31)::int/-1;

Program received signal SIGFPE, Arithmetic exception.
0x7f858001f8c6 in int4div (fcinfo=0x7f8581b30320)

root@awork2:/home/andres# grep -E '^Sig(Cgt|Ign)' /proc/18084/status
SigIgn: 01301800
SigCgt: 000180006207

Andres
-- 
 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] plperl sigfpe reset can crash the server

2012-08-23 Thread Andres Freund
On Friday, August 24, 2012 06:55:04 AM Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On Thursday, August 23, 2012 12:17:22 PM Andres Freund wrote:
  While debugging an instance of this bug I noticed that plperlu always
  
  removes the SIGFPE handler and sets it to ignore:
  In fact it can be used to crash the server:
 Um ... how exactly can that happen, if the signal is now ignored?
My man 2 signal tells me:
According  to POSIX, the behavior of a process is undefined after it ignores 
a SIGFPE, SIGILL, or SIGSEGV signal that was not generated by kill(2) or 
raise(3).

Killing the process is a kind of undefined behaviour ;)

Andres

-- 
 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] plperl sigfpe reset can crash the server

2012-08-23 Thread Andres Freund
On Friday, August 24, 2012 07:19:42 AM Andres Freund wrote:
 On Friday, August 24, 2012 06:55:04 AM Tom Lane wrote:
  Andres Freund and...@2ndquadrant.com writes:
   On Thursday, August 23, 2012 12:17:22 PM Andres Freund wrote:
   While debugging an instance of this bug I noticed that plperlu always
   
   removes the SIGFPE handler and sets it to ignore:
   In fact it can be used to crash the server:
  Um ... how exactly can that happen, if the signal is now ignored?
 
 My man 2 signal tells me:
 According  to POSIX, the behavior of a process is undefined after it
 ignores a SIGFPE, SIGILL, or SIGSEGV signal that was not generated by
 kill(2) or raise(3).
 
 Killing the process is a kind of undefined behaviour ;)
And its done explicitly in linux:

In 

./arch/x86/kernel/traps.c:
void math_error(struct pt_regs *regs, int error_code, int trapnr)
{
...
force_sig_info(SIGFPE, info, task);
}

and

./kernel/signal.c:
 * Force a signal that the process can't ignore: if necessary
 * we unblock the signal and change any SIG_IGN to SIG_DFL.
 *
 * Note: If we unblock the signal, we always reset it to SIG_DFL,
 * since we do not want to have a signal handler that was blocked
 * be invoked when user space had explicitly blocked it.
 *
 * We don't want to have recursive SIGSEGV's etc, for example,
 * that is why we also clear SIGNAL_UNKILLABLE.
 */
int
force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
...

Absolutely obvious. Imo sigaction should simply return -1 and set errno to 
EINVAL if somebody sets SIGFPE to SIG_IGN then...

Andres
-- 
 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] plperl sigfpe reset can crash the server

2012-08-23 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 Um ... how exactly can that happen, if the signal is now ignored?

 My man 2 signal tells me:
 According  to POSIX, the behavior of a process is undefined after it ignores
 a SIGFPE, SIGILL, or SIGSEGV signal that was not generated by kill(2) or 
 raise(3).

So I guess the real question there is: WTF is perl doing setting the
handling to SIG_IGN?

Even if you grant the proposition that perl knows what it's doing in
terms of its internal behavior, which given the above seems doubtful,
it has no business overriding a host application's signal settings
like that.

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] plperl sigfpe reset can crash the server

2012-08-23 Thread Andres Freund
On Friday, August 24, 2012 07:33:01 AM Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  Um ... how exactly can that happen, if the signal is now ignored?
  
  My man 2 signal tells me:
  According  to POSIX, the behavior of a process is undefined after it
  ignores a SIGFPE, SIGILL, or SIGSEGV signal that was not generated by
  kill(2) or raise(3).
 
 So I guess the real question there is: WTF is perl doing setting the
 handling to SIG_IGN?
 
 Even if you grant the proposition that perl knows what it's doing in
 terms of its internal behavior, which given the above seems doubtful,
 it has no business overriding a host application's signal settings
 like that.

./pod/perl581delta.pod:
At startup Perl blocks the SIGFPE signal away since there isn't much
Perl can do about it.  Previously this blocking was in effect also for
programs executed from within Perl.  Now Perl restores the original
SIGFPE handling routine, whatever it was, before running external
programs.

perl.h also has some tidbits:

/*
 * initialise to avoid floating-point exceptions from overflow, etc
 */
#ifndef PERL_FPU_INIT
#  ifdef HAS_FPSETMASK
#if HAS_FLOATINGPOINT_H
#  include floatingpoint.h
#endif
/* Some operating systems have this as a macro, which in turn expands to a 
comma
   expression, and the last sub-expression is something that gets calculated,
   and then they have the gall to warn that a value computed is not used. 
Hence
   cast to void.  */
#define PERL_FPU_INIT (void)fpsetmask(0)
#  else
#if defined(SIGFPE)  defined(SIG_IGN)  !defined(PERL_MICRO)
#  define PERL_FPU_INIT   PL_sigfpe_saved = (Sighandler_t) 
signal(SIGFPE, SIG_IGN)
#  define PERL_FPU_PRE_EXEC   { Sigsave_t xfpe; rsignal_save(SIGFPE, 
PL_sigfpe_saved, xfpe);
#  define PERL_FPU_POST_EXECrsignal_restore(SIGFPE, xfpe); }
#else
#  define PERL_FPU_INIT

#endif
#  endif
#endif
#ifndef PERL_FPU_PRE_EXEC
#  define PERL_FPU_PRE_EXEC   {
#  define PERL_FPU_POST_EXEC  }
#endif

That doesn't sound very well reasoned and especially not very well tested to 
me.

Andres
-- 
 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