Re: [HACKERS] Full GUID support

2011-07-04 Thread Dave Page
Should be in 9.0.5/9.1b3

On Sunday, July 3, 2011, Michael Gould
mgo...@intermodalsoftwaresolutions.net wrote:
gt; Does this look to be something that will surface around for 9.1
gt;
gt; Sent from Samsung mobile
gt;
gt; Dave Page lt;dp...@pgadmin.orggt; wrote:
gt;
gt;gt;On Sunday, July 3, 2011, Michael Gould
gt;gt;lt;mgo...@intermodalsoftwaresolutions.netgt; wrote:
gt;gt;amp;gt; Peter,
gt;gt;amp;gt;
gt;gt;amp;gt; I don't believe that the library that the contrib
module runs with can run
gt;gt;amp;gt; on Window 64 bit servers or even Windows 7 64 bit.
That is problem as most
gt;gt;amp;gt; shops are using 64 bit OS and if Window the contrib module is
gt;gt;going to fail.
gt;gt;amp;gt;  Taking the responsibility to handle this internally
means that you can
gt;gt;amp;gt; write your own implementation not based on a libary
that can't handle
gt;gt;amp;gt; Windows 64 bit.
gt;gt;
gt;gt;The next release of the installers will, now that we have a 64 Windows
gt;gt;port of ossp-uuid.
gt;gt;
gt;gt;Even If that weren't the case, integrating the type wouldn't fix the
gt;gt;problem anyway, unless you're suggesting we implement our own UUID
gt;gt;generator (which isn't nearly as straightforward as it might seem, as
gt;gt;I understand it)..
gt;gt;
gt;gt;--
gt;gt;Dave Page
gt;gt;Blog: http://pgsnake.blogspot.com
gt;gt;Twitter: @pgsnake
gt;gt;
gt;gt;EnterpriseDB UK: http://www.enterprisedb.com
gt;gt;The Enterprise PostgreSQL Company
gt;gt;
gt;gt;--
gt;gt;Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
gt;gt;To make changes to your subscription:
gt;gt;http://www.postgresql.org/mailpref/pgsql-hackers
gt;

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: 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] flex on win64 - workaround for flex: fatal internal error, exec failed

2011-07-04 Thread Brar Piening

schrieb Craig Ringer:
I haven't managed to figure out exactly what's broken. For the benefit 
of anyone else having problems like this or who might find this via a 
search later, though: just install msys and use the current flex from 
msys. It works perfectly in Pg's build environment and it's easy to 
install, so you don't need to stuff around trying to get flex to work.


Grab mingw-get-inst from 
http://sourceforge.net/projects/mingw/files/Automated%20MinGW%20Installer/mingw-get-inst/ 
and use it to install msys and the minimalist mingw. You won't be 
using mingw, but it won't hurt you, and it's easier than installing 
msys standalone.


Edit src\tools\msvc\buildenv.pl (or create it if necessary) and use it 
to add C:\MinGW\msys\1.0\bin to your PATH. Adjust as appropriate if 
your MinGW install dir was different.  Eg:


$ENV{PATH}='C:\MinGW\msys\1.0\bin;' .  $ENV{PATH};


As you might also want to use git for developing with postgres its 
possible that all you need is already in place.


As I use msysgit my usual way of dealing with flex and bison 
dependencies is putting $ENV{PATH}=$ENV{PATH} . ';C:\Program Files 
(x86)\Git\bin'; into my buildenv.pl


Regards,

Brar



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


[HACKERS] Potential NULL dereference found in typecmds.c

2011-07-04 Thread Michael Mueller
Hi folks,

Sentry found this error last night, and it looks serious enough to
report.  The error was introduced in commit 426cafc.  Here's the code
in question, starting at line 2096:

if (!found)
{
con = NULL; /* keep compiler quiet */
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg(constraint \%s\ of domain \%s\ does not exist,
constrName, NameStr(con-conname;
}

It sets 'con' to NULL and then in the next statement, dereferences it.
I'm not sure if it's possible to reach this path, but if it is
reachable it will cause a crash.

Best Regards,
Mike

-- 
Mike Mueller
Phone: (401) 405-1525
Email: mmuel...@vigilantsw.com

http://www.vigilantsw.com/

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


Re: [HACKERS] Full GUID support

2011-07-04 Thread Michael Gould
Dave,

This is wonderful news.

Best Regards

Michael Gould




Dave Page dp...@pgadmin.org wrote:
 Should be in 9.0.5/9.1b3
 
 On Sunday, July 3, 2011, Michael Gould
 mgo...@intermodalsoftwaresolutions.net wrote:
  Does this look to be something that will surface around for 9.1
 
  Sent from Samsung mobile
 
  Dave Page dp...@pgadmin.org wrote:
 
 On Sunday, July 3, 2011, Michael Gould
 mgo...@intermodalsoftwaresolutions.net wrote:
  Peter,
 
  I don't believe that the library that the contrib
 module runs with can run
  on Window 64 bit servers or even Windows 7 64 bit.
 That is problem as most
  shops are using 64 bit OS and if Window the contrib module is
 going to fail.
   Taking the responsibility to handle this internally
 means that you can
  write your own implementation not based on a libary
 that can't handle
  Windows 64 bit.
 
 The next release of the installers will, now that we have a 64 Windows
 port of ossp-uuid.
 
 Even If that weren't the case, integrating the type wouldn't fix the
 problem anyway, unless you're suggesting we implement our own UUID
 generator (which isn't nearly as straightforward as it might seem, as
 I understand it)..
 
 --
 Dave Page
 Blog: http://pgsnake.blogspot.com
 Twitter: @pgsnake
 
 EnterpriseDB UK: 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
 
 
 -- 
 Dave Page
 Blog: http://pgsnake.blogspot.com
 Twitter: @pgsnake
 
 EnterpriseDB UK: 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
 

--
Michael Gould, Managing Partner
Intermodal Software Solutions, LLC
904.226.0978
904.592.5250 fax



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


[HACKERS] keepalives_* parameters usefullness

2011-07-04 Thread Jaime Casanova

Hi,

AFAIU TFM if i set keepalives_* parameters in a conninfo they set the
internal counters to these values so if i execute:


conn = PQconnectdb(host=192.168.204.10 keepalives=1 keepalives_idle=45 
keepalives_interval=5 keepalives_count=5); 


that means that the client's connection to the server in 192.168.204.10
will wait 45 seconds after the last packet sent to the server and then
will sent a probe every 5 seconds, 5 times...

if all of the above is right then if in the client i execute a PQexec() when
the connection has already drop it should detect the failure after 1
minute 10 seconds. is that right or am i misunderstanding this?

the reason i ask is that when i use that exact conninfo it detects the
failure condition after 15 minutes always... well, actually it waits the
same time even if i doesn't set anything...

i even used getsockopt() to ensure TCP_KEEPIDLE was being setted and
tried to set it myself with setsockopt() with the same results.

BTW, this paper (http://es.scribd.com/doc/2586622/tcpkeepalivehowto) on
section 4.2. The setsockopt function call page 9 says that to set
TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT we should use level SOL_TCP
but on src/interfaces/libpq/fe-connect.c we use IPPROTO_TCP instead.

any leads?

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL 
Soporte 24x7, desarrollo, capacitación y servicios

-- 
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] keepalives_* parameters usefullness

2011-07-04 Thread Simon Riggs
On Mon, Jul 4, 2011 at 9:42 AM, Jaime Casanova ja...@2ndquadrant.com wrote:

 i even used getsockopt() to ensure TCP_KEEPIDLE was being setted and
 tried to set it myself with setsockopt() with the same results.

There's a list of preconditions as to when these settings take effect,
otherwise they are a no-op.

-- 
 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] Online base backup from the hot-standby

2011-07-04 Thread Jun Ishiduka

 When the standby restarts after it crashes during recovery, it always
 checks whether recovery has reached the backup end location by
 using minRecoveryPoint even though the standby doesn't start from
 the backup. This looks odd.

Certainly.

But, in this case, the state before recovery starts is lost.
Therefore, postgres can not see that the backup got from whether 
standby server or master.

What should?
Should use pg_control?
 Ex. 
   * Add 'Where to get backup' to pg_control. (default 'none')
   * When recovery starts, it checks it whether 'none'.
  * When minRecoveryPoint equals 0/0, change 'master'.
  * When minRecoveryPoint do not equals 0/0, change 'slave'.
   * When it reached the end of recovery, change 'none' .


 - XLogRecPtrIsInvalid(ControlFile-backupStartPoint))
 + (XLogRecPtrIsInvalid(ControlFile-backupStartPoint) ||
 +  reachedControlMinRecoveryPoint == true))

 The flag 'reachedControlMinRecoveryPoint' is really required? When recovery
 reaches minRecoveryPoint, ControlFile-backupStartPoint is reset to zero. So
 we can check whether recovery has reached minRecoveryPoint or not by only
 doing XLogRecPtrIsInvalid(ControlFile-backupStartPoint). No?

Yes.
'reachedControlMinRecoveryPoint' is unnecessary.


 We should check if recovery has reached minRecoveryPoint before calling
 CheckRecoveryConsistency() after reading new WAL record? Otherwise,
 even if recovery has reached minRecoveryPoint, the standby cannot think
 that it's in consistent state until it reads new WAL record.

This is a same sequence with a case of backup end location.
It should be no changed.


 + if 
 (XLByteLT(ControlFile-minRecoveryPoint, EndRecPtr))
 + 
 ControlFile-minRecoveryPoint = EndRecPtr;

 Why does ControlFile-minRecoveryPoint need to be set to EndRecPtr?

Yes.
I delete it.


Jun Ishizuka
NTT Software Corporation
TEL:045-317-7018
E-Mail: ishizuka@po.ntts.co.jp




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


Re: [HACKERS] flex on win64 - workaround for flex: fatal internal error, exec failed

2011-07-04 Thread Craig Ringer

On 4/07/2011 2:36 PM, Brar Piening wrote:


As you might also want to use git for developing with postgres its
possible that all you need is already in place.

As I use msysgit my usual way of dealing with flex and bison
dependencies is putting $ENV{PATH}=$ENV{PATH} . ';C:\Program Files
(x86)\Git\bin'; into my buildenv.pl


Good thought. I hadn't realized msysgit bundled flex and bison - that's 
really handy. I generally use git from a `vsvars' cmd.exe shell not bash 
simply because some tools don't play well with bash, and I only have git 
on the PATH so I didn't realize flex was bundled. Handy!


I wonder if the docs should be recommending using msys's flex and bison 
(via msysgit or mingw) rather than GnuWin32? Pointing to a matching m4, 
flex and bison that're kept up to date and easy to install might be a 
good thing.


--
Craig Ringer

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


Re: [HACKERS] Cascade replication

2011-07-04 Thread Simon Riggs
On Tue, Jun 14, 2011 at 6:08 AM, Fujii Masao masao.fu...@gmail.com wrote:

 The standby must not accept replication connection from that standby itself.
 Otherwise, since any new WAL data would not appear in that standby,
 replication cannot advance any more. As a safeguard against this, I 
 introduced
 new ID to identify each instance. The walsender sends that ID as the fourth
 field of the reply of IDENTIFY_SYSTEM, and then walreceiver checks whether
 the IDs are the same between two servers. If they are the same, which means
 that the standby is just connecting to that standby itself, so walreceiver
 emits ERROR.

Thanks for waiting for review.

This part of the patch is troubling me. I think you have identified an
important problem, but this solution doesn't work fully.

If we allow standbys to connect to other standbys then we have
problems with standbys not being connected to master. This can occur
with a 1-step connection, as you point out, but it could also occur
with a 2-step, 3-step or more connection, where a circle of standbys
are all depending upon each other. Your solution only works for 1-step
connections. Solving that problem in a general sense might be more
dangerous than leaving it alone. I think we should think some more
about the issues there and approach them as a separate problem.

I think we should remove that and just focus on the main problem, for
now. That will make it a simpler patch and easier to commit.

-- 
 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] Crash dumps

2011-07-04 Thread Radosław Smogura

On Mon, 04 Jul 2011 12:58:46 +0800, Craig Ringer wrote:

On 15/06/2011 2:37 AM, Radosław Smogura wrote:

Hello,

Because, I work a little bit on streaming protocol and from time to 
time
I have crashes. I want ask if you wont crash reporting (this is one 
of

minors products from mmap playing) those what I have there is mmaped
areas, and call stacks, and some other stuff.


Core files already contain all that, don't they? They omit shared
memory segments by default on most platforms, but should otherwise be
quite complete.

The usual approach on UNIXes and linux is to use the built-in OS
features to generate a core dump of a crashing process then analyze 
it
after the fact. That way the crash is over as fast as possible and 
you
can get services back up and running before spending the time, CPU 
and

I/O required to analyze the core dump.


Actually this, what I was thinking about was, to add dumping of GUC, 
etc. List of mappings came from when I tired to mmap PostgreSQL, and due 
to many of errors, which sometimes occurred in unexpected places, I was 
in need to add something that will be useful for me and easy to analyse 
(I could simple find pointer, and then check which region failed). The 
idea to try to evolve this come later.


I think report should looks like:
This is crash report of PostgreSQL database, generated on
Here is list of GUC variables:
Here is list of files:
Here is backtrace:
Here is detailed backtrace:
Here is list of m-mappings (you may get what library are linked in)
Here is your free memory
Here is your disk usage
Here is your custom addition


This based reports works
for Linux with gdb, but there is some pluggable architecture, which
connects with segfault


Which process does the debugging? Does the crashing process fork() a
copy of gdb to debug its self?

One thing I've been interested in is giving the postmaster (or more
likely a helper for the postmaster) the ability to handle backend is
crashing messages, attach a debugger to the crashing backend and
generate a dump and/or backtrace. This might be workable in cases
where in-process debugging can't be done due to a smashed stack, full
heap causing malloc() failure, etc.


Currently I do everything in segfault handler (no fork), but I like the 
idea of fork (in segfault), this may resolve some problems.


Regards,
Radosław Smogura


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


Initial Review: JSON contrib modul was: Re: [HACKERS] Another swing at JSON

2011-07-04 Thread Bernd Helmle



--On 18. Juni 2011 12:29:38 +0200 Bernd Helmle maili...@oopsware.de wrote:


Similar problems occur with a couple other modules I tried (hstore,
intarray).


Hmm, works for me. Seems you have messed up your installation in some way
(build against current -HEAD but running against a 9.1?).

I'm going to review in the next couple of days again.


A bit later than expected, but here is my summary on the patch:

-- Patch Status

The patch applies cleanly. Since it's primarily targeted as an addition to 
contrib/, it doesn't touch the backend source tree (besides documentation TOC 
entries), but adds a new subdirectory in contrib/json. The patch is in context 
diff as requested.


-- Functionality

The patch as is provides a basic implementation for a JSON datatype. It 
includes normalization and validation of JSON strings. It adds the datatype 
implementation as an extension.


The implementation focus on the datatype functionality only, there is no 
additional support for special operators. Thus, i think the comments in the 
control file (and docs) promises actually more than the contrib module will 
deliver:


+comment = 'data type for storing and manipulating JSON content'

I'm not sure, if manipulating is a correct description. Maybe i missed it, 
but i didn't see functions to manipulate JSON strings directly, for example, 
adding an object to a JSON string, ...


-- Coding

The JSON datatype is implemented as a variable length character type, thus 
allows very large JSON strings to be stored. It transparently decides when to 
escape unicode code points depending on the selected client- and 
server-encoding.


Validation is done through its own JSON parser. The parser itself is a 
recursive descent parser. It is noticable that the parser seems to define its 
own is_space() and is_digit() functions, e.g.:


+#define is_space(c) ((c) == '\t' || (c) == '\n' || (c) == '\r' || (c) == ' ')

Wouldn't it be more clean to use isspace() instead?

This code block in function json_escape_unicode() looks suspicious:

+   /* Convert to UTF-8, if necessary. */
+   {
+   const char *orig = json;
+   json = (const char *)
+   pg_do_encoding_conversion((unsigned char *) json, length,
+ GetDatabaseEncoding(), PG_UTF8);
+   if (json != orig)
+   length = strlen(json);
+   }

Seems it *always* wants to convert the string. Isn't there a if condition 
missing?


There's a commented code fragment missing, which should be removed (see last 
two lines of the snippet below):


+static unsigned int
+read_hex16(const char *in)
+{
+   unsigned int i;
+   unsigned int tmp;
+   unsigned int ret = 0;
+
+   for (i = 0; i  4; i++)
+   {
+   char c = *in++;
+
+   Assert(is_hex_digit(c));
+
+   if (c = '0'  c = '9')
+   tmp = c - '0';
+   else if (c = 'A'  c = 'F')
+   tmp = c - 'A' + 10;
+   else /* if (c = 'a'  c = 'f') */
+   tmp = c - 'a' + 10;

json.c introduces new appendStringInfo* functions, e.g.
appendStringInfoEscape() and appendStringInfoUtf8(). Maybe it's better
to name them to something different, since it may occur someday that the backend
will introduce the same notion with a different meaning. They are static,
but why not naming them into something like jsonAppendStringInfoUtf8() or 
similar?


-- Regression Tests

The patch includes a fairly complete regression test suite, which covers much
of the formatting, validating and input functionality of the JSON datatype. It 
also tests UNICODE sequences and input encoding with other server encoding than 
UTF-8.



--
Thanks

Bernd

--
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] Crash dumps

2011-07-04 Thread Craig Ringer

On 4/07/2011 7:03 PM, Radosław Smogura wrote:


Actually this, what I was thinking about was, to add dumping of GUC,
etc. List of mappings came from when I tired to mmap PostgreSQL, and due
to many of errors, which sometimes occurred in unexpected places, I was
in need to add something that will be useful for me and easy to analyse
(I could simple find pointer, and then check which region failed). The
idea to try to evolve this come later.


Why not produce a tool that watches the datadir for core files and 
processes them? Most but not all of the info you listed should be able 
to be extracted from a core file. Things like GUCs should be extractable 
with a bit of gdb scripting - and with much less chance of crashing than 
trying to read them from a possibly corrupt heap within a crashing backend.


To capture any information not available from the core, you can enlist 
the postmaster's help. It gets notified when a child crashes and should 
be able to capture things like the memory and disk state. See void 
reaper(SIGNAL_ARGS) in postmaster.c and HandleChildCrash(...) . If 
nothing else, the postmaster could probably fork a child crashed 
helper to collect data, analyse the core file, email the report to the 
admin, etc.


About the only issue there is that the postmaster relies on the exit 
status to trigger the reaper code. Once an exit status is available, the 
crashed process is gone, so the free memory will reflect the memory 
state after the backend dies, and shared memory's state will have moved 
on from how it was when the backend was alive.


For that reason, it'd be handy if a backend could trap SIGSEGV and 
reliably tell the postmaster I'm crashing! so the postmaster could 
fork a helper to capture any additional info the backend needs to be 
alive for. Then the helper can gcore() the backend, or the backend can 
just clear the SIGSEGV handler and kill(11) its self to keep on crashing 
and generate a core.


Unfortunately, reliably and segfault don't go together. You don't 
want a crashing postmaster writing to shared memory so it can't use shm 
to tell the postmaster it's dying. Signals are ... interesting ... at 
the best of times, but would probably still be the best bet. The 
postmaster could install a SIGUSR[whatever] or RT signal handler that 
takes a siginfo so it knows the pid of the signal sender. The crashing 
backend could signal the postmaster with an agreed signal to say I'm 
crashing and let the postmaster clean it up. The problem with this is 
that a lost signal (for any reason) would cause a zombie backend to hang 
around waiting to be killed by a postmaster that never heard it was 
crashing.



BTW, the win32 crash dump handler would benefit from being able to use 
some of the same facilities. In particular, being able to tell the 
postmaster Argh, ogod I'm crashing, fork something to dump my core! 
rather than trying to self-dump would be great. It'd also allow the 
addition of extra info like GUC data, last few lines of logs etc to the 
minidump, something that the win32 crash dump handler cannot currently 
do safely.


--
Craig Ringer

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


Re: [HACKERS] per-column generic option

2011-07-04 Thread Shigeru Hanada
(2011/07/04 10:17), Shigeru Hanada wrote:
 (2011/07/03 18:50), Kohei KaiGai wrote:
 I checked the per-column generic option patch.
 Right now, I have nothing to comment on anymore.
 So, it should be reviewed by committers.
 
 Thanks for the review!.

I would like to propose adding force_not_null support to file_fdw, as
the first use case of per-column FDW option.  Attached patch, which
assumes that per_column_option_v3.patch has been applied, implements
force_not_null option as per-column FDW option.

Overview

This option is originally supported by COPY FROM command, so I think
it's reasonable to support it in file_fdw too.  It would provides more
flexible parsing capability.  In fact, this option has been supported
by the internal routines which are shared with COPY FROM, but currently
we don't have any way to specify it.

Difference between COPY
===
For COPY FROM, FORCE_NOT_NULL is specified as a list of column names
('*' is not allowed).  For file_fdw, per-table FDW option can be used
like other options, but then file_fdw needs parser which can identify
valid column.  I think it's too much work, so I prefer per-column FDW
option which accepts boolean value string.  The value 'true' means that
the column doesn't be matched against NULL string, same as ones listed
for COPY FROM.

Example:

If you have created a foreign table with:

  CREATE FOREIGN TABLE foo (
c1 int OPTIONS (force_not_null 'false'),
c2 text OPTIONS (force_not_null 'true')
  ) SERVER file OPTIONS (file '/path/to/file', format 'csv', null '');

values which are read from the file for c1 are matched against
null-representation-string '', but values for c2 are NOT.  Empty strings
for c2 are stored as empty strings; they don't treated as NULL value.

Regards,
-- 
Shigeru Hanada
diff --git a/contrib/file_fdw/data/text.csv b/contrib/file_fdw/data/text.csv
index ...bd4c327 .
*** a/contrib/file_fdw/data/text.csv
--- b/contrib/file_fdw/data/text.csv
***
*** 0 
--- 1,4 
+ 123,123
+ abc,abc
+ NULL,NULL
+ ABC,ABC
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 466c015..caf9c86 100644
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 23,29 
--- 23,31 
  #include foreign/fdwapi.h
  #include foreign/foreign.h
  #include miscadmin.h
+ #include nodes/makefuncs.h
  #include optimizer/cost.h
+ #include utils/syscache.h
  
  PG_MODULE_MAGIC;
  
*** static struct FileFdwOption valid_option
*** 56,71 
{escape, ForeignTableRelationId},
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
  
/*
 * force_quote is not supported by file_fdw because it's for COPY TO.
 */
  
-   /*
-* force_not_null is not supported by file_fdw.  It would need a parser
-* for list of columns, not to mention a way to check the column list
-* against the table.
-*/
  
/* Sentinel */
{NULL, InvalidOid}
--- 58,69 
{escape, ForeignTableRelationId},
{null, ForeignTableRelationId},
{encoding, ForeignTableRelationId},
+   {force_not_null, AttributeRelationId},/* specified as boolean 
value */
  
/*
 * force_quote is not supported by file_fdw because it's for COPY TO.
 */
  
  
/* Sentinel */
{NULL, InvalidOid}
*** static void fileGetOptions(Oid foreignta
*** 111,116 
--- 109,115 
  static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
   const char *filename,
   Cost *startup_cost, Cost *total_cost);
+ static List * get_force_not_null(Oid relid);
  
  
  /*
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 144,149 
--- 143,149 
List   *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
Oid catalog = PG_GETARG_OID(1);
char   *filename = NULL;
+   char   *force_not_null = NULL;
List   *other_options = NIL;
ListCell   *cell;
  
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 197,203 
 buf.data)));
}
  
!   /* Separate out filename, since ProcessCopyOptions won't allow 
it */
if (strcmp(def-defname, filename) == 0)
{
if (filename)
--- 197,206 
 buf.data)));
}
  
!   /*
!* Separate out filename and force_not_null, since 
ProcessCopyOptions
!* won't allow them.
!*/
if (strcmp(def-defname, filename) == 0)
{
if (filename)
*** file_fdw_validator(PG_FUNCTION_ARGS)
*** 206,211 
--- 209,228 
  

Re: [HACKERS] Potential NULL dereference found in typecmds.c

2011-07-04 Thread Magnus Hagander
On Sat, Jul 2, 2011 at 20:10, Michael Mueller mmuel...@vigilantsw.com wrote:
 Hi folks,

 Sentry found this error last night, and it looks serious enough to
 report.  The error was introduced in commit 426cafc.  Here's the code
 in question, starting at line 2096:

    if (!found)
    {
        con = NULL;     /* keep compiler quiet */
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
                 errmsg(constraint \%s\ of domain \%s\ does not exist,
                        constrName, NameStr(con-conname;
    }

 It sets 'con' to NULL and then in the next statement, dereferences it.
 I'm not sure if it's possible to reach this path, but if it is
 reachable it will cause a crash.

This code is no longer present in git head, *removed* by commit
426cafc. Not added by it. at least that's how I read the history...

However, it still looks to me like we could get to that code with
con=NULL - if the while loop is never executed. Perhaps this is a
can-never-happen situation? Alvaro?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] Potential NULL dereference found in typecmds.c

2011-07-04 Thread Peter Geoghegan
On 4 July 2011 13:53, Magnus Hagander mag...@hagander.net wrote:
 This code is no longer present in git head, *removed* by commit
 426cafc. Not added by it. at least that's how I read the history...

 However, it still looks to me like we could get to that code with
 con=NULL - if the while loop is never executed. Perhaps this is a
 can-never-happen situation? Alvaro?

Seems slightly academic IMHO. No code path should dereference an
invariably NULL or wild pointer.

-- 
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] Potential NULL dereference found in typecmds.c

2011-07-04 Thread Heikki Linnakangas

On 04.07.2011 16:07, Peter Geoghegan wrote:

On 4 July 2011 13:53, Magnus Hagandermag...@hagander.net  wrote:

This code is no longer present in git head, *removed* by commit
426cafc. Not added by it. at least that's how I read the history...

However, it still looks to me like we could get to that code with
con=NULL - if the while loop is never executed. Perhaps this is a
can-never-happen situation? Alvaro?


Seems slightly academic IMHO. No code path should dereference an
invariably NULL or wild pointer.


That error message is bogus anyway:


if (!found)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg(constraint \%s\ of domain \%s\ does not 
exist,
constrName, 
NameStr(con-conname;


It passes con-conname as the name of the domain, which is just wrong. 
It should be TypeNameToString(typename) instead. The second error 
message that follows has the same bug.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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


Re: [HACKERS] Crash dumps

2011-07-04 Thread Radoslaw Smogura
Information if backend crashed should go fast to master, to kill others as fast 
as possible. This what i thought is to use socket urgent data, but this require 
to span small thread in master (i think oob data may not be processed in secure 
way).

From one hand processing core dump may be good, but from other hand those may 
take huge area. Using it in any case will require to build PostgreSQL with 
debugging symbols.

Regards,
Radoslaw Smogura
(mobile)

-Original Message-
From: Craig Ringer
Sent: 4 lipca 2011 13:57
To: Radosław Smogura
Cc: PG Hackers
Subject: Re: [HACKERS] Crash dumps

On 4/07/2011 7:03 PM, Radosław Smogura wrote:

 Actually this, what I was thinking about was, to add dumping of GUC,
 etc. List of mappings came from when I tired to mmap PostgreSQL, and due
 to many of errors, which sometimes occurred in unexpected places, I was
 in need to add something that will be useful for me and easy to analyse
 (I could simple find pointer, and then check which region failed). The
 idea to try to evolve this come later.

Why not produce a tool that watches the datadir for core files and 
processes them? Most but not all of the info you listed should be able 
to be extracted from a core file. Things like GUCs should be extractable 
with a bit of gdb scripting - and with much less chance of crashing than 
trying to read them from a possibly corrupt heap within a crashing backend.

To capture any information not available from the core, you can enlist 
the postmaster's help. It gets notified when a child crashes and should 
be able to capture things like the memory and disk state. See void 
reaper(SIGNAL_ARGS) in postmaster.c and HandleChildCrash(...) . If 
nothing else, the postmaster could probably fork a child crashed 
helper to collect data, analyse the core file, email the report to the 
admin, etc.

About the only issue there is that the postmaster relies on the exit 
status to trigger the reaper code. Once an exit status is available, the 
crashed process is gone, so the free memory will reflect the memory 
state after the backend dies, and shared memory's state will have moved 
on from how it was when the backend was alive.

For that reason, it'd be handy if a backend could trap SIGSEGV and 
reliably tell the postmaster I'm crashing! so the postmaster could 
fork a helper to capture any additional info the backend needs to be 
alive for. Then the helper can gcore() the backend, or the backend can 
just clear the SIGSEGV handler and kill(11) its self to keep on crashing 
and generate a core.

Unfortunately, reliably and segfault don't go together. You don't 
want a crashing postmaster writing to shared memory so it can't use shm 
to tell the postmaster it's dying. Signals are ... interesting ... at 
the best of times, but would probably still be the best bet. The 
postmaster could install a SIGUSR[whatever] or RT signal handler that 
takes a siginfo so it knows the pid of the signal sender. The crashing 
backend could signal the postmaster with an agreed signal to say I'm 
crashing and let the postmaster clean it up. The problem with this is 
that a lost signal (for any reason) would cause a zombie backend to hang 
around waiting to be killed by a postmaster that never heard it was 
crashing.


BTW, the win32 crash dump handler would benefit from being able to use 
some of the same facilities. In particular, being able to tell the 
postmaster Argh, ogod I'm crashing, fork something to dump my core! 
rather than trying to self-dump would be great. It'd also allow the 
addition of extra info like GUC data, last few lines of logs etc to the 
minidump, something that the win32 crash dump handler cannot currently 
do safely.

--
Craig Ringer



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


Re: [HACKERS] non-superuser reserved connections? connection pools?

2011-07-04 Thread Magnus Hagander
On Mon, Jul 4, 2011 at 00:01, Michael Glaesemann g...@seespotcode.net wrote:
 It would be nice to be able to set aside a few connections for 
 non-superusers, such as stats-monitoring connections. There's often no reason 
 to grant these users superuser privileges (they're just observers, and 
 security-definer functions can make anything visible that they may need)), 
 but at the same time you want them to be able to connect even when the 
 normal pool of slots may be full.

 I googled a bit, assuming there had been discussion of something similar in 
 the past, but didn't find anything.

 I'm not sure what configuration would look like. Perhaps there's a 
 generalized connection pool concept that's missing, extending the current 
 superuser connection pool specified by the superuser_reserved_connections 
 GUC something like:

 CREATE CONNECTION POOL stats WITH LIMIT 10; -- 40 connections allotted for 
 the foo connection pool.
 ALTER ROLE nagios WITH CONNECTION POOL foo; -- the nagios role is allowed to 
 take a connection from the foo connection pool.

 Right now, of course, connection limits are set in postgresql.conf and only 
 alterable on restart, so perhaps there could be a connection_pools GUC, 
 something along the lines of:

 connection_pools={stats=3,superuser=10}
 max_connections=100

 The connections allotted to superuser would have the same meaning as the 
 current superuser_reserved_connections GUC.

 Does this seem to be a useful feature to anyone else?

Yeah, I'd definitely find it useful. Gives you a bit more flexibility
than just using connection limiting on the individual user (though in
your specific case here, that might work, if all your stats processes
are with the same user).

No comments on the implementation suggestions at this point ;)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] Crash dumps

2011-07-04 Thread Tom Lane
Craig Ringer cr...@postnewspapers.com.au writes:
 Why not produce a tool that watches the datadir for core files and 
 processes them? ...

By and large, our attitude has been that Postgres shouldn't be crashing
often enough to make this sort of infrastructure worthwhile.  Developer
time spent on it would be far better spent on fixing the bugs instead.

 For that reason, it'd be handy if a backend could trap SIGSEGV and 
 reliably tell the postmaster I'm crashing! so the postmaster could 
 fork a helper to capture any additional info the backend needs to be 
 alive for. ...
 Unfortunately, reliably and segfault don't go together.

Yeah.  I think there's no chance at all that we'd accept patches pointed
in this direction.  They'd be more likely to decrease the system's
reliability than anything else.  Aside from the difficulty of doing
anything at all reliably in an already-failing process, once we realize
that something is badly wrong it's important to kill off all other
backends ASAP.  That reduces the window for any possible corruption of
shared memory to make it into on-disk state.  So interposing a helper
to fool around with the failed process doesn't sound good at all.

In practice I think you can generally get everything of interest
out of the core file, so it's not clear to me that there's any win
available from this line of thought anyhow.

regards, tom lane

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


[HACKERS] Problem installing odbc and .Net drivers on Windows 7 64 Ultimate

2011-07-04 Thread Michael Gould
I am getting the following error when I run the install from stackbuilder.


Error trying to install file destination ${installdir} resolves to empty
value.


 


Does anyone know what might be causing this and how I can fix it.


 


Best Regards


Michael Gould




Re: [HACKERS] Problem installing odbc and .Net drivers on Windows 7 64 Ultimate

2011-07-04 Thread Peter Geoghegan
On 4 July 2011 15:57, Michael Gould
mgo...@intermodalsoftwaresolutions.net wrote:
 I am getting the following error when I run the install from stackbuilder.

 Error trying to install file destination ${installdir} resolves to empty
 value.



 Does anyone know what might be causing this and how I can fix it.

This is the developer's mailing list. You should ask this question on
the pgsql-general mailing list.

-- 
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] Potential NULL dereference found in typecmds.c

2011-07-04 Thread Alvaro Herrera
Excerpts from Heikki Linnakangas's message of lun jul 04 09:14:11 -0400 2011:
 On 04.07.2011 16:07, Peter Geoghegan wrote:

 That error message is bogus anyway:
 
  if (!found)
  ereport(ERROR,
  (errcode(ERRCODE_UNDEFINED_OBJECT),
   errmsg(constraint \%s\ of domain \%s\ does not exist,
  constrName, NameStr(con-conname;
 
 It passes con-conname as the name of the domain, which is just wrong. 
 It should be TypeNameToString(typename) instead. The second error 
 message that follows has the same bug.

Um, evidently this code has a problem.  I'll fix.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


[HACKERS] proper format for printing GetLastError()

2011-07-04 Thread Peter Eisentraut
About half of our code prints GetLastError() using %d after casting it
to int (actually, about half of that half uses %i, another thing to sort
out, perhaps), and the other half uses %lu without casting.  I gather
from online documentation that GetLastError() returns DWORD, which
appears to be unsigned 32 bits.  So using %lu appears to be more
correct.  Any arguments against standardizing on %lu?

Secondly, it might also be good if we could standardize on printing

actual message: error code %lu

instead of just

actual message: %lu

Thirdly, why are we not trying to print a textual message?



-- 
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] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Heikki Linnakangas
Ok, here's a new patch, addressing the issues Fujii raised, and with a 
bunch of stylistic changes of my own. Also, I committed a patch to 
remove silent_mode, so the fork_process() changes are now gone. I'm 
going to sleep over this and review once again tomorrow, and commit if 
it still looks good to me and no-one else reports new issues.


There's two small issues left:

I don't like the names POSTMASTER_FD_WATCH and POSTMASTER_FD_OWN. At a 
quick glance, it's not at all clear which is which. I couldn't come up 
with better names, so for now I just added some comments to clarify 
that. I would find WRITE/READ more clear, but to make sense of that you 
need to how the pipe is used. Any suggestions or opinions on that?


The BUGS section of Linux man page for select(2) says:


   Under Linux, select() may report a socket file descriptor as ready for
   reading,  while nevertheless a subsequent read blocks.  This could for
   example happen when data has arrived but  upon  examination  has  wrong
   checksum and is discarded.  There may be other circumstances in which a
   file descriptor is spuriously reported as ready.  Thus it may be  safer
   to use O_NONBLOCK on sockets that should not block.


So in theory, on Linux you might WaitLatch might sometimes incorrectly 
return WL_POSTMASTER_DEATH. None of the callers check for 
WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before 
assuming the postmaster has died, so that won't affect correctness at 
the moment. I doubt that scenario can even happen in our case, select() 
on a pipe that is never written to. But maybe we should add add an 
assertion to WaitLatch to assert that if select() reports that the 
postmaster pipe has been closed, PostmasterIsAlive() also returns false.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 10165,10171  retry:
  	/*
  	 * Wait for more WAL to arrive, or timeout to be reached
  	 */
! 	WaitLatch(XLogCtl-recoveryWakeupLatch, 500L);
  	ResetLatch(XLogCtl-recoveryWakeupLatch);
  }
  else
--- 10165,10171 
  	/*
  	 * Wait for more WAL to arrive, or timeout to be reached
  	 */
! 	WaitLatch(XLogCtl-recoveryWakeupLatch, WL_LATCH_SET | WL_TIMEOUT, 500L);
  	ResetLatch(XLogCtl-recoveryWakeupLatch);
  }
  else
*** a/src/backend/port/unix_latch.c
--- b/src/backend/port/unix_latch.c
***
*** 93,98 
--- 93,99 
  #endif
  
  #include miscadmin.h
+ #include postmaster/postmaster.h
  #include storage/latch.h
  #include storage/shmem.h
  
***
*** 179,209  DisownLatch(volatile Latch *latch)
   * Wait for given latch to be set or until timeout is exceeded.
   * If the latch is already set, the function returns immediately.
   *
!  * The 'timeout' is given in microseconds, and -1 means wait forever.
!  * On some platforms, signals cause the timeout to be restarted, so beware
!  * that the function can sleep for several times longer than the specified
!  * timeout.
   *
   * The latch must be owned by the current process, ie. it must be a
   * backend-local latch initialized with InitLatch, or a shared latch
   * associated with the current process by calling OwnLatch.
   *
!  * Returns 'true' if the latch was set, or 'false' if timeout was reached.
   */
! bool
! WaitLatch(volatile Latch *latch, long timeout)
  {
! 	return WaitLatchOrSocket(latch, PGINVALID_SOCKET, false, false, timeout)  0;
  }
  
  /*
!  * Like WaitLatch, but will also return when there's data available in
!  * 'sock' for reading or writing. Returns 0 if timeout was reached,
!  * 1 if the latch was set, 2 if the socket became readable or writable.
   */
  int
! WaitLatchOrSocket(volatile Latch *latch, pgsocket sock, bool forRead,
!   bool forWrite, long timeout)
  {
  	struct timeval tv,
  			   *tvp = NULL;
--- 180,211 
   * Wait for given latch to be set or until timeout is exceeded.
   * If the latch is already set, the function returns immediately.
   *
!  * The 'timeout' is given in microseconds. It must be = 0 if WL_TIMEOUT
!  * event is given, otherwise it is ignored. On some platforms, signals cause
!  * the timeout to be restarted, so beware that the function can sleep for
!  * several times longer than the specified timeout.
   *
   * The latch must be owned by the current process, ie. it must be a
   * backend-local latch initialized with InitLatch, or a shared latch
   * associated with the current process by calling OwnLatch.
   *
!  * Returns bit field indicating which condition(s) caused the wake-up. Note
!  * that if multiple wake-up conditions are true, there is no guarantee that
!  * we return all of them in one call, but we will return at least one.
   */
! int
! WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
  {
! 	return WaitLatchOrSocket(latch, wakeEvents, 

Re: [HACKERS] [BUGS] BUG #6083: psql script line numbers incorrectly count \copy data

2011-07-04 Thread Tom Lane
Steve Haslam araq...@googlemail.com writes:
 ... Apparently, the data read from \copy
 is incrementing the script line number counter?

Yeah, so it is.  That is correct behavior for COPY FROM STDIN,
but not so much for copying from a separate file.

The attached patch seems like an appropriate fix.  However, I'm unsure
whether to apply it to released branches ... does anyone think this
might break somebody's application?

regards, tom lane

diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 5e69d29b6cbeab56aa0c85e85c3edce46d06efac..ebe5ee9ea551b858a4ad6119322405109d3212d8 100644
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
*** handleCopyIn(PGconn *conn, FILE *copystr
*** 586,592 
  }
  			}
  
! 			pset.lineno++;
  		}
  	}
  
--- 586,593 
  }
  			}
  
! 			if (copystream == pset.cur_cmd_source)
! pset.lineno++;
  		}
  	}
  

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


[HACKERS] Perl 5.8 requirement for compiling on windows?

2011-07-04 Thread Marios Vodas
Hello,

I want to ask why is there a requirement for 5.8 version of Perl to compile
under windows?
In this page of the documentation
http://www.postgresql.org/docs/9.0/static/install-windows-full.html#AEN23848it
sais: Note: version 5.8 is required.
I was able to compile PostgreSQL 9.0.4 using Active Perl 5.14 (only some
warnings but the build was successful).
Should I necessarily use 5.8?

Thank you


Re: [HACKERS] Perl 5.8 requirement for compiling on windows?

2011-07-04 Thread Heikki Linnakangas

On 04.07.2011 19:30, Marios Vodas wrote:

Hello,

I want to ask why is there a requirement for 5.8 version of Perl to compile
under windows?
In this page of the documentation
http://www.postgresql.org/docs/9.0/static/install-windows-full.html#AEN23848it
sais: Note: version 5.8 is required.
I was able to compile PostgreSQL 9.0.4 using Active Perl 5.14 (only some
warnings but the build was successful).
Should I necessarily use 5.8?


Surely that means 5.8 or later. Given that 5.8 is very old by now, 
perhaps we should just remove that sentence.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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


Re: [HACKERS] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Florian Pflug
On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
   Under Linux, select() may report a socket file descriptor as ready for
   reading,  while nevertheless a subsequent read blocks.  This could for
   example happen when data has arrived but  upon  examination  has  wrong
   checksum and is discarded.  There may be other circumstances in which a
   file descriptor is spuriously reported as ready.  Thus it may be  safer
   to use O_NONBLOCK on sockets that should not block.
 
 So in theory, on Linux you might WaitLatch might sometimes incorrectly return 
 WL_POSTMASTER_DEATH. None of the callers check for WL_POSTMASTER_DEATH return 
 code, they call PostmasterIsAlive() before assuming the postmaster has died, 
 so that won't affect correctness at the moment. I doubt that scenario can 
 even happen in our case, select() on a pipe that is never written to. But 
 maybe we should add add an assertion to WaitLatch to assert that if select() 
 reports that the postmaster pipe has been closed, PostmasterIsAlive() also 
 returns false.

The correct solution would be to read() from the pipe after select()
returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
EAGAIN. To prevent that read() from blocking if the read event was indeed
spurious, O_NONBLOCK must be set on the pipe but that patch does that already.

Btw, with the death-watch / life-sign / whatever infrastructure in place,
shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?

best regards,
Florian Pflug


-- 
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] Perl 5.8 requirement for compiling on windows?

2011-07-04 Thread Marios Vodas
Yes, or at least change it by adding or later. It is confusing without 
it...

Thank you for your response.

On 4/7/2011 7:33 μμ, Heikki Linnakangas wrote:

On 04.07.2011 19:30, Marios Vodas wrote:

Hello,

I want to ask why is there a requirement for 5.8 version of Perl to 
compile

under windows?
In this page of the documentation
http://www.postgresql.org/docs/9.0/static/install-windows-full.html#AEN23848it 


sais: Note: version 5.8 is required.
I was able to compile PostgreSQL 9.0.4 using Active Perl 5.14 (only some
warnings but the build was successful).
Should I necessarily use 5.8?


Surely that means 5.8 or later. Given that 5.8 is very old by now, 
perhaps we should just remove that sentence.





--
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] Crash dumps

2011-07-04 Thread Radosław Smogura
Tom Lane t...@sss.pgh.pa.us Monday 04 of July 2011 16:32:32
 Craig Ringer cr...@postnewspapers.com.au writes:
  Why not produce a tool that watches the datadir for core files and
  processes them? ...
 
 By and large, our attitude has been that Postgres shouldn't be crashing
 often enough to make this sort of infrastructure worthwhile.  Developer
 time spent on it would be far better spent on fixing the bugs instead.
 
  For that reason, it'd be handy if a backend could trap SIGSEGV and
  reliably tell the postmaster I'm crashing! so the postmaster could
  fork a helper to capture any additional info the backend needs to be
  alive for. ...
  Unfortunately, reliably and segfault don't go together.
 
 Yeah.  I think there's no chance at all that we'd accept patches pointed
 in this direction.  They'd be more likely to decrease the system's
 reliability than anything else.  Aside from the difficulty of doing
 anything at all reliably in an already-failing process, once we realize
 that something is badly wrong it's important to kill off all other
 backends ASAP.  That reduces the window for any possible corruption of
 shared memory to make it into on-disk state.  So interposing a helper
 to fool around with the failed process doesn't sound good at all.
 
 In practice I think you can generally get everything of interest
 out of the core file, so it's not clear to me that there's any win
 available from this line of thought anyhow.
 
   regards, tom lane
I asked about crash reports becaus of at this time there was thread about 
crashing in live system.

There is one win, I think, users will faster send crash report, then core dump 
(from many reasons, size, number of confidential information, etc).

Such report may be quite usefull, for reasonable bug finding.

It may be attached as contrib too, but I noticed and we agree that report 
generation should not affect speed of shoutdown.

PostgreSQL will look better - server application that generates crash reports 
looks better then no generating. Just a bit of marketing ;)

Regards,
Radek.

-- 
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] keepalives_* parameters usefullness

2011-07-04 Thread Jaime Casanova
Simon Riggs si...@2ndquadrant.com writes:

 On Mon, Jul 4, 2011 at 9:42 AM, Jaime Casanova ja...@2ndquadrant.com wrote:

 i even used getsockopt() to ensure TCP_KEEPIDLE was being setted and
 tried to set it myself with setsockopt() with the same results.

 There's a list of preconditions as to when these settings take effect,
 otherwise they are a no-op.


do we know what those preconditions are? TFM just says:

keepalives_idle
Controls the number of seconds of inactivity after which TCP should send
a keepalive message to the server. A value of zero uses the system
default. This parameter is ignored for connections made via a
Unix-domain socket, or if keepalives are disabled. It is only supported
on systems where the TCP_KEEPIDLE or TCP_KEEPALIVE socket option is
available, and on Windows; on other systems, it has no effect.


which made me think that on my debian system it should work so maybe we
want to document those preconditions, even a statement indicating that
other factors can prevent this working as expected would be good

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL 
Soporte 24x7, desarrollo, capacitación y servicios

-- 
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 #6083: psql script line numbers incorrectly count \copy data

2011-07-04 Thread David Fetter
On Mon, Jul 04, 2011 at 12:02:12PM -0400, Tom Lane wrote:
 Steve Haslam araq...@googlemail.com writes:
  ... Apparently, the data read from \copy is incrementing the
  script line number counter?
 
 Yeah, so it is.  That is correct behavior for COPY FROM STDIN, but
 not so much for copying from a separate file.
 
 The attached patch seems like an appropriate fix.  However, I'm
 unsure whether to apply it to released branches ... does anyone
 think this might break somebody's application?

No.

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

-- 
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] avoid including rel.h in execnodes.h

2011-07-04 Thread Alvaro Herrera
Excerpts from Tom Lane's message of vie jul 01 18:20:50 -0400 2011:
 Alvaro Herrera alvhe...@alvh.no-ip.org writes:

  I also included rel.h in spi.h, because it was previously indirectly
  included via execnodes.h and with this patch it would no longer be,
  which is a problem because it'd cause external code to fail to compile.
 
 If we think that not including rel.h unnecessarily is a good thing, then
 that should surely apply to external code as well.  So -1 for that bit.
 It's not like we have not removed stuff from spi.h before.

Thanks, committed with that change.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 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] non-superuser reserved connections? connection pools?

2011-07-04 Thread Michael Glaesemann

On Jul 4, 2011, at 10:09, Magnus Hagander wrote:

 On Mon, Jul 4, 2011 at 00:01, Michael Glaesemann g...@seespotcode.net wrote:
 It would be nice to be able to set aside a few connections for 
 non-superusers, such as stats-monitoring connections. There's often no 
 reason to grant these users superuser privileges (they're just observers, 
 and security-definer functions can make anything visible that they may 
 need)), but at the same time you want them to be able to connect even when 
 the normal pool of slots may be full.

snip/

 connection_pools={stats=3,superuser=10}
 max_connections=100
 
 The connections allotted to superuser would have the same meaning as the 
 current superuser_reserved_connections GUC.
 
 Does this seem to be a useful feature to anyone else?
 
 Yeah, I'd definitely find it useful. Gives you a bit more flexibility
 than just using connection limiting on the individual user (though in
 your specific case here, that might work, if all your stats processes
 are with the same user).

Good point. It's another way to think of managing connections.

Michael Glaesemann
grzm seespotcode 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] Perl 5.8 requirement for compiling on windows?

2011-07-04 Thread Heikki Linnakangas

On 04.07.2011 19:40, Marios Vodas wrote:

On 4/7/2011 7:33 μμ, Heikki Linnakangas wrote:

On 04.07.2011 19:30, Marios Vodas wrote:

I want to ask why is there a requirement for 5.8 version of Perl to
compile
under windows?
In this page of the documentation
http://www.postgresql.org/docs/9.0/static/install-windows-full.html#AEN23848it

sais: Note: version 5.8 is required.
I was able to compile PostgreSQL 9.0.4 using Active Perl 5.14 (only some
warnings but the build was successful).
Should I necessarily use 5.8?


Surely that means 5.8 or later. Given that 5.8 is very old by now,
perhaps we should just remove that sentence.


 Yes, or at least change it by adding or later. It is confusing without
 it...

Ok, I've fixed the docs to say or later. I did it somewhat arbitrarily 
back to 8.3, as that's as far as git cherry-pick worked without conflicts.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

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


Re: [HACKERS] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Peter Geoghegan
On 4 July 2011 16:53, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 Ok, here's a new patch, addressing the issues Fujii raised, and with a bunch
 of stylistic changes of my own. Also, I committed a patch to remove
 silent_mode, so the fork_process() changes are now gone. I'm going to sleep
 over this and review once again tomorrow, and commit if it still looks good
 to me and no-one else reports new issues.

Looks good.

 I don't like the names POSTMASTER_FD_WATCH and POSTMASTER_FD_OWN. At a quick
 glance, it's not at all clear which is which. I couldn't come up with better
 names, so for now I just added some comments to clarify that. I would find
 WRITE/READ more clear, but to make sense of that you need to how the pipe is
 used. Any suggestions or opinions on that?

We could bikeshed about that until the cows come home, but we're not
going to come up with names that make the purpose of each evident at a
glance - it's too involved. If we could, we would have thought of them
already. Besides, I've probably already written all the client code
those macros will ever have.

On 4 July 2011 17:36, Florian Pflug f...@phlo.org wrote:
 On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
       Under Linux, select() may report a socket file descriptor as ready 
 for
       reading,  while nevertheless a subsequent read blocks.  This could 
 for
       example happen when data has arrived but  upon  examination  has  
 wrong
       checksum and is discarded.  There may be other circumstances in which 
 a
       file descriptor is spuriously reported as ready.  Thus it may be  
 safer
       to use O_NONBLOCK on sockets that should not block.

 So in theory, on Linux you might WaitLatch might sometimes incorrectly 
 return WL_POSTMASTER_DEATH. None of the callers check for 
 WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before 
 assuming the postmaster has died, so that won't affect correctness at the 
 moment. I doubt that scenario can even happen in our case, select() on a 
 pipe that is never written to. But maybe we should add add an assertion to 
 WaitLatch to assert that if select() reports that the postmaster pipe has 
 been closed, PostmasterIsAlive() also returns false.

 The correct solution would be to read() from the pipe after select()
 returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
 EAGAIN. To prevent that read() from blocking if the read event was indeed
 spurious, O_NONBLOCK must be set on the pipe but that patch does that already.

Let's have some perspective on this. We're talking about a highly
doubtful chance that latches may wake when they shouldn't. Latches are
typically expected to wake up for a variety of reasons, and if that
occurred in the archiver's case with my patch applied, I think we'd
just go asleep again without anything happening. It seems likely that
latch client code in general will never trip up on this, as long as
its not exclusively relying on the waitLatch() return value to report
pm death.

Maybe we should restore the return value of WaitLatch to its previous
format (so it doesn't return a bitmask)? That way, we don't report
that the Postmaster died, and therefore clients are required to call
PostmasterIsAlive() to be sure. In any case, I'm in favour of the
assertion.

 Btw, with the death-watch / life-sign / whatever infrastructure in place,
 shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?

Hmm, maybe. That seems like a separate issue though, that can be
addressed with another patch. It does have the considerable
disadvantage of making Heikki's proposed assertion failure useless. Is
the implementation of PostmasterIsAlive() really a problem at the
moment?

-- 
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] %ENV warnings during builds

2011-07-04 Thread Andrew Dunstan



On 07/03/2011 05:14 PM, Brar Piening wrote:

schrieb Magnus Hagander:

I think you've stumbled on just about all the bits of the MSVC build
system we haven't perlized. Maybe we should complete that task, and turn
clean.bat, pgbison.bat and pgflex.bat into pure one-line wrappers. 
(It was

done for builddoc just a few weeks ago).
Yeah, give nthat you can't get anything useful done without perl
anyway, I don't see any argument for keeping them at this point.

I've already stumbled into this while preparing the VS2010 support and 
came to the same conclusion.
In my VS2010 support patch I've already created perl replacements for 
those two and removed the batch files completely.
Certainly those two could also stay around as mere wrappers but in my 
opinion they only mess up the directory without adding any relevant 
benefit.



Hmm, I missed that you had done this. Here are two replacement perl 
scripts I knocked up, but haven't yet tested. One of the things about 
them is that they remove knowledge of particular .l and .y files. and 
instead get the required invocation options from the relevant Makefiles. 
I think that's a lot better than the horrid hackery we have in the batch 
files.


cheers

andrew




pgflex.pl
Description: Perl program


pgbison.pl
Description: Perl program

-- 
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] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Florian Pflug
On Jul4, 2011, at 23:11 , Peter Geoghegan wrote:
 On 4 July 2011 17:36, Florian Pflug f...@phlo.org wrote:
 On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
   Under Linux, select() may report a socket file descriptor as ready 
 for
   reading,  while nevertheless a subsequent read blocks.  This could 
 for
   example happen when data has arrived but  upon  examination  has  
 wrong
   checksum and is discarded.  There may be other circumstances in 
 which a
   file descriptor is spuriously reported as ready.  Thus it may be  
 safer
   to use O_NONBLOCK on sockets that should not block.
 
 So in theory, on Linux you might WaitLatch might sometimes incorrectly 
 return WL_POSTMASTER_DEATH. None of the callers check for 
 WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before 
 assuming the postmaster has died, so that won't affect correctness at the 
 moment. I doubt that scenario can even happen in our case, select() on a 
 pipe that is never written to. But maybe we should add add an assertion to 
 WaitLatch to assert that if select() reports that the postmaster pipe has 
 been closed, PostmasterIsAlive() also returns false.
 
 The correct solution would be to read() from the pipe after select()
 returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
 EAGAIN. To prevent that read() from blocking if the read event was indeed
 spurious, O_NONBLOCK must be set on the pipe but that patch does that 
 already.
 
 Let's have some perspective on this. We're talking about a highly
 doubtful chance that latches may wake when they shouldn't.

Yeah, as long as there's just a spurious wake up, sure. However,
having WaitLatch() indicate a postmaster death in that case seems
dangerous. Some caller, sooner or later, is bound to get it wrong,
i.e. forget to re-check PostmasterIsAlive.

 Maybe we should restore the return value of WaitLatch to its previous
 format (so it doesn't return a bitmask)? That way, we don't report
 that the Postmaster died, and therefore clients are required to call
 PostmasterIsAlive() to be sure.

That'd solve the issue too.

 In any case, I'm in favour of the assertion.

I don't really see the value in that assertion. It'd cause spurious
assertion failures in the case of spurious events reported by select().
If we do expect such event, we should close the hole instead of asserting.
If we don't, then what's the point of the assert.

BTW, do we currently retry the select() on EINTR (meaning a signal has
arrived)? If we don't, that'd be an additional source of spurious returns
from select.

 Btw, with the death-watch / life-sign / whatever infrastructure in place,
 shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?
 
 Hmm, maybe. That seems like a separate issue though, that can be
 addressed with another patch. It does have the considerable
 disadvantage of making Heikki's proposed assertion failure useless. Is
 the implementation of PostmasterIsAlive() really a problem at the
 moment?

I'm not sure that there is currently a guarantee that PostmasterIsAlive
will returns false immediately after select() indicates postmaster
death. If e.g. the postmaster's parent is still running (which happens
for example if you launch postgres via daemontools), the re-parenting of
backends to init might not happen until the postmaster zombie has been
vanquished by its parent's call of waitpid(). It's not entirely
inconceivable for getppid() to then return the (dead) postmaster's pid
until that waitpid() call has occurred.

But agreed, this is probably best handled by a separate patch.

best regards,
Florian Pflug


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


Re: [HACKERS] Review of patch Bugfix for XPATH() if expression returns a scalar value

2011-07-04 Thread Florian Pflug
Hi Radosław,

Do you still have objections, or did I manage to convince you?

Also, aside from the question of whether to escape or not, did
you find any issues with either the patch (like spurious whitespace,
...) or the code, or is the patch OK implementation-wise?

best regards,
Florian Pflug


-- 
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] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Peter Geoghegan
On 4 July 2011 22:42, Florian Pflug f...@phlo.org wrote:
 If we do expect such event, we should close the hole instead of asserting.
 If we don't, then what's the point of the assert.

You can say the same thing about any assertion. I'm not going to
attempt to close the hole because I don't believe that there is one. I
would be happy to see your read() from the pipe after select() test
asserted though.

 BTW, do we currently retry the select() on EINTR (meaning a signal has
 arrived)? If we don't, that'd be an additional source of spurious returns
 from select.

Why might it be? WaitLatch() is currently documented to potentially
have its timeout invalidated by the process receiving a signal, which
is the exact opposite problem. We do account for this within the
archiver calling code though, and I remark upon it in a comment there.

 I'm not sure that there is currently a guarantee that PostmasterIsAlive
 will returns false immediately after select() indicates postmaster
 death. If e.g. the postmaster's parent is still running (which happens
 for example if you launch postgres via daemontools), the re-parenting of
 backends to init might not happen until the postmaster zombie has been
 vanquished by its parent's call of waitpid(). It's not entirely
 inconceivable for getppid() to then return the (dead) postmaster's pid
 until that waitpid() call has occurred.

Yes, this did occur to me - it's hard to reason about what exactly
happens here, and probably impossible to have the behaviour guaranteed
across platforms, however unlikely it seems. I'd like to hear what
Heikki has to say about asserting or otherwise verifying postmaster
death in the case of apparent postmaster death wake-up.

-- 
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] keepalives_* parameters usefullness

2011-07-04 Thread Fujii Masao
On Tue, Jul 5, 2011 at 4:00 AM, Jaime Casanova ja...@2ndquadrant.com wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 There's a list of preconditions as to when these settings take effect,
 otherwise they are a no-op.


 do we know what those preconditions are?

The keepalives don't work at least on linux when the connection is
terminated after sending a packet and before receiving TCP-level ACK.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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


Re: Initial Review: JSON contrib modul was: Re: [HACKERS] Another swing at JSON

2011-07-04 Thread Joseph Adams
Thanks for reviewing my patch!

On Mon, Jul 4, 2011 at 7:10 AM, Bernd Helmle maili...@oopsware.de wrote:
 +comment = 'data type for storing and manipulating JSON content'

 I'm not sure, if manipulating is a correct description. Maybe i missed it,
 but i didn't see functions to manipulate JSON strings directly, for example,
 adding an object to a JSON string, ...

Indeed.  I intend to add manipulation functions in the future.  The
words and manipulating shouldn't be there yet.

 -- Coding
 ...
 ... It is noticable that the parser seems to define
 its own is_space() and is_digit() functions, e.g.:

 +#define is_space(c) ((c) == '\t' || (c) == '\n' || (c) == '\r' || (c) == '
 ')

 Wouldn't it be more clean to use isspace() instead?

isspace() accepts '\f', '\v', and sometimes other characters as well,
depending on locale.  Likewise, isdigit() accepts some non-ASCII
characters in addition to [0-9], depending on the locale and platform.
 Thus, to avoid parsing a superset of the JSON spec, I can't use the
ctype.h functions.  I should add a comment with this explanation.

 This code block in function json_escape_unicode() looks suspicious:

 +   /* Convert to UTF-8, if necessary. */
 +   {
 +       const char *orig = json;
 +       json = (const char *)
 +           pg_do_encoding_conversion((unsigned char *) json, length,
 +                                     GetDatabaseEncoding(), PG_UTF8);
 +       if (json != orig)
 +           length = strlen(json);
 +   }

 Seems it *always* wants to convert the string. Isn't there a if condition
 missing?

pg_do_encoding_conversion does this check already.  Perhaps I should
rephrase the comment slightly:

+   /* Convert to UTF-8 (only if necessary). */

 There's a commented code fragment missing, which should be removed (see last
 two lines of the snippet below):

 +static unsigned int
 +read_hex16(const char *in)
 ...
 +               Assert(is_hex_digit(c));
 +
 +               if (c = '0'  c = '9')
 +                       tmp = c - '0';
 +               else if (c = 'A'  c = 'F')
 +                       tmp = c - 'A' + 10;
 +               else /* if (c = 'a'  c = 'f') */
 +                       tmp = c - 'a' + 10;

That comment is there for documentation purposes, to indicate the
range check that's skipped because we know c is a hex digit, and [a-f]
is the only thing it could be (and must be) at that line.  Should it
still be removed?

 json.c introduces new appendStringInfo* functions, e.g.
 appendStringInfoEscape() and appendStringInfoUtf8(). Maybe it's better
 to name them to something different, since it may occur someday that the
 backend
 will introduce the same notion with a different meaning. They are static,
 but why not naming them into something like jsonAppendStringInfoUtf8() or
 similar?

I agree.

 -- Regression Tests
...
 It also tests UNICODE sequences and input encoding with other server
 encoding than UTF-8.

It tests with other *client* encodings than UTF-8, but not other
server encodings than UTF-8.  Is it possible to write tests for
different server encodings?

-- Self-review

There's a minor bug in the normalization code:

 select $$ \u0009 $$::json;
   json
--
 \u0009
(1 row)

This should produce \t instead.

I'm thinking that my expect_*/next_*pop_char/... parsing framework is
overkill, and that, for a syntax as simple as JSON's, visibly messy
parsing code like this:

if (s  e  (*s == 'E' || *s == 'e'))
{
s++;
if (s  e  (*s == '+' || *s == '-'))
s++;
if (!(s  e  is_digit(*s)))
return false;
do
s++;
while (s  e  is_digit(*s));
}

would be easier to maintain than the deceptively elegant-looking code
currently in place:

if (optional_char_cond(s, e, *s == 'E' || *s == 'e'))
{
optional_char_cond(s, e, *s == '+' || *s == '-');
skip1_pred(s, e, is_digit);
}

I don't plan on gutting the current macro-driven parser just yet.
When I do, the new parser will support building a parse tree (only
when needed), and the parsing functions will have signatures like
this:

static bool parse_value(const char **sp, const char *e, JsonNode **out);
static bool parse_string(const char **sp, const char *e, StringInfo 
*out);
...

The current patch doesn't provide manipulation features where a parse
tree would come in handy.  I'd rather hold off on rewriting the parser
until such features are added.

I'll try to submit a revised patch within the next couple days.

Thanks!

- Joey

-- 
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] Latch implementation that wakes on postmaster death on both win32 and Unix

2011-07-04 Thread Fujii Masao
On Tue, Jul 5, 2011 at 1:36 AM, Florian Pflug f...@phlo.org wrote:
 On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
       Under Linux, select() may report a socket file descriptor as ready 
 for
       reading,  while nevertheless a subsequent read blocks.  This could 
 for
       example happen when data has arrived but  upon  examination  has  
 wrong
       checksum and is discarded.  There may be other circumstances in which 
 a
       file descriptor is spuriously reported as ready.  Thus it may be  
 safer
       to use O_NONBLOCK on sockets that should not block.

 So in theory, on Linux you might WaitLatch might sometimes incorrectly 
 return WL_POSTMASTER_DEATH. None of the callers check for 
 WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before 
 assuming the postmaster has died, so that won't affect correctness at the 
 moment. I doubt that scenario can even happen in our case, select() on a 
 pipe that is never written to. But maybe we should add add an assertion to 
 WaitLatch to assert that if select() reports that the postmaster pipe has 
 been closed, PostmasterIsAlive() also returns false.

 The correct solution would be to read() from the pipe after select()
 returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
 EAGAIN. To prevent that read() from blocking if the read event was indeed
 spurious, O_NONBLOCK must be set on the pipe but that patch does that already.

+1

The syslogger read() from the pipe after select(), then it thinks EOF
has arrived and
there is no longer write-side process if the return value of read() is
just zero.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] Cascade replication

2011-07-04 Thread Fujii Masao
On Mon, Jul 4, 2011 at 6:24 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Tue, Jun 14, 2011 at 6:08 AM, Fujii Masao masao.fu...@gmail.com wrote:

 The standby must not accept replication connection from that standby itself.
 Otherwise, since any new WAL data would not appear in that standby,
 replication cannot advance any more. As a safeguard against this, I 
 introduced
 new ID to identify each instance. The walsender sends that ID as the fourth
 field of the reply of IDENTIFY_SYSTEM, and then walreceiver checks whether
 the IDs are the same between two servers. If they are the same, which means
 that the standby is just connecting to that standby itself, so walreceiver
 emits ERROR.

 Thanks for waiting for review.

Thanks for the review!

 This part of the patch is troubling me. I think you have identified an
 important problem, but this solution doesn't work fully.

 If we allow standbys to connect to other standbys then we have
 problems with standbys not being connected to master. This can occur
 with a 1-step connection, as you point out, but it could also occur
 with a 2-step, 3-step or more connection, where a circle of standbys
 are all depending upon each other. Your solution only works for 1-step
 connections. Solving that problem in a general sense might be more
 dangerous than leaving it alone. I think we should think some more
 about the issues there and approach them as a separate problem.

 I think we should remove that and just focus on the main problem, for
 now. That will make it a simpler patch and easier to commit.

I agree to focus on the main problem first. I removed that. Attached
is the updated version.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 1998,2004  SET ENABLE_SEQSCAN TO OFF;
  doesn't keep any extra segments for standby purposes, and the number
  of old WAL segments available to standby servers is a function of
  the location of the previous checkpoint and status of WAL
! archiving.  This parameter has no effect on restartpoints.
  This parameter can only be set in the
  filenamepostgresql.conf/ file or on the server command line.
 /para
--- 1998,2004 
  doesn't keep any extra segments for standby purposes, and the number
  of old WAL segments available to standby servers is a function of
  the location of the previous checkpoint and status of WAL
! archiving.
  This parameter can only be set in the
  filenamepostgresql.conf/ file or on the server command line.
 /para
***
*** 2105,2111  SET ENABLE_SEQSCAN TO OFF;
  synchronous replication is enabled, individual transactions can be
  configured not to wait for replication by setting the
  xref linkend=guc-synchronous-commit parameter to
! literallocal/ or literaloff/.
 /para
 para
  This parameter can only be set in the filenamepostgresql.conf/
--- 2105,2112 
  synchronous replication is enabled, individual transactions can be
  configured not to wait for replication by setting the
  xref linkend=guc-synchronous-commit parameter to
! literallocal/ or literaloff/. This parameter has no effect on
! cascade replication.
 /para
 para
  This parameter can only be set in the filenamepostgresql.conf/
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***
*** 877,884  primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
--- 877,921 
   network delay, or that the standby is under heavy load.
  /para
 /sect3
+   /sect2
+ 
+   sect2 id=cascade-replication
+titleCascade Replication/title
  
+indexterm zone=high-availability
+ primaryCascade Replication/primary
+/indexterm
+para
+ Cascade replication feature allows the standby to accept the replication
+ connections and stream WAL records to another standbys. This is useful
+ for reducing the number of standbys connecting to the master and reducing
+ the overhead of the master, when you have many standbys.
+/para
+para
+ The cascading standby sends not only WAL records received from the
+ master but also those restored from the archive. So even if the replication
+ connection in higher level is terminated, you can continue cascade replication.
+/para
+para
+ Cascade replication is asynchronous. Note that synchronous replication
+ (see xref linkend=synchronous-replication) has no effect on cascade
+ replication.
+/para
+para
+ Promoting the cascading standby terminates all the cascade replication
+ connections which it uses. This is because the timeline becomes different
+ between standbys, and they cannot 

Re: [HACKERS] Online base backup from the hot-standby

2011-07-04 Thread Fujii Masao
2011/7/4 Jun Ishiduka ishizuka@po.ntts.co.jp:

 When the standby restarts after it crashes during recovery, it always
 checks whether recovery has reached the backup end location by
 using minRecoveryPoint even though the standby doesn't start from
 the backup. This looks odd.

 Certainly.

 But, in this case, the state before recovery starts is lost.
 Therefore, postgres can not see that the backup got from whether
 standby server or master.

 What should?
 Should use pg_control?
  Ex.
   * Add 'Where to get backup' to pg_control. (default 'none')
   * When recovery starts, it checks it whether 'none'.
      * When minRecoveryPoint equals 0/0, change 'master'.
      * When minRecoveryPoint do not equals 0/0, change 'slave'.
   * When it reached the end of recovery, change 'none' .

What about using backupStartPoint to check whether this recovery
started from the backup or not?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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