date:20121008

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

2012-10-08 Thread Joachim Wieland

On Mon, Oct 8, 2012 at 4:00 PM, Tom Lane  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

Re: [HACKERS] Support for REINDEX CONCURRENTLY

2012-10-08 Thread Michael Paquier

On Tue, Oct 9, 2012 at 8:14 AM, Jim Nasby  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] getopt() and strdup()

On Mon, Oct  8, 2012 at 04:33:29PM -0400, Tom Lane wrote:
> Bruce Momjian  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  http://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] MemSetLoop ignoring the 'val' parameter

Andres Freund  writes:
> On Tuesday, October 09, 2012 12:56:16 AM Tom Lane wrote:
>> Andres Freund  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] Support for REINDEX CONCURRENTLY

Jim Nasby  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.

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


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


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

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


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

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

On Tuesday, October 09, 2012 12:56:16 AM Tom Lane wrote:
> Andres Freund  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] MemSetLoop ignoring the 'val' parameter

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

[HACKERS] Placement of permissions checks for large object operations

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

On 8 October 2012 21:35, Robert Haas  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

Re: [HACKERS] Support for REINDEX CONCURRENTLY

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 :(...

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

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


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

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

[HACKERS] MemSetLoop ignoring the 'val' parameter

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] why repl_gram.h?

2012-10-08 Thread Magnus Hagander

On Oct 6, 2012 3:00 AM, "Peter Eisentraut"  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

Re: [HACKERS] sortsupport for text

On Mon, Oct 8, 2012 at 12:26 PM, Peter Geoghegan  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] getopt() and strdup()

Bruce Momjian  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] embedded list v3

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

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

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

2012-10-08 Thread Kohei KaiGai

2012/10/8 Dean Rasheed :
> On 8 October 2012 15:57, Kohei KaiGai  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 


-- 
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  wrote:
> Heikki Linnakangas  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] 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  wrote:

2012/10/8 Tom Lane :

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

[HACKERS] getopt() and strdup()

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

2012-10-08 Thread Dean Rasheed

On 8 October 2012 15:57, Kohei KaiGai  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

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"  writes:

On Oct 5, 2012, at 6:12 PM, Tom Lane  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] Bad Data back Door

2012-10-08 Thread David E. Wheeler

On Oct 8, 2012, at 11:13 AM, Tom Lane  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

"David E. Wheeler"  writes:
> On Oct 5, 2012, at 6:12 PM, Tom Lane  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 5, 2012, at 6:12 PM, Tom Lane  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] why can't plpgsql return a row-expression?

Pavel Stehule  writes:
> 2012/10/8 Tom Lane :
>> 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] Logging parameters values on bind error

Jehan-Guillaume de Rorthais  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] sortsupport for text

On 8 October 2012 16:30, Robert Haas  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] why can't plpgsql return a row-expression?

2012-10-08 Thread Pavel Stehule

2012/10/8 Tom Lane :
> Robert Haas  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

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

2012-10-08 Thread Jeff Janes

On Mon, Oct 8, 2012 at 8:58 AM, Bruce Momjian  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

Re: [HACKERS] Warnings from fwrite() in git head

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

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

[HACKERS] Warnings from fwrite() in git head

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
zic.c:1514: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1752: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1753: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1754: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1755: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1756: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1757: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1758: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1759: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1760: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1772: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result
zic.c:1785: warning: ignoring return value of ‘fwrite’, declared with 
attribute warn_unused_result

Here is the first C line that generated the warning message:

(void) fwrite((void *) buf, (size_t) sizeof buf, (size_t) 1, fp);

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.  

However, the only place I see fwrite() cast to void is in zic.c, and I
see this code in pg_upgrade/utils.c:

/*
 * There's nothing much we can do about it if fwrite fails, but some
 * platforms declare fwrite with warn_unused_result.  Do a little
 * dance with casting to void to shut up the compiler in such cases.
 */
size_t  rc;

rc = fwrite(message, strlen(message), 1, log_opts.internal);
/* if we are using OVERWRITE_MESSAGE, add newline to log file */
if (strchr(message, '\r') != NULL)
rc = fwrite("\n", 1, 1, log_opts.internal);
(void) rc;

which was added by this commit:

commit b98fd52a5516365e4a90d523cfed906eb7b3e67a
Author: Tom Lane 
Date:   Wed Sep 5 14:36:47 2012 -0400

Silence -Wunused-result warning in contrib/pg_upgrade.

This is just neatnik-ism, but since we do it for comparable code in 
elog.c,
we may as well do it here.

Should we give zic.c the same treatment?

-- 
  Bruce Momjian  http://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

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

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

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


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

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



-- 
KaiGai Kohei 


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

On Mon, Oct 8, 2012 at 8:47 AM, Peter Geoghegan  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] Add FET to Default and Europe.txt

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

2012-10-08 Thread Julien Tachoires

2012/10/8 Tom Lane :
> Julien Tachoires  writes:
>> 2012/10/8 Tom Lane :
>>> 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

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  http://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

On Mon, Oct  8, 2012 at 11:10:08AM -0400, Tom Lane wrote:
> Bruce Momjian  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  http://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

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

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

2012-10-08 Thread Julien Tachoires

2012/10/8 Robert Haas :
> On Mon, Oct 8, 2012 at 3:49 AM, Julien Tachoires  wrote:
>> 2012/10/8 Tom Lane :
>>> Julien Tachoires  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] Improving psql \ds

Julien Tachoires  writes:
> 2012/10/8 Tom Lane :
>> 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] Add FET to Default and Europe.txt

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?

-- 
  Bruce Momjian  http://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

On Mon, Oct 8, 2012 at 3:49 AM, Julien Tachoires  wrote:
> 2012/10/8 Tom Lane :
>> Julien Tachoires  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] [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 
> 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

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

Hi,

PL/pgsql seems to have a strange restriction regarding "RETURN".
Normally, you can write "RETURN ".  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] Missing OID define

On Fri, Oct 5, 2012 at 3:32 AM, Albe Laurenz  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

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

On Thu, Oct 4, 2012 at 6:12 AM, Amit kapila  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] enhanced error fields

On 20 August 2012 14:09, Pavel Stehule  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] Rethinking placement of latch self-pipe initialization

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

On 5 October 2012 04:37, Tom Lane  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] Deparsing DDL command strings

2012-10-08 Thread Dimitri Fontaine

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

[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] Regarding identifying a foreign scan

2012-10-08 Thread Merlin Moncure

On Sun, Oct 7, 2012 at 10:29 AM, Tom Lane  wrote:
> Atri Sharma  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

Re: [HACKERS] sortsupport for text

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: Promoting a standby during base backup (was Re: [HACKERS] Switching timeline over streaming replication)

On 4 October 2012 18:07, Fujii Masao  wrote:
> On Thu, Oct 4, 2012 at 4:59 PM, Heikki Linnakangas
>  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] 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: select processor to optimize for, one of:
64clhelp:/favor: 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 /? 2>NUL|") || die "cl command not found";
+   open(P, "cl /? 2>&1 |") || die "cl command not found";
while ()
{
-   if (/^\/favor:{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

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

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

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


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

On 8 October 2012 11:14, Marc Balmer  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] 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  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

On 8 October 2012 09:05, Heikki Linnakangas  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] Improving psql \ds

2012-10-08 Thread Julien Tachoires

2012/10/8 Tom Lane :
> Julien Tachoires  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


On 08.10.2012 10:03, Simon Riggs wrote:

On 6 October 2012 22:40, Tom Lane  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] Bad Data back Door

2012-10-08 Thread David E. Wheeler

On Oct 8, 2012, at 12:25 AM, "Albe Laurenz"  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] Bad Data back Door

2012-10-08 Thread Albe Laurenz

Tom Lane wrote:
> "David E. Wheeler"  writes:
>> On Oct 5, 2012, at 6:12 PM, Tom Lane  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] Rethinking placement of latch self-pipe initialization

On 7 October 2012 18:27, Tom Lane  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] Add FET to Default and Europe.txt