Re: [HACKERS] FOREIGN TABLE doc fix

2011-06-11 Thread Robert Haas
2011/6/9 Shigeru Hanada :
> Attached patch includes fixes for FOREIGN TABLE documents:

I committed the changes to ALTER FOREIGN TABLE, but I think the
changes to CREATE FOREIGN TABLE need more thought.  The first of the
two hunks you've proposed to add doesn't seem necessary to me, and the
second one seems like it belongs in a chapter on how to write a
foreign data wrapper correctly, rather than here.

-- 
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] On-the-fly index tuple deletion vs. hot_standby

2011-06-11 Thread Robert Haas
On Sat, Jun 11, 2011 at 11:40 PM, Noah Misch  wrote:
> We currently achieve that wait-free by first marking the page with the next
> available xid and then reusing it when that mark (btpo.xact) predates the
> oldest running xid (RecentXmin).  (At the moment, I'm failing to work out why
> this is OK with scans from transactions that haven't allocated an xid, but I
> vaguely recall convincing myself it was fine at one point.)  It would indeed
> also be enough to call GetLockConflicts(locktag-of-index, AccessExclusiveLock)
> and check whether any of the returned transactions have PGPROC.xmin below the
> mark.  That's notably more expensive than just comparing RecentXmin, so I'm
> not sure how well it would pay off overall.  However, it could only help us on
> the master.  (Not strictly true, but any way I see to extend it to the standby
> has critical flaws.)  On the master, we can see a conflicting transaction and
> put off reusing the page.  By the time the record hits the standby, we have to
> apply it, and we might have a running transaction that will hold a lock on the
> index for the next, say, 72 hours.  At such times, vacuum_defer_cleanup_age or
> hot_standby_feedback ought to prevent the recovery stall.
>
> This did lead me to realize that what we do in this regard on the standby can
> be considerably independent from what we do on the master.  If fruitful, the
> standby can prove the absence of a scan holding a right-link in a completely
> different fashion.  So, we *could* take the cleanup-lock approach on the
> standby without changing very much on the master.

Well, I'm generally in favor of trying to fix this problem without
changing what the master does.  It's a weakness of our replication
technology that the standby has no better way to cope with a cleanup
operation on the master than to start killing queries, but then again
it's a weakness of our MVCC technology that we don't reuse space
quickly enough and end up with bloat.  I hear a lot more complaints
about the second weakness than I do about the first.

At any rate, if taking a cleanup lock on the right-linked page on the
standby is sufficient to fix the problem, that seems like a far
superior solution in any case.  Presumably the frequency of someone
having a pin on that particular page will be far lower than any
matching based on XID or heavyweight locks.  And the vast majority of
such pins should disappear before the startup process feels obliged to
get out its big hammer.

-- 
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] psql: missing tab completions for COMMENT ON

2011-06-11 Thread Robert Haas
On Sat, May 28, 2011 at 10:38 PM, Josh Kupershmidt  wrote:
> Hi all,
>
> psql's auto-complete support for COMMENT ON was missing support for a
> few object types:
>
> 1.) EXTENSION and PROCEDURAL LANGUAGE are now auto-complete candidates
> for COMMENT ON [TAB]. Lists of extensions and procedural languages
> should also be filled in when a user types
>  COMMENT ON EXTENSION [TAB]
>  COMMENT ON PROCEDURAL LANGUAGE [TAB]

I don't see much point in auto-completing COMMENT ON PROCEDURAL
LANGUAGE.  We already complete COMMENT ON LANGUAGE.  I agree with
adding EXTENSION (so done).

> 2.) This part of tab-complete.c looked like a spurious leftover:
>
> *** psql_completion(char *text, int start, i
> *** 1580,1592 
>
>                COMPLETE_WITH_LIST(list_TRANS2);
>        }
>        else if ((pg_strcasecmp(prev4_wd, "COMMENT") == 0 &&
>                          pg_strcasecmp(prev3_wd, "ON") == 0) ||
>                         (pg_strcasecmp(prev6_wd, "COMMENT") == 0 &&
> !                         pg_strcasecmp(prev5_wd, "ON") == 0) ||
> !                        (pg_strcasecmp(prev5_wd, "ON") == 0 &&
> !                         pg_strcasecmp(prev4_wd, "TEXT") == 0 &&
> !                         pg_strcasecmp(prev3_wd, "SEARCH") == 0))
>                COMPLETE_WITH_CONST("IS");
>
> Since we want these choices to be filled in for COMMENT ON TEXT SEARCH [TAB]:
>        {"CONFIGURATION", "DICTIONARY", "PARSER", "TEMPLATE", NULL};
>
> which were already being handled correctly in an above block.

Hmm, yeah.  But while I'm looking at it, I notice that this will
handle object-type names that are one word or three words, but not two
words.  And we now have FOREIGN TABLE.  Committed your change plus a
bit of additional hackery to address that issue.

> One piece that I gave up on trying to fix is the auto-completion for
> {OPERATOR, OPERATOR CLASS, OPERATOR FAMILY}, since getting it working
> correctly would be a real hassle. There's the trouble of whether to
> auto-complete operators for OPERATOR [TAB], or whether to fill in
> {CLASS, FAMILY} instead. Plus the auto-completes for 'USING
> index_method'.
>
> While wasting time on OPERATOR [TAB], I realized we're being a bit
> overeager with this bit:
>
>    else if ((pg_strcasecmp(prev4_wd, "COMMENT") == 0 &&
>              pg_strcasecmp(prev3_wd, "ON") == 0) ||
>             (pg_strcasecmp(prev6_wd, "COMMENT") == 0 &&
>              pg_strcasecmp(prev5_wd, "ON") == 0))
>        COMPLETE_WITH_CONST("IS");
>
> which will auto-complete e.g.
>  COMMENT ON AGGREGATE avg [TAB]
> with 'IS', when instead we'd want the possible argument types to avg,
> or nothing at all. Same deal with a few other object types, but it's
> probably not worth worrying about (at least, I'm not worrying about it
> at the moment).

The whole tab completion machinery is pretty much just throwing darts
while blindfolded, but the effort to replace it with something better
is so large that we just keep poking at it the way it is...

-- 
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] On-the-fly index tuple deletion vs. hot_standby

2011-06-11 Thread Noah Misch
Hi Robert,

On Sat, Jun 11, 2011 at 08:55:28PM -0400, Robert Haas wrote:
> On Fri, Apr 22, 2011 at 11:10 AM, Noah Misch  wrote:
> > On Tue, Mar 15, 2011 at 10:22:59PM -0400, Noah Misch wrote:
> >> On Mon, Mar 14, 2011 at 01:56:22PM +0200, Heikki Linnakangas wrote:
> >> > On 12.03.2011 12:40, Noah Misch wrote:
> >> >> The installation that inspired my original report recently upgraded 
> >> >> from 9.0.1
> >> >> to 9.0.3, and your fix did significantly decrease its conflict 
> >> >> frequency. ?The
> >> >> last several conflicts I have captured involve XLOG_BTREE_REUSE_PAGE 
> >> >> records.
> >> >> (FWIW, the index has generally been pg_attribute_relid_attnam_index.) 
> >> >> ?I've
> >> >> attached a test script demonstrating the behavior. ?_bt_page_recyclable 
> >> >> approves
> >> >> any page deleted no more recently than RecentXmin, because we need only 
> >> >> ensure
> >> >> that every ongoing scan has witnessed the page as dead. ?For the hot 
> >> >> standby
> >> >> case, we need to account for possibly-ongoing standby transactions. 
> >> >> ?Using
> >> >> RecentGlobalXmin covers that, albeit with some pessimism: we really 
> >> >> only need
> >> >> LEAST(RecentXmin, PGPROC->xmin of walsender_1, .., PGPROC->xmin of 
> >> >> walsender_N)
> >> >> - vacuum_defer_cleanup_age. ?Not sure the accounting to achieve that 
> >> >> would pay
> >> >> off, though. ?Thoughts?
> >> >
> >> > Hmm, instead of bloating the master, I wonder if we could detect more
> >> > accurately if there are any on-going scans, in the standby. For example,
> >> > you must hold a lock on the index to scan it, so only transactions
> >> > holding the lock need to be checked for conflict.
> >>
> >> That would be nice. ?Do you have an outline of an implementation in mind?
> >
> > In an attempt to resuscitate this thread, here's my own shot at that. 
> > ?Apologies
> > in advance if it's just an already-burning straw man.
> >
> > I didn't see any way to take advantage of checking for the heavyweight lock 
> > that
> > any index scan would need to hold.
> 
> Have you looked at the logic in ResolveRecoveryConflictWithLock(), and
> at GetLockConflicts()?
> 
> I am a little fuzzy on how the btree stuff works, but it seems to me
> that you are looking for transactions that both have an xmin before
> some threshold and also hold an AccessShareLock on some relation.
> GetLockConflicts() will provide the latter, at least.

Thanks for taking a look.

For the purpose of B-tree page reuse, we don't directly care about the xmin of
any active snapshot.  We need only prove that no active scan is paused
adjacent to the page, holding a right-link to it.

We currently achieve that wait-free by first marking the page with the next
available xid and then reusing it when that mark (btpo.xact) predates the
oldest running xid (RecentXmin).  (At the moment, I'm failing to work out why
this is OK with scans from transactions that haven't allocated an xid, but I
vaguely recall convincing myself it was fine at one point.)  It would indeed
also be enough to call GetLockConflicts(locktag-of-index, AccessExclusiveLock)
and check whether any of the returned transactions have PGPROC.xmin below the
mark.  That's notably more expensive than just comparing RecentXmin, so I'm
not sure how well it would pay off overall.  However, it could only help us on
the master.  (Not strictly true, but any way I see to extend it to the standby
has critical flaws.)  On the master, we can see a conflicting transaction and
put off reusing the page.  By the time the record hits the standby, we have to
apply it, and we might have a running transaction that will hold a lock on the
index for the next, say, 72 hours.  At such times, vacuum_defer_cleanup_age or
hot_standby_feedback ought to prevent the recovery stall.

This did lead me to realize that what we do in this regard on the standby can
be considerably independent from what we do on the master.  If fruitful, the
standby can prove the absence of a scan holding a right-link in a completely
different fashion.  So, we *could* take the cleanup-lock approach on the
standby without changing very much on the master.

nm

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


Re: [HACKERS] Range Types and extensions

2011-06-11 Thread Robert Haas
On Thu, Jun 9, 2011 at 6:26 PM, Florian Pflug  wrote:
> On Jun8, 2011, at 17:46 , Jeff Davis wrote:
>> It looks like the type input function may be a problem, because it
>> doesn't look like it knows what the collation is yet. In other words,
>> PG_GET_COLLATION() is zero for the type input function.
>>
>> But I need to do a comparison to find out if the range is valid or not.
>> For instance:
>>  '[a, Z)'::textrange
>> is valid in "en_US" but not "C".
>
> Maybe that check should just be removed? If one views the range
> '[L, U)' as a concise way of expressing "L <= x AND x < U" for some
> x, then allowing the case L > U seems quite natural. There won't
> be any such x of course, but the range is still valid, just empty.
>
> Actually, thinking for this a bit, I believe this is the only
> way text ranges can support collations. If the validity of a range
> depends on the collation, then changing the collation after creation
> seems weird, since it can make previous valid ranges invalid and
> vice versa.
>
> There could be a function RANGE_EMPTY() which people can put into
> their CHECK constraints if they don't want such ranges to sneak
> into their tables...

I think the collation is going to have to be baked into the type
definition, no?  You can't just up and change the collation of the
column as you could for a straight text column, if that might cause
the contents of some rows to be viewed as invalid.

-- 
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] procpid?

2011-06-11 Thread Robert Haas
On Sat, Jun 11, 2011 at 9:56 PM, Cédric Villemain
 wrote:
> 2011/6/12 Robert Haas :
>> On Sat, Jun 11, 2011 at 9:15 PM, Joshua D. Drake  
>> wrote:
>>> On 6/11/2011 1:23 PM, Bruce Momjian wrote:

> There is a difference between a project name and something that directly
> affects usability. +1 on fixing this. IMO, we don't create a new pid
> column, we just fix the problem. If we do it for 9.2, we have 18 months
> to communicate the change.

 Uh, I am the first one I remember complaining about this so I don't see
 why we should break compatibility for such a low-level problem.
>>>
>>> Because it is a very real problem with an easy fix. We have 18 months to
>>> publicize that fix. I mean really? This is a no-brainer.
>>
>> I really don't see what the big deal with calling it the process PID
>> rather than just the PID is.  Changing something like this forces
>> pgAdmin and every other application out there that is built to work
>> with PG to make a code change to keep working with PG.  That seems
>> like pushing a lot of unnecessary work on other people for what is
>> basically a minor cosmetic issue.
>
> I agree.
> This is at least a use-case for something^Wfeature like 'create
> synonym', allowing smooth end-user's application upgrade on schema
> update. I am not claiming that we need that, it just seems a good
> usecase for column alias/synonym.

I had the same thought.  I'm not sure that this particular example
would be worthwhile even if we had a column synonym facility.  But at
least if we were bent on changing it we could do it without breaking
things.

-- 
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] hot standby startup, visibility map, clog

2011-06-11 Thread Robert Haas
On Thu, Jun 9, 2011 at 5:14 AM, Daniel Farina  wrote:
> The first fact is that turning off hot standby will let the cluster
> start up, but only after seeing a spate of messages like these (dozen
> or dozens, not thousands):
>
> 2011-06-09 08:02:32 UTC  LOG:  restored log file
> "0002002C00C0" from archive
> 2011-06-09 08:02:33 UTC  WARNING:  xlog min recovery request
> 2C/C1F09658 is past current point 2C/C037B278
> 2011-06-09 08:02:33 UTC  CONTEXT:  writing block 0 of relation
> base/16385/16784_vm
>        xlog redo insert: rel 1663/16385/128029; tid 114321/63
> 2011-06-09 08:02:33 UTC  LOG:  restartpoint starting: xlog
>
> Most importantly, *all* such messages are in visibility map forks
> (_vm).  I reasonably confident that my code does not start reading
> data until pg_start_backup() has returned, and blocks on
> pg_stop_backup() after having read all the data.  Also, the mailing
> list correspondence at
> http://archives.postgresql.org/pgsql-hackers/2010-11/msg02034.php
> suggests that the visibility map is not flushed at checkpoints, so
> perhaps with some poor timing an old page can wander onto disk even
> after a checkpoint barrier that pg_start_backup waits for. (I have not
> yet found the critical section that makes visibilitymap buffers immune
> to checkpoint though).

I don't believe there's anything that makes visibilitymap buffers
immune to checkpoint.  I might be wrong.  You could probably find out
by inserting a few lines of code into the buffer manager to inspect
each buffer tag as the buffer is written out and elog() if the fork
number is VISIBILITYMAP_FORKNUM.  There is some weirdness around how
the LSNs of visibility map pages are set, however.  See
visibilitymap_set()/clear().  Clearing a visibility map bit is done
without touching the LSN at all; setting the bit advances the LSN to
that of the heap page, if it currently precedes that value.  It's not
obvious to me whether or how that's related.

I'm a bit puzzled by the warning message itself, too.  I think that in
order to generate that warning, you need to try to flush a page to
disk whose LSN is higher than the current minimum recovery point.  And
the minimum recovery point is initially set to point to the redo
pointer from the last checkpoint.  But during an online backup, it's
expected that there may be pages on disk with LSNs higher than the
redo pointer from the last checkpoint.  So actually now I'm not sure
why people don't see this warning whenever a lengthy recovery is
required to reach consitency, especially with small values of
shared_buffers.  I'm probably missing something here...

One thing that might make it easier to understand what's going on here
is to crank the log level up to DEBUG2 and post the full output from a
recovery that exhibits this problem, rather than just a few lines.

-- 
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] procpid?

2011-06-11 Thread Cédric Villemain
2011/6/12 Robert Haas :
> On Sat, Jun 11, 2011 at 9:15 PM, Joshua D. Drake  
> wrote:
>> On 6/11/2011 1:23 PM, Bruce Momjian wrote:
>>>
 There is a difference between a project name and something that directly
 affects usability. +1 on fixing this. IMO, we don't create a new pid
 column, we just fix the problem. If we do it for 9.2, we have 18 months
 to communicate the change.
>>>
>>> Uh, I am the first one I remember complaining about this so I don't see
>>> why we should break compatibility for such a low-level problem.
>>
>> Because it is a very real problem with an easy fix. We have 18 months to
>> publicize that fix. I mean really? This is a no-brainer.
>
> I really don't see what the big deal with calling it the process PID
> rather than just the PID is.  Changing something like this forces
> pgAdmin and every other application out there that is built to work
> with PG to make a code change to keep working with PG.  That seems
> like pushing a lot of unnecessary work on other people for what is
> basically a minor cosmetic issue.

I agree.
This is at least a use-case for something^Wfeature like 'create
synonym', allowing smooth end-user's application upgrade on schema
update. I am not claiming that we need that, it just seems a good
usecase for column alias/synonym.


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



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


Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache

2011-06-11 Thread Robert Haas
On Thu, Jun 9, 2011 at 3:09 PM, Kohei KaiGai  wrote:
> Here is two level lookups.
> The first is from object identifiers to security label; it can be boosted
> using syscache mechanism. The second is from security labels to
> access control decision; it can be boosted using userspace avc.

OK.  Let's have two separate patches, then.

Thinking a bit more about the issue of adding a syscache, I suspect
it's probably OK to use that mechanism rather than inventing something
more complicated that can kick out entries on an LRU basis.  Even if
you accessed a few tens of thousands of entries, the cache shouldn't
grow to more than a few megabytes.  And that's presumably going to be
rare, and could happen with other types of objects (such as tables)
using the existing system caches.  So I guess it's probably premature
optimization to worry about the issue now.

I would, however, like to see some performance results indicating how
much the cache helps, and how much memory it eats up in the process.

-- 
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] procpid?

2011-06-11 Thread Robert Haas
On Sat, Jun 11, 2011 at 9:15 PM, Joshua D. Drake  wrote:
> On 6/11/2011 1:23 PM, Bruce Momjian wrote:
>>
>>> There is a difference between a project name and something that directly
>>> affects usability. +1 on fixing this. IMO, we don't create a new pid
>>> column, we just fix the problem. If we do it for 9.2, we have 18 months
>>> to communicate the change.
>>
>> Uh, I am the first one I remember complaining about this so I don't see
>> why we should break compatibility for such a low-level problem.
>
> Because it is a very real problem with an easy fix. We have 18 months to
> publicize that fix. I mean really? This is a no-brainer.

I really don't see what the big deal with calling it the process PID
rather than just the PID is.  Changing something like this forces
pgAdmin and every other application out there that is built to work
with PG to make a code change to keep working with PG.  That seems
like pushing a lot of unnecessary work on other people for what is
basically a minor cosmetic issue.

-- 
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] libpq SSL with non-blocking sockets

2011-06-11 Thread Robert Haas
On Thu, Jun 9, 2011 at 10:39 AM, Martin Pihlak  wrote:
> In PQputCopyData #2 it is visible that the first SSL_write called from
> pqFlush failed with SSL_ERROR_WANT_WRITE. The next SSL_write should
> have been a retry with the same parameters, but instead was passed a
> buffer with a different address and length. Hence the "bad write
> retry". Some googling turned out similar issues for other projects
> using SSL with non-blocking sockets.

I'm not surprised.  Setting the API so that it requires the same
buffer (and not just a buffer pointing to the same data, or with the
same initial contents) was maybe not the greatest design decision.

> The possible workarounds are to disable SSL or to disable non-blocking
> libpq connections. Both are not always possible - security reasons, 3rd
> party applications, drivers, etc. So I think this should be fixed in
> libpq. Not sure exactly how though. It would seem that for the
> PQputCopyData the best would be to return 0 to indicate that the
> operation should be retried. No idea for the other possible cases of
> SSL_write() retry though. What do you think?

It seems to me we should try to paper over the problem within libpq
rather than allowing it to percolate any higher up the call stack.
One idea is that we could add outBuffer2/outBufSize2 to struct
pg_conn, or something along those lines with less obviously stupid
naming.  Normally those would be unused, but in the special case where
SSL indicates that we must retry the call with the same arguments, we
set a flag that "freezes" the out buffer and forces any further data
to be stuffed into outBuffer2.  If or when the operation finally
succeeds, we then move the data from outBuffer2 into outBuffer.

-- 
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] procpid?

2011-06-11 Thread Joshua D. Drake

On 6/11/2011 1:23 PM, Bruce Momjian wrote:



There is a difference between a project name and something that directly
affects usability. +1 on fixing this. IMO, we don't create a new pid
column, we just fix the problem. If we do it for 9.2, we have 18 months
to communicate the change.

Uh, I am the first one I remember complaining about this so I don't see
why we should break compatibility for such a low-level problem.



Because it is a very real problem with an easy fix. We have 18 months to 
publicize that fix. I mean really? This is a no-brainer.


JD


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


Re: [HACKERS] wrong message on REASSIGN OWNED

2011-06-11 Thread Robert Haas
On Thu, Jun 9, 2011 at 1:26 PM, Jaime Casanova  wrote:
> on shdepReassignOwned() we have this message, which is obviously wrong
> we are not dropping objects just reassigning them...
> """
>                       ereport(ERROR,
>
> (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
>                                   errmsg("cannot drop objects owned
> by %s because they are "
>                                                  "required by the
> database system",
>                                                  
> getObjectDescription(&obj;
> """
>
> but haven't thought of a good way of rephrase it

"can't reassign objects owned by %s because this user is internal to
the database system" ?

-- 
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] On-the-fly index tuple deletion vs. hot_standby

2011-06-11 Thread Robert Haas
On Fri, Apr 22, 2011 at 11:10 AM, Noah Misch  wrote:
> On Tue, Mar 15, 2011 at 10:22:59PM -0400, Noah Misch wrote:
>> On Mon, Mar 14, 2011 at 01:56:22PM +0200, Heikki Linnakangas wrote:
>> > On 12.03.2011 12:40, Noah Misch wrote:
>> >> The installation that inspired my original report recently upgraded from 
>> >> 9.0.1
>> >> to 9.0.3, and your fix did significantly decrease its conflict frequency. 
>> >>  The
>> >> last several conflicts I have captured involve XLOG_BTREE_REUSE_PAGE 
>> >> records.
>> >> (FWIW, the index has generally been pg_attribute_relid_attnam_index.)  
>> >> I've
>> >> attached a test script demonstrating the behavior.  _bt_page_recyclable 
>> >> approves
>> >> any page deleted no more recently than RecentXmin, because we need only 
>> >> ensure
>> >> that every ongoing scan has witnessed the page as dead.  For the hot 
>> >> standby
>> >> case, we need to account for possibly-ongoing standby transactions.  Using
>> >> RecentGlobalXmin covers that, albeit with some pessimism: we really only 
>> >> need
>> >> LEAST(RecentXmin, PGPROC->xmin of walsender_1, .., PGPROC->xmin of 
>> >> walsender_N)
>> >> - vacuum_defer_cleanup_age.  Not sure the accounting to achieve that 
>> >> would pay
>> >> off, though.  Thoughts?
>> >
>> > Hmm, instead of bloating the master, I wonder if we could detect more
>> > accurately if there are any on-going scans, in the standby. For example,
>> > you must hold a lock on the index to scan it, so only transactions
>> > holding the lock need to be checked for conflict.
>>
>> That would be nice.  Do you have an outline of an implementation in mind?
>
> In an attempt to resuscitate this thread, here's my own shot at that.  
> Apologies
> in advance if it's just an already-burning straw man.
>
> I didn't see any way to take advantage of checking for the heavyweight lock 
> that
> any index scan would need to hold.

Have you looked at the logic in ResolveRecoveryConflictWithLock(), and
at GetLockConflicts()?

I am a little fuzzy on how the btree stuff works, but it seems to me
that you are looking for transactions that both have an xmin before
some threshold and also hold an AccessShareLock on some relation.
GetLockConflicts() will provide the latter, at least.

-- 
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] Creating new remote branch in git?

2011-06-11 Thread Bruce Momjian
Greg Smith wrote:
> On 06/10/2011 12:19 AM, Alex Hunsaker wrote:
> > It looks like if you push the remote branch first everything should work 
> > nicely:
> > git checkout master
> > git push origin origin:refs/heads/REL9_1_STABLE
> > git fetch # fetch the new branch
> > git checkout REL9_1_STABLE
> 
> This is basically the state of the art right now for the most frequently 
> deployed versions of git.  I don't think checking out master first is 
> necessary though.
> 
> Potentially useful automation/trivia for alternate approaches includes:
> 
> 1) Write a little script to do this messy chore, so you don't have to 
> remember this weird "create a new branch using a full refspec" syntax.  
> There is an example named git-create-branch along with a short tutorial 
> on this subject at 
> http://www.zorched.net/2008/04/14/start-a-new-branch-on-your-remote-git-repository/
> 
> 2) Use git_remote_branch https://github.com/webmat/git_remote_branch 
> which is the swiss army knife of remote branch hackery automation.
> 
> 3) Rather than manually hack the config files, use "git config" to do 
> it.  Not sure if this is completely workable, but something like this 
> might connect the newly created branch to your local one after pushing 
> it out, without actually opening the config with an editor:
> 
> git config branch.REL9_1_STABLE.remote origin
> git config branch.REL9_1_STABLE.merge refs/heads/REL9_1_STABLE
> 
> 4) Use a system with git>=1.7.0, which adds:
> 
> git branch --set-upstream REL9_1_STABLE origin/REL9_1_STABLE

Uh, I think someone needs to add this to our wiki:

http://wiki.postgresql.org/wiki/Working_with_Git
http://wiki.postgresql.org/wiki/Committing_with_Git

I needed this when using git-new-workdir so at least it is needed there;
I am unclear how wide this is needed so I cannot add it.

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

  + It's impossible for everything to be true. +

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


Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?

2011-06-11 Thread Noah Misch
On Wed, Mar 30, 2011 at 04:48:26PM -0400, Robert Haas wrote:
> On Tue, Mar 29, 2011 at 2:29 PM, Noah Misch  wrote:
> >> It's actually not
> >> clear to me what the user could usefully do other than trying to
> >> preserve his transaction by setting a high deadlock_timeout - what is
> >> the use case, other than that?
> >
> > The other major use case is reducing latency in deadlock-prone 
> > transactions. ?By
> > reducing deadlock_timeout for some or all involved transactions, the error 
> > will
> > arrive earlier.
> 
> Good point.
> 
> >> Is it worth thinking about having an explicit setting for deadlock
> >> priority? ?That'd be more work, of course, and I'm not sure it it's
> >> worth it, but it'd also provide stronger guarantees than you can get
> >> with this proposal.
> >
> > That is a better UI for the first use case. ?I have only twice wished to 
> > tweak
> > deadlock_timeout: once for the use case you mention, another time for that
> > second use case. ?Given that, I wouldn't have minded a rough UI. ?If you'd 
> > use
> > this often and assign more than two or three distinct priorities, you'd 
> > probably
> > appreciate the richer UI. ?Not sure how many shops fall in that group.
> 
> Me neither.  If making the deadlock timeout PGC_SUSET is independently
> useful, I don't object to doing that first, and then we can wait and
> see if anyone feels motivated to do more.

Here's the patch for that.  Not much to it.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 3981969..f94782f 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 5258,5264  dynamic_library_path = 
'C:\tools\postgresql;H:\my_project\lib;$libdir'
  practice. On a heavily loaded server you might want to raise it.
  Ideally the setting should exceed your typical transaction time,
  so as to improve the odds that a lock will be released before
! the waiter decides to check for deadlock.
 
  
 
--- 5258,5265 
  practice. On a heavily loaded server you might want to raise it.
  Ideally the setting should exceed your typical transaction time,
  so as to improve the odds that a lock will be released before
! the waiter decides to check for deadlock.  Only superusers can change
! this setting.
 
  
 
diff --git a/src/backend/utils/index 92391ed..48ffe95 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 1532,1539  static struct config_int ConfigureNamesInt[] =
},
  
{
!   /* This is PGC_SIGHUP so all backends have the same value. */
!   {"deadlock_timeout", PGC_SIGHUP, LOCK_MANAGEMENT,
gettext_noop("Sets the time to wait on a lock before 
checking for deadlock."),
NULL,
GUC_UNIT_MS
--- 1532,1539 
},
  
{
!   /* This is PGC_SUSET to prevent hiding from log_lock_waits. */
!   {"deadlock_timeout", PGC_SUSET, LOCK_MANAGEMENT,
gettext_noop("Sets the time to wait on a lock before 
checking for deadlock."),
NULL,
GUC_UNIT_MS

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


Re: [HACKERS] Identifying no-op length coercions

2011-06-11 Thread Noah Misch
On Sat, Jun 11, 2011 at 03:03:18PM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > Good points.  I'm thinking, then, add an Expr argument to 
> > simplify_function()
> > and have the CoerceViaIO branch of eval_const_expressions_mutator() pass 
> > NULL
> > for both its simplify_function() calls.  If simplify_function() gets a NULL 
> > Expr
> > for a function that has a protransform, synthesize a FuncExpr based on its 
> > other
> > arguments before calling the transform function.  Seem reasonable?  Granted,
> > that would be dead code until someone applies a transform function to a 
> > type I/O
> > function, which could easily never happen.  Perhaps just ignore the 
> > transform
> > function when we started with a CoerceViaIO node?
> 
> Until we actually have a use-case for simplifying I/O functions like this,
> I can't see going out of our way for it ...

Sounds good.  Updated patch attached, incorporating your ideas.  Before applying
it, run this command to update the uninvolved pg_proc.h DATA entries:
  perl -pi -e 's/PGUID(\s+\d+){4}/$& 0/' src/include/catalog/pg_proc.h

Thanks,
nm
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 8504555..5d1a447 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 4337,4342 
--- 4337,4349 
   
  
   
+   protransform
+   regproc
+   pg_proc.oid
+   Calls to function can be simplified by this other 
function
+  
+ 
+  
proisagg
bool

diff --git a/src/backend/catalog/index 6250b07..92be0a7 100644
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***
*** 304,309  ProcedureCreate(const char *procedureName,
--- 304,310 
values[Anum_pg_proc_procost - 1] = Float4GetDatum(procost);
values[Anum_pg_proc_prorows - 1] = Float4GetDatum(prorows);
values[Anum_pg_proc_provariadic - 1] = ObjectIdGetDatum(variadicType);
+   values[Anum_pg_proc_protransform - 1] = ObjectIdGetDatum(InvalidOid);
values[Anum_pg_proc_proisagg - 1] = BoolGetDatum(isAgg);
values[Anum_pg_proc_proiswindow - 1] = BoolGetDatum(isWindowFunc);
values[Anum_pg_proc_prosecdef - 1] = BoolGetDatum(security_definer);
diff --git a/src/backend/commands/taindex 2c9f855..563a1b2 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 56,61 
--- 56,62 
  #include "nodes/nodeFuncs.h"
  #include "nodes/parsenodes.h"
  #include "optimizer/clauses.h"
+ #include "optimizer/planner.h"
  #include "parser/parse_clause.h"
  #include "parser/parse_coerce.h"
  #include "parser/parse_collate.h"
***
*** 3495,3501  ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, 
LOCKMODE lockmode)
{
NewColumnValue *ex = lfirst(l);
  
!   ex->exprstate = ExecPrepareExpr((Expr *) ex->expr, estate);
}
  
notnull_attrs = NIL;
--- 3496,3503 
{
NewColumnValue *ex = lfirst(l);
  
!   /* expr already planned */
!   ex->exprstate = ExecInitExpr((Expr *) ex->expr, NULL);
}
  
notnull_attrs = NIL;
***
*** 4398,4404  ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, 
Relation rel,
  
newval = (NewColumnValue *) 
palloc0(sizeof(NewColumnValue));
newval->attnum = attribute.attnum;
!   newval->expr = defval;
  
tab->newvals = lappend(tab->newvals, newval);
tab->rewrite = true;
--- 4400,4406 
  
newval = (NewColumnValue *) 
palloc0(sizeof(NewColumnValue));
newval->attnum = attribute.attnum;
!   newval->expr = expression_planner(defval);
  
tab->newvals = lappend(tab->newvals, newval);
tab->rewrite = true;
***
*** 6706,6711  ATPrepAlterColumnType(List **wqueue,
--- 6708,6716 
/* Fix collations after all else */
assign_expr_collations(pstate, transform);
  
+   /* Plan the expr now so we can accurately assess the need to 
rewrite. */
+   transform = (Node *) expression_planner((Expr *) transform);
+ 
/*
 * Add a work queue item to make ATRewriteTable update the 
column
 * contents.
diff --git a/src/backend/optimizer/utilindex 2914c39..e9fb750 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
***
*** 106,114  static List *simplify_and_arguments(List *args,
   eval_const_expressions_context 
*context,
   bool *haveNull, bool *forceFalse);
  static Node *simplify_boolean_equality(Oid opno, List *args);
! static Expr *simplify_function(Oid funcid,
!

Re: [HACKERS] procpid?

2011-06-11 Thread Bruce Momjian
Joshua D. Drake wrote:
> On 6/11/2011 1:02 AM, Jaime Casanova wrote:
> > On Sat, Jun 11, 2011 at 12:02 AM, Jim Nasby  wrote:
> >> It's damn annoying... enough so that I'd personally be in favor of 
> >> creating a pid column that has the same data so we can deprecate
> >> procpid and eventually remove it...
> > well, if we will start changing bad picked names we will have a *lot*
> > of work to do... starting by the project's name ;)
> 
> There is a difference between a project name and something that directly 
> affects usability. +1 on fixing this. IMO, we don't create a new pid 
> column, we just fix the problem. If we do it for 9.2, we have 18 months 
> to communicate the change.

Uh, I am the first one I remember complaining about this so I don't see
why we should break compatibility for such a low-level problem.

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

  + It's impossible for everything to be true. +

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


Re: [HACKERS] Small SSI issues

2011-06-11 Thread Dan Ports
On Sat, Jun 11, 2011 at 01:38:31PM -0500, Kevin Grittner wrote:
> I'm not concerned about references covered by
> SerializableXactHashLock.  I am more concerned about some of the
> tests for whether the (MySerializableXact == InvalidSerializableXact)
> checks and any other tests not covered by that lock are OK without it
> (and OK with it).  Since my knowledge of weak memory ordering
> behavior is, well, weak I didn't want to try to make that call.

Oh, those checks are definitely not an issue -- MySerializableXact
itself (the pointer, not the thing it's pointing to) is in
backend-local memory, so it won't change under us.

The volatile qualifier (as written) doesn't help with that anyway, it
attaches to the data being pointed to, not the pointer itself.

Dan

-- 
Dan R. K. Ports  MIT CSAILhttp://drkp.net/

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


Re: [HACKERS] pgbench--new transaction type

2011-06-11 Thread Jeff Janes
On Sun, May 29, 2011 at 7:04 PM, Greg Smith  wrote:
> On 05/29/2011 03:11 PM, Jeff Janes wrote:
>>
>> If you use "pgbench -S -M prepared" at a scale where all data fits in
>> memory, most of what you are benchmarking is network/IPC chatter, and
>> table locking.
>
> If you profile it, you'll find a large amount of the time is actually spent
> doing more mundane things, like memory allocation.  The network and locking
> issues are really not the bottleneck at all in a surprising number of these
> cases.

I wouldn't expect IPC chatter to show up in profiling, because it
costs wall time, but not CPU time.  The time spent might be attributed
to the kernel, or to pgbench, or to nothing at all.

As part of the "Eviscerating the parser" discussion, I made a hack
that had exec_simple_query do nothing but return a dummy
completionTag.  So there was no parsing, planning, or execution.
Under this mode, I got 44758 TPS, or 22.3 microsecons/transaction,
which should represent the cost of IPC chatter and pgbench overhead.

The breakdown I get, in microseconds per item, are:

53.70  cost of a select and related overhead via -S -M prepared.  of which:
22.34  cost of IPC and pgbench roundtrip, estimated via above discussion
16.91  cost of the actual execution of the select statement, estimated
via the newly suggested -P mode.

14.45  residual usec cost, covering table locking, transaction begin
and end, plus measurement errors.

Because all my tests were single-client, the cost of locking would be
much lower than they would be in contended cases.  However, I wouldn't
trust profiling to accurate reflect the locking time anyway, for the
same reason I don't trust it for IPC chatter--wall time is consumed
but not spent on the CPU, so is not counted by profiling.

As you note memory allocation consumes much profiling time.  However,
memory allocation is a low level operation which is always in support
of some higher purpose, such as parsing, execution, or taking locks.
My top down approach attempts to assign these bottom-level costs to
the proper higher level purpose.


> Your patch isn't really dependent on your being right about the
> cause here, which means this doesn't impact your submissions any.  Just
> wanted to clarify that what people expect are slowing things down in this
> situation and what actually shows up when you profile are usually quite
> different.
>
> I'm not sure whether this feature makes sense to add to pgbench, but it's
> interesting to have it around for developer testing.

Yes, this is what I thought the opinion might be.  But there is no
repository of such "useful for developer testing" patches, other than
this mailing list.  That being the case, would it even be worthwhile
to fix it up more and submit it to commitfest?

>> some numbers for single client runs on 64-bit AMD Opteron Linux:
>> 12,567 sps  under -S
>> 19,646 sps  under -S -M prepared
>> 58,165 sps  under -P
>>
>
> 10,000 is too big of a burst to run at once.  The specific thing I'm
> concerned about is what happens if you try this mode when using "-T" to
> enforce a runtime limit, and your SELECT rate isn't high.  If you're only
> doing 100 SELECTs/second because your scale is big enough to be seek bound,
> you could overrun by nearly two minutes.

OK.  I wouldn't expect someone to want to use -P under scales that
cause that to happen, but perhaps it should deal with it more
gracefully.  I picked 10,000 just because it obviously large enough
for any hardware I have, or will have for the foreseeable future.

> I think this is just a matter of turning the optimization around a bit.
>  Rather than starting with a large batch size and presuming that's ideal, in
> this case a different approach is really needed.  You want the smallest
> batch size that gives you a large win here.  My guess is that most of the
> gain here comes from increasing batch size to something in the 10 to 100
> range; jumping to 10K is probably overkill.  Could you try some smaller
> numbers and see where the big increases start falling off at?

I've now used a variety of sizes (powers of 2 up to 8192, plus 1);
and the results fit very well to a linear equation, with 17.3
usec/inner select plus 59.0 usec/outer invocation.

So at a loop of 512, you would have overhead of 59.0/512=0.115 out of
total time of 17.4, or 0.7% overhead.  So that should be large enough.

Thanks for the other suggestions.

Cheers,

Jeff

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


Re: [HACKERS] [BUG] Denormal float values break backup/restore

2011-06-11 Thread Jeroen Vermeulen

On 2011-06-11 01:57, Tom Lane wrote:


(5) Lobby your libc providers to make strtod accept denormals without
throwing ERANGE.  There is no obvious reason why it shouldn't.  (On at
least one of my boxes, it does.)


The standard either explicitly allows or requires this behaviour 
(depending on which text I look at) for underflow.


AIUI that is defined to be a little vague, but includes denormalized 
numbers that would undergo any rounding at all.  It says that on 
overflow the conversion should return the appropriate HUGE_VAL variant, 
and set ERANGE.  On underflow it returns a reasonably appropriate value 
(and either may or must set ERANGE, which is the part that isn't clear 
to me).


ISTM the appropriate response to ERANGE combined with a "small" return 
value is to either ignore or report the rounding error, but accept the 
return value.  The current code in float.c doesn't check the return 
value at all and treats all ERANGE conditions as equal.



Jeroen

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


Re: [HACKERS] Identifying no-op length coercions

2011-06-11 Thread Tom Lane
Noah Misch  writes:
> Good points.  I'm thinking, then, add an Expr argument to simplify_function()
> and have the CoerceViaIO branch of eval_const_expressions_mutator() pass NULL
> for both its simplify_function() calls.  If simplify_function() gets a NULL 
> Expr
> for a function that has a protransform, synthesize a FuncExpr based on its 
> other
> arguments before calling the transform function.  Seem reasonable?  Granted,
> that would be dead code until someone applies a transform function to a type 
> I/O
> function, which could easily never happen.  Perhaps just ignore the transform
> function when we started with a CoerceViaIO node?

Until we actually have a use-case for simplifying I/O functions like this,
I can't see going out of our way for it ...

regards, tom lane

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


Re: [HACKERS] Small SSI issues

2011-06-11 Thread Kevin Grittner
> Dan Ports  wrote:
> On Fri, Jun 10, 2011 at 09:43:58PM +0300, Heikki Linnakangas wrote:
 
>>> Do checks such as that argue for keeping the volatile flag, or do
>>> you think we can drop it if we make those changes? (That would
>>> also allow dropping a number of casts which exist just to avoid
>>> warnings.)
>>
>> I believe we can drop it, I'll double-check.
> 
> Yes, dropping it seems like the thing to do. It's been on my list
> for a while. We are not really getting anything out of declaring it
> volatile since we cast the volatile qualifier away most of the
> time.
 
I'm not concerned about references covered by
SerializableXactHashLock.  I am more concerned about some of the
tests for whether the (MySerializableXact == InvalidSerializableXact)
checks and any other tests not covered by that lock are OK without it
(and OK with it).  Since my knowledge of weak memory ordering
behavior is, well, weak I didn't want to try to make that call.
 
-Kevin

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


Re: [HACKERS] Identifying no-op length coercions

2011-06-11 Thread Noah Misch
On Sat, Jun 11, 2011 at 02:11:55PM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > We originally discussed having the transform function take and return Expr
> > nodes.  It turns out that simplify_function() does not have the Expr, 
> > probably
> > because the particular choice of node type among the many that can convey a
> > function call does not matter to it.  The same should be true of a transform
> > function -- it should do the same thing whether the subject call arrives 
> > from
> > a FuncExpr or an OpExpr.  Therefore, I changed the transform function
> > signature to "Expr *protransform(List *args)".
> 
> That seems like the wrong thing.  For one thing, it makes it quite
> impossible to share one transform support function among multiple
> functions/operators, since it won't know which one the argument list
> is for.  More generally, I foresee the transform needing to know
> most of the other information that might be in the FuncExpr or OpExpr.
> It certainly would need access to the collation, and it would probably
> like to be able to copy the parse location to whatever it outputs,
> and so forth and so on.  So I really think passing the node to the
> function is the most future-proof way to do it.  If that means you
> need to rejigger things a bit in clauses.c, so be it.

Good points.  I'm thinking, then, add an Expr argument to simplify_function()
and have the CoerceViaIO branch of eval_const_expressions_mutator() pass NULL
for both its simplify_function() calls.  If simplify_function() gets a NULL Expr
for a function that has a protransform, synthesize a FuncExpr based on its other
arguments before calling the transform function.  Seem reasonable?  Granted,
that would be dead code until someone applies a transform function to a type I/O
function, which could easily never happen.  Perhaps just ignore the transform
function when we started with a CoerceViaIO node?

> > The large pg_proc.h diff mostly just adds protransform = 0 to every 
> > function.
> > Due to the resulting patch size, I've compressed it.  There are 
> > new/otherwise
> > changed DATA lines for OIDs 669 and 3097 only.
> 
> The chances that this part will still apply cleanly by the time anyone
> gets around to committing it are nil.  I'd suggest you break it into two
> separate patches, one that modifies the existing lines to add the
> protransform = 0 column and then one to apply the remaining deltas in
> the file.  I envision the eventual committer doing the first step
> on-the-fly (perhaps with an emacs macro, at least that's the way I
> usually do it) and then wanting a patchable diff for the rest.  Or if
> you wanted to be really helpful, you could provide a throwaway perl
> script to do the modifications of the existing lines ...

That would be better; I'll do it for the next version.

> I haven't read the actual patch, these are just some quick reactions
> to your description.

Thanks,
nm

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


Re: [HACKERS] Identifying no-op length coercions

2011-06-11 Thread Tom Lane
Noah Misch  writes:
> We originally discussed having the transform function take and return Expr
> nodes.  It turns out that simplify_function() does not have the Expr, probably
> because the particular choice of node type among the many that can convey a
> function call does not matter to it.  The same should be true of a transform
> function -- it should do the same thing whether the subject call arrives from
> a FuncExpr or an OpExpr.  Therefore, I changed the transform function
> signature to "Expr *protransform(List *args)".

That seems like the wrong thing.  For one thing, it makes it quite
impossible to share one transform support function among multiple
functions/operators, since it won't know which one the argument list
is for.  More generally, I foresee the transform needing to know
most of the other information that might be in the FuncExpr or OpExpr.
It certainly would need access to the collation, and it would probably
like to be able to copy the parse location to whatever it outputs,
and so forth and so on.  So I really think passing the node to the
function is the most future-proof way to do it.  If that means you
need to rejigger things a bit in clauses.c, so be it.

> The large pg_proc.h diff mostly just adds protransform = 0 to every function.
> Due to the resulting patch size, I've compressed it.  There are new/otherwise
> changed DATA lines for OIDs 669 and 3097 only.

The chances that this part will still apply cleanly by the time anyone
gets around to committing it are nil.  I'd suggest you break it into two
separate patches, one that modifies the existing lines to add the
protransform = 0 column and then one to apply the remaining deltas in
the file.  I envision the eventual committer doing the first step
on-the-fly (perhaps with an emacs macro, at least that's the way I
usually do it) and then wanting a patchable diff for the rest.  Or if
you wanted to be really helpful, you could provide a throwaway perl
script to do the modifications of the existing lines ...

I haven't read the actual patch, these are just some quick reactions
to your description.

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] procpid?

2011-06-11 Thread Joshua D. Drake

On 6/11/2011 1:02 AM, Jaime Casanova wrote:

On Sat, Jun 11, 2011 at 12:02 AM, Jim Nasby  wrote:

It's damn annoying... enough so that I'd personally be in favor of creating a 
pid column that has the same data so we can deprecate
procpid and eventually remove it...

well, if we will start changing bad picked names we will have a *lot*
of work to do... starting by the project's name ;)


There is a difference between a project name and something that directly 
affects usability. +1 on fixing this. IMO, we don't create a new pid 
column, we just fix the problem. If we do it for 9.2, we have 18 months 
to communicate the change.


Joshua D. Drake



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


[HACKERS] REL9_1_STABLE branch created

2011-06-11 Thread Tom Lane
Please remember to double-patch anything that should go into 9.1.

For the record, the correct formula seems to be

$ git pull
Current branch master is up to date.
$ git push origin master:refs/heads/REL9_1_STABLE
Total 0 (delta 0), reused 0 (delta 0)
To ssh://g...@gitmaster.postgresql.org/postgresql.git
 * [new branch]  master -> REL9_1_STABLE

after which you can check out the branch locally according to whatever
formula you're already using (a separate workdir in my case).

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] Core Extensions relocation

2011-06-11 Thread Greg Smith

Peter Eisentraut wrote:

For the directory name, I'd prefer either src/extensions (since there is
more than one), or if you want to go for short somehow, src/ext.  (Hmm,
I guess the installation subdirectory is also called "extension".  But
it felt wrong on first reading anyway.)
  


I jumped between those two a couple of times myself, settling on 
"extension" to match the installation location as you figured out.  
Assuming that name shouldn't change at this point, this seemed the best 
way to name the new directory, even though I agree it seems weird at first.




What version did you branch this off? :)
  


Long enough ago that apparently I've missed some major changes; Magnus 
already pointed out I needed to revisit how MODULEDIR was used.  Looks 
like I need to rebuild the first patch in this series yet again, which 
shouldn't be too bad.  The second time I did that, I made the commits 
atomic enough that the inevitable third one would be easy.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us



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


Re: [HACKERS] creating CHECK constraints as NOT VALID

2011-06-11 Thread Dean Rasheed
On 11 June 2011 16:40, Dean Rasheed  wrote:
> On 11 June 2011 14:40, Thom Brown  wrote:
>> On 11 June 2011 14:32, Dean Rasheed  wrote:
>>> On 1 June 2011 23:47, Alvaro Herrera  wrote:

 Here's a complete patch with all this stuff, plus doc additions and
 simple regression tests for the new ALTER DOMAIN commands.

    Enable CHECK constraints to be declared NOT VALID

    This means that they can initially be added to a large existing table
    without checking its initial contents, but new tuples must comply to
    them; a separate pass invoked by ALTER TABLE / VALIDATE can verify
    existing data and ensure it complies with the constraint, at which point
    it is marked validated and becomes a normal part of the table ecosystem.

>>>
>>> I think that you also need to update the constraint exclusion code
>>> (get_relation_constraints() or nearby), otherwise the planner might
>>> exclude a relation on the basis of a CHECK constraint that is not
>>> currently VALID.
>>
>> Do the standards explicitly stipulate an expected behaviour for this?
>
> No I believe that this is a PostgreSQL-specific optimisation, and we
> need to ensure that queries return the correct results with
> constraint_exclusion on.
>
>> And does such a problem affect the invalid foreign key change too?
>
> No only CHECK constraints (and possibly NOT NULL constraints in the future).
>
> Regards,
> Dean
>

Since you've mentioned the SQL spec, its worth noting that whilst I think
that this feature will be very useful, it's not the feature in the SQL
spec (at least not in my version).

The SQL spec feature is to mark a constraint as NOT ENFORCED, which
means that no data (existing or new) is checked against the
constraint. It's as if the constraint were not present at all. In
Oracle this corresponds to the syntax

  ALTER TABLE mytable ENABLE/DISABLE myconstraint

which is actually quite handy during a bulk load/update - disable all
your constraints, do the bulk operation and then re-enable them
(automatically re-validating them). This is better than dropping and
re-creating the constraints because you don't need to remember all the
constraint definitions.

I can see both features being quite useful in different situations.

Regards,
Dean

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


Re: [HACKERS] creating CHECK constraints as NOT VALID

2011-06-11 Thread Dean Rasheed
On 11 June 2011 14:40, Thom Brown  wrote:
> On 11 June 2011 14:32, Dean Rasheed  wrote:
>> On 1 June 2011 23:47, Alvaro Herrera  wrote:
>>>
>>> Here's a complete patch with all this stuff, plus doc additions and
>>> simple regression tests for the new ALTER DOMAIN commands.
>>>
>>>    Enable CHECK constraints to be declared NOT VALID
>>>
>>>    This means that they can initially be added to a large existing table
>>>    without checking its initial contents, but new tuples must comply to
>>>    them; a separate pass invoked by ALTER TABLE / VALIDATE can verify
>>>    existing data and ensure it complies with the constraint, at which point
>>>    it is marked validated and becomes a normal part of the table ecosystem.
>>>
>>
>> I think that you also need to update the constraint exclusion code
>> (get_relation_constraints() or nearby), otherwise the planner might
>> exclude a relation on the basis of a CHECK constraint that is not
>> currently VALID.
>
> Do the standards explicitly stipulate an expected behaviour for this?

No I believe that this is a PostgreSQL-specific optimisation, and we
need to ensure that queries return the correct results with
constraint_exclusion on.

> And does such a problem affect the invalid foreign key change too?

No only CHECK constraints (and possibly NOT NULL constraints in the future).

Regards,
Dean

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


Re: [HACKERS] creating CHECK constraints as NOT VALID

2011-06-11 Thread Thom Brown
On 11 June 2011 14:32, Dean Rasheed  wrote:
> On 1 June 2011 23:47, Alvaro Herrera  wrote:
>>
>> Here's a complete patch with all this stuff, plus doc additions and
>> simple regression tests for the new ALTER DOMAIN commands.
>>
>>    Enable CHECK constraints to be declared NOT VALID
>>
>>    This means that they can initially be added to a large existing table
>>    without checking its initial contents, but new tuples must comply to
>>    them; a separate pass invoked by ALTER TABLE / VALIDATE can verify
>>    existing data and ensure it complies with the constraint, at which point
>>    it is marked validated and becomes a normal part of the table ecosystem.
>>
>
> I think that you also need to update the constraint exclusion code
> (get_relation_constraints() or nearby), otherwise the planner might
> exclude a relation on the basis of a CHECK constraint that is not
> currently VALID.

Do the standards explicitly stipulate an expected behaviour for this?
And does such a problem affect the invalid foreign key change too?

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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


Re: [HACKERS] creating CHECK constraints as NOT VALID

2011-06-11 Thread Dean Rasheed
On 1 June 2011 23:47, Alvaro Herrera  wrote:
>
> Here's a complete patch with all this stuff, plus doc additions and
> simple regression tests for the new ALTER DOMAIN commands.
>
>    Enable CHECK constraints to be declared NOT VALID
>
>    This means that they can initially be added to a large existing table
>    without checking its initial contents, but new tuples must comply to
>    them; a separate pass invoked by ALTER TABLE / VALIDATE can verify
>    existing data and ensure it complies with the constraint, at which point
>    it is marked validated and becomes a normal part of the table ecosystem.
>

I think that you also need to update the constraint exclusion code
(get_relation_constraints() or nearby), otherwise the planner might
exclude a relation on the basis of a CHECK constraint that is not
currently VALID.

Regards,
Dean

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


Re: [HACKERS] procpid?

2011-06-11 Thread Jaime Casanova
On Sat, Jun 11, 2011 at 12:02 AM, Jim Nasby  wrote:
>
> It's damn annoying... enough so that I'd personally be in favor of creating a 
> pid column that has the same data so we can deprecate
> procpid and eventually remove it...

well, if we will start changing bad picked names we will have a *lot*
of work to do... starting by the project's name ;)

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

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