Re: [HACKERS] Add FET to Default and Europe.txt

2012-10-08 Thread Simon Riggs
On 6 October 2012 22:40, Tom Lane t...@sss.pgh.pa.us wrote:

 Thus for example MSK apparently now means GMT+4 not GMT+3.  We can
 change the tznames entry for that, but should we get rid of MSD
 entirely?  Some input from the Russians on this list would be helpful.
...
 Comments?

It shouldn't be our job to make decisions like this on behalf of the world.

The TZ database clearly changes over time, so doing this accurately
would mean we'd need to hold start and stop dates for each code so it
can be used appropriately with specific times. Which would mean
holding data in much finer detail that the source data, which is a
little bizarre.

* We should deprecate all use of timezone abbreviations in our
internal code, if any.

* Ship only the current tz file, as shipped by IANA with as few prep
steps as possible

* Make the tz file configurable, so people can be more explicit about
what *they* mean by certain codes, to avoid the need for choosing
between countries. For example, someone may have hardcoded particular
codes with the understanding they relate to one particular TZ - if we
make changes, we will alter the application logic, so that must be
able to be put back for backwards compatibility.

-- 
 Simon Riggs   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] Rethinking placement of latch self-pipe initialization

2012-10-08 Thread Simon Riggs
On 7 October 2012 18:27, Tom Lane t...@sss.pgh.pa.us wrote:
 Sean Chittenden recently reported that 9.2 can crash after logging
 FATAL: pipe() failed if the kernel is short of file descriptors:
 http://archives.postgresql.org/pgsql-general/2012-10/msg00202.php

 The only match to that error text is in initSelfPipe().  What I believe
 is happening is that InitProcess is calling OwnLatch which calls
 initSelfPipe, and the latter fails, and then the postmaster thinks that
 was a backend crash because we have armed the dead-man switch but not
 set up on_shmem_exit(ProcKill) which would disarm it.

 It's possible we could fix this by changing the order of operations
 in InitProcess and OwnLatch, but it'd be tricky, not least because
 ProcKill calls DisownLatch which asserts that OwnLatch was done.

 What I think would be a better idea is to fix things so that OwnLatch
 cannot fail except as a result of internal logic errors, by splitting
 out the acquisition of the self-pipe into a separate function named say
 InitializeLatchSupport.  The question then becomes where to put the
 InitializeLatchSupport calls.  My first thought is to put them near the
 signal-setup stanzas for the various processes (ie, the pqsignal calls)
 similarly to what we did recently for initialization of timeout support.
 However there might be a better idea.

 Comments?

We still have to consider how Postgres would operate without the
latches. I don't see that it can, so a shutdown seems appropriate. Is
the purpose of this just to allow a cleaner and more informative
shutdown? Or do you think we can avoid?

If we did move the init calls, would that alter things for code that
creates new used defined latches?

-- 
 Simon Riggs   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] Bad Data back Door

2012-10-08 Thread Albe Laurenz
Tom Lane wrote:
 David E. Wheeler da...@justatheory.com writes:
 On Oct 5, 2012, at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Now, having said that, I think it has to be the reponsibility of the
FDW
 to apply any required check ... which makes this a bug report
against
 oracle_fdw, not the core system.  (FWIW, contrib/file_fdw depends on
the
 COPY code, which will check encoding.)

 I agree that this is a bug in oracle_fdw (well, potentially;
ultimately,
 it's Oracle that's lying about the encoding of those text values).
 But I think that it would be much more useful overall -- not
 to mention more database-like -- for PostgreSQL to provide a way to
 enforce it. That is, to consider foreign tables to be an input like
 COPY or SQL, and to validate values before displaying them.

 It is the FDW's responsibility to deal with this.  We expect it to
hand
 back valid tuples; it is not reasonable to disassemble them looking
for
 mistakes (and we couldn't catch most mistakes, anyway).  If the
 interface were defined in terms of text, we could do checking above
the
 FDW level ... but it isn't.

As the author I agree that this is a bug in oracle_fdw.

This was caused by ignorance on my part:  I had assumed that the
type input functions would perform the necessary checks, but it
seems like that is not the case.  I'll look into it.

Oracle does not care much about correct encoding.
If client character set and database character set are the same,
Oracle does not bother to check the data.  This is probably how
WINDOWS-1252 characters slipped into the UTF-8 database in question.
I consider this a bug in Oracle, but never reported it, because
I don't have much hope that Oracle would see it as a problem
given their habitually sloppy handling of encoding issues.

Yours,
Laurenz Albe


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


Re: [HACKERS] Bad Data back Door

2012-10-08 Thread David E. Wheeler
On Oct 8, 2012, at 12:25 AM, Albe Laurenz laurenz.a...@wien.gv.at wrote:

 As the author I agree that this is a bug in oracle_fdw.

Thanks. Should I file a report somewhere?

 This was caused by ignorance on my part:  I had assumed that the
 type input functions would perform the necessary checks, but it
 seems like that is not the case.  I'll look into it.

Thank you!

 Oracle does not care much about correct encoding.
 If client character set and database character set are the same,
 Oracle does not bother to check the data.  This is probably how
 WINDOWS-1252 characters slipped into the UTF-8 database in question.
 I consider this a bug in Oracle, but never reported it, because
 I don't have much hope that Oracle would see it as a problem
 given their habitually sloppy handling of encoding issues.

Yeah, same here. I've been looking into write a function to try to fix 
poorly-encoded data, though, but haven't got far, because CONVERT() does not 
indicate failure. If you have any insight on this, I'd appreciate your thoughts 
on this Stack Overflow question:

  http://stackoverflow.com/q/12717363/79202

Thanks,

David



smime.p7s
Description: S/MIME cryptographic signature


Re: [HACKERS] Add FET to Default and Europe.txt

2012-10-08 Thread Heikki Linnakangas

On 08.10.2012 10:03, Simon Riggs wrote:

On 6 October 2012 22:40, Tom Lanet...@sss.pgh.pa.us  wrote:


Thus for example MSK apparently now means GMT+4 not GMT+3.  We can
change the tznames entry for that, but should we get rid of MSD
entirely?  Some input from the Russians on this list would be helpful.

...

Comments?


It shouldn't be our job to make decisions like this on behalf of the world.


I wish it wasn't, too. But I don't think there's any upstream for this 
information. The tz library has abbreviations for timezones, but they're 
not unique.



* Make the tz file configurable, so people can be more explicit about
what *they* mean by certain codes, to avoid the need for choosing
between countries. For example, someone may have hardcoded particular
codes with the understanding they relate to one particular TZ - if we
make changes, we will alter the application logic, so that must be
able to be put back for backwards compatibility.


It is configurable. See 
http://www.postgresql.org/docs/devel/static/datetime-config-files.html. 
We're just discussing what the defaults should be.


- Heikki


--
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] Improving psql \ds

2012-10-08 Thread Julien Tachoires
2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Julien Tachoires jul...@gmail.com writes:
 About \ds behaviour, I think to add 2 columns :
 - 'LastValue'
 - 'Increment'

 That would make the command a great deal slower, since it would have to
 access each sequence to get that info.  I don't object to adding such
 columns to \ds+, but I don't think it's a good idea to put them in the
 basic command.

Ok.

 The other problem you're going to have here is that there is in fact no
 such command as \ds (nor \ds+); rather, it's a special case of
 \dtsvi.  As such, putting any relkind-specific info into the result is
 problematic.  I think you're going to have to invent a different command
 if you want there to be sequence-specific columns.

Yes, that's why I plan to create a new function listSequences() and
call it from src/bin/psql/command.c :
+   /* \d* commands */
+   else if (cmd[0] == 'd')
+   {
...
+   case 's':
+   success = listSequences(pattern, show_verbose);
+   break;

Cheers,


-- 
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] Add FET to Default and Europe.txt

2012-10-08 Thread Simon Riggs
On 8 October 2012 09:05, Heikki Linnakangas hlinnakan...@vmware.com wrote:

 * Make the tz file configurable, so people can be more explicit about
 what *they* mean by certain codes, to avoid the need for choosing
 between countries. For example, someone may have hardcoded particular
 codes with the understanding they relate to one particular TZ - if we
 make changes, we will alter the application logic, so that must be
 able to be put back for backwards compatibility.


 It is configurable. See
 http://www.postgresql.org/docs/devel/static/datetime-config-files.html.
 We're just discussing what the defaults should be.

The problem there is that the default is Default, so you have no
idea what you are accepting and so are unlikely to be brave enough to
alter it.

If default were named Postgres9 then people would at least
understand that hackers had decided a few things and they might want
to override it.

So I think we need 2 new settings: one called Postgres92, one called
Postgres93. 93 has the new settings and is the default.

Default would no longer map to anything, to make sure we have an
explicit break of compatibility.

-- 
 Simon Riggs   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] Add FET to Default and Europe.txt

2012-10-08 Thread Marc Balmer
Am 08.10.12 11:07, schrieb Simon Riggs:
 On 8 October 2012 09:05, Heikki Linnakangas hlinnakan...@vmware.com wrote:
 
 * Make the tz file configurable, so people can be more explicit about
 what *they* mean by certain codes, to avoid the need for choosing
 between countries. For example, someone may have hardcoded particular
 codes with the understanding they relate to one particular TZ - if we
 make changes, we will alter the application logic, so that must be
 able to be put back for backwards compatibility.


 It is configurable. See
 http://www.postgresql.org/docs/devel/static/datetime-config-files.html.
 We're just discussing what the defaults should be.
 
 The problem there is that the default is Default, so you have no
 idea what you are accepting and so are unlikely to be brave enough to
 alter it.
 
 If default were named Postgres9 then people would at least
 understand that hackers had decided a few things and they might want
 to override it.
 
 So I think we need 2 new settings: one called Postgres92, one called
 Postgres93. 93 has the new settings and is the default.
 
 Default would no longer map to anything, to make sure we have an
 explicit break of compatibility.

Removing the timezone abbreviations from the default settings is
probably the wrong approach.  People use them, I use them, and my
original request to add FET came from the fact that someone wanted to
use it.

As long as we have a type timestamp with timezone, there should also
be a way use and specify timezones in a usual format - by default.

I think the problem we face is more of a maintainer nature than of a
technical nature.  Someone has to maintain this information.  But then
all other projects, mostly BSDs, that I was involved with, managed to
keep this information more or less up to date.

A good starting point would be to take the timezone information directly
from the the files IANA distributes, instead of manually copying and
maintaining them in a separate file.  If no one else does, I can try to
maintain these files.  Those who know about my work, know that I do a
lot of time related software (support for time signal receivers in
OpenBSD, e.g.).

So my vote - if I have one - is to keep the TZ information, but maybe
maintain it better, if needed.

Oh, and as a side note, I did not check how often the TZ database gets
updated in PostgreSQL, if someone actually maintains it, I did not want
to imply you do a bad job ;)




-- 
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] Add FET to Default and Europe.txt

2012-10-08 Thread Simon Riggs
On 8 October 2012 11:14, Marc Balmer m...@msys.ch wrote:

 So I think we need 2 new settings: one called Postgres92, one called
 Postgres93. 93 has the new settings and is the default.

 Default would no longer map to anything, to make sure we have an
 explicit break of compatibility.

 Removing the timezone abbreviations from the default settings is
 probably the wrong approach.  People use them, I use them, and my
 original request to add FET came from the fact that someone wanted to
 use it.

Sorry for any confusion. I've not suggested removing timezone
abbreviations, nor do I wish to do so.

I did suggest that we do not rely upon TZ abbreviations in any code
shipped by PostgreSQL project, but I suspect that is a non-problem
anyway.

-- 
 Simon Riggs   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] more suitable messages for analyze on hot standby.

2012-10-08 Thread Heikki Linnakangas

On 08.10.2012 04:04, Tomonari Katsumata wrote:

I work with streaming replication and hot standby.
And I noticed that error message is not proper when I issue
ANALYZE command to standby.

...



This is not big problem, but error message should report
actual thing. please check it.


Thanks, committed with some stylistic changes.

- Heikki


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


Re: Promoting a standby during base backup (was Re: [HACKERS] Switching timeline over streaming replication)

2012-10-08 Thread Heikki Linnakangas

On 04.10.2012 20:07, Fujii Masao wrote:

On Thu, Oct 4, 2012 at 4:59 PM, Heikki Linnakangas

But I wonder why promoting a standby renders the backup invalid in the first
place? Fujii, Simon, can you explain that?


Simon had the same question and I answered it before.

http://archives.postgresql.org/message-id/cahgqgwfu04oo8yl5sucdjvq3brni7wtfzty9wa2kxtznhic...@mail.gmail.com
---

You say
If the standby is promoted to the master during online backup, the
backup fails.
but no explanation of why?

I could work those things out, but I don't want to have to, plus we
may disagree if I did.


If the backup succeeds in that case, when we start an archive recovery from that
backup, the recovery needs to cross between two timelines. Which means that
we need to set recovery_target_timeline before starting recovery. Whether
recovery_target_timeline needs to be set or not depends on whether the standby
was promoted during taking the backup. Leaving such a decision to a user seems
fragile.


pg_control is backed up last, it would contain the new timeline. No need 
to set recovery_target_timeline.



pg_basebackup -x ensures that all required files are included in the backup and
we can start recovery without restoring any file from the archive. But
if the standby is promoted during the backup, the timeline history
file would become
an essential file for recovery, but it's not included in the backup.


That is true. We could teach it to include the timeline history file, 
though.



The situation may change if your switching-timeline patch has been committed.
It's useful if we can continue the backup even if the standby is promoted.


It wouldn't help with pg_basebackup -x, although it would allow 
streaming replication to fetch the timeline history file.


I guess it's best to keep that restriction for now. But I'll add a TODO 
item for this.


- Heikki


--
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] Libxml2 load error on Windows

2012-10-08 Thread Noah Misch
On Mon, Jun 18, 2012 at 02:08:29PM +0500, Talha Bin Rizwan wrote:
 PostgreSQL 9.2 Windows build is having trouble with the XML support:
 http://postgresql.1045698.n5.nabble.com/9-2-beta1-libxml2-can-t-be-loaded-on-Windows-td5710672.html

 Therefore, I used first approach to get libxml2 version number and include
 the #define HAVE_XMLSTRUCTUREDERRORCONTEXT in pg_config.h if libxml2 version
 is greater than 20703 (which effectively is 2.7.3). Please find attached
 patch libxml2_win_v1.patch.

Tom fixed this differently; effectively, we now use a check like you describe
on all platforms, not just Windows:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=470d0b9789981bc91a8ef2654911d80ab6a6be57

Since that commit makes your patch obsolete, I'm marking it Rejected.  Even
so, thank you for calling attention to the problem and submitting this fix.

nm


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


[HACKERS] PQntuples and PQgetvalue problem.

2012-10-08 Thread zafer yagmuroglu

Hello,

In my C++ program, i want to partition sql table, there is sample code below. 
It is working right. the problem is after func  called 2 or 3 times in func1, 
PQntuples results 0 although tablename exist.  Also when i disabled PQntuples 
at if statement, other psql function PQgetvalue(res) return error that is row 
number 0 is out of range 0..-1there are a couple function call same as func1(to 
check tablename ), at func. when i disabled one of them another at PQntuples 
funcx return 0.  i check so may times. it is so weird.
So i think you can help me.
Best regards.
Zafer,

Sample code : 

func1{
  char cmd[100] = ;
  snprintf(cmd,sizeof(cmd),select tablename from pg_tables where 
tablename='%s',  tablename);

  PGresult *res = PQexec(conn, cmd );

  if (PQresultStatus(res) != PGRES_TUPLES_OK)
   {
 logger.error(PQexec: %s, PQerrorMessage(conn));
 PQclear(res);
 wexit(1);
   }

  if (PQntuples(res) == 0)
//
  } 

  func {
  PGresult *res;
  func1();
  res=PQexec(conn,Select count(*) from persons);
  if (PQresultStatus(res) != PGRES_TUPLES_OK)
  {
  printf(PQexec: %s, PQerrorMessage(conn));
  PQclear(res);
  exit(1);
  }
  if( atoi(PQgetvalue(res,0,0)) != 0)
// some code PQexec()}}   

Re: [HACKERS] Visual Studio 2012 RC

2012-10-08 Thread Noah Misch
On Sun, Oct 07, 2012 at 08:30:22PM +0200, Brar Piening wrote:
 Noah Misch wrote:
 I'm marking this patch Waiting on Author, but the changes needed to  
 get it Ready for Committer are fairly trivial. Thanks, nm 

 Thanks for your review and sorry for my delayed response - I've been on  
 vacation.

 I'll look into adressing your comments and suggestions within the next  
 few days.

Thanks.  I decided to try a 32-bit build, but Solution::DeterminePlatform
detected it as x64.  Its shibboleth is no longer valid; the cl.exe shipping
with VS 2012 Express for Desktop has a /favor option for both architectures:

32clhelp:/favor:blend|ATOM select processor to optimize for, one of:
64clhelp:/favor:blend|AMD64|INTEL64|ATOM select processor to optimize for, 
one of:

Overlaying the first attached change fixed detection for this particular
compiler, but I have not checked compatibility with older versions.  Do you
have VS 2008 and/or VS 2010 handy?  Having worked around that, the build
eventually failed like this:

 Creating library Debug\postgres\postgres.lib and object 
Debug\postgres\postgres.exp
postgres.exp : error LNK2001: unresolved external symbol 
_xmm@41f0 
[c:\cygwin\home\nm\src\pg\postgresql\postgres.vcxproj]
postgres.exp : error LNK2001: unresolved external symbol 
_xmm@80008000 
[c:\cygwin\home\nm\src\pg\postgresql\postgres.vcxproj]
postgres.exp : error LNK2001: unresolved external symbol 
_xmm@8000800080008000 
[c:\cygwin\home\nm\src\pg\postgresql\postgres.vcxproj]
.\Debug\postgres\postgres.exe : fatal error LNK1120: 3 unresolved externals 
[c:\cygwin\home\nm\src\pg\postgresql\postgres.vcxproj]
  The command exited with code 1120.
Done executing task Link -- FAILED.

This compiler emits _xmm symbols automatically, where needed.  The second
attached change lets the build complete and pass tests, but I can't readily
explain why it's necessary.  In the 64-bit build, the _xmm symbols export
normally (albeit, I presume, needlessly).  I hoped to find some rationale for
the preexisting gendef.pl exclusion of _real, which seems to resemble _xmm in
origin and use.  Magnus/anyone, can you shed light on our exclusion of _real
symbols from .def files?

In any event, if these incremental changes seem sane to you, please merge them
into your next version.

nm
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index e51665c..218ca8d 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -63,13 +63,12 @@ sub DeterminePlatform
 {
my $self = shift;
 
-   # Determine if we are in 32 or 64-bit mode. Do this by seeing if CL has
-   # 64-bit only parameters.
+   # Examine CL help output to determine if we are in 32 or 64-bit mode.
$self-{platform} = 'Win32';
-   open(P, cl /? 2NUL|) || die cl command not found;
+   open(P, cl /? 21 |) || die cl command not found;
while (P)
{
-   if (/^\/favor:/)
+   if (/for x64/)
{
$self-{platform} = 'x64';
last;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index ab65c46..8ef0422 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -40,6 +40,7 @@ while ($ARGV[0]/*.obj)
next if $pieces[6] =~ /^\(/;
next if $pieces[6] =~ /^__real/;
next if $pieces[6] =~ /^__imp/;
+   next if $pieces[6] =~ /^__xmm/;
next if $pieces[6] =~ /NULL_THUNK_DATA$/;
next if $pieces[6] =~ /^__IMPORT_DESCRIPTOR/;
next if $pieces[6] =~ /^__NULL_IMPORT/;

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


Re: Promoting a standby during base backup (was Re: [HACKERS] Switching timeline over streaming replication)

2012-10-08 Thread Simon Riggs
On 4 October 2012 18:07, Fujii Masao masao.fu...@gmail.com wrote:
 On Thu, Oct 4, 2012 at 4:59 PM, Heikki Linnakangas
 hlinnakan...@vmware.com wrote:
 On 03.10.2012 18:15, Amit Kapila wrote:

 On Tuesday, October 02, 2012 4:21 PM Heikki Linnakangas wrote:

 Hmm, should a base backup be aborted when the standby is promoted? Does
 the promotion render the backup corrupt?


 I think currently it does so. Pls refer
 1.
 do_pg_stop_backup(char *labelfile, bool waitforarchive)
 {
 ..
 if (strcmp(backupfrom, standby) == 0  !backup_started_in_recovery)
  ereport(ERROR,

 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
   errmsg(the standby was promoted during
 online backup),
   errhint(This means that the backup
 being
 taken is corrupt 
   and should not be used.
 
   Try taking another
 online
 backup.)));
 ..

 }


 Okay. I think that check in do_pg_stop_backup() actually already ensures
 that you don't end up with a corrupt backup, even if the standby is promoted
 while a backup is being taken. Admittedly it would be nicer to abort it
 immediately rather than error out at the end.

 But I wonder why promoting a standby renders the backup invalid in the first
 place? Fujii, Simon, can you explain that?

 Simon had the same question and I answered it before.

 http://archives.postgresql.org/message-id/cahgqgwfu04oo8yl5sucdjvq3brni7wtfzty9wa2kxtznhic...@mail.gmail.com
 ---
 You say
 If the standby is promoted to the master during online backup, the
 backup fails.
 but no explanation of why?

 I could work those things out, but I don't want to have to, plus we
 may disagree if I did.

 If the backup succeeds in that case, when we start an archive recovery from 
 that
 backup, the recovery needs to cross between two timelines. Which means that
 we need to set recovery_target_timeline before starting recovery. Whether
 recovery_target_timeline needs to be set or not depends on whether the standby
 was promoted during taking the backup. Leaving such a decision to a user seems
 fragile.

I accepted your answer before, but I think it should be challenged
now. This is definitely a time when you really want that backup, so
invalidating it for such a weak reason is not useful, even if I
understand your original thought.

Something that has concerned me is that we don't have an explicit
timeline change record. We *say* we do that at shutdown checkpoints,
but that is recorded in the new timeline. So we have the strange
situation of changing timeline at two separate places.

When we change timeline we really should generate one last WAL on the
old timeline that marks an explicit change of timeline and a single
exact moment when the timeline change takes place. With PITR we are
unable to do that, because any timeline can fork at any point. With
smooth switchover we have a special case that is not anything goes
and there is a good case for not incrementing the timeline at all.

This is still a half-formed thought, but at least you should know I'm
in the debate.

-- 
 Simon Riggs   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] sortsupport for text

2012-10-08 Thread Peter Geoghegan
Do you think you can follow through on this soon, Robert? I don't
believe that there are any outstanding issues. I'm not going to make
an issue of the fact that strxfrm() hasn't been taken advantage of. If
you could please post a new revision, with the suggested alterations
(that you agree with), I'd be happy to take another look.

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


Re: [HACKERS] Regarding identifying a foreign scan

2012-10-08 Thread Merlin Moncure
On Sun, Oct 7, 2012 at 10:29 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Atri Sharma atri.j...@gmail.com writes:
 Does that mean that using (some) global storage is the cause of the problem?

 If you're using global storage for state that needs to be replicated
 per-scan, yes, probably.  But it's hard to be sure when you're being
 so vague about what you're doing.

yeah -- the problem here is that Atri is not using
ForeignScanState-fdw_state properly for storage (this in jdbc-fdw).
I think he knows what to do -- he's going to fix it up.

merlin


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


[HACKERS] odd alter_generic regression failures

2012-10-08 Thread Andrew Dunstan



We seem to have an intermittent failure on the alter_generic tests that 
look like this:


  SET SESSION AUTHORIZATION regtest_alter_user1;
  CREATE FUNCTION alt_func1(int) RETURNS int LANGUAGE sql
AS 'SELECT $1 + 1';
   + ERROR:  permission denied for language sql
  CREATE FUNCTION alt_func2(int) RETURNS int LANGUAGE sql
AS 'SELECT $1 - 1';
   + ERROR:  permission denied for language sql
  CREATE AGGREGATE alt_agg1 (
sfunc1 = int4pl, basetype = int4, stype1 = int4, initcond = 0
  );

This has been seen on at least two buildfarm members: chough (Windows/MSVC) and 
smilodon (NetBSD/gcc). The fact that it's intermittent is rather worrying and 
puzzling.

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] Deparsing DDL command strings

2012-10-08 Thread Dimitri Fontaine
Tom Lane t...@sss.pgh.pa.us writes:
 going to be required below that ... but the point I'm trying to make is
 that it would be a one-and-done task.  Adding a requirement to be able
 to decompile raw parse trees will be a permanent drag on every type of
 SQL feature addition.

I'll show some examples of very involved command (CREATE and ALTER TABLE
are the most complex we have I think) and some very simple commands
(DROP TABLE is one of the simplest), so that we can make up our minds on
that angle.

 Then we want to qualify object names. Some type names have already been
 taken care of apparently by the parser here, relation names not yet and
 we need to cope with non existing relation names.

 Which is exactly what you *won't* be able to do anything about when
 looking at a raw parse tree.  It's just a different presentation of the
 query string.

So, I'm currently adding the deparsing to the existing only event we
have, which is ddl_command_start. That's maybe not the best place where
to do it, I even now wonder if we can do it there at all.

Doing the same thing at ddl_command_end would allow us have all the
information we need and leave nothing to magic guesses: full schema
qualification of all objects involved, main object(s) OIDs available,
all the jazz.

 Well, yeah.  Anything else is magic not code.

Well, prepending an object name with the first entry of the current
search_path as its schema is not that far a stretch when the object is
being created, as far as I see it. It's more reasonable to document that
the rewritten no-ambiguities command string is only available for
ddl_command_end events, though.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] [COMMITTERS] pgsql: Disable _FORTIFY_SOURCE with ICC

2012-10-08 Thread Peter Geoghegan
On 5 October 2012 04:37, Tom Lane t...@sss.pgh.pa.us wrote:
 A bit later: testing on an F17 box (glibc-2.15-56.fc17.x86_64)
 confirms Peter G's complaint, and also confirms that putting
 the above into port/linux.h (instead of the template file) fixes the
 problem.  Don't know how to disable it on ICC, but I suppose there's
 some way to test for ICC via an #if.

I was using F17 + glibc 2.15 too.

http://www.acsu.buffalo.edu/~charngda/icc.html indicates that ICC
defines a macro __ICC, which evaluates to an integer constant
representing the ICC version numbers. I guess that that particular
source isn't very trustworthy, but that's what we have a build farm
for.

Small patch that hopefully fixes everything for everyone is attached.
However, when I build at -O2 with the system GCC (on the same F17 box
with no changes to CFLAGS; just ./configure --with-libxml
--prefix=/home/peter/pgsql --enable-depend) this patch (when applied
to today's master) sees the following warnings raised:

zic.c: In function ‘writezone’:
zic.c:1752:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1753:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1754:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1755:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1756:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1757:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1758:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1759:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1760:3: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1772:4: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c:1785:4: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c: In function ‘puttzcode64’:
zic.c:1514:2: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
zic.c: In function ‘puttzcode’:
zic.c:1505:2: warning: ignoring return value of ‘fwrite’, declared
with attribute warn_unused_result [-Wunused-result]
In file included from gram.y:13502:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:16247:23: warning: unused variable ‘yyg’ [-Wunused-variable]

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


fortify_source_fix.2012_10_08.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] Rethinking placement of latch self-pipe initialization

2012-10-08 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 We still have to consider how Postgres would operate without the
 latches. I don't see that it can, so a shutdown seems appropriate. Is
 the purpose of this just to allow a cleaner and more informative
 shutdown? Or do you think we can avoid?

The point is that right now, if a new backend fails its initialization
at this specific step, that gets translated into a database-wide crash
and restart cycle, for no good reason.  It should just result in that
particular session failing.

 If we did move the init calls, would that alter things for code that
 creates new used defined latches?

Only to the extent that it'd have to make sure it called the
initialization function at some appropriate point.  It's not like
required initialization functions are a foreign concept in our code.

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] enhanced error fields

2012-10-08 Thread Peter Geoghegan
On 20 August 2012 14:09, Pavel Stehule pavel.steh...@gmail.com wrote:
 (eelog-2012-08-20.diff)

This patch has the following reference to relerror.c:



*** a/src/include/utils/rel.h
--- b/src/include/utils/rel.h
***
*** 394,397  typedef struct StdRdOptions
--- 394,402 
  extern void RelationIncrementReferenceCount(Relation rel);
  extern void RelationDecrementReferenceCount(Relation rel);

+ /* routines in utils/error/relerror.c */



Indeed, the new C file has been separately added to a makefile:

! OBJS = assert.o elog.o relerror.o

However, the new file itself does not appear in this patch. Therefore,
the code does not compile. Please post a revision with the new file
included.

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


Re: [HACKERS] [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown

2012-10-08 Thread Robert Haas
On Thu, Oct 4, 2012 at 6:12 AM, Amit kapila amit.kap...@huawei.com wrote:
 1. One new configuration parameter wal_receiver_timeout is added to detect 
 timeout at receiver task.
 2. Existing parameter replication_timeout is renamed to wal_sender_timeout.

-1 from me on a backward compatibility break here.  I don't know what
else to call the new GUC (replication_server_timeout?) but I'm not
excited about breaking existing conf files, nor do I particularly like
the proposed new names.

-- 
Robert Haas
EnterpriseDB: 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: [HACKERS] Missing OID define

2012-10-08 Thread Robert Haas
On Fri, Oct 5, 2012 at 3:32 AM, Albe Laurenz laurenz.a...@wien.gv.at wrote:
 Phil Sorber wrote:
 Thom Brown and I were doing some hacking the other day and came across
 this missing define. We argued over who was going to send the patch in
 and I lost. So here it is.

 +1

 I have been missing this #define.

After extensive review, I have committed this patch.

-- 
Robert Haas
EnterpriseDB: 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


[HACKERS] why can't plpgsql return a row-expression?

2012-10-08 Thread Robert Haas
Hi,

PL/pgsql seems to have a strange restriction regarding RETURN.
Normally, you can write RETURN expression.  But if the function
returns a row type, then you can only write RETURN variable or
RETURN NULL.

rhaas=# create type xyz as (a int, b int);
CREATE TYPE
rhaas=# select row(1,2)::xyz;
  row
---
 (1,2)
(1 row)

rhaas=# create or replace function return_xyz() returns xyz as $$
rhaas$# begin
rhaas$# return row(1,2)::xyz;
rhaas$# end$$ language plpgsql;
ERROR:  RETURN must specify a record or row variable in function returning row
LINE 3: return row(1,2)::xyz;
   ^

Off the top of my head, I can't think of any reason for this
restriction, nor can I find any code comments or anything in the
commit log which explains the reason for it.  Does anyone know why we
don't allow this?

-- 
Robert Haas
EnterpriseDB: 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: [HACKERS] [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown

2012-10-08 Thread Amit Kapila
 On Monday, October 08, 2012 7:38 PM Robert Haas wrote:
 On Thu, Oct 4, 2012 at 6:12 AM, Amit kapila amit.kap...@huawei.com
 wrote:
  1. One new configuration parameter wal_receiver_timeout is added to
 detect timeout at receiver task.
  2. Existing parameter replication_timeout is renamed to
 wal_sender_timeout.
 
 -1 from me on a backward compatibility break here.  I don't know what
 else to call the new GUC (replication_server_timeout?) but I'm not
 excited about breaking existing conf files, nor do I particularly like
 the proposed new names.

How about following:
1. replication_client_timeout -- shouldn't it be client as new configuration
is for wal receiver
2. replication_standby_timeout

If we introduce a new parameter for wal receiver, wouldn't
replication_timeout be confusing for user?

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] Improving psql \ds

2012-10-08 Thread Robert Haas
On Mon, Oct 8, 2012 at 3:49 AM, Julien Tachoires jul...@gmail.com wrote:
 2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Julien Tachoires jul...@gmail.com writes:
 About \ds behaviour, I think to add 2 columns :
 - 'LastValue'
 - 'Increment'

 That would make the command a great deal slower, since it would have to
 access each sequence to get that info.  I don't object to adding such
 columns to \ds+, but I don't think it's a good idea to put them in the
 basic command.

 Ok.

 The other problem you're going to have here is that there is in fact no
 such command as \ds (nor \ds+); rather, it's a special case of
 \dtsvi.  As such, putting any relkind-specific info into the result is
 problematic.  I think you're going to have to invent a different command
 if you want there to be sequence-specific columns.

 Yes, that's why I plan to create a new function listSequences() and
 call it from src/bin/psql/command.c :
 +   /* \d* commands */
 +   else if (cmd[0] == 'd')
 +   {
 ...
 +   case 's':
 +   success = listSequences(pattern, 
 show_verbose);
 +   break;

What happens if the user does this:

\dis

-- 
Robert Haas
EnterpriseDB: 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: [HACKERS] Improving psql \ds

2012-10-08 Thread Tom Lane
Julien Tachoires jul...@gmail.com writes:
 2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 The other problem you're going to have here is that there is in fact no
 such command as \ds (nor \ds+); rather, it's a special case of
 \dtsvi.  As such, putting any relkind-specific info into the result is
 problematic.  I think you're going to have to invent a different command
 if you want there to be sequence-specific columns.

 Yes, that's why I plan to create a new function listSequences() and
 call it from src/bin/psql/command.c :
 + /* \d* commands */
 + else if (cmd[0] == 'd')
 + {
 ...
 + case 's':
 + success = listSequences(pattern, show_verbose);
 + break;

No, that's not acceptable, because it will break cases such as \dstv.

The \dxxx command namespace is already direly overloaded; I'm not sure
cramming more behaviors into it is a good idea anyway.  Perhaps
something along the line of \seqs would be a usable substitute.

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] Improving psql \ds

2012-10-08 Thread Julien Tachoires
2012/10/8 Robert Haas robertmh...@gmail.com:
 On Mon, Oct 8, 2012 at 3:49 AM, Julien Tachoires jul...@gmail.com wrote:
 2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Julien Tachoires jul...@gmail.com writes:
 About \ds behaviour, I think to add 2 columns :
 - 'LastValue'
 - 'Increment'

 That would make the command a great deal slower, since it would have to
 access each sequence to get that info.  I don't object to adding such
 columns to \ds+, but I don't think it's a good idea to put them in the
 basic command.

 Ok.

 The other problem you're going to have here is that there is in fact no
 such command as \ds (nor \ds+); rather, it's a special case of
 \dtsvi.  As such, putting any relkind-specific info into the result is
 problematic.  I think you're going to have to invent a different command
 if you want there to be sequence-specific columns.

 Yes, that's why I plan to create a new function listSequences() and
 call it from src/bin/psql/command.c :
 +   /* \d* commands */
 +   else if (cmd[0] == 'd')
 +   {
 ...
 +   case 's':
 +   success = listSequences(pattern, 
 show_verbose);
 +   break;

 What happens if the user does this:

 \dis

hmm, you're right, this is not a good idea.


-- 
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 can't plpgsql return a row-expression?

2012-10-08 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 ERROR:  RETURN must specify a record or row variable in function returning row

 Off the top of my head, I can't think of any reason for this
 restriction, nor can I find any code comments or anything in the
 commit log which explains the reason for it.  Does anyone know why we
 don't allow this?

Laziness, probably.  Feel free to have at 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


Re: [HACKERS] Add FET to Default and Europe.txt

2012-10-08 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Could we pull an abbreviation list from one of the BSDs?

And that would be authoritative why?

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] Add FET to Default and Europe.txt

2012-10-08 Thread Bruce Momjian
On Mon, Oct  8, 2012 at 11:10:08AM -0400, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Could we pull an abbreviation list from one of the BSDs?
 
 And that would be authoritative why?

It would be somone else maintaining it.  :-)

-- 
  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] Add FET to Default and Europe.txt

2012-10-08 Thread Marc Balmer
Am 08.10.12 16:53, schrieb Bruce Momjian:
 On Mon, Oct  8, 2012 at 12:14:15PM +0200, Marc Balmer wrote:
 A good starting point would be to take the timezone information directly
 from the the files IANA distributes, instead of manually copying and
 maintaining them in a separate file.  If no one else does, I can try to
 maintain these files.  Those who know about my work, know that I do a
 lot of time related software (support for time signal receivers in
 OpenBSD, e.g.).
 
 Could we pull an abbreviation list from one of the BSDs?

Imo, the data should be pulled from the official source.  Else an
unneeded dependency from BSD is likely created...




-- 
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] Add FET to Default and Europe.txt

2012-10-08 Thread Bruce Momjian
On Mon, Oct  8, 2012 at 05:15:18PM +0200, Marc Balmer wrote:
 Am 08.10.12 16:53, schrieb Bruce Momjian:
  On Mon, Oct  8, 2012 at 12:14:15PM +0200, Marc Balmer wrote:
  A good starting point would be to take the timezone information directly
  from the the files IANA distributes, instead of manually copying and
  maintaining them in a separate file.  If no one else does, I can try to
  maintain these files.  Those who know about my work, know that I do a
  lot of time related software (support for time signal receivers in
  OpenBSD, e.g.).
  
  Could we pull an abbreviation list from one of the BSDs?
 
 Imo, the data should be pulled from the official source.  Else an
 unneeded dependency from BSD is likely created...

I thought there was no official source of abbreviations that deals with
duplicates by choosing the most appropriate one.  Is there?  Where is
it?

-- 
  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] Improving psql \ds

2012-10-08 Thread Julien Tachoires
2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Julien Tachoires jul...@gmail.com writes:
 2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 The other problem you're going to have here is that there is in fact no
 such command as \ds (nor \ds+); rather, it's a special case of
 \dtsvi.  As such, putting any relkind-specific info into the result is
 problematic.  I think you're going to have to invent a different command
 if you want there to be sequence-specific columns.

 Yes, that's why I plan to create a new function listSequences() and
 call it from src/bin/psql/command.c :
 + /* \d* commands */
 + else if (cmd[0] == 'd')
 + {
 ...
 + case 's':
 + success = listSequences(pattern, show_verbose);
 + break;

 No, that's not acceptable, because it will break cases such as \dstv.

 The \dxxx command namespace is already direly overloaded; I'm not sure
 cramming more behaviors into it is a good idea anyway.  Perhaps
 something along the line of \seqs would be a usable substitute.

Ok. Does having both '\ds' and , let's call it '\seqs', won't be
source of confusion for the user ?


-- 
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] Add FET to Default and Europe.txt

2012-10-08 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 Sorry for any confusion. I've not suggested removing timezone
 abbreviations, nor do I wish to do so.

 I did suggest that we do not rely upon TZ abbreviations in any code
 shipped by PostgreSQL project, but I suspect that is a non-problem
 anyway.

No, we don't rely on them AFAIK; certainly we don't use them in data
output.  But people expect to be able to use them in data input.

The idea of tracking date ranges in which a TZ abbreviation is valid
is an interesting one, but it's vastly more effort than I for one
am willing to put into the problem --- not just in coding the
infrastructure, but gathering and maintaining the source data.
[ reflects for a bit... ]  I guess in principle we could pull change
information out of the IANA database rather than having to maintain it
ourselves.  But still what you're suggesting is an awful lot of work.

The other thing that the abbreviation list files are doing for us is
providing a user-configurable way to resolve conflicting abbreviations,
for instance IST (the Indians and the Israelis both use this, but not to
mean the same thing).  This requirement isn't ever going to go away.

The Default list represents some considered judgements as to what the
most widely useful set of abbreviations is.  We don't get to abdicate
the position of having to make those judgements; nobody would thank us
for breaking their database configurations because we can't decide.

I still think that we need some input from actual Russians as to what
is the currently most useful set of abbreviations for Russian time
zones.  Armchair speculation from the other side of the globe isn't
going to be helpful to them.

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] sortsupport for text

2012-10-08 Thread Robert Haas
On Mon, Oct 8, 2012 at 8:47 AM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 Do you think you can follow through on this soon, Robert? I don't
 believe that there are any outstanding issues. I'm not going to make
 an issue of the fact that strxfrm() hasn't been taken advantage of. If
 you could please post a new revision, with the suggested alterations
 (that you agree with), I'd be happy to take another look.

I don't have any plans to work on this further.  I think all of the
criticism that has been leveled at this patch is 100% bogus, and I
greatly dislike the changes that have been proposed.  That may not be
fair, but it's how I feel, and in light of that feeling it does not
make sense for me to pursue this.  Sorry.

-- 
Robert Haas
EnterpriseDB: 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: [HACKERS] Move postgresql_fdw_validator into dblink

2012-10-08 Thread Kohei KaiGai
Hanada-san,

The attached patch is a revised one according to my previous
suggestion. It re-defines PQconninfoOption *options as static
variable with NULL initial value, then, PQconndefaults() shall
be invoked at once. The default options never changed during
duration of the backend process, so here is no reason why we
allocate and free this object for each validator invocation.

If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.

Thanks,

2012/9/20 Kohei KaiGai kai...@kaigai.gr.jp:
 Hanada-san,

 I checked your patch. It can be applied to the latest master branch
 without any conflicts, and regression tests were fine.

 Unlike the original postgresql_fdw_validator(), the new
 dblink_fdw_validator() has wise idea; that pulls list of connection
 options from libpq, instead of self-defined static table.
 I basically agree not to have multiple source of option list.

 +   /*
 +* Get list of valid libpq options.  It contains default values too, but 
 we
 +* don't care the values.  Obtained list is allocated with malloc, so we
 +* must free it before leaving this function.
 +*/
 +   options = PQconndefaults();
 +   if (options == NULL)
 +   ereport(ERROR,
 +   (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
 +errmsg(out of memory),
 +errdetail(could not get libpq default connection 
 options)));

 I doubt whether PQconndefaults needs to be invoked for each
 validator calls. At least, supported option list of libpq should be
 never changed during run-time. So, I think PQconndefaults()
 should be called only once at first invocation, then option list
 can be stored in static variables or somewhere.
 As source code comments says, it is allocated with malloc, thus,
 we don't worry about unintentional release. :-)

 I don't think other part of this patch is arguable.

 Thanks,

 2012/9/13 Shigeru HANADA shigeru.han...@gmail.com:
 Kaigai-san,

 (2012/09/13 16:56), Kohei KaiGai wrote:
 What about your plan to upstream contrib/pgsql_fdw module on the upcoming
 commit-fest?

 I will post pgsql_fdw patch (though it will be renamed to
 postgresql_fdw) in opening CF (2012 Sep), as soon as I resolve a bug in
 ANALYZE support, maybe on tomorrow. :-)

 Even though I understand the point I noticed (miss-synchronization of sub-
 transaction block between local and remote side) is never easy problem to
 solve, it is worth to get the patch on the table of discussion.
 In my opinion, it is an idea to split-off the transaction control portion as
 a limitation of current version. For example, just raise an error when
 the foreign-table being referenced in sub-transaction block; with explicit
 description in the document.

 I agree not to support synchronize TX block between remote and local, at
 least in next CF (I mean keeping remote TX open until local COMMIT or
 ABORT).  It would require 2PC and many issues to be solved, so I'd like
 to focus fundamental part first.  OTOH, using foreign tables in
 sub-transaction seems essential to me.

 Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
 contrib/pgsql_fdw patch based on this patch.

 Thanks for your volunteer :-)

 Regards,
 --
 Shigeru HANADA



 --
 KaiGai Kohei kai...@kaigai.gr.jp



-- 
KaiGai Kohei kai...@kaigai.gr.jp


dblink_fdw_validator.v2.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] Add FET to Default and Europe.txt

2012-10-08 Thread Heikki Linnakangas

On 08.10.2012 18:26, Tom Lane wrote:

The other thing that the abbreviation list files are doing for us is
providing a user-configurable way to resolve conflicting abbreviations,
for instance IST (the Indians and the Israelis both use this, but not to
mean the same thing).  This requirement isn't ever going to go away.


Locale-specific abbreviation lists would be nice. While abbreviations 
are not unique globally, they have established meanings in particular 
countries and regions. For example, IST is not unique, but if you are in 
India (en_IN) and spell out IST, you probably mean Indian Standard time.


- Heikki


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


[HACKERS] static-if-inline

2012-10-08 Thread Alvaro Herrera
The ilist patch from Andres Freund introduces a cute trick for defining
maybe-inline functions, which works regardless of whether the compiler
supports inlining, and eliminates the need to write the code twice
(first in the header and also the .c file.)  It's really quite a simple
thing, but the whole topic on inlining has generated a lot of debate.

So to remove that controversy out of said patch, here's a preliminary
one: get rid of duplicate definitions in sortsupport.c, list.c, mcxt.c
(which are currently defined as static inline in their respective
headers for compilers that support it, and as regular functions in the
.c files for those that don't).

What's being done in this patch is exactly what would be done in the
ilist code.  So if there are problems with this, please speak up.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/nodes/list.c
--- b/src/backend/nodes/list.c
***
*** 15,20 
--- 15,23 
   */
  #include postgres.h
  
+ /* see pg_list.h */
+ #define PG_LIST_INCLUDE_DEFINITIONS
+ 
  #include nodes/pg_list.h
  
  
***
*** 1224,1254  list_copy_tail(const List *oldlist, int nskip)
  }
  
  /*
-  * pg_list.h defines inline versions of these functions if allowed by the
-  * compiler; in which case the definitions below are skipped.
-  */
- #ifndef USE_INLINE
- 
- ListCell *
- list_head(const List *l)
- {
- 	return l ? l-head : NULL;
- }
- 
- ListCell *
- list_tail(List *l)
- {
- 	return l ? l-tail : NULL;
- }
- 
- int
- list_length(const List *l)
- {
- 	return l ? l-length : 0;
- }
- #endif   /* ! USE_INLINE */
- 
- /*
   * Temporary compatibility functions
   *
   * In order to avoid warnings for these function definitions, we need
--- 1227,1232 
*** a/src/backend/utils/mmgr/mcxt.c
--- b/src/backend/utils/mmgr/mcxt.c
***
*** 21,26 
--- 21,29 
  
  #include postgres.h
  
+ /* see palloc.h */
+ #define MCXT_INCLUDE_DEFINITIONS
+ 
  #include utils/memutils.h
  
  
***
*** 696,723  repalloc(void *pointer, Size size)
  }
  
  /*
-  * MemoryContextSwitchTo
-  *		Returns the current context; installs the given context.
-  *
-  * palloc.h defines an inline version of this function if allowed by the
-  * compiler; in which case the definition below is skipped.
-  */
- #ifndef USE_INLINE
- 
- MemoryContext
- MemoryContextSwitchTo(MemoryContext context)
- {
- 	MemoryContext old;
- 
- 	AssertArg(MemoryContextIsValid(context));
- 
- 	old = CurrentMemoryContext;
- 	CurrentMemoryContext = context;
- 	return old;
- }
- #endif   /* ! USE_INLINE */
- 
- /*
   * MemoryContextStrdup
   *		Like strdup(), but allocate from the specified context
   */
--- 699,704 
*** a/src/backend/utils/sort/sortsupport.c
--- b/src/backend/utils/sort/sortsupport.c
***
*** 15,20 
--- 15,23 
  
  #include postgres.h
  
+ /* See sortsupport.h */
+ #define SORTSUPPORT_INCLUDE_DEFINITIONS
+ 
  #include fmgr.h
  #include utils/lsyscache.h
  #include utils/sortsupport.h
***
*** 29,78  typedef struct
  
  
  /*
-  * sortsupport.h defines inline versions of these functions if allowed by the
-  * compiler; in which case the definitions below are skipped.
-  */
- #ifndef USE_INLINE
- 
- /*
-  * Apply a sort comparator function and return a 3-way comparison result.
-  * This takes care of handling reverse-sort and NULLs-ordering properly.
-  */
- int
- ApplySortComparator(Datum datum1, bool isNull1,
- 	Datum datum2, bool isNull2,
- 	SortSupport ssup)
- {
- 	int			compare;
- 
- 	if (isNull1)
- 	{
- 		if (isNull2)
- 			compare = 0;		/* NULL = NULL */
- 		else if (ssup-ssup_nulls_first)
- 			compare = -1;		/* NULL  NOT_NULL */
- 		else
- 			compare = 1;		/* NULL  NOT_NULL */
- 	}
- 	else if (isNull2)
- 	{
- 		if (ssup-ssup_nulls_first)
- 			compare = 1;		/* NOT_NULL  NULL */
- 		else
- 			compare = -1;		/* NOT_NULL  NULL */
- 	}
- 	else
- 	{
- 		compare = (*ssup-comparator) (datum1, datum2, ssup);
- 		if (ssup-ssup_reverse)
- 			compare = -compare;
- 	}
- 
- 	return compare;
- }
- #endif   /* ! USE_INLINE */
- 
- /*
   * Shim function for calling an old-style comparator
   *
   * This is essentially an inlined version of FunctionCall2Coll(), except
--- 32,37 
*** a/src/include/c.h
--- b/src/include/c.h
***
*** 744,749  typedef NameData *Name;
--- 744,764 
  #endif /* HAVE__BUILTIN_TYPES_COMPATIBLE_P */
  
  
+ /*
+  * Inlining support
+  *
+  * Allow modules to declare functions that are inlined if the compiler supports
+  * it.  The functions must be defined in its header, protected by a special
+  * #ifdef that the .c file defines.  If the compiler does not support inlines,
+  * the function definitions are pulled in by the .c file as regular (not
+  * inline) symbols; the prototype must also exist in the header in that case.
+  */
+ #ifdef USE_INLINE
+ #define STATIC_IF_INLINE static inline
+ #else
+ #define 

Re: [HACKERS] Add FET to Default and Europe.txt

2012-10-08 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes:
 On 08.10.2012 18:26, Tom Lane wrote:
 The other thing that the abbreviation list files are doing for us is
 providing a user-configurable way to resolve conflicting abbreviations,
 for instance IST (the Indians and the Israelis both use this, but not to
 mean the same thing).  This requirement isn't ever going to go away.

 Locale-specific abbreviation lists would be nice.

Yeah, that's a good thought, although the lack of standardization of
locale names seems to make it a bit hard to implement.  My first idea
was look for a tznames file matching the value of LC_TIME, and if
found, concatenate its contents with 'Default'.  But there are enough
ways to spell en_IN to make that a bit problematic, and besides which
a user in India might well be running with C locale anyway.  Still,
there might be a useful incremental usability gain there.

We aren't ever going to get out of the need for user configurability
though.  For instance, if a user in India writes EST, is he thinking
of the Australian or American meaning?  It's plausible that either might
be preferred, depending on who that user interacts with regularly.

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] static-if-inline

2012-10-08 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 What's being done in this patch is exactly what would be done in the
 ilist code.  So if there are problems with this, please speak up.

Stylistic gripe: in palloc.h you didn't follow the pattern of
#ifndef USE_INLINE
extern ...
#endif
before (rather than after) the function definitions.  Nitpicky I know,
but if we're trying to create a pattern, we should be consistent.

The comment added in c.h has some grammar issues, too.  Looks good
otherwise.

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] Warnings from fwrite() in git head

2012-10-08 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 I am seeing the following warnings in git head from zic.c:
   zic.c:1505: warning: ignoring return value of ‘fwrite’, declared 
 with attribute warn_unused_result

Yeah, this is probably a consequence of the _FORTIFY_SOURCE addition.
I believe that ratchets up warning pickiness as well as other things.

I'm inclined to think we should make zic.c bail out on write errors.
Otherwise, make install could fail to notice an out-of-disk-space
situation during install.  The analogy you are drawing to ignoring
errors while writing log messages is quite faulty.

We're overdue for another round of syncing the tz code with upstream,
too.  Might be best to do that first, just in case Olson's crew already
fixed this.

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] Warnings from fwrite() in git head

2012-10-08 Thread Jeff Janes
On Mon, Oct 8, 2012 at 8:58 AM, Bruce Momjian br...@momjian.us wrote:
 I am seeing the following warnings in git head from zic.c:

 zic.c:1505: warning: ignoring return value of ‘fwrite’, declared with 
 attribute warn_unused_result
...

 Seems casting to void is not enough.  Not sure why the error just
 started appearing for me last week.  I don't see any recent gcc updates.
 This is gcc version 4.4.5-8 on Debian Squeeze.

I think it was the addition of _FORTIFY_SOURCE

I get the warnings in zic.c, but also I was also getting them in a
different file.  But now I only see zic ones in newest HEAD.

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


[HACKERS] Logging parameters values on bind error

2012-10-08 Thread Jehan-Guillaume de Rorthais
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Hi,

We were struggling today with some java code binding values violating
some constraints in a prepared statement.

We don't own the code and couldn't make tests with it. So we tried to
find if PostgreSQL was able to log binded values when the BIND
operation fail and found that this is not possible in current release:
the error raised while planing the statement is not caught.

PFA a patch that catch any error while creating the query plan and add
parameters values to the error message if log_statement or
log_min_duration_statement would have logged it.

Comments?
- -- 
Jehan-Guillaume de Rorthais
http://www.dalibo.com
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlBy/cgACgkQXu9L1HbaT6Li5QCdEa9Zc4g302znpHmrwB9dnRBI
JSwAnR2Poil0QAP6b+TflM2ebDCPLq3G
=HU3H
-END PGP SIGNATURE-
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***
*** 1715,1726  exec_bind_message(StringInfo input_message)
  
  	pq_getmsgend(input_message);
  
! 	/*
! 	 * Obtain a plan from the CachedPlanSource.  Any cruft from (re)planning
! 	 * will be generated in MessageContext.  The plan refcount will be
! 	 * assigned to the Portal, so it will be released at portal destruction.
! 	 */
! 	cplan = GetCachedPlan(psrc, params, false);
  
  	/*
  	 * Now we can define the portal.
--- 1715,1738 
  
  	pq_getmsgend(input_message);
  
! 	PG_TRY();
! 	{
! 		/*
! 		 * Obtain a plan from the CachedPlanSource.  Any cruft from (re)planning
! 		 * will be generated in MessageContext.  The plan refcount will be
! 		 * assigned to the Portal, so it will be released at portal destruction.
! 		 */
! 
! 		cplan = GetCachedPlan(psrc, params, false);
! 	}
! 	PG_CATCH();
! 	{
! 		if (check_log_statement(portal-stmts) || check_log_duration(msec_str, false) == 2)
! 			errdetail_params(params);
! 
! 		PG_RE_THROW();
! 	}
! 	PG_END_TRY();
  
  	/*
  	 * Now we can define the portal.

-- 
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 can't plpgsql return a row-expression?

2012-10-08 Thread Pavel Stehule
2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Robert Haas robertmh...@gmail.com writes:
 ERROR:  RETURN must specify a record or row variable in function returning 
 row

 Off the top of my head, I can't think of any reason for this
 restriction, nor can I find any code comments or anything in the
 commit log which explains the reason for it.  Does anyone know why we
 don't allow this?

 Laziness, probably.  Feel free to have at it.

I wrote patch some years ago. It was rejected from performance reasons
- because every row had to be casted to resulted type.

Pavel




 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


-- 
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] sortsupport for text

2012-10-08 Thread Peter Geoghegan
On 8 October 2012 16:30, Robert Haas robertmh...@gmail.com wrote:
 I don't have any plans to work on this further.  I think all of the
 criticism that has been leveled at this patch is 100% bogus, and I
 greatly dislike the changes that have been proposed.  That may not be
 fair, but it's how I feel, and in light of that feeling it does not
 make sense for me to pursue this.  Sorry.

If it was the case that you were only 50% of the way to getting
something committable, I guess I'd understand; this is, after all, a
volunteer effort, and you are of course free to pursue or not pursue
whatever you like. It's more like 95% though, so I have a hard time
understanding this attitude.

The only criticism that I imagine that you could be referring to is my
criticism (which was seconded by Tom) concerning the approach to
sizing a buffer that is used as state between successive calls to the
sortsupport routine. This was a fairly trivial point even within the
context of this patch. It was regrettable that it got so out of hand,
but that certainly wasn't my intention.

I must say that I find this disheartening as a reviewer, particularly
since it comes from someone who reviews a lot of patches (including
most of my own), and has often complained about the difficulties that
reviewers face. Is it really so hard for you to accept a criticism
from me? I didn't expect you to fully accept my criticism; I only
expected you to take it into consideration, and possibly let it go for
the sake of progress.

For what it's worth, I have high regard for your work generally. In
all sincerity, this appears to me to be a slight, and I find it
upsetting, both on a personal level, and because it creates a concern
about being able to work with you in the future, which is certainly
something that I've benefited from in the past, and hope to continue
to benefit from.

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


Re: [HACKERS] Logging parameters values on bind error

2012-10-08 Thread Tom Lane
Jehan-Guillaume de Rorthais j...@dalibo.com writes:
 PFA a patch that catch any error while creating the query plan and add
 parameters values to the error message if log_statement or
 log_min_duration_statement would have logged it.

If that works, it's only by accident --- you're not supposed to add more
info to an error object after the error has been thrown.  What's worse,
if the original error had a DETAIL message, you're overwriting it.

The right way to do this type of thing, which is also cheaper than
PG_TRY on most platforms, is to set up an ErrorContextCallback stack
entry to call a function that adds extra information as a CONTEXT line.
There are lots of examples in the code base.

However ... I'm unconvinced that this can work safely at all.  Note the
check in errdetail_params that causes it to not do anything in an
aborted transaction.  Once you have caught an error, the current
transaction really has to be considered aborted, even if xact.c hasn't
been told about it yet.  So this patch is cheating with both hands, and
it will break given the right combination of error and parameter
datatypes.

A less dangerous approach would be to only attempt to print parameter
values that were supplied in text format.

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] why can't plpgsql return a row-expression?

2012-10-08 Thread Tom Lane
Pavel Stehule pavel.steh...@gmail.com writes:
 2012/10/8 Tom Lane t...@sss.pgh.pa.us:
 Laziness, probably.  Feel free to have at it.

 I wrote patch some years ago. It was rejected from performance reasons
 - because every row had to be casted to resulted type.

I don't recall that patch in any detail, but it's not apparent to me
that an extra cast step *must* be required to implement this.  In the
cases that are supported now, surely we could notice that no additional
work is required.

It's also worth commenting that if we were to switch the storage of
composite-type plpgsql variables to HeapTuple, as has been suggested
here for other reasons, the performance tradeoffs in this area would
likely change completely anyway.

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] Bad Data back Door

2012-10-08 Thread David E. Wheeler
On Oct 5, 2012, at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Now, having said that, I think it has to be the reponsibility of the FDW
 to apply any required check ... which makes this a bug report against
 oracle_fdw, not the core system.  (FWIW, contrib/file_fdw depends on the
 COPY code, which will check encoding.)

FWIW, I believe that dblink does not check encoding.

Best,

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] Bad Data back Door

2012-10-08 Thread Tom Lane
David E. Wheeler da...@justatheory.com writes:
 On Oct 5, 2012, at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Now, having said that, I think it has to be the reponsibility of the FDW
 to apply any required check ... which makes this a bug report against
 oracle_fdw, not the core system.  (FWIW, contrib/file_fdw depends on the
 COPY code, which will check encoding.)

 FWIW, I believe that dblink does not check encoding.

In dblink's case, that boils down to trusting a remote instance of
Postgres to get this right, which doesn't seem totally unreasonable.
But I wouldn't object to adding checks there if someone wanted to submit
a patch.

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] Bad Data back Door

2012-10-08 Thread David E. Wheeler
On Oct 8, 2012, at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 FWIW, I believe that dblink does not check encoding.
 
 In dblink's case, that boils down to trusting a remote instance of
 Postgres to get this right, which doesn't seem totally unreasonable.
 But I wouldn't object to adding checks there if someone wanted to submit
 a patch.

Yeah, I found this because we had a dblink to another PostgreSQL server's table 
with data populated from oracle_fdw. I guess trusting is reasonable, though.

I wonder about dbi-link, though…

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] Bad Data back Door

2012-10-08 Thread Andrew Dunstan


On 10/08/2012 02:13 PM, Tom Lane wrote:

David E. Wheeler da...@justatheory.com writes:

On Oct 5, 2012, at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:

Now, having said that, I think it has to be the reponsibility of the FDW
to apply any required check ... which makes this a bug report against
oracle_fdw, not the core system.  (FWIW, contrib/file_fdw depends on the
COPY code, which will check encoding.)

FWIW, I believe that dblink does not check encoding.

In dblink's case, that boils down to trusting a remote instance of
Postgres to get this right, which doesn't seem totally unreasonable.
But I wouldn't object to adding checks there if someone wanted to submit
a patch.


It does do:

PQsetClientEncoding(conn, GetDatabaseEncodingName());


I'd be mildly reluctant to do anything more except possibly as an 
option, unless it could be shown to have minimal performance impact.


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] [v9.3] Row-Level Security

2012-10-08 Thread Dean Rasheed
On 8 October 2012 15:57, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 The attached patch is a refreshed version towards the latest master branch,
 to fix up patch conflicts.
 Here is no other difference from the previous revision.

 Thanks,


I had another look at this over the weekend and I found couple of
additional problems (test cases attached):

1). It is possible to define a RLS qual that refers back to the table
that it's defined on, in such a way that causes infinite recursion in
the planner, giving ERROR:  stack depth limit exceeded. I think it
would be preferable to trap this and report a more meaningful error
back to the user, along similar lines to a self-referencing view.

2). In other cases it is possible to define a RLS qual that refers to
another table with a RLS qual in such a way that the second table's
RLS qual is not checked, thus allowing a user to bypass the security
check.

3). If a RLS qual refers to a view it errors, since the RLS quals are
added after rule expansion, and so the view is not rewritten.

To me this suggests that perhaps the expansion of RLS quals should be
done in the rewriter. I've not thought that through in any detail, but
ISTM that a RIR rule could add a table with a RLS qual, and a RLS qual
could add a relation with a RIR rule that needs expanding, and so the
2 need to be processed together. This could also make use of the
existing recursion-checking code in the rewriter.

Regards,
Dean


test.sql
Description: Binary data

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


[HACKERS] getopt() and strdup()

2012-10-08 Thread Bruce Momjian
A while ago I noticed that in some places we strdup/pg_strdup() optarg
strings from getopt(), and in some places we don't.

If we needed the strdup(), the missing cases should generate errors.  If
we don't need them, the strdup() is unnecessary, and research confirms
they are unnecessary.  Should we remove the extra strdup/pg_strdup()
calls, for consistency.

I think we might have had old platforms that required it, but none are
still supported today.

-- 
  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] Improving psql \ds

2012-10-08 Thread Gavin Flower

On 09/10/12 03:53, Robert Haas wrote:

On Mon, Oct 8, 2012 at 3:49 AM, Julien Tachoires jul...@gmail.com wrote:

2012/10/8 Tom Lane t...@sss.pgh.pa.us:

Julien Tachoires jul...@gmail.com writes:

About \ds behaviour, I think to add 2 columns :
- 'LastValue'
- 'Increment'

That would make the command a great deal slower, since it would have to
access each sequence to get that info.  I don't object to adding such
columns to \ds+, but I don't think it's a good idea to put them in the
basic command.

Ok.


The other problem you're going to have here is that there is in fact no
such command as \ds (nor \ds+); rather, it's a special case of
\dtsvi.  As such, putting any relkind-specific info into the result is
problematic.  I think you're going to have to invent a different command
if you want there to be sequence-specific columns.

Yes, that's why I plan to create a new function listSequences() and
call it from src/bin/psql/command.c :
+   /* \d* commands */
+   else if (cmd[0] == 'd')
+   {
...
+   case 's':
+   success = listSequences(pattern, show_verbose);
+   break;

What happens if the user does this:

\dis


Hmm...

'According' to Terry Prachett,that is probably short hand for telling 
the backend to go to Hell = 'dis' is apparently another name for 'Hell'!
(In his book 'Truth', commander Vimes has a personal dis-organiser - as 
the 'dis-organiser' is run by a demon from Hell.)


Now back to more appropriate comments...


--
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] Add FET to Default and Europe.txt

2012-10-08 Thread Christopher Browne
On Mon, Oct 8, 2012 at 11:54 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Heikki Linnakangas hlinnakan...@vmware.com writes:
 On 08.10.2012 18:26, Tom Lane wrote:
 The other thing that the abbreviation list files are doing for us is
 providing a user-configurable way to resolve conflicting abbreviations,
 for instance IST (the Indians and the Israelis both use this, but not to
 mean the same thing).  This requirement isn't ever going to go away.

 Locale-specific abbreviation lists would be nice.

 Yeah, that's a good thought, although the lack of standardization of
 locale names seems to make it a bit hard to implement.  My first idea
 was look for a tznames file matching the value of LC_TIME, and if
 found, concatenate its contents with 'Default'.  But there are enough
 ways to spell en_IN to make that a bit problematic, and besides which
 a user in India might well be running with C locale anyway.  Still,
 there might be a useful incremental usability gain there.

 We aren't ever going to get out of the need for user configurability
 though.  For instance, if a user in India writes EST, is he thinking
 of the Australian or American meaning?  It's plausible that either might
 be preferred, depending on who that user interacts with regularly.

That sounds pretty cool, but coolness isn't the right way to
evaluate whether this is good or not.

If we introduce cases where peoples' expectations are liable to be
disappointed (e.g. - they get the wrong EST, and report a bug on
that), then we lose.

We have, in effect, been treating the handling of time zones in a
manner where we're imagining there's general agreement on their
interpretation.  We presently get bug reports (and are
losing/getting it not as right as would be nice) in cases where we
leave TZ symbols out, where they could have been included.

The scenario where we could unambiguously include time zones is where
the symbols are unique.  If we were to include *all* uniquely-named
symbols, that would minimize the number of complaints about missing
zones, whilst evading the cases where the symbols are non-unique.
That might be worth considering, though it'll certainly attract
complaints in that some odd-ball zones would be included whilst
well-known ones wouldn't.

I would tend to think that local variations (e.g. - having a list for
LC_TIME=en_IN) heads us into a morass of complexity.  As you suggest,
two different people using en_IN might have different preferences for
what EST should mean.

That being said, if we had a way to configure preferred localizations,
people could set up their own set of associations.  You want your own
morass?  You can build it yourself...
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, How would the Lone Ranger handle this?


-- 
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] [v9.3] Row-Level Security

2012-10-08 Thread Kohei KaiGai
2012/10/8 Dean Rasheed dean.a.rash...@gmail.com:
 On 8 October 2012 15:57, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 The attached patch is a refreshed version towards the latest master branch,
 to fix up patch conflicts.
 Here is no other difference from the previous revision.

 Thanks,


 I had another look at this over the weekend and I found couple of
 additional problems (test cases attached):

 1). It is possible to define a RLS qual that refers back to the table
 that it's defined on, in such a way that causes infinite recursion in
 the planner, giving ERROR:  stack depth limit exceeded. I think it
 would be preferable to trap this and report a more meaningful error
 back to the user, along similar lines to a self-referencing view.

 2). In other cases it is possible to define a RLS qual that refers to
 another table with a RLS qual in such a way that the second table's
 RLS qual is not checked, thus allowing a user to bypass the security
 check.

 3). If a RLS qual refers to a view it errors, since the RLS quals are
 added after rule expansion, and so the view is not rewritten.

 To me this suggests that perhaps the expansion of RLS quals should be
 done in the rewriter. I've not thought that through in any detail, but
 ISTM that a RIR rule could add a table with a RLS qual, and a RLS qual
 could add a relation with a RIR rule that needs expanding, and so the
 2 need to be processed together. This could also make use of the
 existing recursion-checking code in the rewriter.

Thanks for your checks. I missed some cases that you suggested.

The reason why we need to put RLS expansion at planner stage is
requirement towards plan cache invalidation. Due to special case
handling for superuser, plan cache has to be invalidated if user-id
to run executor was switched since planner stage. The planner shall
be invoked again, but not rewritter, on its invalidation.

Probably, it make sense to invoke rewriter's logic to solve RLS
policy from planner stage (that allows plan-cache invalidation).
Let me investigate the code of rewriter.

Best regards,
-- 
KaiGai Kohei kai...@kaigai.gr.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] getopt() and strdup()

2012-10-08 Thread Andrew Dunstan


On 10/08/2012 02:40 PM, Bruce Momjian wrote:

A while ago I noticed that in some places we strdup/pg_strdup() optarg
strings from getopt(), and in some places we don't.

If we needed the strdup(), the missing cases should generate errors.  If
we don't need them, the strdup() is unnecessary, and research confirms
they are unnecessary.  Should we remove the extra strdup/pg_strdup()
calls, for consistency.

I think we might have had old platforms that required it, but none are
still supported today.



ISTR there was a requirement at least on some platforms to use 
strdup/pg_strdup if the individual argument could be deformed, or the 
argument vector could be deformed. But maybe my memory is astray.


I'm all for consistency, but only if we're darn sure it's not going to 
break things.


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] Add FET to Default and Europe.txt

2012-10-08 Thread Tom Lane
Christopher Browne cbbro...@gmail.com writes:
 The scenario where we could unambiguously include time zones is where
 the symbols are unique.  If we were to include *all* uniquely-named
 symbols, that would minimize the number of complaints about missing
 zones, whilst evading the cases where the symbols are non-unique.
 That might be worth considering, though it'll certainly attract
 complaints in that some odd-ball zones would be included whilst
 well-known ones wouldn't.

That sounds good in the abstract ... however, consider that two of the
ambiguous abbreviations are EST and CST, which means that taking a hard
line would piss off every American east of the Mississippi, likewise
over half of Canada, not to mention some proportion of Australians.
Can you say user revolt?  Projects have been forked for less.

We can't just refuse to deal with this ambiguity.  We have to have some
very-low-pain way to install settings that will please those large
fractions of our user base.  Moreover, if that very-low-pain way isn't
the exact same way it's been done for the last half dozen releases,
you'll already have managed to annoy those selfsame large fractions.
You'd better have a good reason for changing 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


Re: [HACKERS] embedded list v3

2012-10-08 Thread Andres Freund
Hi Peter,

On Monday, October 08, 2012 09:43:51 PM Peter Geoghegan wrote:
 Pendantry: This should be in alphabetical order:
 
 ! OBJS = stringinfo.o ilist.o
Argh. Youve said that before. Somehow I reintroduced it...

 I notice that the patch (my revision) produces a whole bunch of
 warnings like this with Clang, though not GCC:
 
 
 
 [peter@peterlaptop cache]$ clang -O2 -g -Wall -Wmissing-prototypes
 -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
 -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
 -fwrapv -fexcess-precision=standard -g -I../../../../src/include
 -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -I/usr/include/libxml2   -c -o
 catcache.o catcache.c -MMD -MP -MF .deps/catcache.Po
 clang: warning: argument unused during compilation:
 '-fexcess-precision=standard'
 catcache.c:457:21: warning: expression result unused [-Wunused-value]
 CatCache   *ccp = slist_container(CatCache, cc_next,
 cache_iter.cur);
 
 ^~
 ../../../../src/include/lib/ilist.h:721:3: note: expanded from macro
 'slist_container'
 (AssertVariableIsOfTypeMacro(ptr, slist_node *),...
  ^
 ../../../../src/include/c.h:735:2: note: expanded from macro
 'AssertVariableIsOfTypeMacro'
 StaticAssertExpr(__builtin_types_compatible_p(__typeof__(varname),
 typename), \
 ^
 ../../../../src/include/c.h:710:46: note: expanded from macro
 'StaticAssertExpr' ({ StaticAssertStmt(condition, errmessage); true; })
 ^
 ../../../../src/include/c.h:185:15: note: expanded from macro 'true'
 #define true((bool) 1)
  ^  ~
 
 *** SNIP SNIP SNIP ***
 
 catcache.c:1818:21: warning: expression result unused [-Wunused-value]
 CatCache   *ccp = slist_container(CatCache, cc_next,
 iter.cur); ^~~~
 ../../../../src/include/lib/ilist.h:722:3: note: expanded from macro
 'slist_container'
  AssertVariableIsOfTypeMacro(((type*)NULL)-membername,
 slist_node), \
  ^
 ../../../../src/include/c.h:735:2: note: expanded from macro
 'AssertVariableIsOfTypeMacro'
 StaticAssertExpr(__builtin_types_compatible_p(__typeof__(varname),
 typename), \
 ^
 ../../../../src/include/c.h:710:46: note: expanded from macro
 'StaticAssertExpr' ({ StaticAssertStmt(condition, errmessage); true; })
 ^
 ../../../../src/include/c.h:185:15: note: expanded from macro 'true'
 #define true((bool) 1)
  ^  ~
 28 warnings generated.
 
 
 
 I suppose that this is something that's going to have to be addressed
 sooner or later.
That looks like an annoying warning. Split of StaticAssertExpr into 
StaticAssertExpr and StaticAssertExprBool?

 This seems kind of arbitrary:
 
   /* The socket number we are listening for connections on */
   int PostPortNumber;
 +
   /* The directory names for Unix socket(s) */
   char   *Unix_socket_directories;
 +
   /* The TCP listen address(es) */
   char   *ListenAddresses;
Yep, no idea why I added the spaces.

 This looks funny:
 
 + #endif   /* defined(USE_INLINE) ||
 +  * 
 defined(ILIST_DEFINE_FUNCTIONS) */
 
 I tend to consider the 80-column thing a recommendation more than a
 requirement.
Thats pgindent's doing. I couldn't convince it not to do that short of making 
it a multiline comment with 's.

 A further stylistic gripe is that this:
 
 + #define dlist_container(type, membername, ptr)  
  
\
 + (AssertVariableIsOfTypeMacro(ptr, dlist_node *),
  \
 +  AssertVariableIsOfTypeMacro(((type *) NULL)-membername, dlist_node),  
 \ +((type *)((char *)(ptr) - offsetof(type, membername))) 
 
 \
 +  )
 
 Should probably look like this:
 
 + #define dlist_container(type, membername, ptr)  
  
\
 + (AssertVariableIsOfTypeMacro(ptr, dlist_node *),
  \
 +  AssertVariableIsOfTypeMacro(((type *) NULL)-membername, dlist_node),  
 \ +((type *)((char *)(ptr) - offsetof(type, membername
Why? I find the former better readable.

 This is a little unclear:
 
 +  * dlist_foreach (iter, db-tables)
 +  * {
 +  * // inside an *_foreach the iterator's .cur field can be used to
 access +  *  // the current element

 This comment:
 
 +  * Singly linked lists are *not* circularly linked (how could they be?)
 in +  * contrast to the doubly linked lists. As no pointer to the last
 list element
 
 Isn't quite accurate, if I've understood you correctly; it is surely
 possible in principle to have a circular singly-linked list.
Its complete crap. One shouldn't write 

Re: [HACKERS] getopt() and strdup()

2012-10-08 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 A while ago I noticed that in some places we strdup/pg_strdup() optarg
 strings from getopt(), and in some places we don't.

 If we needed the strdup(), the missing cases should generate errors.  If
 we don't need them, the strdup() is unnecessary, and research confirms
 they are unnecessary.  Should we remove the extra strdup/pg_strdup()
 calls, for consistency.

What research?  Given the number of different ways argv[] is handled
on different platforms (cf ps_status.c), I am very unwilling to trust
that it's safe to hang onto an argv string for long without strdup'ing
it.

 I think we might have had old platforms that required it, but none are
 still supported today.

And what's your grounds for stating that?  All the alternatives in
ps_status.c are still live code AFAICS.

My feeling is it's more likely to be a good idea to be adding strdup's
than removing them.

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] sortsupport for text

2012-10-08 Thread Robert Haas
On Mon, Oct 8, 2012 at 12:26 PM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 If it was the case that you were only 50% of the way to getting
 something committable, I guess I'd understand; this is, after all, a
 volunteer effort, and you are of course free to pursue or not pursue
 whatever you like. It's more like 95% though, so I have a hard time
 understanding this attitude.

 The only criticism that I imagine that you could be referring to is my
 criticism (which was seconded by Tom) concerning the approach to
 sizing a buffer that is used as state between successive calls to the
 sortsupport routine. This was a fairly trivial point even within the
 context of this patch. It was regrettable that it got so out of hand,
 but that certainly wasn't my intention.

 I must say that I find this disheartening as a reviewer, particularly
 since it comes from someone who reviews a lot of patches (including
 most of my own), and has often complained about the difficulties that
 reviewers face. Is it really so hard for you to accept a criticism
 from me? I didn't expect you to fully accept my criticism; I only
 expected you to take it into consideration, and possibly let it go for
 the sake of progress.

 For what it's worth, I have high regard for your work generally. In
 all sincerity, this appears to me to be a slight, and I find it
 upsetting, both on a personal level, and because it creates a concern
 about being able to work with you in the future, which is certainly
 something that I've benefited from in the past, and hope to continue
 to benefit from.

Hey, if me deciding I don't want to work on a patch any more is going
to make you feel slighted, then you're out of luck.  The archives are
littered with people who have decided to stop working on things
because the consensus position on list was different than the approach
that they personally favored, and I have as much right to do that as
anyone else.  I think that you are perfectly entitled to feel
disappointed that I don't like your suggestions, even as I am
disappointed that the patch encountered so much resistance.  But let's
not turn it into a personal issue.  I don't think that you're a bad
person because you don't like my approach, and I hope you don't think
I'm a bad person because I don't like yours.  If you do, then that's
just something we're both going to have to live with, because I am not
going to make a policy of working on things because other people will
be offended if I don't.

As to what the substantive issue is, well, yeah, I don't like the
memory allocation strategy that you and Tom are proposing.  In the
worst case it chews up a lot of extra memory, and in the best case
there's no measurable performance benefit - or at least nobody done
any work to demonstrate that there is one.  I've already pointed out
that, in fact, the strategy of doing power-of-two allocations is
rarely followed for very large allocations, a point to which I believe
no one has responded substantively.  Now I don't think anyone else is
required to respond to that point, but neither am I required to revise
the patch into a form that I don't agree with.  Equally, I will not
presume to commit it in a form that you and Tom do not agree with.
That would be asinine, and I try (not always successfully) not to be
an ass.  It is not as if anyone has phrased this as a maybe-we-should
sort of argument; there have been quite definite arguments on both
sides over apparently strongly-held positions.  I had hoped that this
was going to be a quick and non-controversial win, but 74 email
messages later it has become clear that it will be neither of those
things.

I do not have a problem with accepting review feedback from you or
from anyone else.  There are many examples in the archives of cases
where I have improved my code based on review feedback; indeed, the
possibility of getting high-quality feedback from the very smart
developers who inhabit this mailing list is for me a principle
attraction of working on PostgreSQL, not to mention a major reason why
PostgreSQL is as good a product as it is.  However, that general truth
does not oblige me to accept any particular piece of feedback as
well-founded, and this is hardly the first suggestion that I have
argued against in four years as a PostgreSQL developer, nor the first
of my patches to land on the cutting-room floor.  Nor do all of my
suggestions for other people's patches get adopted either.

-- 
Robert Haas
EnterpriseDB: 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: [HACKERS] why repl_gram.h?

2012-10-08 Thread Magnus Hagander
On Oct 6, 2012 3:00 AM, Peter Eisentraut pete...@gmx.net wrote:

 src/backend/replication/Makefile calls bison with -d to create a
 repl_gram.h header file, but that is not used anywhere.  Is this an
 oversight?

That's probably a copy/paste caused oversight. I don't recall any
particular reason for it.

/Magnus


[HACKERS] MemSetLoop ignoring the 'val' parameter

2012-10-08 Thread Andres Freund
Hi,

#define MemSetLoop(start, val, len) \
do \
{ \
long * _start = (long *) (start); \
long * _stop = (long *) ((char *) _start + (Size) (len)); \
\
while (_start  _stop) \
*_start++ = 0; \
} while (0)

The 'val' parameter is ignored.

Currently it doesn't matter because MemSetLoop is only used with a 0 parameter 
and only so in mcxt.c but it looks like it should be fixed anyway.

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] MemSetLoop ignoring the 'val' parameter

2012-10-08 Thread Andres Freund
On Monday, October 08, 2012 10:39:27 PM Andres Freund wrote:
 The 'val' parameter is ignored.
Trivial patch attached.
-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
From a4056e1110961c64b56d61a88c0d472c58a80579 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Mon, 8 Oct 2012 22:55:14 +0200
Subject: [PATCH] Fix MemSetLoop to not ignore the val parameter

This typo didn't have any bad consequences as the only in-core callsite passes
in 0 which is the value that was assigned instead of val anyway.
---
 src/include/c.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/c.h b/src/include/c.h
index 925d961..8c3ca8c 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -689,7 +689,7 @@ typedef NameData *Name;
 		long * _stop = (long *) ((char *) _start + (Size) (len)); \
 	\
 		while (_start  _stop) \
-			*_start++ = 0; \
+			*_start++ = val; \
 	} while (0)
 
 
-- 
1.7.12.289.g0ce9864.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] Support for REINDEX CONCURRENTLY

2012-10-08 Thread Jim Nasby

On 10/5/12 9:57 PM, Michael Paquier wrote:

In the current version of the patch, at the beginning of process a new index is 
created. It is a twin of the index it has to replace, meaning that it copies 
the dependencies of old index and creates twin entries of the old index even in 
pg_depend and pg_constraint also if necessary. So the old index and the new 
index have exactly the same data in catalog, they are completely decoupled, and 
you do not need to worry about the OID replacements and the visibility 
consequences.


Yeah, what's the risk to renaming an index during concurrent access? The only thing I can 
think of is an old backend referring to the wrong index name in an elog. 
That's certainly not great, but could possibly be dealt with.

Are there any other things that are directly tied to the name of an index (or 
of any object for that matter)?
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] sortsupport for text

2012-10-08 Thread Peter Geoghegan
On 8 October 2012 21:35, Robert Haas robertmh...@gmail.com wrote:
 Hey, if me deciding I don't want to work on a patch any more is going
 to make you feel slighted, then you're out of luck.  The archives are
 littered with people who have decided to stop working on things
 because the consensus position on list was different than the approach
 that they personally favored, and I have as much right to do that as
 anyone else.

Sure you do. However, I doubt very many of those who gave up did so
over so trivial an issue as to how to grow a buffer somewhere, and
those that did usually did not go on to become major contributors, and
certainly not committers. The buffer thing is, as far as I know, the
single solitary point of contention with this patch. We're talking
about 2 lines of code. To give up now is just petulant. There is no
other way of looking at it.

 It is not as if anyone has phrased this as a maybe-we-should
 sort of argument; there have been quite definite arguments on both
 sides over apparently strongly-held positions.  I had hoped that this
 was going to be a quick and non-controversial win, but 74 email
 messages later it has become clear that it will be neither of those
 things.

Many of those 74 emails concerned my completely unrelated digression
into exploiting strxfrm(); we spent a ridiculously long time
discussing how to size this buffer, but it still wasn't anything like
74 messages.

-- 
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] Placement of permissions checks for large object operations

2012-10-08 Thread Tom Lane
While looking at the 64-bit large object patch, I happened to notice
that the LO permissions checks that were added a release or two back
seem to be done exceedingly inefficiently.  To wit, they're done in
lo_read() and lo_write(), which means that if the user say reads an 8MB
large object with an 8K buffer size, we'll repeat the same permissions
check 1000 times.

This is particularly bizarre on the read side, where the code has gone
to probably-unreasonable lengths to make dead certain that it will get
the same answer each time.  But even on the write side, it's not
apparent to me that it's useful or sensible to allow a few writes and
then stop allowing them if some other transaction commits an ACL change
meanwhile.  (There would be a race condition there anyway, since our
transaction might examine the ACL just before somebody else changes it.)

I thought about proposing that the permissions checks be done in lo_open
instead.  However, that would result in semantic changes --- for
instance, it's legal and sometimes useful to supply INV_WRITE and not
actually write, if you want up-to-date read results.  So we can't really
enforce permissions at open time based on the supplied flags.

However, it wouldn't be hard to make the code apply the checks at most
once per lo_open, by keeping flags in the LargeObjectDesc entries
showing whether we've already checked read or write privilege on each
descriptor.  So the check would be made only during the first lo_read
or lo_write call on a given descriptor.

Comments?

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] MemSetLoop ignoring the 'val' parameter

2012-10-08 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 The 'val' parameter is ignored.

This is not broken.  Read the comments for MemSetTest.

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] MemSetLoop ignoring the 'val' parameter

2012-10-08 Thread Andres Freund
On Tuesday, October 09, 2012 12:56:16 AM Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  The 'val' parameter is ignored.
 
 This is not broken.  Read the comments for MemSetTest.
Ah. I was surprised about that already. The comment says that val has to be 
constant though, not that it has to be zero. In my understanding 1 is constant 
as well. Also, why do we even pass in a 'val' parameter in that case?

Feeling a little slow here...

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] Support for REINDEX CONCURRENTLY

2012-10-08 Thread Jim Nasby

On 10/8/12 6:12 PM, Tom Lane wrote:

Jim Nasby j...@nasby.net writes:

Yeah, what's the risk to renaming an index during concurrent access?


SnapshotNow searches for the pg_class row could get broken by *any*
transactional update of that row, whether it's for a change of relname
or some other field.

A lot of these problems would go away if we rejiggered the definition of
SnapshotNow to be more like MVCC.  We have discussed that in the past,
but IIRC it's not exactly a simple or risk-free change in itself.
Still, maybe we should start thinking about doing that instead of trying
to make REINDEX CONCURRENTLY safe given the existing infrastructure.


Yeah, I was just trying to remember what other situations this has come up in. 
My recollection is that there's been a couple other cases where that would be 
useful.

My recollection is also that such a change would be rather large... but it 
might be smaller than all the other work-arounds that are needed because we 
don't have that...
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] Support for REINDEX CONCURRENTLY

2012-10-08 Thread Jim Nasby

On 10/8/12 5:08 PM, Andres Freund wrote:

On Monday, October 08, 2012 11:57:46 PM Jim Nasby wrote:

On 10/5/12 9:57 PM, Michael Paquier wrote:

 In the current version of the patch, at the beginning of process a new
 index is created. It is a twin of the index it has to replace, meaning
 that it copies the dependencies of old index and creates twin entries of
 the old index even in pg_depend and pg_constraint also if necessary. So
 the old index and the new index have exactly the same data in catalog,
 they are completely decoupled, and you do not need to worry about the
 OID replacements and the visibility consequences.


Yeah, what's the risk to renaming an index during concurrent access? The
only thing I can think of is an old backend referring to the wrong index
name in an elog. That's certainly not great, but could possibly be dealt
with.

We cannot have two indexes with the same oid in the catalog, so the two
different names will have to have different oids. Unfortunately the indexes oid
is referred to by other tables (e.g. pg_constraint), so renaming the indexes
while differering in the oid isn't really helpful :(...


Hrm... the claim was made that everything relating to the index, including 
pg_depend and pg_contstraint, got duplicated. But I don't know how you could 
duplicate a constraint without also playing name games. Perhaps name games are 
being played there as well...
 

Right now I don't see anything that would make switching oids easier than
relfilenodes.


Yeah... in order to make either of those schemes work I think there would need 
to non-trivial internal changes so that we weren't just passing around raw 
OIDs/filenodes.

BTW, it occurs to me that this problem might be easier to deal with if we had 
support for accessing the catalog with the same snapshot as the main query was 
using... IIRC that's been discussed in the past for other issues.
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] PQping command line tool

2012-10-08 Thread Jim Nasby

On 10/4/12 11:34 AM, Greg Sabino Mullane wrote:

I was wondering recently if there was any command line tool that
utilized PQping() or PQpingParams(). I searched the code and couldn't
find anything and was wondering if there was any interest to have
something like this included? I wrote something for my purposes of
performing a health check that also supports nagios style status
output. It's probably convenient for scripting purposes as well.

I'm not sure how useful this information would be. Most health
checks (Nagios or otherwise) really only care if things are
working all the up to point A or not, where point A is usually
a simple query such as SELECT 1. Knowing various failure states
as returned by PQping* does not seem to fit into such tools -
any failure needs to be handled manually.


For whatever reason our Nagios setup telnets to the port to see if it's open, 
which means we get a ton of messages in the log about authentication errors or 
some such. It'd be useful to us to have a utility that could cleanly validate 
the server was up and communicating, without having to actually login.
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] Deparsing DDL command strings

2012-10-08 Thread Jim Nasby

On 10/5/12 11:15 AM, Tom Lane wrote:

Also, we need to normalize that command string. Tools needing to look at
it won't want to depend on random white spacing and other oddities.

Instead, they'll get to depend on the oddities of parse transformations
(ie, what's done in the raw grammar vs. what's done in parse_analyze),
which is something we whack around regularly.  Besides which, you've
merely asserted this requirement without providing any evidence that
it's important at all, let alone important enough to justify the kind of
maintenance burden that you want to saddle us with.


I definitely want to be able to parse DDL commands to be able to either enforce 
things or to drive other parts of the system based on what's changing. Without 
the ability to capture (and parse) DDL commands I'm stuck creating wrapper 
functions around anything I want to capture and then trying to ensure that 
everyone uses the wrappers and not the raw DDL commands.

Event triggers that just spit out raw SQL give me the first part of this, but 
not the second part: I'm still stuck trying to parse raw SQL on my own. Having 
normalized SQL to parse should make that a bit easier, but ideally I'd like to 
be able to pull specific elements out of a command. I'd want to be able to do 
things like:

IF command is ALTER TABLE THEN
  FOR EACH subcommand
IF subcommand IS DROP COLUMN THEN
  do something that needs to know what column is being dropped
ELSE IF subcommand IS ADD COLUMN THEN
  do something that needs to know the definition of the column being added

I don't think every bit of that has to be dealt with by the event trigger code 
itself. For example, if you're adding a column to a table and the entries have 
already been made in the catalog, you could query to get the details of the 
column definition if you were given an OID into pg_attributes.

Having said all that, an event system that spits back the raw SQL would 
certainly be better than nothing. But realize that people would still need to 
do parsing on it (ie: replication solutions will need to know what table just 
got ALTER'd).
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] MemSetLoop ignoring the 'val' parameter

2012-10-08 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On Tuesday, October 09, 2012 12:56:16 AM Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
 The 'val' parameter is ignored.

 This is not broken.  Read the comments for MemSetTest.

 Ah. I was surprised about that already. The comment says that val has to be 
 constant though, not that it has to be zero. In my understanding 1 is 
 constant 
 as well. Also, why do we even pass in a 'val' parameter in that case?

Well, first off, the callers should not be aware of the detail that
MemSetTest insists on a val of zero, so they have to pass val even
though it's unused by the current implementation of MemSetLoop.

The callers are responsible for not passing a volatile value there, but
it's hard to dodge that problem given that we're dealing with macros;
if the value changes on repeat evaluation we're screwed anyway.

However, nonvolatile is not constant.  For instance, it's perfectly
fine to pass MemSetTest/Loop a variable for the val that is sometimes
zero and sometimes not.  If we changed the coding as you suggest, the
compiler would probably generate less efficient code since it wouldn't
realize (unless it was quite smart) that MemSetLoop is always filling
with zeroes.

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] getopt() and strdup()

2012-10-08 Thread Bruce Momjian
On Mon, Oct  8, 2012 at 04:33:29PM -0400, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  A while ago I noticed that in some places we strdup/pg_strdup() optarg
  strings from getopt(), and in some places we don't.
 
  If we needed the strdup(), the missing cases should generate errors.  If
  we don't need them, the strdup() is unnecessary, and research confirms
  they are unnecessary.  Should we remove the extra strdup/pg_strdup()
  calls, for consistency.
 
 What research?  Given the number of different ways argv[] is handled
 on different platforms (cf ps_status.c), I am very unwilling to trust
 that it's safe to hang onto an argv string for long without strdup'ing
 it.
 
  I think we might have had old platforms that required it, but none are
  still supported today.
 
 And what's your grounds for stating that?  All the alternatives in
 ps_status.c are still live code AFAICS.
 
 My feeling is it's more likely to be a good idea to be adding strdup's
 than removing them.

Well, what we have now is either wrong or over-kill --- I don't know for
sure which.

-- 
  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] Support for REINDEX CONCURRENTLY

2012-10-08 Thread Michael Paquier
On Tue, Oct 9, 2012 at 8:14 AM, Jim Nasby j...@nasby.net wrote:

 Hrm... the claim was made that everything relating to the index, including
 pg_depend and pg_contstraint, got duplicated. But I don't know how you
 could duplicate a constraint without also playing name games. Perhaps name
 games are being played there as well...

Yes, it is what was originally intended. Please note the pg_constraint
entry was not duplicated correctly in the first version of the patch
because of a bug I already fixed.
I will provide another version soon if necessary.





 Right now I don't see anything that would make switching oids easier than
 relfilenodes.


 Yeah... in order to make either of those schemes work I think there would
 need to non-trivial internal changes so that we weren't just passing around
 raw OIDs/filenodes.

 BTW, it occurs to me that this problem might be easier to deal with if we
 had support for accessing the catalog with the same snapshot as the main
 query was using... IIRC that's been discussed in the past for other issues.

Yes, it would be better and helpful to have such a mechanism even for other
operations.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] Add FET to Default and Europe.txt

2012-10-08 Thread Joachim Wieland
On Mon, Oct 8, 2012 at 4:00 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 We can't just refuse to deal with this ambiguity.  We have to have some
 very-low-pain way to install settings that will please those large
 fractions of our user base.  Moreover, if that very-low-pain way isn't
 the exact same way it's been done for the last half dozen releases,
 you'll already have managed to annoy those selfsame large fractions.
 You'd better have a good reason for changing it.

As previously noted, there are two topics that are being discussed here:

- how to allow users to configure their own timezone abbreviations
and
- how to keep the timezone abbreviations that we ship updated wrt zic changes

Regarding configurability, we currently allow users (=administrators)
to change their abbreviations to whatever they like to use. The
Default file we provide has been taken from the set that used to be
a list that we compiled in back in the 8.x days (and we had this
egregious GUC australian_timezones that decided whether CST/EST was
regarded to be US or Australian timezones - there was no such hack for
any of the other ambiguities).

These timezone abbreviations have developed over several decades, most
of these decades without global communication in mind. They are full
of inconsistencies and irregularities and IMHO any way to select a
proper set for someone automatically is doomed to solve the problem
only partially.

Another funny ambiguity is that the EST timezone in Austalia could
mean both Eastern Standard Time and Eastern Summer Time, having
different offsets depending on what month of the year you're in...

The fact that we don't hear more complaints about the sets actually
suggests that most people are happy with our Default set. In fact,
Marc could just add his FET timezone to his own installations and use
it from there. His request to add it to our Default set however is
perfectly valid and should be discussed.

One thing that could be improved about configurability is the fact
that if you're not an administrator of the database, i.e. if you don't
have write access to where the timezones are defined, you're pretty
much out of luck being able to define your own abbreviations. This is
true in a shared hosting environment for example.


Wrt updating the timezone abbreviation files, I guess what we need is
a parser for the zic database, our own timezone files and a script
that compares the two datasets and spits out any conflicts so that we
can clean them up after manual inspection of the differences. I will
see if I can come up with some scripts in this direction.


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


[HACKERS] TODO item: teach pg_dump about sparsely-stored large objects

2012-10-08 Thread Tom Lane
The backend code for large objects goes to some lengths to be
intelligent about sparsely-written blobs: if you seek out to the middle
of nowhere and write a few bytes, you don't end up allocating space in
pg_largeobject for all the byte positions you skipped over.  However,
pg_dump knows nothing of this.  If you pg_dump a sparsely-stored large
object, it will tediously transfer all those nonexistent zeros from
server to client, and write them into the resulting archive.  And then
when you restore, the blob isn't sparse anymore ... those zeroes become
non-virtual on the database side too.

Admittedly, this is no different than what happens when you try to back
up a sparsely-stored Unix file, at least with simpler backup tools.
But it seems to me we should try a bit harder.

There are a couple of stumbling blocks to making that happen:

* How should pg_dump find out where there are holes?  It would be easy
if it were to look into pg_largeobject, but that would destroy the
ability to use pg_dump as non-superuser.  I think we'd really have to
provide some API to read from a blob in a sparse-storage-aware manner.
The first idea that comes to mind is some way to tell lo_read to stop
reading when it hits a gap (instead of manufacturing zeroes) and then
a new whence option for lo_lseek that tells it to seek to the next
non-dummy data in the blob.

* How do we get pg_dump to make use of the knowledge once it's got it?
The current code in that area is a masterpiece of ugly unreadability;
it's near impossible to tell what connects to what else, and there are
assorted magic switches that completely change the behavior of major
interface functions.  I'd kind of want to rewrite the whole mess before
trying to change its behavior.  I'm also pretty certain that we'd need
an archive format change, though we've certainly done those before so
that isn't a fatal objection.

I'm not planning to do anything about this myself, but if someone is
looking for a project, here's one.

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] Visual Studio 2012 RC

2012-10-08 Thread Noah Misch
One more thing -- I tried a build with NLS support and encountered this:

  src\backend\utils\adt\pg_locale.c(746): error C2039: 'lc_handle' : is not a 
member of 'threadlocaleinfostruct' 
[c:\cygwin\home\nm\src\pg\postgresql\postgres.vcxproj]

That's in the function IsoLocaleName(), which digs around inside a locale_t.
I suppose the (undocumented) content of that structure changed in this newer
CRT (MSVCR110D).

I apologize for the slow drip of problem reports; I just happen to be trying
additional configurations with your patch as a foundation.


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