Re: [HACKERS] SQL/MED - file_fdw

2011-02-15 Thread Itagaki Takahiro
On Tue, Feb 8, 2011 at 00:30, Shigeru HANADA  wrote:
> Fixed version is attached.

I reviewed your latest git version, that is a bit newer than the attached patch.
http://git.postgresql.org/gitweb?p=users/hanada/postgres.git;a=commit;h=0e1a1e1b0e168cb3d8ff4d637747d0ba8f7b8d55

The code still works with small adjustment, but needs to be rebased on the
latest master, especially for extension support and copy API changes.

Here are a list of comments and suggestions:

* You might forget some combination or unspecified options in
file_fdw_validator().
  For example, format == NULL or !csv && header cases. I've not tested all
  cases, but please recheck validations used in BeginCopy().

* estimate_costs() needs own row estimation rather than using baserel.
> > What value does baserel->tuples have?
> > Foreign tables are never analyzed for now. Is the number correct?
> No, baserel->tuples is always 0, and baserel->pages is 0 too.
> In addition, width and rows are set to 100 and 1, as default values.

It means baserel is not reliable at all, right? If so, we need alternative
solution in estimate_costs(). We adjust statistics with runtime relation
size in estimate_rel_size(). Also, we use get_rel_data_width() for not
analyzed tables. We could use similar technique in file_fdw, too.

* Should use int64 for file byte size (or, uint32 in blocks).
unsigned long might be 32bit. ulong is not portable.

* Oid List (list_make1_oid) might be more handy than Const to hold relid
  in FdwPlan.fdw_private.

* I prefer FileFdwExecutionState to FileFdwPrivate, because there are
  two different 'fdw_private' variables in FdwPlan and FdwExecutionState.

* The comment in fileIterate seems wrong. It should be
  /* Store the next tuple as a virtual tuple. */  , right?

* #include  is needed.

-- 
Itagaki Takahiro

-- 
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] XMin Hot Standby Feedback patch

2011-02-15 Thread Simon Riggs
On Wed, 2011-02-16 at 11:25 +0900, Fujii Masao wrote:
> On Wed, Feb 16, 2011 at 6:15 AM, Simon Riggs  wrote:
> > On Tue, 2011-02-15 at 12:15 -0500, Robert Haas wrote:
> >> Looks pretty good to me, though I haven't tested it.  I like some of
> >> the safety valves you put in there, but I don't understand this part
> >
> > Reworked logic covering all feedback, plus tests, plus docs.
> >
> > Last comments before commit please.
> 
> When I started the standby with hot_standby = off and hot_standby_feedback = 
> on,
> I got the following assertion error.
> 
> -
> sby LOG:  streaming replication successfully connected to primary
> TRAP: FailedAssertion("!(((result) >= ((TransactionId) 3)))", File:
> "procarray.c", Line: 1027)
> act LOG:  unexpected EOF on standby connection
> sby LOG:  WAL receiver process (PID 17572) was terminated by signal 6: Aborted
> sby LOG:  terminating any other active server processes
> -

Thanks

> vacuum_defer_cleanup_age on the *standby* should not affect the
> feedback xid.

Not sure, will think some more.

> VACUUM TABLE on the *primary* doesn't use the feedback xid at all.
> Is this intentional?

Yes, I was in the middle of fixing that.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 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] updated patch for foreach stmt

2011-02-15 Thread Tom Lane
Stephen Frost  writes:
> * Pavel Stehule (pavel.steh...@gmail.com) wrote:
>> There is only bad keywords in doc - SCALE instead SLICE and a maybe a
>> usage of slicing need a example.

> Err, yeah, a couple of stupid documentation issues, sorry about that.

Applied with assorted cleanup.  I left the syntax as-is, since that
seems to be the plurality position at the moment.

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] Change pg_last_xlog_receive_location not to move backwards

2011-02-15 Thread Fujii Masao
On Tue, Feb 15, 2011 at 9:41 PM, Robert Haas  wrote:
> On Tue, Feb 15, 2011 at 12:34 AM, Fujii Masao  wrote:
>> You suggest that the shared variable Stream tracks the WAL write location,
>> after it's set to the replication starting position? I don't think
>> that the write
>> location needs to be tracked in the shmem because other processes than
>> walreceiver don't use it.
>
> Well, my proposal was to expose it, on the theory that it's useful.
> As we stream the WAL, we write it, so I think for all intents and
> purposes write == stream.  But using it to convey the starting
> position makes more sense if you call it stream than it does if you
> call it write.

Umm.. I could not find any use case to expose the WAL write location
besides flush one. So I'm not sure if it's really useful to track the
write location in the shmem besides the walreceiver-local memory.
What use case do you think of?

Personally the term "stream" sounds more ambiguous than "write".
I cannot imagine what location the pg_last_xlog_stream_location or
stream_location actually returns, from the function name;  WAL
location that has been received? written? flushed? replayed?
Since the "write" sounds cleaner, I like it.

Regards,

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

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


Re: [HACKERS] updated patch for foreach stmt

2011-02-15 Thread Pavel Stehule
2011/2/16 Tom Lane :
> Andrew Dunstan  writes:
>> On 02/15/2011 08:59 PM, Robert Haas wrote:
>>> Anyhoo, forcing the explicit ARRAY keyword in there seems like pretty
>>> cheap future-proofing to me.  YMMV.
>
>> If this is the syntax that makes you do things like:
>>      FOREACH foo IN ARRAY ARRAY[1,2,3]
>> I have to say I find that pretty darn ugly still.
>
> Yeah, that was the argument against requiring ARRAY.  So it comes down
> to whether you think we need future-proofing here.  I can't immediately
> see any reason for us to need a keyword right there, but ...

the combination of two keywords isn't nice, but we can ensure so
result of expression will has a requested type. It's more verbose,
it's more secure. We can to check a allowed keywords like SCALING in
compile time, we can use a more keywords - A hash type can need a
separation between KEY and VALUE - so any keyword there enables a
higher possibilities in future. We can do it without a auxiliary
keyword too, but parser will be more complex.

Regards

Pavel Stehule

>
>                        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] updated patch for foreach stmt

2011-02-15 Thread Tom Lane
Andrew Dunstan  writes:
> On 02/15/2011 08:59 PM, Robert Haas wrote:
>> Anyhoo, forcing the explicit ARRAY keyword in there seems like pretty
>> cheap future-proofing to me.  YMMV.

> If this is the syntax that makes you do things like:
>  FOREACH foo IN ARRAY ARRAY[1,2,3]
> I have to say I find that pretty darn ugly still.

Yeah, that was the argument against requiring ARRAY.  So it comes down
to whether you think we need future-proofing here.  I can't immediately
see any reason for us to need a keyword right there, but ...

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: pg_ctl failover Re: [HACKERS] Latches, signals, and waiting

2011-02-15 Thread Fujii Masao
On Wed, Feb 16, 2011 at 11:30 AM, Robert Haas  wrote:
> Committed with minor tweaks to comments and documentation.

Thanks a lot!

Regards,

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

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


Re: [HACKERS] why two dashes in extension load files

2011-02-15 Thread Alex Hunsaker
On Tue, Feb 15, 2011 at 14:12, Robert Haas  wrote:
> On Tue, Feb 15, 2011 at 3:26 PM, marcin mank  wrote:
>> how about : we use a single dash as the separator, and if the
>> extension author insists on having a dash in the name, as a punishment
>> he must duplicate the dash, i.e.:
>> uuid--ossp-1.0--5.5.sql
>
> That has a certain poetic justice to it.

Im not sure I see the poetic justice in trying to punish others for
*our* arbitrary naming rules. *shrug*

-- 
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] CommitFest 2011-01 as of 2011-02-04

2011-02-15 Thread Alex Hunsaker
On Mon, Feb 14, 2011 at 09:49, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> Here's where I think we are with this CommitFest.
>
>  Subject: Re: [HACKERS] CommitFest 2011-01 as of 2011-02-04
>
> I'm gonna go out on a limb and hope you meant '2011-02-14' there. :)
>
>> So there are two basic difficulties with wrapping the CommitFest up.
>
> [ ... ] It's been working really rather well, which is due in
> great part to the excellent CF managers (thanks again for being that,
> again).
[ ... ]

>        Thanks again, Robert, you've done an excellent job managing the CF.

I'd like to second this sentiment. I'm positive no one can really
appreciate the amount of work that goes into managing a commitfest
other than the elite few who have.

I imagine its not unlike being a mother: constantly bitching, ( out of
necessity mind you-- don't commit that! its HOT!, did you finish all
of your reviews? no? there are starving people in china! etc.) and yet
underrated, under-thanked and often undervalued.

Anyway, good job and thanks for volunteering to be the "bad" guy that
tries to keep the 9.1 time table sane.

-- 
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] Sync Rep for 2011CF1

2011-02-15 Thread Fujii Masao
On Wed, Feb 16, 2011 at 2:08 AM, Robert Haas  wrote:
> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao  wrote:
>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>>  wrote:
>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>>> sends a status update every time the WAL is flushed. If the walreceiver is
>>> busy receiving and flushing, that would happen once per WAL segment, which
>>> seems sensible.
>>
>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
>> via XLogWalRcvFlush(). This looks unsafe.
>
> Good catch.  Is the cleanest solution to pass a boolean parameter to
> XLogWalRcvFlush() indicating whether we're in the midst of dying?

Agreed if the comment about why such a boolean parameter is
required is added.

Regards,

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

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


Re: [HACKERS] updated patch for foreach stmt

2011-02-15 Thread Andrew Dunstan



On 02/15/2011 08:59 PM, Robert Haas wrote:

On Tue, Feb 15, 2011 at 8:44 PM, Tom Lane  wrote:

Robert Haas  writes:

On Tue, Feb 8, 2011 at 3:26 PM, Stephen Frost  wrote:

Alright, so, like I said, I really like this feature and would like to
see it included.

Amen to that!
I think the syntax Tom suggested before was FOREACH thingy IN ARRAY
arr rather than just FOREACH thingy IN arr.

Actually, I'm on record as saying the opposite: we shouldn't need to
distinguish the exact data type at the syntax level, so long as the
FOREACH construct is understood to mean "iterate through the members of
the composite object produced by this expression":

http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php

I am not, however, wedded to that position --- if people are happier
with explicit use of ARRAY here, I won't fight hard to get rid of it.

Anyway I'm going to start on this patch next, so last chance for
opinions about the syntax ...

Oh, I was looking at this one:

http://archives.postgresql.org/pgsql-hackers/2010-12/msg01557.php

Anyhoo, forcing the explicit ARRAY keyword in there seems like pretty
cheap future-proofing to me.  YMMV.




If this is the syntax that makes you do things like:

FOREACH foo IN ARRAY ARRAY[1,2,3]


I have to say I find that pretty darn ugly still.


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] Re: 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Mark Kirkwood

On 16/02/11 15:59, Greg Stark wrote:

On Wed, Feb 16, 2011 at 2:43 AM, Mark Kirkwood
  wrote:

What's this libbsd then eh? Sure enough it is this guy that defines these
symbols. So it is the way it is being built on the Ubuntu (or Debian)
platform.

Oh, for what it's worth there are several different libedits out there
with various related heritages. It's possible you're looking at two
completely different packages.

On Debian /usr/share/doc//README.Debian is supposed to say
where the upstream source was.



Yeah, good point:

$ dpkg -S /usr/lib/libedit.so.2
libedit2: /usr/lib/libedit.so.2

$ aptitude show libedit2
Package: libedit2
State: installed
Automatically installed: no
Version: 2.11-20080614-1build1
Priority: standard
Section: libs
Maintainer: Ubuntu Developers 
Uncompressed Size: 201k
Depends: libbsd0 (>= 0.0), libc6 (>= 2.11), libncurses5 (>= 5.6+20071006-3)
Description: BSD editline and history libraries
 The editline library provides generic line editing and history functions.

 It slightly resembles GNU readline
Homepage: 
http://ftp.netbsd.org/pub/NetBSD/NetBSD-release-5-0/src/lib/libedit/




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


[HACKERS] Re: 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Greg Stark
Look at the libeditline-dev packages I think those might be more
modern than the libedit packages.

But I'm not sure myself, I don't really know the history, I just
remember being confused by it once in the past.

-- 
greg

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


[HACKERS] Re: 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Greg Stark
On Wed, Feb 16, 2011 at 2:43 AM, Mark Kirkwood
 wrote:
> What's this libbsd then eh? Sure enough it is this guy that defines these
> symbols. So it is the way it is being built on the Ubuntu (or Debian)
> platform.

Oh, for what it's worth there are several different libedits out there
with various related heritages. It's possible you're looking at two
completely different packages.

On Debian /usr/share/doc//README.Debian is supposed to say
where the upstream source was.

-- 
greg

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


Re: [HACKERS] 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Mark Kirkwood

On 16/02/11 15:05, Mark Kirkwood wrote:

On 16/02/11 14:54, Tom Lane wrote:


It's pretty hard to see how those two things would be related.  I think
more likely libedit is providing a function named setproctitle, which
seems like a rather stupid thing for them to have done.


You are correct - it defines setproctitle, good grief.



...for some level of completeness in case this comes up again: it is not 
libedit that is at fault here. I downloaded the src for versions 2 amd 3 
and neither setproctitle not optreset are defined anywhere. Looking at 
my Ubuntu install reveals:


 $ ldd /usr/lib/libedit.so.2
linux-vdso.so.1 =>  (0x7fffd1bd3000)
libbsd.so.0 => /lib/libbsd.so.0 (0x7ff219cc9000)
libncurses.so.5 => /lib/libncurses.so.5 (0x7ff219a85000)
libc.so.6 => /lib/libc.so.6 (0x7ff219701000)
libdl.so.2 => /lib/libdl.so.2 (0x7ff2194fd000)
/lib64/ld-linux-x86-64.so.2 (0x7ff21a12)

What's this libbsd then eh? Sure enough it is this guy that defines 
these symbols. So it is the way it is being built on the Ubuntu (or 
Debian) platform.


Cheers

Mark

--
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] [PERFORM] pgbench to the MAXINT

2011-02-15 Thread Robert Haas
On Fri, Feb 11, 2011 at 8:35 AM, Stephen Frost  wrote:
> Greg,
>
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Greg Smith  writes:
>> > Poking around a bit more, I just discovered another possible approach is
>> > to use erand48 instead of rand in pgbench, which is either provided by
>> > the OS or emulated in src/port/erand48.c  That's way more resolution
>> > than needed here, given that 2^48 pgbench accounts would be a scale of
>> > 2.8M, which makes for a database of about 42 petabytes.
>>
>> I think that might be a good idea --- it'd reduce the cross-platform
>> variability of the results quite a bit, I suspect.  random() is not
>> to be trusted everywhere, but I think erand48 is pretty much the same
>> wherever it exists at all (and src/port/ provides it elsewhere).
>
> Works for me.  Greg, will you be able to work on this change?  If not, I
> might be able to.

Seeing as how this patch has not been updated, I think it's time to
mark this one Returned with Feedback.

-- 
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] Usability tweaks for extension commands

2011-02-15 Thread David E. Wheeler
On Feb 15, 2011, at 5:18 PM, Tom Lane wrote:

> Currently, ALTER EXTENSION UPDATE throws an error if there's nothing to
> do:
> 
> regression=# create extension adminpack ;
> CREATE EXTENSION
> regression=# alter extension adminpack update;
> ERROR:  version to install or update to must be different from old version
> 
> On reflection it seems like this is overly paranoid, and it'd be more
> useful if the ALTER just reported a NOTICE along the lines of "version
> so-and-so is already installed".  Any objections?

Makes sense to me.

> Another thought is that it'd probably be useful for there to be a
> "CREATE OR REPLACE EXTENSION" syntax, with the behavior of "install the
> extension if it's not present, else make sure it's of the specified or
> default version"; this behavior parallels CREATE OR REPLACE LANGUAGE
> which is something we've been refining for awhile.  I am not however
> entirely sure what to do with the SCHEMA option if the extension already
> exists --- we might be able to do SET SCHEMA, but perhaps that's too
> aggressive.

This one is a bit over my head, alas.

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] Replication server timeout patch

2011-02-15 Thread Fujii Masao
On Tue, Feb 15, 2011 at 7:13 AM, Daniel Farina  wrote:
> On Mon, Feb 14, 2011 at 12:48 AM, Fujii Masao  wrote:
>> On Sat, Feb 12, 2011 at 8:58 AM, Daniel Farina  wrote:
>>> Context diff equivalent attached.
>>
>> Thanks for the patch!
>>
>> As I said before, the timeout which this patch provides doesn't work well
>> when the walsender gets blocked in sending WAL. At first, we would
>> need to implement a non-blocking write function as an infrastructure
>> of the replication timeout, I think.
>> http://archives.postgresql.org/message-id/AANLkTi%3DPu2ne%3DVO-%2BCLMXLQh9y85qumLCbBP15CjnyUS%40mail.gmail.com
>
> Interesting point...if that's accepted as required-for-commit, what
> are the perceptions of the odds that, presuming I can write the code
> quickly enough, that there's enough infrastructure/ports already in
> postgres to allow for a non-blocking write on all our supported
> platforms?

I'm not sure if there's already enough infrastructure for a non-blocking
write. But the patch which I submitted before might help to implement that.
http://archives.postgresql.org/message-id/AANLkTinSvcdAYryNfZqd0wepyh1Pf7YX6Q0KxhZjas6a%40mail.gmail.com

Regards,

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

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


Re: pg_ctl failover Re: [HACKERS] Latches, signals, and waiting

2011-02-15 Thread Robert Haas
On Sun, Feb 13, 2011 at 10:18 PM, Fujii Masao  wrote:
> Thanks for the review!
>
> On Thu, Feb 10, 2011 at 11:25 PM, Magnus Hagander  wrote:
>> I see that the docs part of the patch removes the mentioning of
>> reporting servers - is that intentional, or a mistake? Seems that
>> usecase still remains, no?
>
> It was intentional, but I agree with you. I re-added the mention to
> the reporting servers.
>
> On Thu, Feb 10, 2011 at 11:30 PM, Magnus Hagander  wrote:
>> Also, the patch no longer applies, since it conflicts with
>> faa0550572583f51dba25611ab0f1d1c31de559b.
>>
>> Since you (Fujii-san) wrote both of them, feel like rebasing it
>> properly for current master?
>
> Yeah, I rebased the patch to the current git master and attached it.

Committed with minor tweaks to comments and documentation.

-- 
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] XMin Hot Standby Feedback patch

2011-02-15 Thread Fujii Masao
On Wed, Feb 16, 2011 at 6:15 AM, Simon Riggs  wrote:
> On Tue, 2011-02-15 at 12:15 -0500, Robert Haas wrote:
>> Looks pretty good to me, though I haven't tested it.  I like some of
>> the safety valves you put in there, but I don't understand this part
>
> Reworked logic covering all feedback, plus tests, plus docs.
>
> Last comments before commit please.

When I started the standby with hot_standby = off and hot_standby_feedback = on,
I got the following assertion error.

-
sby LOG:  streaming replication successfully connected to primary
TRAP: FailedAssertion("!(((result) >= ((TransactionId) 3)))", File:
"procarray.c", Line: 1027)
act LOG:  unexpected EOF on standby connection
sby LOG:  WAL receiver process (PID 17572) was terminated by signal 6: Aborted
sby LOG:  terminating any other active server processes
-

vacuum_defer_cleanup_age on the *standby* should not affect the
feedback xid.

VACUUM TABLE on the *primary* doesn't use the feedback xid at all.
Is this intentional?

Regards,

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

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


Re: [HACKERS] CommitFest 2011-01 as of 2011-02-04

2011-02-15 Thread Itagaki Takahiro
On Wed, Feb 16, 2011 at 02:14, Andrew Dunstan  wrote:
>> I've been kind of wondering why you haven't already committed it.  If
>> you're confident that the code is in good shape, I don't particularly
>> see any benefit to holding off.
>
> +10. The sooner the better.

Thanks comments. I've applied the COPY API patch.

-- 
Itagaki Takahiro

-- 
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] updated patch for foreach stmt

2011-02-15 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Feb 15, 2011 at 8:44 PM, Tom Lane  wrote:
> > Anyway I'm going to start on this patch next, so last chance for
> > opinions about the syntax ...
> 
> Oh, I was looking at this one:
> 
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01557.php
> 
> Anyhoo, forcing the explicit ARRAY keyword in there seems like pretty
> cheap future-proofing to me.  YMMV.

+1 for this, I don't see it as a big deal, and I would hate to discover
there's some reason we care (I dunno, implicit casts from ARRAY to
hstore ?) in the future that we're not thinking about now.

This also means there's no ambiguity as to what the iterator variable
should be declared as- if you're doing a FOREACH .. ARRAY, then your
iterator is an ARRAY (if it's not a scalar, of course), full stop.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Mark Kirkwood

On 16/02/11 14:54, Tom Lane wrote:


It's pretty hard to see how those two things would be related.  I think
more likely libedit is providing a function named setproctitle, which
seems like a rather stupid thing for them to have done.


You are correct - it defines setproctitle, good grief.

--
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] contrib loose ends: 9.0 to 9.1 incompatibilities

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 7:10 PM, Tom Lane  wrote:
> 1. We could just revert the pg_proc.h changes so that these two
> functions are still shown as taking only 2 arguments.  Since GIN doesn't
> actually look at the signature claimed in pg_proc, this won't break
> anything functionally.  It's pretty ugly though, and potentially will
> confuse people down the road.

-1.

> 2. We could add extra pg_proc.h entries matching the old signatures.
> For the moment these would be stub functions that call the same C code,
> though eventually perhaps they could be changed to throw errors.

+1.

> A related issue is that we similarly changed the signatures of GIN
> support functions that properly belong to intarray and tsearch2.
> That affects what the "unpackaged" conversion scripts need to expect.
>
> What I'm inclined to do there is just change the scripts to absorb
> the old functions as-is without trying to correct their signatures.
> Doing otherwise is a bit painful because they are operator class
> members, and there's no easy way to unhook them from the opclasses
> without dropping the opclasses.  The only other fix I can think of
> is a direct UPDATE on pg_proc to fix the proargtypes entries, which
> would work but seems even uglier.

Hmm.  Can we just invent a way to hook them from the opclasses?  I
have a feeling that now that this extension stuff is in we're going to
discover a bunch of these little utility commands that we managed to
get by without in the past but now that we're getting more organized
about it, we'll need 'em.

> There are some similar issues in pg_trgm as well.  I believe we can fix
> these with the available facilities so long as we don't mind the fact
> that opclasses upgraded from 9.0 installations will be subtly different
> from ones installed fresh in 9.1, for example the new operators being
> considered "loose" in the opfamily instead of being bound into an
> operator class.  While I don't immediately see any problems likely to
> arise from that, it's something that could perhaps bite us eventually.
> But there's no other answer except embarking on a project to materially
> upgrade the capabilities of ALTER OPERATOR CLASS/FAMILY, something I
> really don't want to be doing right now.

Or maybe that answers my question.

> BTW, none of these issues are new with the extensions patch; they are
> things we broke awhile ago.  I'm thinking it's really past time that
> we set up some routine buildfarm-style testing to see if pg_upgrade
> from the previous version still works.

Yeah.

-- 
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] updated patch for foreach stmt

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 8:44 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Feb 8, 2011 at 3:26 PM, Stephen Frost  wrote:
>>> Alright, so, like I said, I really like this feature and would like to
>>> see it included.
>
>> Amen to that!
>
>> I think the syntax Tom suggested before was FOREACH thingy IN ARRAY
>> arr rather than just FOREACH thingy IN arr.
>
> Actually, I'm on record as saying the opposite: we shouldn't need to
> distinguish the exact data type at the syntax level, so long as the
> FOREACH construct is understood to mean "iterate through the members of
> the composite object produced by this expression":
>
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php
>
> I am not, however, wedded to that position --- if people are happier
> with explicit use of ARRAY here, I won't fight hard to get rid of it.
>
> Anyway I'm going to start on this patch next, so last chance for
> opinions about the syntax ...

Oh, I was looking at this one:

http://archives.postgresql.org/pgsql-hackers/2010-12/msg01557.php

Anyhoo, forcing the explicit ARRAY keyword in there seems like pretty
cheap future-proofing to me.  YMMV.

-- 
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] 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Tom Lane
Mark Kirkwood  writes:
> Since libedit is getting some attention right now, I figured I'd try 
> using building with it instead of readline. configuring using:

> ./configure --prefix=/usr/local/pgsql/9.1 --enable-debug 
> --enable-cassert --with-libedit-preferred

> I get this linking postgres:

> postmaster/postmaster.o: In function `PostmasterMain':
> /home/postgres/develop/c/postgresql/src/backend/postmaster/postmaster.c:755: 
> undefined reference to `optreset'
> tcop/postgres.o: In function `process_postgres_switches':
> /home/postgres/develop/c/postgresql/src/backend/tcop/postgres.c:3457: 
> undefined reference to `optreset'
> utils/misc/ps_status.o: In function `set_ps_display':
> /home/postgres/develop/c/postgresql/src/backend/utils/misc/ps_status.c:314: 
> undefined reference to `setproctitle'

> What seems to be going on is that libedit defines optreset, which then 
> messes up the various other bits of logic in utils/misc/ps_status.c so 
> it thinks it has setproctitle on Linux.

It's pretty hard to see how those two things would be related.  I think
more likely libedit is providing a function named setproctitle, which
seems like a rather stupid thing for them to have done.

The optreset thing is probably the same ilk of issue, ie, libedit
providing such a symbol and configure then being fooled into expecting
it to be present in the standard libraries.

We could possibly fix this by delaying the test for libedit till after
we've tested for unrelated stuff, but that seems pretty klugy.

> I'm not sure how important this is, given that even if I (bodge) these 
> errors away, I merely confirm that libedit multi-byte input is busted in 
> the version shipped with Ubuntu 10.10.

There's that too :-(

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] 9.1 (git head) does not compile using --with-libedit-preferred on Ubuntu 10.10

2011-02-15 Thread Mark Kirkwood
Since libedit is getting some attention right now, I figured I'd try 
using building with it instead of readline. configuring using:


./configure --prefix=/usr/local/pgsql/9.1 --enable-debug 
--enable-cassert --with-libedit-preferred


I get this linking postgres:

postmaster/postmaster.o: In function `PostmasterMain':
/home/postgres/develop/c/postgresql/src/backend/postmaster/postmaster.c:755: 
undefined reference to `optreset'

tcop/postgres.o: In function `process_postgres_switches':
/home/postgres/develop/c/postgresql/src/backend/tcop/postgres.c:3457: 
undefined reference to `optreset'

utils/misc/ps_status.o: In function `set_ps_display':
/home/postgres/develop/c/postgresql/src/backend/utils/misc/ps_status.c:314: 
undefined reference to `setproctitle'


What seems to be going on is that libedit defines optreset, which then 
messes up the various other bits of logic in utils/misc/ps_status.c so 
it thinks it has setproctitle on Linux. The other files 
postmaster/postmaster.c and tcop/postgres.c get tricked into thinking 
they have optreset.


I'm not sure how important this is, given that even if I (bodge) these 
errors away, I merely confirm that libedit multi-byte input is busted in 
the version shipped with Ubuntu 10.10.


Cheers

Mark


--
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] updated patch for foreach stmt

2011-02-15 Thread Tom Lane
Robert Haas  writes:
> On Tue, Feb 8, 2011 at 3:26 PM, Stephen Frost  wrote:
>> Alright, so, like I said, I really like this feature and would like to
>> see it included.

> Amen to that!

> I think the syntax Tom suggested before was FOREACH thingy IN ARRAY
> arr rather than just FOREACH thingy IN arr.

Actually, I'm on record as saying the opposite: we shouldn't need to
distinguish the exact data type at the syntax level, so long as the
FOREACH construct is understood to mean "iterate through the members of
the composite object produced by this expression":

http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php

I am not, however, wedded to that position --- if people are happier
with explicit use of ARRAY here, I won't fight hard to get rid of it.

Anyway I'm going to start on this patch next, so last chance for
opinions about the syntax ...

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] Usability tweaks for extension commands

2011-02-15 Thread Tom Lane
Currently, ALTER EXTENSION UPDATE throws an error if there's nothing to
do:

regression=# create extension adminpack ;
CREATE EXTENSION
regression=# alter extension adminpack update;
ERROR:  version to install or update to must be different from old version

On reflection it seems like this is overly paranoid, and it'd be more
useful if the ALTER just reported a NOTICE along the lines of "version
so-and-so is already installed".  Any objections?

Another thought is that it'd probably be useful for there to be a
"CREATE OR REPLACE EXTENSION" syntax, with the behavior of "install the
extension if it's not present, else make sure it's of the specified or
default version"; this behavior parallels CREATE OR REPLACE LANGUAGE
which is something we've been refining for awhile.  I am not however
entirely sure what to do with the SCHEMA option if the extension already
exists --- we might be able to do SET SCHEMA, but perhaps that's too
aggressive.

Thoughts?

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] FOR KEY LOCK foreign keys

2011-02-15 Thread Josh Berkus

> How is such a determination made, exactly?

It's Feb 15th, and portions of the patch need a rework according to the
author.  I'm with Robert on this one.

-- 
  -- Josh Berkus
 PostgreSQL Experts Inc.
 http://www.pgexperts.com

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


[HACKERS] contrib loose ends: 9.0 to 9.1 incompatibilities

2011-02-15 Thread Tom Lane
I've been experimenting with dump/reload of 9.0 contrib-using databases
into 9.1 and then applying CREATE EXTENSION FROM to update the contrib
modules to extension style.  There are some cases that fail :-(.  Most
of them are caused by the GIN extractQuery API changes.  In particular,
a 9.0 dump including intarray will fail altogether with
"ginarrayextract(anyarray, internal) does not exist", and similarly
reloading tsearch2 fails with "gin_extract_tsvector(pg_catalog.tsvector,
internal) does not exist".  The functions do still exist in core, but we
added an extra argument to each.

There seem to be two possible solutions to this:

1. We could just revert the pg_proc.h changes so that these two
functions are still shown as taking only 2 arguments.  Since GIN doesn't
actually look at the signature claimed in pg_proc, this won't break
anything functionally.  It's pretty ugly though, and potentially will
confuse people down the road.

2. We could add extra pg_proc.h entries matching the old signatures.
For the moment these would be stub functions that call the same C code,
though eventually perhaps they could be changed to throw errors.

Preferences anyone?

A related issue is that we similarly changed the signatures of GIN
support functions that properly belong to intarray and tsearch2.
That affects what the "unpackaged" conversion scripts need to expect.
What I'm inclined to do there is just change the scripts to absorb
the old functions as-is without trying to correct their signatures.
Doing otherwise is a bit painful because they are operator class
members, and there's no easy way to unhook them from the opclasses
without dropping the opclasses.  The only other fix I can think of
is a direct UPDATE on pg_proc to fix the proargtypes entries, which
would work but seems even uglier.

There are some similar issues in pg_trgm as well.  I believe we can fix
these with the available facilities so long as we don't mind the fact
that opclasses upgraded from 9.0 installations will be subtly different
from ones installed fresh in 9.1, for example the new operators being
considered "loose" in the opfamily instead of being bound into an
operator class.  While I don't immediately see any problems likely to
arise from that, it's something that could perhaps bite us eventually.
But there's no other answer except embarking on a project to materially
upgrade the capabilities of ALTER OPERATOR CLASS/FAMILY, something I
really don't want to be doing right now.

Comments?

BTW, none of these issues are new with the extensions patch; they are
things we broke awhile ago.  I'm thinking it's really past time that
we set up some routine buildfarm-style testing to see if pg_upgrade
from the previous version still works.

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] pg_upgrade seems a tad broken

2011-02-15 Thread Bruce Momjian
Tom Lane wrote:
> I wrote:
> > I tried to do a pg_upgrade from 9.0.x to HEAD today.  The pg_upgrade run
> > went through without complaint, and I could start the postmaster, but
> > every connection attempt fails with 
> 
> > psql: FATAL:  could not read block 0 in file "base/11964/11683": read only 
> > 0 of 8192 bytes
> 
> > The database OID varies depending on which database I try to connect to,
> > but the filenode doesn't.  In the source 9.0 database, this relfilenode
> > belongs to pg_largeobject_metadata.  I'm not sure whether pg_upgrade
> > would've preserved relfilenode numbering, so that may or may not be a
> > useful hint as to where the problem is.  But in any case it's busted.
> 
> Closer investigation shows that in the new database, relfilenode 11683
> belongs to pg_class_oid_index, which explains why it's being touched
> during backend startup.  It is indeed of zero length, and surely should
> not be.  I can't resist the guess that something about the recently
> added hacks for pg_largeobject_metadata is not right.

I have fixed the bug;  patch attached and applied.  Seems I introduced
it during my pg_upgrade restructuring and didn't run my full regession
test suite after that.  My apologies.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
index dbbc143..0c518a2 100644
--- a/contrib/pg_upgrade/info.c
+++ b/contrib/pg_upgrade/info.c
@@ -48,7 +48,7 @@ gen_db_file_maps(DbInfo *old_db, DbInfo *new_db,
 	for (relnum = 0; relnum < old_db->rel_arr.nrels; relnum++)
 	{
 		RelInfo*old_rel = &old_db->rel_arr.rels[relnum];
-		RelInfo*new_rel = &old_db->rel_arr.rels[relnum];
+		RelInfo*new_rel = &new_db->rel_arr.rels[relnum];
 
 		if (old_rel->reloid != new_rel->reloid)
 			pg_log(PG_FATAL, "mismatch of relation id: database \"%s\", old relid %d, new relid %d\n",

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


Re: [HACKERS] pl/python do not delete function arguments

2011-02-15 Thread Jan Urbański
On 15/02/11 20:39, Peter Eisentraut wrote:
> On tis, 2011-02-15 at 09:58 +0100, Jan Urbański wrote:
>> Because the invocation that actually recurses sets up the scene for
>> failure.
> 
> That's what we're observing, but I can't figure out why it is.  If you
> can, could you explain it?
> 
> It actually makes sense to me that the arguments should be deleted at
> the end of the call.  The data belongs to that call only, and
> PLy_procedure_delete() that would otherwise clean it up is only called
> rarely.
> 
> Apparently, the recursive call ends up deleting the wrong arguments, but
> it's not clear to me why that would affect the next top-level call,
> because that would set up its own arguments again anyway.  In any case,
> perhaps the right fix is to fix PLy_function_delete_args() to delete the
> args correctly.

Aaah, ok, I got it (again). Let me write this in full before I forget
and spend another hour chasing that bug (and boy, bugs that disappear
because you're doing things in the debugger are so annoying). And
actually, my patch doesn't fix it fully :| Let me demonstrate:

CREATE FUNCTION rec(n integer) RETURNS integer AS $$
if n == 0:
return
plpy.notice("before n is %d" % n)
plpy.execute("select rec(0)")
plpy.notice("after n is %d" % n)
$$ LANGUAGE plpythonu;

Without the patch the second plpy.notice raises a NameError. With the
patch the output is:

NOTICE:  before n is 4
CONTEXT:  PL/Python function "rec"
NOTICE:  after n is 0
CONTEXT:  PL/Python function "rec"

What happens? In PLy_function_handler, PLy_function_build_args is
called, and proc->globals is set. After that PLy_procedure_call is
called, which starts executing Python code. The Python code does a call
into C with plpy.execute, and PLy_function_handler gets called (a
reentrant call).

Then PLy_function_build_args is called again. It overwrites the "n"
entry in proc->globals and then PLy_procedure_call gets called, which
drops us back into Python (on the stack there's now C, Python, C,
Python). This second invocation exits quickly because n == 0, and we're
back in C.

Now without my patch, the next thing to happen was deleting the
arguments, which removed "n" from the proc->globals dict. The rest of C
code runs and finally plpy.execute returns and we;re back in Python (the
stack is C, Python).

The second plpy.notice is run, which fetches "n" from the globals, and
not finding it, raises a NameError. With the patch it simply fetches the
overwritten value, namely 0.

The KeyError was a red herring - that's how Python reacted when
evaluating "n in (0, 1)", and if you look in the server log you'll see a
RuntimeWarning complaining about something internal, that doesn't
matter. The bottom line is that PLy_procedure_call is not reentrant
because of proc->globals, and it has to be.

Now when fixing this bug I tries copying the globals dict and restoring
it, but ran into issues (I think the problem was that the function
didn't like running with different globals then the one it has been
compiled with). Not sure what to do with this :( Document it as a caveat
(with or without my patch) and carry on? That sucks quite badly...

Jan

-- 
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: Adjust pg_upgrade error message, array freeing, and add error ch

2011-02-15 Thread Alvaro Herrera
Excerpts from Tom Lane's message of mar feb 15 18:05:59 -0300 2011:
> Bruce Momjian  writes:
> > Adjust pg_upgrade error message, array freeing, and add error check.
> 
> The buildfarm says this patch is broken.

I have just pushed a fix for this.  It's probably not the prettiest
thing in the world, but there doesn't seem to be a place where the
ClusterInfo structs are initialized.  Maybe that merits more cleanup,
not sure.

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

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


Re: [HACKERS] Debian readline/libedit breakage

2011-02-15 Thread Bernd Helmle



--On 15. Februar 2011 18:52:04 +0100 Stefan Kaltenbrunner 
 wrote:



well I have not actually tested - I was just reading the changelog on
http://www.thrysoee.dk/editline/ which claims UTF8 "support" (whatever
that means) in the current code drop.


I tested it--enable-wc doesn't work as you might expect. As Greg 
already said, it will bother you with a strange behavior when deleting 
characters (e.g. it removes half of your prompt) and at least on my mac i 
still wasn't able to enter multibyte characters like german umlauts.


--
Thanks

Bernd

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


Re: [HACKERS] XMin Hot Standby Feedback patch

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 4:15 PM, Simon Riggs  wrote:
> On Tue, 2011-02-15 at 12:15 -0500, Robert Haas wrote:
>> Looks pretty good to me, though I haven't tested it.  I like some of
>> the safety valves you put in there, but I don't understand this part
>
> Reworked logic covering all feedback, plus tests, plus docs.
>
> Last comments before commit please.

What happens if someone has hot_standby_feedback on and then turns it
off?  I think in XLogWalRcvSendReply() you need

if (hot_standby_feedback)
{
 stuff
}
else
{
reply_message.xmin = InvaidXID;
reply_message.epoch = 0;  /* or something */
}

Also this part looks kludgy to me:

+   GetNextXidAndEpoch(&nextXid, &nextEpoch);
+   if (nextXid < reply_message.xmin)
+   nextEpoch--;

How about introducing a GetOldestXminAndEpoch function instead?

Would it make sense to avoid grabbing the ProcArrayLock except when we
truly need to update MyProc->xmin?  ProcessStandbyReplyMessage gets
called a lot...

-- 
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] Extensions vs PGXS' MODULE_PATHNAME handling

2011-02-15 Thread Tom Lane
Dimitri Fontaine  writes:
> Do we want to add such a query in the docs to help pgfoundry authors to
> write their own 'from unpackaged' scripts?

[ scratches head ... ]  Why is your version generating so many
unnecessary @extschema@ uses?

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] NULLs in array_cat vs array || array

2011-02-15 Thread Thom Brown
On 15 February 2011 21:47, Tom Lane  wrote:
> Also, so far as I can see array_cat *is* ||, so I'm not sure what
> discrepancy in behavior you're on about.

You've confused me now.  I had a case where I replaced || with , and
surrounded it with array_cat, and the result differed, and now I can't
recreate it.  I think I should get an early night.

-- 
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

-- 
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] Extensions vs PGXS' MODULE_PATHNAME handling

2011-02-15 Thread Andrew Dunstan



On 02/15/2011 04:49 PM, Dimitri Fontaine wrote:


Ah well sed makes it simpler to read, but it won't be usable in windows.



You can make perl do the same stuff (and perl has psed anyway), and perl 
is required for MSVC builds.


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] NULLs in array_cat vs array || array

2011-02-15 Thread Thom Brown
On 15 February 2011 21:46, Cédric Villemain
 wrote:
> 2011/2/15 Thom Brown :
>> Hi all,
>>
>> I assumed array_cat would behave similarly to array || array, but it
>> appears not when it comes to NULLs.  Shouldn't these have identical
>> functionality?  The attached patch makes it so, although it would
>> break existing code.
>
> There is bugreport and todo entry for that if it helps:
>
> http://archives.postgresql.org/pgsql-bugs/2008-11/msg00032.php

Ah, I see.  More to it than meets the eye.  My bad.

-- 
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

-- 
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] Extensions vs PGXS' MODULE_PATHNAME handling

2011-02-15 Thread Dimitri Fontaine
Tom Lane  writes:
> Just for the archives' sake: the '@extschema@' business did turn out to
> be important, at least for tsearch2 where it's necessary to distinguish
> the objects it's dealing with from similarly-named objects in
> pg_catalog.  So this is what I used to generate the "unpackaged"
> scripts.  Some of them needed manual adjustment later to cover cases
> where 9.1 had diverged from 9.0, but the script could hardly be expected
> to know about that.

Good to know that even contrib needs that!

> #! /bin/sh
>
> MOD="$1"
>
> psql -d testdb -c "create extension $MOD"
>
> (
> echo "/* contrib/$MOD/$MOD--unpackaged--1.0.sql */"
> echo
>
> psql -A -t -d testdb -c "
>   SELECT 'ALTER EXTENSION ' || E.extname || ' ADD '
>   || replace(pg_describe_object(classid, objid, 0),
>  N.nspname, '@extschema@')
>   || ';'
> FROM pg_depend D
>  JOIN pg_extension E ON D.refobjid = E.oid
> AND D.refclassid = E.tableoid
>  JOIN pg_namespace N ON E.extnamespace = N.oid
>   WHERE deptype = 'e' AND E.extname = '$MOD'
>   ORDER BY D.objid
> " | sed -e 's/ADD cast from \(.*\) to \(.*\);/ADD cast (\1 as \2);/' \
>   -e 's/ for access method / using /'
> ) > contrib/$MOD/$MOD--unpackaged--1.0.sql

Ah well sed makes it simpler to read, but it won't be usable in windows.
I now realize also that the second version of this query did some
useless array type filtering.  Adding a replace() step in the query
would not be that ugly I guess, if we wanted to make it so.

Do we want to add such a query in the docs to help pgfoundry authors to
write their own 'from unpackaged' scripts?

CREATE OR REPLACE FUNCTION extension_unpackaged_upgrade_script(text)
  RETURNS SETOF text
  LANGUAGE SQL
AS $$
WITH objs AS (
  SELECT 'ALTER EXTENSION ' || E.extname || ' ADD '
  || replace(pg_describe_object(classid, objid, 0),
 N.nspname, '@extschema@')
  || ';' AS d
FROM pg_depend D
 JOIN pg_extension E ON D.refobjid = E.oid
AND D.refclassid = E.tableoid
 JOIN pg_namespace N ON E.extnamespace = N.oid
  WHERE deptype = 'e' AND E.extname = $1
  ORDER BY D.objid
)
SELECT regexp_replace(replace(d, ' for access method ', ' using '),
  'ADD cast from (.*) to (.*);',
  E'ADD cast (\\1 as \\2);')
  FROM objs
$$;


dim=# select * from extension_unpackaged_upgrade_script('lo');
 extension_unpackaged_upgrade_script
-
 ALTER EXTENSION lo ADD type @extschema@.lo;
 ALTER EXTENSION lo ADD function @extschema@.lo_oid(@extschema@.lo);
 ALTER EXTENSION lo ADD function @extschema@.lo_manage();
(3 rows)

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

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


Re: [HACKERS] NULLs in array_cat vs array || array

2011-02-15 Thread Tom Lane
Thom Brown  writes:
> I assumed array_cat would behave similarly to array || array, but it
> appears not when it comes to NULLs.  Shouldn't these have identical
> functionality?  The attached patch makes it so, although it would
> break existing code.

That patch is the hard way: the right change would be to remove the code
altogether and mark the function strict in pg_proc.  However, the fact
that it's not like that already shows that we went out of our way to
make it so.  I don't think we should undo that decision just because
somebody submits a patch to do so.

Also, so far as I can see array_cat *is* ||, so I'm not sure what
discrepancy in behavior you're on about.

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] NULLs in array_cat vs array || array

2011-02-15 Thread Cédric Villemain
2011/2/15 Thom Brown :
> Hi all,
>
> I assumed array_cat would behave similarly to array || array, but it
> appears not when it comes to NULLs.  Shouldn't these have identical
> functionality?  The attached patch makes it so, although it would
> break existing code.

There is bugreport and todo entry for that if it helps:

http://archives.postgresql.org/pgsql-bugs/2008-11/msg00032.php


>
> Would such a change have any knock-on effect, or cause inconsistency
> with other functions?
>
> Thanks
>
> Thom
>
> --
> Thom Brown
> Twitter: @darkixion
> IRC (freenode): dark_ixion
> Registered Linux user: #516935
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>



-- 
Cédric Villemain               2ndQuadrant
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] NULLs in array_cat vs array || array

2011-02-15 Thread Thom Brown
Hi all,

I assumed array_cat would behave similarly to array || array, but it
appears not when it comes to NULLs.  Shouldn't these have identical
functionality?  The attached patch makes it so, although it would
break existing code.

Would such a change have any knock-on effect, or cause inconsistency
with other functions?

Thanks

Thom

-- 
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935


array_cat_nulls.patch
Description: Binary data

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


Re: [HACKERS] FOR KEY LOCK foreign keys

2011-02-15 Thread Alvaro Herrera
Excerpts from Robert Haas's message of mar feb 15 18:15:38 -0300 2011:

> I am thinking that the statute of limitations has expired on this
> patch, and that we should mark it Returned with Feedback and continue
> working on it for 9.2.  I know it's a valuable feature, but I think
> we're out of time.

Okay, I've marked it as such in the commitfest app.  It'll be in 9.2's
first commitfest.

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

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


Re: [HACKERS] FOR KEY LOCK foreign keys

2011-02-15 Thread David E. Wheeler
On Feb 15, 2011, at 1:15 PM, Robert Haas wrote:

>> Yeah, that bug is fixed with the attached, though I am rethinking this
>> bit.
> 
> I am thinking that the statute of limitations has expired on this
> patch, and that we should mark it Returned with Feedback and continue
> working on it for 9.2.  I know it's a valuable feature, but I think
> we're out of time.

How is such a determination made, exactly?

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] XMin Hot Standby Feedback patch

2011-02-15 Thread Simon Riggs
On Tue, 2011-02-15 at 12:15 -0500, Robert Haas wrote:
> Looks pretty good to me, though I haven't tested it.  I like some of
> the safety valves you put in there, but I don't understand this part

Reworked logic covering all feedback, plus tests, plus docs.

Last comments before commit please.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services
 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 63c6283..30c33fb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2029,6 +2029,10 @@ SET ENABLE_SEQSCAN TO OFF;
 This parameter can only be set in the postgresql.conf
 file or on the server command line.

+   
+You should also consider setting hot_standby_feedback
+as an alternative to using this parameter.
+   
   
  
  
@@ -2121,6 +2125,22 @@ SET ENABLE_SEQSCAN TO OFF;
   
  
 
+ 
+  hot_standby_feedback (boolean)
+  
+   hot_standby_feedback configuration parameter
+  
+  
+   
+Specifies whether or not a hot standby will send feedback to the primary
+about queries currently executing on the standby. This parameter can
+be used to eliminate query cancels caused by cleanup records, though
+it can cause database bloat on the primary for some workloads.
+The default value is off.
+   
+  
+ 
+
  
 

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index a892969..6941e67 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1486,23 +1486,6 @@ if (!triggered)

 

-The most common reason for conflict between standby queries and WAL replay
-is early cleanup.  Normally, PostgreSQL allows
-cleanup of old row versions when there are no transactions that need to
-see them to ensure correct visibility of data according to MVCC rules.
-However, this rule can only be applied for transactions executing on the
-master.  So it is possible that cleanup on the master will remove row
-versions that are still visible to a transaction on the standby.
-   
-
-   
-Experienced users should note that both row version cleanup and row version
-freezing will potentially conflict with standby queries. Running a manual
-VACUUM FREEZE is likely to cause conflicts even on tables with
-no updated or deleted rows.
-   
-
-   
 Once the delay specified by max_standby_archive_delay or
 max_standby_streaming_delay has been exceeded, conflicting
 queries will be cancelled.  This usually results just in a cancellation
@@ -1529,6 +1512,23 @@ if (!triggered)

 

+The most common reason for conflict between standby queries and WAL replay
+is early cleanup.  Normally, PostgreSQL allows
+cleanup of old row versions when there are no transactions that need to
+see them to ensure correct visibility of data according to MVCC rules.
+However, this rule can only be applied for transactions executing on the
+master.  So it is possible that cleanup on the master will remove row
+versions that are still visible to a transaction on the standby.
+   
+
+   
+Experienced users should note that both row version cleanup and row version
+freezing will potentially conflict with standby queries. Running a manual
+VACUUM FREEZE is likely to cause conflicts even on tables with
+no updated or deleted rows.
+   
+
+   
 Users should be clear that tables that are regularly and heavily updated
 on the primary server will quickly cause cancellation of longer running
 queries on the standby. In such cases the setting of a finite value for
@@ -1539,12 +1539,10 @@ if (!triggered)
 

 Remedial possibilities exist if the number of standby-query cancellations
-is found to be unacceptable.  The first option is to connect to the
-primary server and keep a query active for as long as needed to
-run queries on the standby. This prevents VACUUM from removing
-recently-dead rows and so cleanup conflicts do not occur.
-This could be done using  and
-pg_sleep(), or via other mechanisms. If you do this, you
+is found to be unacceptable.  The first option is to set the parameter
+hot_standby_feedback, which prevents VACUUM from
+removing recently-dead rows and so cleanup conflicts do not occur.
+If you do this, you
 should note that this will delay cleanup of dead rows on the primary,
 which may result in undesirable table bloat. However, the cleanup
 situation will be no worse than if the standby queries were running
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 3277da8..e23c4e5 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -45,6 +45,7 @@
 #include "replicati

Re: [HACKERS] FOR KEY LOCK foreign keys

2011-02-15 Thread Robert Haas
On Mon, Feb 14, 2011 at 6:49 PM, Alvaro Herrera
 wrote:
> Excerpts from Marti Raudsepp's message of lun feb 14 19:39:25 -0300 2011:
>> On Fri, Feb 11, 2011 at 09:13, Noah Misch  wrote:
>> > The patch had a trivial conflict in planner.c, plus plenty of offsets.  
>> > I've
>> > attached the rebased patch that I used for review.  For anyone following 
>> > along,
>> > all the interesting hunks touch heapam.c; the rest is largely mechanical.  
>> > A
>> > "diff -w" patch is also considerably easier to follow.
>>
>> Here's a simple patch for the RelationGetIndexAttrBitmap() function,
>> as explained in my last post. I don't know if it's any help to you,
>> but since I wrote it I might as well send it up. This applies on top
>> of Noah's rebased patch.
>
> Got it, thanks.
>
>> I did some tests and it seems to work, although I also hit the same
>> visibility bug as Noah.
>
> Yeah, that bug is fixed with the attached, though I am rethinking this
> bit.

I am thinking that the statute of limitations has expired on this
patch, and that we should mark it Returned with Feedback and continue
working on it for 9.2.  I know it's a valuable feature, but I think
we're out of time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] why two dashes in extension load files

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 3:26 PM, marcin mank  wrote:
> On Tue, Feb 15, 2011 at 9:16 PM, Peter Eisentraut  wrote:
>> On mån, 2011-02-14 at 12:14 -0500, Tom Lane wrote:
>>> I guess the real question is what's Peter's concrete objection to the
>>> double-dash method?
>>
>> It just looks a bit silly and error prone.  And other packaging systems
>> have been doing without it for decades.
>
> how about : we use a single dash as the separator, and if the
> extension author insists on having a dash in the name, as a punishment
> he must duplicate the dash, i.e.:
> uuid--ossp-1.0--5.5.sql

That has a certain poetic justice to it.

-- 
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] review: FDW API

2011-02-15 Thread Tom Lane
Heikki Linnakangas  writes:
> On 15.02.2011 21:13, Tom Lane wrote:
>> Hmm.  I don't have a problem with adding relkind to the planner's
>> RelOptInfo, but it seems to me that if parse analysis needs to know
>> this, you have put functionality into parse analysis that does not
>> belong there.

> Possibly. We throw the existing errors, for example if you try to do 
> "FOR UPDATE OF foo" where foo is a set-returning function, in 
> transformLockingClause(), so it seemed like the logical place to check 
> for foreign tables too.

> Hmm, one approach would be to go ahead and create the RowMarkClauses for 
> all relations in the parse analysis phase, foreign or not, and throw the 
> error later, in preprocess_rowmarks().

I think moving the error check downstream would be a good thing.
Consider for example the case of applying FOR UPDATE to a view.  You
can't know what that entails until after the rewriter expands the view.
IIRC, at the moment we're basically duplicating the tests between parse
analysis and the planner, but it's not clear what the value of that is.

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] review: FDW API

2011-02-15 Thread Heikki Linnakangas

On 15.02.2011 21:00, Robert Haas wrote:

On Tue, Feb 15, 2011 at 1:40 PM, Heikki Linnakangas
  wrote:

I'm actually surprised we don't need to distinguish them in more places, but
nevertheless it feels like we should have that info available more
conveniently, and without requiring a catalog lookup like get_rel_relkind()
does. At first I thought we should add a field to RelOptInfo, but that
doesn't help transformLockingClause. Adding the field to RangeTblEntry seems
quite invasive, and it doesn't feel like the right place to cache that kind
of information anyway. Thoughts on that?


Maybe it should be a new RTEKind.


That would be even more invasive.

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

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


Re: [HACKERS] review: FDW API

2011-02-15 Thread Heikki Linnakangas

On 15.02.2011 21:13, Tom Lane wrote:

Heikki Linnakangas  writes:

As the patch stands, we have to do get_rel_relkind() in a couple of
places in parse analysis and the planner to distinguish a foreign table
from a regular one. As the patch stands, there's nothing in
RangeTblEntry (which is what we have in transformLockingClause) or
RelOptInfo (for set_plain_rel_pathlist) to directly distinguish them.


Hmm.  I don't have a problem with adding relkind to the planner's
RelOptInfo, but it seems to me that if parse analysis needs to know
this, you have put functionality into parse analysis that does not
belong there.


Possibly. We throw the existing errors, for example if you try to do 
"FOR UPDATE OF foo" where foo is a set-returning function, in 
transformLockingClause(), so it seemed like the logical place to check 
for foreign tables too.


Hmm, one approach would be to go ahead and create the RowMarkClauses for 
all relations in the parse analysis phase, foreign or not, and throw the 
error later, in preprocess_rowmarks(). preprocess_rowmarks() doesn't 
currently know if each RowMarkClause was created by "... FOR UPDATE" or 
"FOR UPDATE OF foo", but to be consistent with the logic in 
transformLockingClause() it would need to. For the former case, it would 
need to just ignore foreign tables but for the latter it would need to 
throw an error. I guess we could add an "explicit" flag to RowMarkClause 
for that.


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

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


Re: [HACKERS] why two dashes in extension load files

2011-02-15 Thread David E. Wheeler
On Feb 15, 2011, at 12:32 PM, Tom Lane wrote:

> Aside from the double-dash method, we kicked around using colons and
> pluses as separators (and then forbidding just those characters in
> extension and version names).  Any of those would be workable, but it's
> not clear to me that any of them have any particular cosmetic advantage
> over any others.  In any case, the time to be voting on them is past so
> far as I'm concerned.  The work is already done and I'm uneager to do it
> over on one person's say-so.

I'd prefer a single character, but can live with -- just fine.

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 two dashes in extension load files

2011-02-15 Thread Tom Lane
Peter Eisentraut  writes:
> On mån, 2011-02-14 at 12:14 -0500, Tom Lane wrote:
>> I guess the real question is what's Peter's concrete objection to the
>> double-dash method?

> It just looks a bit silly and error prone.  And other packaging systems
> have been doing without it for decades.

I can't claim close familiarity with Debian's conventions in this
matter, but I do know about RPM's, and I'm uneager to duplicate that
silliness.  Magic conversion of dots to underscores (sometimes),
complete inability to determine which part of the package filename is
which without external knowledge, etc.

Aside from the double-dash method, we kicked around using colons and
pluses as separators (and then forbidding just those characters in
extension and version names).  Any of those would be workable, but it's
not clear to me that any of them have any particular cosmetic advantage
over any others.  In any case, the time to be voting on them is past so
far as I'm concerned.  The work is already done and I'm uneager to do it
over on one person's say-so.

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 two dashes in extension load files

2011-02-15 Thread marcin mank
On Tue, Feb 15, 2011 at 9:16 PM, Peter Eisentraut  wrote:
> On mån, 2011-02-14 at 12:14 -0500, Tom Lane wrote:
>> I guess the real question is what's Peter's concrete objection to the
>> double-dash method?
>
> It just looks a bit silly and error prone.  And other packaging systems
> have been doing without it for decades.

how about : we use a single dash as the separator, and if the
extension author insists on having a dash in the name, as a punishment
he must duplicate the dash, i.e.:
uuid--ossp-1.0--5.5.sql

Greetings
Marcin Mańk

-- 
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 two dashes in extension load files

2011-02-15 Thread Tom Lane
Peter Eisentraut  writes:
> On mån, 2011-02-14 at 15:08 -0500, Tom Lane wrote:
>> Umm ... we are not requiring version names to be numbers.

> That's certainly interesting.  Why?

There isn't any packaging system anywhere on the planet that requires
them to be purely numeric.  By the time you get done allowing for
multiple dots and "alpha" or "beta" and other such stuff, you might
as well try to be agnostic about what they contain.

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 two dashes in extension load files

2011-02-15 Thread Peter Eisentraut
On mån, 2011-02-14 at 12:14 -0500, Tom Lane wrote:
> I guess the real question is what's Peter's concrete objection to the
> double-dash method?

It just looks a bit silly and error prone.  And other packaging systems
have been doing without it for decades.


-- 
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 two dashes in extension load files

2011-02-15 Thread Peter Eisentraut
On mån, 2011-02-14 at 15:08 -0500, Tom Lane wrote:
> Umm ... we are not requiring version names to be numbers.

That's certainly interesting.  Why?



-- 
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] sepgsql contrib module

2011-02-15 Thread Kohei Kaigai

> -Original Message-
> From: Robert Haas [mailto:robertmh...@gmail.com]
> Sent: 15 February 2011 16:52
> To: Tom Lane
> Cc: Andrew Dunstan; Kohei Kaigai; Stephen Frost; KaiGai Kohei; PgHacker
> Subject: Re: [HACKERS] sepgsql contrib module
> 
> On Tue, Feb 15, 2011 at 11:41 AM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane  wrote:
> >>> Robert Haas  writes:
>  Those are good points.  My point was just that you can't actually
>  build that file at the time you RUN the regression tests, because you
>  have to build it first, then install it, then run the regression
>  tests.  It could be a separate target, like 'make policy', but I don't
>  think it works to make it part of 'make installcheck'.
> >
> >>> So?  Once you admit that you can do that, it's a matter of a couple
> more
> >>> lines to make the installcheck target depend on the policy target iff
> >>> selinux was enabled.
> >
> >> Sure, you could do that, but I don't see what problem it would fix.
> >> You'd still have to build and manually install the policy before you
> >> could run make installcheck.  And once you've done that, you don't
> >> need to rebuild it every future time you run make installcheck.
> >
> > Oh, I see: you're pointing out the root-only "semodule" step that has
> to
> > be done in between there.  Good point.  But the current arrangement is
> > still a mistake: the required contents of sepgsql-regtest.pp depend on
> > the configuration of the test system, which can't be known at build
> > time.
> >
> > So what we should do is offer a "make policy" target and alter the test
> > instructions to say you should do that and then run semodule.  Or maybe
> > just put the whole "make -f /usr/share/selinux/devel/Makefile" dance
> > into the instructions --- it doesn't look to me like our makefile
> > infrastructure really has anything useful to add to that.
> 
> Yeah, agreed.
> 
I also agree with this direction. The policy type depends on individual 
installations,
it is not easy to assume on build time.
Please wait for a small patch to remove this rule from Makefile and update 
documentation.

As a side note, we can have a build option that does not require selinux 
enabled.
The reason why Makefile of selinux tries to /selinux/mls is that we don't 
specify
MLS=1 or MLS=0 explicitly.
IIRC, the specfile of RHEL/Fedora gives all the Makefile parameters explicitly, 
thus,
selinux does not need to be enabled on the build server.
However, it is not a solution in this case. It is not easy to estimate the 
required
policy type and existence of MLS support on build time.

Thanks,
--
NEC Europe Ltd, Global Competence Center
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] pl/python do not delete function arguments

2011-02-15 Thread Peter Eisentraut
On tis, 2011-02-15 at 09:58 +0100, Jan Urbański wrote:
> Because the invocation that actually recurses sets up the scene for
> failure.

That's what we're observing, but I can't figure out why it is.  If you
can, could you explain it?

It actually makes sense to me that the arguments should be deleted at
the end of the call.  The data belongs to that call only, and
PLy_procedure_delete() that would otherwise clean it up is only called
rarely.

Apparently, the recursive call ends up deleting the wrong arguments, but
it's not clear to me why that would affect the next top-level call,
because that would set up its own arguments again anyway.  In any case,
perhaps the right fix is to fix PLy_function_delete_args() to delete the
args correctly.



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


Re: [HACKERS] review: FDW API

2011-02-15 Thread Tom Lane
Heikki Linnakangas  writes:
> As the patch stands, we have to do get_rel_relkind() in a couple of 
> places in parse analysis and the planner to distinguish a foreign table 
> from a regular one. As the patch stands, there's nothing in 
> RangeTblEntry (which is what we have in transformLockingClause) or 
> RelOptInfo (for set_plain_rel_pathlist) to directly distinguish them.

Hmm.  I don't have a problem with adding relkind to the planner's
RelOptInfo, but it seems to me that if parse analysis needs to know
this, you have put functionality into parse analysis that does not
belong there.

(No, I haven't read the 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] review: FDW API

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 1:40 PM, Heikki Linnakangas
 wrote:
> I'm actually surprised we don't need to distinguish them in more places, but
> nevertheless it feels like we should have that info available more
> conveniently, and without requiring a catalog lookup like get_rel_relkind()
> does. At first I thought we should add a field to RelOptInfo, but that
> doesn't help transformLockingClause. Adding the field to RangeTblEntry seems
> quite invasive, and it doesn't feel like the right place to cache that kind
> of information anyway. Thoughts on that?

Maybe it should be a new RTEKind.

-- 
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] Extensions vs PGXS' MODULE_PATHNAME handling

2011-02-15 Thread Tom Lane
I wrote:
> Dimitri Fontaine  writes:
>> I think you'd be interested into this reworked SQL query.  It should be
>> providing exactly the script file you need as an upgrade from unpackaged.

> This seems overly complicated.  I have a version of it that I'll publish
> as soon as I've tested it on all the contrib modules ...

Just for the archives' sake: the '@extschema@' business did turn out to
be important, at least for tsearch2 where it's necessary to distinguish
the objects it's dealing with from similarly-named objects in
pg_catalog.  So this is what I used to generate the "unpackaged"
scripts.  Some of them needed manual adjustment later to cover cases
where 9.1 had diverged from 9.0, but the script could hardly be expected
to know about that.

#! /bin/sh

MOD="$1"

psql -d testdb -c "create extension $MOD"

(
echo "/* contrib/$MOD/$MOD--unpackaged--1.0.sql */"
echo

psql -A -t -d testdb -c "
  SELECT 'ALTER EXTENSION ' || E.extname || ' ADD '
  || replace(pg_describe_object(classid, objid, 0),
 N.nspname, '@extschema@')
  || ';'
FROM pg_depend D
 JOIN pg_extension E ON D.refobjid = E.oid
AND D.refclassid = E.tableoid
 JOIN pg_namespace N ON E.extnamespace = N.oid
  WHERE deptype = 'e' AND E.extname = '$MOD'
  ORDER BY D.objid
" | sed -e 's/ADD cast from \(.*\) to \(.*\);/ADD cast (\1 as \2);/' \
-e 's/ for access method / using /'
) > contrib/$MOD/$MOD--unpackaged--1.0.sql


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 support for logging the current role

2011-02-15 Thread Stephen Frost
* Andrew Dunstan (and...@dunslane.net) wrote:
> On 02/15/2011 11:13 AM, Stephen Frost wrote:
> >Think I suggested that at one point.  I'm all for doing that on a major
> >version change like this one, but I think we already had some concerns
> >about that on this thread (Andrew maybe?).
> 
> I could live with it for a release if I thought we had a clear path
> ahead, but I think there are some design issues that we need to
> think about before we start providing for header lines and variable
> formats in CSV logs, particularly w.r.t. log rotation etc. So I'm
> slightly nervous about going ahead with this right now.

I believe the suggestion that Robert and I were talking about above was
to just unilatterally change the CSV log file output format to include
current_role.  No header lines, no variable output format, etc.

I do think we can make header lines and variable output work, if we can
get agreement on what the semantics should be.

> You don't really make your case any better by continuing this
> argument from years ago. I can tell you from experience that the CSV
> HEADER feature is distinctly useful as it is. If you want to add a
> mode that uses the header line as a column list on import, then make
> that case, and I'll support it in fact, but it's not an alternative
> to having the header ignored, which is a feature I would vigorously
> resist removing. 

I'm not really interested in removing it.  I guess I have a vain hope
that with arguing I'll convince someone to take up the mantle of
implementing the 'use header' option. :)  Not getting much traction
though, so I expect I'll work on it this summer.

> (Incidentally, I think it won't be trivial - the
> COPY code expects to know the columns by the time it opens the
> file).

Thanks for that insight, I'll take a look at how things work and see if
I can come up with a sensible proposal.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] XMin Hot Standby Feedback patch

2011-02-15 Thread Simon Riggs
On Tue, 2011-02-15 at 12:20 -0500, Robert Haas wrote:
> On Tue, Feb 15, 2011 at 12:15 PM, Robert Haas  wrote:
> > On another disk, I think that those warning messages are a bad idea.
> > That could fill up someone's disk really quickly.
> 
> On another disk?  What the heck am I talking about?
> 
> On another point?  On another note?  Anyway, you get the idea... hopefully.

Yeh. I quite liked it as a metaphorical turn of phrase.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 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] arrays as pl/perl input arguments [PATCH]

2011-02-15 Thread David E. Wheeler
On Feb 15, 2011, at 9:49 AM, Alexey Klyukin wrote:

>>> After I re-added the closing  in plperl.sgml:235 these errors 
>>> disappeared, and the
>>> resulting html looks fine too. v10 with just this single change is attached.
>> 
>> So is this ready for committer?
> 
> Yes.

Awesom, thanks Alexey & Alex!

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] Debian readline/libedit breakage

2011-02-15 Thread Stefan Kaltenbrunner

On 02/15/2011 12:37 PM, Greg Stark wrote:

On Tue, Feb 15, 2011 at 6:12 AM, Stefan Kaltenbrunner
  wrote:

from what I can see upstream libedit actually has utf8 support for a while
now (as well as some other fixes) but the debian libedit version (and also
the one of other distributions) is way too old for that so maybe most of the
issues would be mood if debian updated to a newer libedit version...


There's utf8 support and then there's utf8 support. last I saw libedit
didn't actually stop you from using utf8 and things kind of worked,
but none of the editing commands understand what the multibyte
characters were or understood what column position you were in so you
could easily end up deleting half a character or with the insertion
point in the middle of a character.


well I have not actually tested - I was just reading the changelog on 
http://www.thrysoee.dk/editline/ which claims UTF8 "support" (whatever 
that means) in the current code drop.



Stefan

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


Re: [HACKERS] Fix for Index Advisor related hooks

2011-02-15 Thread Tom Lane
Gurjeet Singh  writes:
> On Tue, Feb 15, 2011 at 8:24 AM, Heikki Linnakangas <
> heikki.linnakan...@enterprisedb.com> wrote:
>> On 11.02.2011 22:44, Gurjeet Singh wrote:
>>> One one hand get_actual_variable_range() expects that virtual indexes do
>>> not
>>> have an OID assigned, on the other hand explain_get_index_name_hook() is
>>> handed just an index's OID to get its name back; IMHO these are based on
>>> two
>>> conflicting assumptions about whether a virtual index will have an OID
>>> assigned.

>> The new hook takes an index oid as argument, so I gather that you resolved
>> the contradiction by deciding that fictitious indexes have OIDs. How do you
>> assign those OIDs? Do fictitious indexes have entries in pg_index?

> No, a fictitious index does not touch pg_index. The  Index Advisor uses
> GetNewOid(pg_class) to generate a new OID for the fictitious index.

That seems like a very expensive, and lock-inducing, way of assigning a
fictitious OID.  They don't need to be globally unique.  I suggest you
consider the idea I suggested back in 2007:

 * In this toy example we just assign all hypothetical indexes
 * OID 0, and the explain_get_index_name hook just prints
 * .  In a realistic situation we'd probably
 * assume that OIDs smaller than, say, 100 are never the OID of
 * any real index, allowing us to identify one of up to 100
 * hypothetical indexes per plan.  Then we'd need to save aside
 * some state data that would let the explain hooks print info
 * about the selected index.

As far as the immediate problem goes, I agree that
get_actual_variable_range is mistaken, but I think a cleaner and cheaper
solution would be to add a bool "hypothetical" to IndexOptInfo.

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] psql -l doesn't process psqlrc

2011-02-15 Thread Robert Haas
On Sun, Feb 13, 2011 at 7:52 AM, Peter Eisentraut  wrote:
> psql -l doesn't process psqlrc.  Historically, this was probably not
> useful, hence no one cared.  But with the linestyle option it's useful.
> So I propose the attached tweak.

As a violent hater of the new linestyle, +1 from me.

-- 
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] arrays as pl/perl input arguments [PATCH]

2011-02-15 Thread Alexey Klyukin

On Feb 15, 2011, at 7:45 PM, David E. Wheeler wrote:

> On Feb 15, 2011, at 6:39 AM, Alexey Klyukin wrote:
> 
>> After I re-added the closing  in plperl.sgml:235 these errors 
>> disappeared, and the
>> resulting html looks fine too. v10 with just this single change is attached.
> 
> So is this ready for committer?

Yes.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.





-- 
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] arrays as pl/perl input arguments [PATCH]

2011-02-15 Thread David E. Wheeler
On Feb 15, 2011, at 6:39 AM, Alexey Klyukin wrote:

> After I re-added the closing  in plperl.sgml:235 these errors 
> disappeared, and the
> resulting html looks fine too. v10 with just this single change is attached.

So is this ready for committer?

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] extensions and psql

2011-02-15 Thread Dimitri Fontaine
Tom Lane  writes:
> Sure I did: \dx+

And I believe I did test that.  Sorry for the noise, really.  (shame)

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

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


Re: [HACKERS] Replication server timeout patch

2011-02-15 Thread Robert Haas
On Mon, Feb 14, 2011 at 5:13 PM, Daniel Farina  wrote:
> On Mon, Feb 14, 2011 at 12:48 AM, Fujii Masao  wrote:
>> On Sat, Feb 12, 2011 at 8:58 AM, Daniel Farina  wrote:
>>> Context diff equivalent attached.
>>
>> Thanks for the patch!
>>
>> As I said before, the timeout which this patch provides doesn't work well
>> when the walsender gets blocked in sending WAL. At first, we would
>> need to implement a non-blocking write function as an infrastructure
>> of the replication timeout, I think.
>> http://archives.postgresql.org/message-id/AANLkTi%3DPu2ne%3DVO-%2BCLMXLQh9y85qumLCbBP15CjnyUS%40mail.gmail.com
>
> Interesting point...if that's accepted as required-for-commit, what
> are the perceptions of the odds that, presuming I can write the code
> quickly enough, that there's enough infrastructure/ports already in
> postgres to allow for a non-blocking write on all our supported
> platforms?

Are you working on 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] CommitFest 2011-01 as of 2011-02-04

2011-02-15 Thread Chris Browne
robertmh...@gmail.com (Robert Haas) writes:
> It does, but frankly I don't see much reason to change it, since it's
> been working pretty well on the whole.  Andrew was on point when he
> mentioned that it's not obvious what committers get out of working on
> other people's patches.  Obviously, the answer is, well, they get a
> better PostgreSQL, and that's ultimately good for all of us.  But the
> trickiest part of this whole process is that, on the one hand, it's
> not fair for committers to ignore other people's patches, but on the
> other hand, it's not fair to expect committers to sacrifice getting
> their own projects done to get other people's projects done.

I had two interesting germane comments in my RSS feed this morning, both
entitled "Please send a patch"

   http://www.lucas-nussbaum.net/blog/?p=630

Where Lucas suggests that, when someone requests an enhancement, the
retort "Please send a patch" mayn't be the best idea, because the
one receiving the requests may be many times better at contributing
such changes than the one making the request.

   http://hezmatt.org/~mpalmer/blog/general/please_send_a_patch.html

"On the other hand, Lucas, remember that each time you ask someone
to take some time to implement your pet feature request, you take
some time away from her that could be used to contribute something
in an area where she gives a damn."

These are *both* true statements, and, in order to grow the community
that is capable of enhancing the system, there is merit to the careful
application of both positions.

There's stuff that Tom should do :-).  And absent the general
availability of cloning machines, we need to have people improving their
skills so that there are more that are capable of submitting (and
evaluating and committing) usable patches.
-- 
wm(X,Y):-write(X),write('@'),write(Y). wm('cbbrowne','gmail.com').
http://linuxdatabases.info/info/languages.html
Signs  of  a Klingon  Programmer -  14.  "Our  competitors are without
honor!"

-- 
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] extensions and psql

2011-02-15 Thread Tom Lane
Dimitri Fontaine  writes:
> I realize that you didn't keep the \dx behavior I had, that when given
> an extension name it would list all the objects contained in the
> extension.

Sure I did: \dx+

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] XMin Hot Standby Feedback patch

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 12:15 PM, Robert Haas  wrote:
> On another disk, I think that those warning messages are a bad idea.
> That could fill up someone's disk really quickly.

On another disk?  What the heck am I talking about?

On another point?  On another note?  Anyway, you get the idea... hopefully.

-- 
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] extensions and psql

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:37 AM, Dimitri Fontaine
 wrote:
> Do we want to get that back in, and in which psql command?  It could
> well be that having \dx list extension and \dx name list extension's
> objects wasn't the best design around, and it could be that it's not
> useful enough, but I know I liked to have a psql shortcut to do that.

+1 for having a psql command to do that.

-- 
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] XMin Hot Standby Feedback patch

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:42 AM, Simon Riggs  wrote:
> Patch attached, no docs yet, but the patch is clear.
>
> I'm looking to commit this in next 24 hours barring objections and/or
> test failures.

Looks pretty good to me, though I haven't tested it.  I like some of
the safety valves you put in there, but I don't understand this part:

+   /*
+* Feedback from standby should move us
forwards, but not too far.
+* Avoid grabbing XidGenLock here in case of
waits, so use
+* a heuristic instead.
+*/

What's XIDGenLock got to do with it?

On another disk, I think that those warning messages are a bad idea.
That could fill up someone's disk really quickly.

-- 
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] CommitFest 2011-01 as of 2011-02-04

2011-02-15 Thread Andrew Dunstan



On 02/15/2011 06:55 AM, Robert Haas wrote:

On Tue, Feb 15, 2011 at 3:31 AM, Itagaki Takahiro
  wrote:

On Tue, Feb 15, 2011 at 01:27, Robert Haas  wrote:

However, file_fdw is in pretty serious trouble because (1) the copy
API patch that it depends on still isn't committed and (2) it's going
to be utterly broken if we don't do something about the
client_encoding vs. file_encoding problem; there was a patch to do
that in this CF, but we gave up on it.

Will we include the copy API patch in 9.1 even if we won't have file_fdw?
Personally, I want to include the APIs because someone can develop file_fdw
as a third party extension for 9.1 using the infrastructure. The extension
will lack of file encoding support, but still useful for many cases.

I've been kind of wondering why you haven't already committed it.  If
you're confident that the code is in good shape, I don't particularly
see any benefit to holding off.



+10. The sooner the better.

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] Sync Rep for 2011CF1

2011-02-15 Thread Robert Haas
On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao  wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>  wrote:
>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>> sends a status update every time the WAL is flushed. If the walreceiver is
>> busy receiving and flushing, that would happen once per WAL segment, which
>> seems sensible.
>
> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
> via XLogWalRcvFlush(). This looks unsafe.

Good catch.  Is the cleanest solution to pass a boolean parameter to
XLogWalRcvFlush() indicating whether we're in the midst of dying?

-- 
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] Sync Rep for 2011CF1

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 1:11 AM, Fujii Masao  wrote:
> On Mon, Feb 14, 2011 at 2:08 PM, Fujii Masao  wrote:
>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>>  wrote:
>>> I committed the patch with those changes, and some minor comment tweaks and
>>> other kibitzing.
>
> I have another comment:
>
> The description of wal_receiver_status_interval is in "18.5.4.
> Streaming Replication".
> But I think that it should be moved to "18.5.5. Standby Servers" since
> it's a parameter
> to control the behavior of the standby server rather than that of the master.

Fixed.

-- 
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] Sync Rep for 2011CF1

2011-02-15 Thread Robert Haas
On Mon, Feb 14, 2011 at 12:08 AM, Fujii Masao  wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>  wrote:
>> I committed the patch with those changes, and some minor comment tweaks and
>> other kibitzing.
>
> +            * 'd' means a standby reply wrapped in a COPY BOTH packet.
> +            */
>
> Typo: s/COPY BOTH/CopyData

Fixed.

> +   msgtype = pq_getmsgbyte(&input_message);
> +   if (msgtype != 'r')
> +       ereport(COMMERROR,
> +               (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                errmsg("unexpected message type %c", msgtype)));
>
> I think that proc_exit(0) needs to be called in error case.

Fixed.

> +   static StringInfoData input_message;
> +   StandbyReplyMessage reply;
> +   char msgtype;
> +
> +   initStringInfo(&input_message);
>
> Doesn't the repeat of initStringInfo() cause the memory leak?

Yeah.  Fixed, I hope.

> @@ -518,6 +584,7 @@ WalSndLoop(void)
>        {
>            if (!XLogSend(output_message, &caughtup))
>                break;
> +           ProcessRepliesIfAny();
>
> Why is ProcessRepliesIfAny() required there?

I'm not sure if that's 100% necessary, but it seems harmless enough.

> We added new columns "write_location", "flush_location" and
> "apply_location". So, for the sake of consistency, the column
> name "sent_location" should be changed to "send_location"?

I was thinking about stream_location or streaming_location, per
discussion on the other thread.

-- 
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] Fix for Index Advisor related hooks

2011-02-15 Thread Tom Lane
Gurjeet Singh  writes:
> Also attached is the patch expose_IndexSupportInitialize.patch, that makes
> the static function IndexSupportInitialize() global so that the Index
> Advisor doesn't have to reinvent the wheel to prepare an index structure
> with opfamilies and opclasses.

We are *not* doing that.  That's a private function and will remain so.

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] XMin Hot Standby Feedback patch

2011-02-15 Thread Simon Riggs
On Tue, 2011-02-15 at 18:49 +0200, Heikki Linnakangas wrote:

> It would be wise to also transmit the epoch in addition to xmin, to 
> avoid confusion if the standby is > 2 billion transactions behind.

Yes, good idea, thanks.

That has to be the record for the fastest patch review. ;-)

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 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] Add support for logging the current role

2011-02-15 Thread Andrew Dunstan



On 02/15/2011 11:13 AM, Stephen Frost wrote:

* Robert Haas (robertmh...@gmail.com) wrote:

Well, I guess the other option is to just add it to the format, full
stop.  But as someone pointed out previously, that's not a terribly
scalable solution, but perhaps it could be judged adequate for this
particular case.

Think I suggested that at one point.  I'm all for doing that on a major
version change like this one, but I think we already had some concerns
about that on this thread (Andrew maybe?).



I could live with it for a release if I thought we had a clear path 
ahead, but I think there are some design issues that we need to think 
about before we start providing for header lines and variable formats in 
CSV logs, particularly w.r.t. log rotation etc. So I'm slightly nervous 
about going ahead with this right now.



While I generally agree with the principal, I also wonder if it might
be better to just add this field in log_line_prefix and wait for
someone to complain about that as other than a theoretical matter.

I might be working against myself, but I'll complain right now about the
lack of any way to have a header on the CSV logs and that you don't get
to control what fields are logged.  That said, I'm not currently using
them either, so my vote doesn't count for much.  Of course, I'll also
complain about the lack of any way to get PG to respect the header,
forcing me to do fun things like:

for file in *results*; do
HEADER=`head -1 $file`
sed -e 's:""::g'<  $file | \
psql -d beac -h sauron -c \
"\copy my_table ($HEADER) from STDIN with csv header"
done

on a regular basis.  How forcing me to do that rather than asking
someone else to use 'tail -n +2' makes sense is beyond me..



You don't really make your case any better by continuing this argument 
from years ago. I can tell you from experience that the CSV HEADER 
feature is distinctly useful as it is. If you want to add a mode that 
uses the header line as a column list on import, then make that case, 
and I'll support it in fact, but it's not an alternative to having the 
header ignored, which is a feature I would vigorously resist removing. 
(Incidentally, I think it won't be trivial - the COPY code expects to 
know the columns by the time it opens the file).


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] XMin Hot Standby Feedback patch

2011-02-15 Thread Heikki Linnakangas

On 15.02.2011 18:52, Robert Haas wrote:

On Tue, Feb 15, 2011 at 11:49 AM, Heikki Linnakangas
  wrote:

It would be wise to also transmit the epoch in addition to xmin, to avoid
confusion if the standby is>  2 billion transactions behind.


That case is probably hopelessly broken anyway.


I don't expect the feedback to do anything too useful in case of xid 
wraparound, but at least the master would recognize the situation and 
know to ignore the bogus xmin from that standby.


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

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


Re: [HACKERS] XMin Hot Standby Feedback patch

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:49 AM, Heikki Linnakangas
 wrote:
> On 15.02.2011 18:42, Simon Riggs wrote:
>>
>> On Sat, 2011-02-12 at 14:11 -0800, Daniel Farina wrote:
>>>
>>> This is another bit of the syncrep patch split out.
>>>
>>> I will revisit the replication timeout one Real Soon, I promise -- but
>>> I have a couple things to do today that may delay that until the
>>> evening.
>>>
>>>
>>> https://github.com/fdr/postgres/commit/ad3ce9ac62f0e128d7d1fd20d47184f867056af1
>>>
>>> Context diff supplied here.
>>
>> Greg just tipped me off to this thread a few hours ago. I saw your other
>> work on timeouts which looks good.
>>
>> I've reworked this feature myself, and its roughly the same thing you
>> have posted, so I will just add on to this thread. The major change from
>> my earlier patch is that the logic around setting xmin on the master is
>> considerably tighter, and correctly uses locking.
>
> It would be wise to also transmit the epoch in addition to xmin, to avoid
> confusion if the standby is > 2 billion transactions behind.

That case is probably hopelessly broken anyway.

-- 
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] sepgsql contrib module

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:41 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane  wrote:
>>> Robert Haas  writes:
 Those are good points.  My point was just that you can't actually
 build that file at the time you RUN the regression tests, because you
 have to build it first, then install it, then run the regression
 tests.  It could be a separate target, like 'make policy', but I don't
 think it works to make it part of 'make installcheck'.
>
>>> So?  Once you admit that you can do that, it's a matter of a couple more
>>> lines to make the installcheck target depend on the policy target iff
>>> selinux was enabled.
>
>> Sure, you could do that, but I don't see what problem it would fix.
>> You'd still have to build and manually install the policy before you
>> could run make installcheck.  And once you've done that, you don't
>> need to rebuild it every future time you run make installcheck.
>
> Oh, I see: you're pointing out the root-only "semodule" step that has to
> be done in between there.  Good point.  But the current arrangement is
> still a mistake: the required contents of sepgsql-regtest.pp depend on
> the configuration of the test system, which can't be known at build
> time.
>
> So what we should do is offer a "make policy" target and alter the test
> instructions to say you should do that and then run semodule.  Or maybe
> just put the whole "make -f /usr/share/selinux/devel/Makefile" dance
> into the instructions --- it doesn't look to me like our makefile
> infrastructure really has anything useful to add to that.

Yeah, agreed.

-- 
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] XMin Hot Standby Feedback patch

2011-02-15 Thread Heikki Linnakangas

On 15.02.2011 18:42, Simon Riggs wrote:

On Sat, 2011-02-12 at 14:11 -0800, Daniel Farina wrote:

This is another bit of the syncrep patch split out.

I will revisit the replication timeout one Real Soon, I promise -- but
I have a couple things to do today that may delay that until the
evening.

https://github.com/fdr/postgres/commit/ad3ce9ac62f0e128d7d1fd20d47184f867056af1

Context diff supplied here.


Greg just tipped me off to this thread a few hours ago. I saw your other
work on timeouts which looks good.

I've reworked this feature myself, and its roughly the same thing you
have posted, so I will just add on to this thread. The major change from
my earlier patch is that the logic around setting xmin on the master is
considerably tighter, and correctly uses locking.


It would be wise to also transmit the epoch in addition to xmin, to 
avoid confusion if the standby is > 2 billion transactions behind.


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

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


Re: [HACKERS] XMin Hot Standby Feedback patch

2011-02-15 Thread Simon Riggs
On Sat, 2011-02-12 at 14:11 -0800, Daniel Farina wrote:
> This is another bit of the syncrep patch split out.
> 
> I will revisit the replication timeout one Real Soon, I promise -- but
> I have a couple things to do today that may delay that until the
> evening.
> 
> https://github.com/fdr/postgres/commit/ad3ce9ac62f0e128d7d1fd20d47184f867056af1
> 
> Context diff supplied here.

Greg just tipped me off to this thread a few hours ago. I saw your other
work on timeouts which looks good.

I've reworked this feature myself, and its roughly the same thing you
have posted, so I will just add on to this thread. The major change from
my earlier patch is that the logic around setting xmin on the master is
considerably tighter, and correctly uses locking.

Patch attached, no docs yet, but the patch is clear.

I'm looking to commit this in next 24 hours barring objections and/or
test failures.

> Note that this information is not exposed via catalog in the original
> syncrep patch, and is not here. Do we want that kind of reporting?

Probably, but its a small change and will conflict with other work for
now.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services
 
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 3277da8..c4ebf97 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -45,6 +45,7 @@
 #include "replication/walreceiver.h"
 #include "storage/ipc.h"
 #include "storage/pmsignal.h"
+#include "storage/procarray.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
@@ -56,6 +57,7 @@ bool		am_walreceiver;
 
 /* GUC variable */
 int			wal_receiver_status_interval;
+bool		hot_standby_feedback;
 
 /* libpqreceiver hooks to these when loaded */
 walrcv_connect_type walrcv_connect = NULL;
@@ -610,10 +612,14 @@ XLogWalRcvSendReply(void)
 	reply_message.apply = GetXLogReplayRecPtr();
 	reply_message.sendTime = now;
 
-	elog(DEBUG2, "sending write %X/%X flush %X/%X apply %X/%X",
+	if (hot_standby_feedback)
+		reply_message.xmin = GetOldestXmin(true, false);
+
+	elog(DEBUG2, "sending write %X/%X flush %X/%X apply %X/%X xmin %u",
  reply_message.write.xlogid, reply_message.write.xrecoff,
  reply_message.flush.xlogid, reply_message.flush.xrecoff,
- reply_message.apply.xlogid, reply_message.apply.xrecoff);
+ reply_message.apply.xlogid, reply_message.apply.xrecoff,
+ reply_message.xmin);
 
 	/* Prepend with the message type and send it. */
 	buf[0] = 'r';
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 3ad95b4..36eb3a9 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -53,6 +53,7 @@
 #include "storage/ipc.h"
 #include "storage/pmsignal.h"
 #include "storage/proc.h"
+#include "storage/procarray.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
@@ -498,6 +499,7 @@ ProcessStandbyReplyMessage(void)
 	static StringInfoData input_message;
 	StandbyReplyMessage	reply;
 	char msgtype;
+	TransactionId newxmin = InvalidTransactionId;
 
 	initStringInfo(&input_message);
 
@@ -524,10 +526,11 @@ ProcessStandbyReplyMessage(void)
 
 	pq_copymsgbytes(&input_message, (char *) &reply, sizeof(StandbyReplyMessage));
 
-	elog(DEBUG2, "write %X/%X flush %X/%X apply %X/%X ",
+	elog(DEBUG2, "write %X/%X flush %X/%X apply %X/%X xmin %u",
 		 reply.write.xlogid, reply.write.xrecoff,
 		 reply.flush.xlogid, reply.flush.xrecoff,
-		 reply.apply.xlogid, reply.apply.xrecoff);
+		 reply.apply.xlogid, reply.apply.xrecoff,
+		 reply.xmin);
 
 	/*
 	 * Update shared state for this WalSender process
@@ -543,6 +546,74 @@ ProcessStandbyReplyMessage(void)
 		walsnd->apply = reply.apply;
 		SpinLockRelease(&walsnd->mutex);
 	}
+
+	/*
+	 * Update the WalSender's proc xmin to allow it to be visible
+	 * to snapshots. This will hold back the removal of dead rows
+	 * and thereby prevent the generation of cleanup conflicts
+	 * on the standby server.
+	 */
+	if (TransactionIdIsValid(reply.xmin))
+	{
+		TransactionId safeLimit;
+#define HOT_STANDBY_FEEDBACK_HORIZON		 10
+
+		if (!TransactionIdIsValid(MyProc->xmin))
+		{
+			/*
+			 * Initialise MyProc->xmin from standby feedback.
+			 * Don't allow use oldestXmin if it is too far back.
+			 */
+			safeLimit = ReadNewTransactionId() - HOT_STANDBY_FEEDBACK_HORIZON;
+			if (!TransactionIdIsNormal(safeLimit))
+safeLimit = FirstNormalTransactionId;
+
+			if (TransactionIdPrecedes(safeLimit, reply.xmin))
+			{
+TransactionId oldestXmin = GetOldestXmin(true, true);
+
+if (TransactionIdPrecedes(oldestXmin, reply.xmin))
+	newxmin = reply.xmin;
+else
+	newxmin = oldestXmin;
+			}
+			else
+ereport(WARNING,
+		(errmsg("standby xmin is far in the past"),
+		 errhint("standby must catch up before hot standby feedback is effective.")));
+		}
+		else
+		{
+			/

Re: [HACKERS] sepgsql contrib module

2011-02-15 Thread Tom Lane
Robert Haas  writes:
> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> Those are good points.  My point was just that you can't actually
>>> build that file at the time you RUN the regression tests, because you
>>> have to build it first, then install it, then run the regression
>>> tests.  It could be a separate target, like 'make policy', but I don't
>>> think it works to make it part of 'make installcheck'.

>> So?  Once you admit that you can do that, it's a matter of a couple more
>> lines to make the installcheck target depend on the policy target iff
>> selinux was enabled.

> Sure, you could do that, but I don't see what problem it would fix.
> You'd still have to build and manually install the policy before you
> could run make installcheck.  And once you've done that, you don't
> need to rebuild it every future time you run make installcheck.

Oh, I see: you're pointing out the root-only "semodule" step that has to
be done in between there.  Good point.  But the current arrangement is
still a mistake: the required contents of sepgsql-regtest.pp depend on
the configuration of the test system, which can't be known at build
time.

So what we should do is offer a "make policy" target and alter the test
instructions to say you should do that and then run semodule.  Or maybe
just put the whole "make -f /usr/share/selinux/devel/Makefile" dance
into the instructions --- it doesn't look to me like our makefile
infrastructure really has anything useful to add to that.

regards, tom lane

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


[HACKERS] extensions and psql

2011-02-15 Thread Dimitri Fontaine
Hi,

I realize that you didn't keep the \dx behavior I had, that when given
an extension name it would list all the objects contained in the
extension.  Now that's a pretty simple query:

  select pg_describe_object(classid, objid, 0)
from pg_depend d 
join pg_extension e on d.refclassid = e.tableoid 
   and d.refobjid = e.oid
   and d.deptype = 'e'
   where e.extname = 'hstore' 
order by objid;

Do we want to get that back in, and in which psql command?  It could
well be that having \dx list extension and \dx name list extension's
objects wasn't the best design around, and it could be that it's not
useful enough, but I know I liked to have a psql shortcut to do that.

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

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


Re: [HACKERS] Sync Rep for 2011CF1

2011-02-15 Thread Simon Riggs
On Tue, 2011-02-15 at 01:45 -0500, Jaime Casanova wrote:
> On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs  wrote:
> >
> > Here's the latest patch for sync rep.
> >
> 
> I was looking at this code and found something in SyncRepWaitOnQueue
> we declare a timeout variable that is a long and another that is a
> boolean (this last one in the else part of the "if
> (!IsOnSyncRepQueue())"), and then use the boolean one as if it were
> the long one

OK, thanks.

> +   else
> +   {
> +   bool release = false;
> +   bool timeout = false;
> +
> +   SpinLockAcquire(&queue->qlock);
> +
> +   /*
> +* Check the LSN on our queue and if its moved far enough then
> +* remove us from the queue. First time through this is
> +* unlikely to be far enough, yet is possible. Next time we are
> +* woken we should be more lucky.
> +*/
> +   if (XLByteLE(XactCommitLSN, queue->lsn))
> +   release = true;
> +   else if (timeout > 0 &&
> +   
> TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
> +   now,
> +   timeout))
> +   {
> +   release = true;
> +   timeout = true;
> +   }
> 
> the other two things are on postgresql.conf.sample:
> - we have two replication_timeout_client, obviously one of them should
> be replication_timeout_server
> - synchronous_replication_feedback is off by default, but docs says otherwise
> 
> i also have been testing this, until now the only issue i have found
> is that if i set allow_standalone_primary to off and there isn't a
> standby connected i need to stop the server with -m immediate which is
> at least surprising

I think that code is being ripped out, so will check again later.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/books/
 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] Add support for logging the current role

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:13 AM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> Well, I guess the other option is to just add it to the format, full
>> stop.  But as someone pointed out previously, that's not a terribly
>> scalable solution, but perhaps it could be judged adequate for this
>> particular case.
>
> Think I suggested that at one point.  I'm all for doing that on a major
> version change like this one, but I think we already had some concerns
> about that on this thread (Andrew maybe?).
>
>> While I generally agree with the principal, I also wonder if it might
>> be better to just add this field in log_line_prefix and wait for
>> someone to complain about that as other than a theoretical matter.
>
> I might be working against myself, but I'll complain right now about the
> lack of any way to have a header on the CSV logs and that you don't get
> to control what fields are logged.  That said, I'm not currently using
> them either, so my vote doesn't count for much.  Of course, I'll also
> complain about the lack of any way to get PG to respect the header,
> forcing me to do fun things like:
>
> for file in *results*; do
>        HEADER=`head -1 $file`
>        sed -e 's:""::g' < $file | \
>            psql -d beac -h sauron -c \
>            "\copy my_table ($HEADER) from STDIN with csv header"
> done
>
> on a regular basis.  How forcing me to do that rather than asking
> someone else to use 'tail -n +2' makes sense is beyond me..

It's not an either/or proposition.  We could certainly support header
on/off/ignore, with the new extensible COPY syntax.

-- 
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] pageinspect's infomask and infomask2 as smallint

2011-02-15 Thread Heikki Linnakangas

On 15.02.2011 18:03, Tom Lane wrote:

Robert Haas  writes:

On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane  wrote:

What risk?  And at least we'd be trying to do it cleanly, in a manner
that should work for at least 99% of users.  AFAICT, Heikki's proposal
is "break it for everyone, and damn the torpedoes".



I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
because incompatibilities are an expected part of major release
upgrades, but don't break it in 9.0 and prior, because it's not
particularly important and we don't want to change behavior or risk
breaking things in minor releases".


Right, that's what I meant.


No, nobody was proposing changing it before 9.1 (or at least I didn't
think anybody was).  What's under discussion is how much effort to put
into making a 9.0-to-9.1 upgrade go smoothly for people who have the
function installed.


Oh, never mind then.

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

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


Re: [HACKERS] Add support for logging the current role

2011-02-15 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
> Well, I guess the other option is to just add it to the format, full
> stop.  But as someone pointed out previously, that's not a terribly
> scalable solution, but perhaps it could be judged adequate for this
> particular case.

Think I suggested that at one point.  I'm all for doing that on a major
version change like this one, but I think we already had some concerns
about that on this thread (Andrew maybe?).

> While I generally agree with the principal, I also wonder if it might
> be better to just add this field in log_line_prefix and wait for
> someone to complain about that as other than a theoretical matter.

I might be working against myself, but I'll complain right now about the
lack of any way to have a header on the CSV logs and that you don't get
to control what fields are logged.  That said, I'm not currently using
them either, so my vote doesn't count for much.  Of course, I'll also
complain about the lack of any way to get PG to respect the header,
forcing me to do fun things like:

for file in *results*; do
HEADER=`head -1 $file`
sed -e 's:""::g' < $file | \
psql -d beac -h sauron -c \
"\copy my_table ($HEADER) from STDIN with csv header"
done

on a regular basis.  How forcing me to do that rather than asking
someone else to use 'tail -n +2' makes sense is beyond me..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] sepgsql contrib module

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> Those are good points.  My point was just that you can't actually
>> build that file at the time you RUN the regression tests, because you
>> have to build it first, then install it, then run the regression
>> tests.  It could be a separate target, like 'make policy', but I don't
>> think it works to make it part of 'make installcheck'.
>
> So?  Once you admit that you can do that, it's a matter of a couple more
> lines to make the installcheck target depend on the policy target iff
> selinux was enabled.

Sure, you could do that, but I don't see what problem it would fix.
You'd still have to build and manually install the policy before you
could run make installcheck.  And once you've done that, you don't
need to rebuild it every future time you run make installcheck.

-- 
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] pageinspect's infomask and infomask2 as smallint

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 11:03 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane  wrote:
>>> What risk?  And at least we'd be trying to do it cleanly, in a manner
>>> that should work for at least 99% of users.  AFAICT, Heikki's proposal
>>> is "break it for everyone, and damn the torpedoes".
>
>> I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
>> because incompatibilities are an expected part of major release
>> upgrades, but don't break it in 9.0 and prior, because it's not
>> particularly important and we don't want to change behavior or risk
>> breaking things in minor releases".
>
> No, nobody was proposing changing it before 9.1 (or at least I didn't
> think anybody was).  What's under discussion is how much effort to put
> into making a 9.0-to-9.1 upgrade go smoothly for people who have the
> function installed.

Oh, I see, never mind me then...  feel free to make that go smoothly.  :-)

-- 
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 support for logging the current role

2011-02-15 Thread Stephen Frost
Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Given that this has been like this right along, I don't see why it's
> all that urgent to force a half-baked solution into 9.1.  I'm also
> concerned that if we do do that, you'll lose motivation to work on
> cleaning it up for 9.2 ;-)

The addition to log_line_prefix is hardly 'half-baked' as a solution, to
that problem (I just pulled the hunks from the rest of the patch as they
were completely independent, and tested them).  I've also gone and added
the csvlog_fields/csvlog_header patch to the 2011-Next commitfest. :P

I've also already started looking at changing syslogger to have it
figure out if it should be writing a header out or not.  If we can
decide what semantics we should have when the log file exists and we're
not planning to rotate it on startup, it won't be hard for me to
implement them (well, I hope).

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Add support for logging the current role

2011-02-15 Thread Robert Haas
On Tue, Feb 15, 2011 at 10:57 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> Something along these lines would be OK with me (I haven't yet
>> validated every detail), but there were previous objections to adding
>> any new fields to log_line_prefix until we had a flexible CSV format.
>> I think that's raising the bar a bit too high, personally, but I don't
>> have the only vote around here...
>
> I think I was the one objecting.  I don't necessarily say that we have
> to have a "flexible" CSV format, but I do say that facilities that are
> available in log_line_prefix and not in CSV logs are a bad thing.

Well, I guess the other option is to just add it to the format, full
stop.  But as someone pointed out previously, that's not a terribly
scalable solution, but perhaps it could be judged adequate for this
particular case.

While I generally agree with the principal, I also wonder if it might
be better to just add this field in log_line_prefix and wait for
someone to complain about that as other than a theoretical matter.

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


  1   2   >