Re: [HACKERS] Cancelling idle in transaction state

2009-12-05 Thread Simon Riggs
On Sat, 2009-12-05 at 18:13 -0700, James Pye wrote:
> On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
> > ...
> 
> I'm not volunteering here, but having worked with the protocol, I do have a 
> suggestion:

Thanks. Looks like good input. With the further clarification that we
use NOTIFY it seems a solution is forming.

Any other takers?

-- 
 Simon Riggs   www.2ndQuadrant.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] Cancelling idle in transaction state

2009-12-05 Thread Robert Haas
On Sat, Dec 5, 2009 at 10:15 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I think this line of thinking is on the right track.  The server
>> should certainly not send back an immediate ERROR response, because
>> that will definitely confuse the client.  Of course, any subsequent
>> commands will report ERRORs until the client rolls back.  But it also
>> seems highly desirable for the server to send some sort of immediate,
>> asynchronous notification, so that a sufficiently smart client can
>> immediately report the error back to the user or take such other
>> action as it deems appropriate.
>
> If you must have that, send a NOTICE.

Ah ha!  I missed that one.  That's perfect.

> I don't actually see the point
> though.  If the client was as smart and well-coded as all that, it
> wouldn't be sitting on an open transaction in the first place.

Think about an interactive client.  It's not the client's fault that
the user has chosen to begin a transaction and then sit there
cogitating, but the client would like to let the user know right away
that their current transaction is defunct.

...Robert

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


Re: [HACKERS] Cancelling idle in transaction state

2009-12-05 Thread Tom Lane
Robert Haas  writes:
> I think this line of thinking is on the right track.  The server
> should certainly not send back an immediate ERROR response, because
> that will definitely confuse the client.  Of course, any subsequent
> commands will report ERRORs until the client rolls back.  But it also
> seems highly desirable for the server to send some sort of immediate,
> asynchronous notification, so that a sufficiently smart client can
> immediately report the error back to the user or take such other
> action as it deems appropriate.

If you must have that, send a NOTICE.  I don't actually see the point
though.  If the client was as smart and well-coded as all that, it
wouldn't be sitting on an open transaction in the first place.

> Currently, it appears that the only messages that the server can send
> back asynchronously are ParameterStatus and NotificationResponse.

Using either of those is completely inappropriate.

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] Cancelling idle in transaction state

2009-12-05 Thread Robert Haas
On Sat, Dec 5, 2009 at 8:13 PM, James Pye  wrote:
> I think the answer here is that the server should *not* report the 
> cancellation.
>
> Rather, it should mark the transaction as failed and let the client 
> eventually sync its state on subsequent requests that will result in 
> InFailedTransaction ERRORs.
>
[...]
> Also, if immediate notification is seen as a necessity, a WARNING with a 
> special code could be leveraged. Oh, or maybe use a dedicated LISTEN/NOTIFY 
> channel? "LISTEN pg_darn_admin_zapped_my_xact;" to opt-in for transaction 
> cancellation events that occur in *this* backend.. [Note: this is in addition 
> to COMMITs emitting ERRORs]

I think this line of thinking is on the right track.  The server
should certainly not send back an immediate ERROR response, because
that will definitely confuse the client.  Of course, any subsequent
commands will report ERRORs until the client rolls back.  But it also
seems highly desirable for the server to send some sort of immediate,
asynchronous notification, so that a sufficiently smart client can
immediately report the error back to the user or take such other
action as it deems appropriate.

Currently, it appears that the only messages that the server can send
back asynchronously are ParameterStatus and NotificationResponse.  So
we need to decide whether it's feasible/better to shoehorn this
functionality into one of those message types, or whether we should
bump the protocol version and add a new message type (cue: panic in
the streets).  On first examination (and I am not an expert in this
area), ParameterStatus would seem to be the better choice, because it
appears to me that all clients must be prepared to cope with such
messages, whereas in theory a client might be unprepared for a
NotificationResponse if it never executes LISTEN.  (It seems clearly
preferable not to require clients to issue an explicit LISTEN in order
to enable this feature.)

Going with that theory, we could pick a "magical" parameter status
value, either something like __transaction_cancelled, or maybe even
something that contains a character that isn't even legal in a normal
parameter, like $transaction_cancelled, if we don't think that will
break any clients.  Then we could just report a value change for this
whenever an idle transaction is cancelled.  Clients who ignore this
will find out when they next issue a query; others will know
immediately.

...Robert

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


Re: [HACKERS] Block-level CRC checks

2009-12-05 Thread Greg Stark
It can save space because the line pointers have less alignment  
requirements. But I don't see any point in the current state.


--
Greg

On 2009-12-04, at 3:48 PM, Tom Lane  wrote:


Greg Stark  writes:

I'm not sure why I said "including ctid". We would have to move
everything transactional to the line pointer, including xmin, xmax,
ctid, all the hint bits, the updated flags, hot flags, etc. The only
things left in the tuple header would be things that have to be there
such as HAS_OIDS, HAS_NULLS, natts, hoff, etc. It would be a pretty
drastic change, though a fairly logical one. I recall someone  
actually

submitted a patch to separate out the transactional bits anyways a
while back, just to save a few bytes in in-memory tuples. If we could
save on disk-space usage it would be a lot more compelling. But it
doesn't look to me like it really saves enough often enough to be
worth so much code churn.


It would also break things for indexes, which don't need all that  
stuff

in their line pointers.

More to the point, moving the same bits to someplace else on the page
doesn't save anything at all.

   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] Cancelling idle in transaction state

2009-12-05 Thread James Pye
On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
> ...

I'm not volunteering here, but having worked with the protocol, I do have a 
suggestion:

>>  This allows locks to be released, but it is complex to report the
>>  cancellation back to the client.



I think the answer here is that the server should *not* report the cancellation.

Rather, it should mark the transaction as failed and let the client eventually 
sync its state on subsequent requests that will result in InFailedTransaction 
ERRORs.

With such a solution, COMMITs issued to administrator cancelled transactions 
should result in an ERROR. Well, I suppose that would only be a requirement 
when:

BEGIN;
... some work ...



COMMIT; <-- client needs to know that this failed,
and it should be something louder than
a "ROLLBACK" tag. :P


So, if a command were issued to a cancelled transaction prior to a COMMIT:

BEGIN;
... some work ...


SELECT * FROM something; -- fails, IFX ERROR emitted to client
COMMIT; <-- client was already notified of
the xact failure by a prior command's error,
so the normal "ROLLBACK" would be fine.



Also, if immediate notification is seen as a necessity, a WARNING with a 
special code could be leveraged. Oh, or maybe use a dedicated LISTEN/NOTIFY 
channel? "LISTEN pg_darn_admin_zapped_my_xact;" to opt-in for transaction 
cancellation events that occur in *this* backend.. [Note: this is in addition 
to COMMITs emitting ERRORs]

I can't see immediate notification being useful excepting some rather strange 
situations where the client left the transaction idle to go do other expensive 
operations that "should" be immediately interrupted if this particular 
transaction were to be cancelled for some reason.. Such a situation might even 
make sense if those "expensive operations" somehow depended on the locks held 
by the transaction, but I think that's a stretch. Not to mention that the 
client could just occasionally poll the transaction with 'SELECT 1's; no 
special WARNING or NOTIFY's would be necessary.
-- 
Sent 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 feature allowing to launch shell commands

2009-12-05 Thread Greg Smith

Michael Paquier wrote:
> 3) Should consider how :variable interpretation should work in a 
\[set]shell call
It is supported now. I implemented this, I made a test with your perl 
script, my own tests and it worked, at least no error appeared :)
It looks like how you did this is a little less complicated than I had 
hoped for.  Let me show you some examples of how I think this should 
work.  Say naccounts = 100 and bid=123 already:


\setshell aid skew.sh :naccounts
   runs "skew.sh 100"

\setshell aid skew.sh a ::naccounts c
 runs "skew.sh a :naccounts c" - do not substitute the variable if "::" 
appears, just replace with ":".  Otherwise, there's no way to include a 
real ":" in the command line


\setshell aid skew.h aid :naccounts :bid
 runs "shew.sh 100 123"

From looking at the code, I think that only case (1) is supported by 
the bits you added (haven't actually re-tested it since I know you're 
still working).  Also, having that same substitution logic work for both 
shell and setshell should would be nice here.


As far as reducing the amount of stuff in doCustom goes, I think what 
you want for this specific part is a subroutine you can pass a string 
that has some number of :variable references in it, returning a new 
string with them having the substituted values inserted in there.  
That's something I think it would be easier to get right as a standalone 
function first, and then both shell and setshell could use it.


> Do you have an idea of what kind of tests could be done? I don't have 
so much experience

> about common regression tests linked to pgbench.

None of the regression tests use pgbench yet, partly because contrib 
modules like it aren't necessarily even built before the main regression 
tests are run.  Also, it's hard to write a pgbench-based test using the 
current pg_regress framework.  The complete non-determinism on the TPS 
scores for example makes it hard to avoid having a diff against any 
standard regression result provided.  I think it would be nice to add a 
very complicated script that uses all these weird features for 
regression test purposes, but it's not so clear how we would enforce 
running it automatically to catch if a future change broke something.


As far as the rest of your concerns, once we get this to code complete 
and all the bugs squashed, I can take a pass at cleaning up your docs 
and making sure there's not any performance regression as part of my 
final review.  What you've added in there so far is good enough for now, 
I just didn't want to do the docs changes from scratch myself (and I 
thought it would be useful for you to get a bit of practice on that too, 
since they're usually required for patch submissions).  If you can make 
the three examples above all work, and get rid of the threading bug, 
I'll be clear to take care of docs review/performanc tests and then pass 
this over for a committer to look at.  It would be nice if the code was 
refactored a bit too first, just because it's less likely to be rejected 
for "the code is ugly" reasons if that's done first.  That sort of 
rejection is always a real possibility with this project, particularly 
for something like this where it's not as obvious to everyone what the 
feature is useful for.


--
Greg Smith2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
g...@2ndquadrant.com  www.2ndQuadrant.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] YAML Was: CommitFest status/management

2009-12-05 Thread Euler Taveira de Oliveira
Ron Mayer escreveu:
> While there's no great way to make this a contrib module today,
> would it make sense to add such hooks for an eventual module system?
> 
I don't think so. It's easier to code a converter tool.


-- 
  Euler Taveira de Oliveira
  http://www.timbira.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] Patch for information_schema performance

2009-12-05 Thread Peter Eisentraut
On fre, 2009-09-25 at 00:55 +0200, Joachim Wieland wrote:
> the attached patch addresses the performance issues of the
> authorization related views from information_schema (BUG #4596). It
> implements what Tom suggests in
> 
> http://archives.postgresql.org/pgsql-bugs/2008-12/msg00144.php
> 
> In the cases that I have tested both the new and the old view return
> the same data but I'd appreciate more tests. The patch currently does
> not remove the original views but renames them from xyz to old_xyz, so
> you can run your own tests of the new view definition vs. the old one.

Committed with a bunch of cleanup.


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


Re: [HACKERS] operator exclusion constraints

2009-12-05 Thread David Fetter
On Fri, Dec 04, 2009 at 11:35:52AM -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Dec 3, 2009 at 7:42 PM, Jeff Davis  wrote:
> >> On Thu, 2009-12-03 at 19:00 -0500, Tom Lane wrote:
> >>> I'm starting to go through this patch now. �I thought the
> >>> consensus was to refer to them as just "exclusion constraints"?
> >>> �I'm not seeing that the word "operator" really adds anything.
> >> 
> >> I assume you're referring to the name used in documentation and
> >> error messages. I didn't see a clear consensus, but the relevant
> >> thread is here:
> >> 
> >> http://archives.postgresql.org/message-id/1258227283.708.108.ca...@jdavis
> >> 
> >> "Exclusion Constraints" is fine with me, as are the other options
> >> listed in that email.
> 
> > Yeah, I don't remember any such consensus either, but it's not a
> > dumb name.  I have been idly wondering throughout this process
> > whether we should try to pick a name that conveys the fact that
> > these constraints are inextricably tied to the opclass/index
> > machinery - but I'm not sure it's possible to really give that
> > flavor in a short phrase, or that it's actually important to do
> > so.  IOW... "whatever".  :-)
> 
> Well, unique constraints are tied to the opclass/index machinery
> too.
> 
> Unless there's loud squawks I'm going to exercise committer's
> prerogative and make all the docs and messages just say "exclusion
> constraint".

We have "constraint exclusion" already, which could make this
confusing.  How about either the original "operator exclusion
constraint" or the slightly easier "whatever constraint" names?

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

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

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


Re: [HACKERS] PostgreSQL Release Support Policy

2009-12-05 Thread Simon Riggs
On Sat, 2009-12-05 at 15:33 -0500, Tom Lane wrote:

> In any case there's no need for someone to move off a version instantly
> the day after the last release for it.  So I really don't see why you
> think there would be "panic updates".

Hmm, well, I wasn't imagining it as a wholly rational response.  I guess
the existence of such a panic remains to be proven amongst our users,
who I should give more credit to than their counterparts that still use
other products.

-- 
 Simon Riggs   www.2ndQuadrant.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] Reading recovery.conf earlier

2009-12-05 Thread Simon Riggs
On Sat, 2009-12-05 at 15:43 -0500, Tom Lane wrote:
> Simon Riggs  writes:
> > I'm planning to read recovery.conf earlier in the startup process, so we
> > can make a few things more "recovery aware". It's a nice-to-have only.
> 
> Say what?  It's read at the beginning already.

Before the beginning then. :-)

Two reasons to move it earlier in the startup sequence of the server

* Some data structures are only required in HS mode. We don't know we're
in HS mode until we created shared memory and started the Startup
process. If we knew ahead of time, we could skip adding the structures.

* Some things in postgresql.conf need to be overridden in HS mode, for
example default_transaction_read_only = false. Again, we don't know
we're in HS mode until later. 

So I would want to read recovery.conf before we read postgresql.conf

Also, AFAIK, it's not easily possible to have dependencies between
settings in postgresql.conf, unless the dependencies are expressed by
ordering the dependent parameters in alphabetic order. So putting the
parameters in postgresql.conf wouldn't help much.

-- 
 Simon Riggs   www.2ndQuadrant.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] Hot standby, misc issues

2009-12-05 Thread Heikki Linnakangas
Simon Riggs wrote:
> On Fri, 2009-12-04 at 10:23 +0200, Heikki Linnakangas wrote:
>>> @Heikki: Why is error checking in KnownAssignedXidsRemove() #ifdef'd
>> out?? 
>>
>> It's explained in the comment:
>> /* XXX: This can still happen: If a transaction with a subtransaction
>>  * that haven't been reported yet aborts, and no WAL records have been
>>  * written using the subxid, the abort record will contain that subxid
>>  * and we haven't seen it before.
>>  */
> 
> Just realised that this occurs again because the call to
> RecordKnownAssignedTransactionIds() was removed from
> xact_commit_abort().
> 
> I'm guessing you didn't like the call in that place for some reason,
> since I smile while I remember it has been removed twice(!) even though
> I put "do not remove" comments on it to describe this corner case.
> 
> Not going to put it back a third time.

:-). Well, it does seem pointless to add entries to the hash table, just
to remove them at the very next line. But you're right, we should still
advance latestObservedXid, and if we do that, we need to memorize any
not-yet-seen XIDs in the known-assigned xids array. So that
RecordKnownAssignedTransactionIds() call needs to be put back.

BTW, if you want to resurrect the check in KnownAssignedXidsRemove(),
you also need to not complain before you reach the running-xacts record
and open up for read-only connections.

-- 
  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] Reading recovery.conf earlier

2009-12-05 Thread Tom Lane
Simon Riggs  writes:
> I'm planning to read recovery.conf earlier in the startup process, so we
> can make a few things more "recovery aware". It's a nice-to-have only.

Say what?  It's read at the beginning already.

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] PostgreSQL Release Support Policy

2009-12-05 Thread Tom Lane
Simon Riggs  writes:
> Could I request we change these dates slightly to

The intent of the policy is that the last formal minor release for a
branch will happen no earlier than the specified times.  It might well
be later, since we're not going to schedule updates specially for this
--- it'd be whenever the next set of updates occur, and that would more
likely be driven by bugs in newer branches, not the oldest one.

In any case there's no need for someone to move off a version instantly
the day after the last release for it.  So I really don't see why you
think there would be "panic updates".  There's so much fuzz in when a
version would be effectively dead that a few months either way in the
nominal date wouldn't make any difference anyway.

regards, tom lane

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


Re: [HACKERS] YAML Was: CommitFest status/management

2009-12-05 Thread Ron Mayer
Josh Berkus wrote:
>> ... YAML for easier readability ...
> 
> "almost as good" ... I agree with Kevin that it's more readable.
> 
> Again, if there were a sensible way to do YAML as a contrib module, I'd
> go for that, but there isn't.

Perhaps that's be a direction this could take?   What would
it take for this feature to be a demo/example for some future
modules system.

It seems like there have been a few recent features related
to decorating output (UTF8 tables, YAML explain, \d... updates).

While there's no great way to make this a contrib module today,
would it make sense to add such hooks for an eventual module system?




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


Re: [HACKERS] Adding support for SE-Linux security

2009-12-05 Thread Ron Mayer
Robert Haas wrote:
> On Thu, Dec 3, 2009 at 5:23 PM, Josh Berkus  wrote:
>> Kaigai, you've said that you could get SELinux folks involved in the
>> patch review.  I think it's past time that they were; please solicit them.
> 
> Actually, we tried that already, in a previous iteration of this
> discussion.  Someone actually materialized and commented on a few
> things.  The problem, as I remember it, was that they didn't know much
> about PostgreSQL, so we didn't get very far with it.  Unfortunately, I
> can't find the relevant email thread at the moment.

IIRC, at least a couple of the guys mentioned on the NSA's
SE-Linux page[1] participated - Joshua Brindle[2]  and Chad
Sellers[3] (in addition to Kaigai/NEC who's credited on the
NSA site as well).  Perhaps one or two others too - but with
common names it's hard to guess.

Links to the threads with Chad and Joshua below.

[1] http://www.nsa.gov/research/selinux/contrib.shtml
[2] http://www.google.com/search?q=site%3Aarchives.postgresql.org+brindle
[3] http://www.google.com/search?q=site%3Aarchives.postgresql.org+chad+sellers


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


Re: [HACKERS] Cancelling idle in transaction state

2009-12-05 Thread Simon Riggs
I'd be very grateful to any hackers out there who are looking for a
project before close of 8.5 to consider working on this. It's dang
useful, both for Hot Standby and normal processing.

(You'll have the added bonus of proving you're smarter than me!)

On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
> Added to TODO:
>   
>   Allow administrators to cancel multi-statement idle
>   transactions
>   
>   This allows locks to be released, but it is complex to report the
>   cancellation back to the client.
>   
>   * 
> http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php 
> 
> ---
> 
> Simon Riggs wrote:
> > Currently SIGINT is ignored during  in transaction, but we have
> > recently agreed to allow this to cancel the transaction. We said we
> > would do this in all cases, so this is a separate feature/patch (though
> > Hot Standby requires it).
> > 
> > A simple change allows the transaction to be cancelled, but there are
> > some loose ends that I wish to discuss.
> > 
> > If we are running a statement and a cancel is received, then we return
> > the ERROR to the client, who is expecting it. If we cancel a transaction
> > while the connection is idle, we have no way of signalling to the client
> > program this has occurred. So the client finds out about this much
> > later, not in fact until the next message is sent.
> > 
> > Is there a mechanism for communicating the state back to the client?
> > Will this be handled correctly with existing code? psql appears to be
> > confused by a cancelled backend.
> > 
> > I'm not familiar with these aspects of the code, so some clear
> > suggestions are needed to allow me to work this out. I'm worried that
> > this will delay things further otherwise.
> > 
> > -- 
> >  Simon Riggs   www.2ndQuadrant.com
> >  PostgreSQL Training, Services and Support
> > 

> 
> -- 
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
> 
>   + If your life is a hard drive, Christ can be your backup. +
> 
-- 
 Simon Riggs   www.2ndQuadrant.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] New VACUUM FULL

2009-12-05 Thread Jeff Davis
On Fri, 2009-12-04 at 19:49 -0500, Michael Glaesemann wrote:
> > I tested a variety of situations during my review, and everything  
> > worked
> > as I expected.
> 
> Would there be a way for you to package the scenarios you tested into  
> a suite?

Except for the simplest tests, they aren't easily moved to pg_regress.
For instance, how do you tell if a user table's relfilenode actually
changed? Easy to do manually, but tough to do with pg_regress.

Regards,
Jeff Davis



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


Re: [HACKERS] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Alvaro Herrera
Tom Lane escribió:
> "David E. Wheeler"  writes:
> > Tom, what's your objection to Shlib load time being user-visible?
> 
> It's not really designed to be user-visible.  Let me give you just
> two examples:
> 
> * We call a plperl function for the first time in a session, causing
> plperl.so to be loaded.  Later the transaction fails and is rolled
> back.

I don't think there's any way for this to work sanely unless the library
has been loaded previously.  What about allowing those settings only if
plperl is specified in shared_preload_libraries?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
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] Summary and Plan for Hot Standby

2009-12-05 Thread Heikki Linnakangas
Simon Riggs wrote:
> On Sun, 2009-11-15 at 16:07 +0200, Heikki Linnakangas wrote:
> 
>> - The assumption that b-tree vacuum records don't need conflict
>> resolution because we did that with the additional cleanup-info record
>> works ATM, but it hinges on the fact that we don't delete any tuples
>> marked as killed while we do the vacuum. That seems like a low-hanging
>> fruit that I'd actually like to do now that I spotted it, but will then
>> need to fix b-tree vacuum records accordingly. 
> 
> You didn't make a change, so I wonder whether you realised no change was
> required or that you still think change is necessary, but have left it
> to me? Not sure.
> 
> I've investigated this but can't see any problem or need for change.

Sorry if I was unclear: it works as it is. But *if* we change the b-tree
vacuum to also delete index tuples marked with LP_DEAD, we have a problem.

> I think its important that we note this assumption though.

Yeah, a comment is in order.

-- 
  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] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Tim Bunce
On Sat, Dec 05, 2009 at 11:41:36AM -0500, Tom Lane wrote:
> Tim Bunce  writes:
> > I'll modify the patch to disable the SPI functions during
> > initialization (both on_perl_init and on_(un)trusted_init). 
> 
> Yeah, in the shower this morning I was thinking that not loading
> SPI till after the on_init code runs would alleviate the concerns
> about transactionality and permissions --- that would ensure that
> whatever on_init does affects only the Perl world and not the database
> world.
> 
> However, we're not out of the woods yet.  In a trusted interpreter
> (plperl not plperlu), is the on_init code executed before we lock down
> the interpreter with Safe?

The on_perl_init code (PGC_SUSET) is run before Safe is loaded.

The on_trusted_init code (PGC_USERSET) is run inside Safe.

> I would think it has to be since the main point AFAICS is to let you
> preload code via "use".

The main use case being targeted at the moment for on_trusted_init
is setting values in %_SHARED, perhaps to enable debugging.

Inside Safe you'll only be able to 'use' modules that have already been
loaded inside Safe. In my draft patch that's currently just strict and
warnings.

(I am also adding an interface to enable DBAs to configure what gets
loaded into the Safe compartment and what gets shared with it.
That'll be the way extra modules can be used by plperl.
It'll be used via on_perl_init so be controlled via the DBA.)

> I can hardly imagine DBAs wanting to vet a few thousand lines of
> random Perl code to see if it contains anything that could be
> subverted.  For example, the ability to scribble on database files
> (like say pg_hba.conf) would almost surely be easy to come by.

It's surely better to give the DBA that option than to remove the choice
entirely.

> If you're willing to also confine the feature to plperlu, then maybe
> the risk level could be decreased from insane to merely unreasonable.

I believe I can arrange for the SPI functions to be disabled during
on_*_init for both plperl and plperlu. Hopefully then the default risk
level will be better than unreasonable :)

Tim.

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


[HACKERS] Reading recovery.conf earlier

2009-12-05 Thread Simon Riggs

I'm planning to read recovery.conf earlier in the startup process, so we
can make a few things more "recovery aware". It's a nice-to-have only.

This won't be part of the HS patch though.

Proposal is to split out the couple of lines in
readRecoveryCommandFile() that set important state and make it read in
an option block that can be used by caller. It would then be called by
both postmaster (earlier in startup) and again later by startup process,
as happens now. I want to do it that way so I can read file before we
create shared memory, so I don't have to worry about passing details via
shared memory itself.

It will allow some tidy up in HS patch but isn't going to be intrusive.

Not thinking about lexers and stuff though at this stage, even if it is
on the todo list.

Any vetos?

-- 
 Simon Riggs   www.2ndQuadrant.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] Hot standby, misc issues

2009-12-05 Thread Simon Riggs
On Fri, 2009-12-04 at 10:23 +0200, Heikki Linnakangas wrote:
> 
> > @Heikki: Why is error checking in KnownAssignedXidsRemove() #ifdef'd
> out?? 
> 
> It's explained in the comment:
> /* XXX: This can still happen: If a transaction with a subtransaction
>  * that haven't been reported yet aborts, and no WAL records have been
>  * written using the subxid, the abort record will contain that subxid
>  * and we haven't seen it before.
>  */

Just realised that this occurs again because the call to
RecordKnownAssignedTransactionIds() was removed from
xact_commit_abort().

I'm guessing you didn't like the call in that place for some reason,
since I smile while I remember it has been removed twice(!) even though
I put "do not remove" comments on it to describe this corner case.

Not going to put it back a third time.

-- 
 Simon Riggs   www.2ndQuadrant.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] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Tom Lane
Andrew Dunstan  writes:
> If we turn Tim's proposal down, I suspect someone will create a fork of 
> plperl that allows it anyway - it's not like it needs anything changed 
> elsewhere in the backend - it would be a drop-in replacement, pretty much.

The question is not about whether we think it's useful; the question
is about whether it's safe.

> I think if we do this the on_perl_init setting should probably be 
> PGC_POSTMASTER, which would remove any issue about it changing 
> underneath us.

Yes, if the main intended usage is in combination with preloading perl
at postmaster start, it would be pointless to imagine that PGC_SIGHUP
is useful anyway.

regards, tom lane

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


Re: [HACKERS] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Andrew Dunstan



Tim Bunce wrote:

That doesn't even begin to cover the problems with allowing any of
this to happen inside the postmaster.  Recall that the postmaster
does not have any database access.  Furthermore, it is a very long
established reliability principle around here that the postmaster
process should do as little as possible, because every thing that it
does creates another opportunity to have a nonrecoverable failure.
The postmaster can recover if a child crashes, but the other way
round, not so much.



I hope the combination of disabling the SPI functions during
initialization, and documenting the risks of combining on_perl_init and
shared_preload_libraries, is sufficient.


  


We already do a lot during library load - plperl's _PG_init() calls 
plperl_init_interp() which sets up an interpreter, runs the boot code, 
loads the Dynaloader and bootstraps the SPI module.


Pre-loading perl libraries in forking servers has well known benefits, 
as Robert Haas noted.


We're not talking about touching the database at all.

If we turn Tim's proposal down, I suspect someone will create a fork of 
plperl that allows it anyway - it's not like it needs anything changed 
elsewhere in the backend - it would be a drop-in replacement, pretty much.


Here's a concrete example of something I was working on just yesterday, 
where it would be useful. One of my clients has a Postgres based 
application that needs to talk to a number of foreign databases, mostly 
SQLServer. In some cases it pulls data from them, in this new case we 
are pushing lots of data at arbitrary times into SQLServer, using 
plperlu with DBI/DBD::Sybase. We would probably get a significant 
performance gain if we could have DBI and DBD::Sybase preloaded. The 
application does use connection pooling, but every so often a function 
call will take significantly longer because it occurs in a new backend 
that is having to reload the libraries.


I think if we do this the on_perl_init setting should probably be 
PGC_POSTMASTER, which would remove any issue about it changing 
underneath us.


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] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Tom Lane
Tim Bunce  writes:
> I'll modify the patch to disable the SPI functions during
> initialization (both on_perl_init and on_(un)trusted_init). 

Yeah, in the shower this morning I was thinking that not loading
SPI till after the on_init code runs would alleviate the concerns
about transactionality and permissions --- that would ensure that
whatever on_init does affects only the Perl world and not the database
world.

However, we're not out of the woods yet.  In a trusted interpreter
(plperl not plperlu), is the on_init code executed before we lock down
the interpreter with Safe?  I would think it has to be since the main
point AFAICS is to let you preload code via "use".  But then what is
left of the security guarantees of plperl?  I can hardly imagine DBAs
wanting to vet a few thousand lines of random Perl code to see if it
contains anything that could be subverted.  For example, the ability
to scribble on database files (like say pg_hba.conf) would almost surely
be easy to come by.

If you're willing to also confine the feature to plperlu, then maybe
the risk level could be decreased from insane to merely unreasonable.

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] PostgreSQL Release Support Policy

2009-12-05 Thread Simon Riggs
On Fri, 2009-12-04 at 16:36 +, Dave Page wrote:

> End Of Life (EOL) dates:
> 
> Version   EOL Date
> PostgreSQL 7.4July 2010 (extended)
> PostgreSQL 8.0July 2010 (extended)
> PostgreSQL 8.1November 2010
> PostgreSQL 8.2December 2011
> PostgreSQL 8.3February 2013
> PostgreSQL 8.4July 2014

Could I request we change these dates slightly to

PostgreSQL 8.1  February 2011
PostgreSQL 8.2  February 2012

with absolutely no intention to extend the support window any worthwhile amount.

That way we have

* A regular pattern of de-release, so everybody is clearer, i.e.
PostgreSQL 8.1 February 2011 
PostgreSQL 8.2 February 2012
PostgreSQL 8.3 February 2013 

* We don't have de-support right at Thanksgiving or Christmas, since
people may do panic-stricken upgrades.

I much prefer February as a time for panic, if that choice is available,
which I realise it may not be.

-- 
 Simon Riggs   www.2ndQuadrant.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] [GENERAL] pg_attribute.attnum - wrong column ordinal?

2009-12-05 Thread Peter Eisentraut
On tor, 2009-12-03 at 10:09 -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Should we recast the attributes and columns views in information_schema?
> > I notice they still use attnum.
> 
> I'd vote against it, at least until we have something better than a
> row_number solution.  That would create another huge performance penalty
> on views that are already ungodly slow.

Should be easy to test the performance impact of this, since the limit
for columns per table is 1600.


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


Re: [HACKERS] First feature patch for plperl - draft [PATCH]

2009-12-05 Thread Tim Bunce
On Sat, Dec 05, 2009 at 01:21:22AM -0500, Tom Lane wrote:
> "David E. Wheeler"  writes:
> > Tom, what's your objection to Shlib load time being user-visible?
> 
> It's not really designed to be user-visible.  Let me give you just
> two examples:
> 
> * We call a plperl function for the first time in a session, causing
> plperl.so to be loaded.  Later the transaction fails and is rolled
> back.  If loading plperl.so caused some user-visible things to happen,
> should those be rolled back?

No. Establishing initial state, no matter how that's triggered, is not
part of a transaction.

> * We call a plperl function for the first time in a session, causing
> plperl.so to be loaded.  This happens in the context of a superuser
> calling a non-superuser security definer function, or perhaps vice
> versa.  Whose permissions apply to whatever the on_load code tries
> to do?  (Hint: every answer is wrong.)

I'll modify the patch to disable the SPI functions during
initialization (both on_perl_init and on_(un)trusted_init). 

Would that address your concerns?

> That doesn't even begin to cover the problems with allowing any of
> this to happen inside the postmaster.  Recall that the postmaster
> does not have any database access.  Furthermore, it is a very long
> established reliability principle around here that the postmaster
> process should do as little as possible, because every thing that it
> does creates another opportunity to have a nonrecoverable failure.
> The postmaster can recover if a child crashes, but the other way
> round, not so much.

I hope the combination of disabling the SPI functions during
initialization, and documenting the risks of combining on_perl_init and
shared_preload_libraries, is sufficient.

Tim.

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


Re: [HACKERS] Adding support for SE-Linux security

2009-12-05 Thread Bruce Momjian
Robert Haas wrote:
> > I offered to review it. ?I was going to mostly review the parts that
> > impacted our existing code, and I wasn't going to be able to do a
> > thorough job of the SE-Linux-specific files.
> 
> Review it and commit it, after making whatever modifications are
> necessary?  Or review it in part, leaving the final review and commit
> to someone else?
> 
> I just read through the latest version of this patch and it does
> appear to be in significantly better shape than the versions I read
> back in July.  So it might not require a Herculean feat of strength to
> get this in, but I still think it's going to be a big job.  There's a
> lot of code here that needs to be verified and in some cases probably
> cleaned up or restructured.  If you're prepared to take it on, I'm not
> going to speak against that, other than to say that I think you have
> your work cut out for you.

This is no harder than many of the other seemingly crazy things I have
done, e.g. Win32 port, client library threading.  If this is a feature
we should have, I will get it done or get others to help me complete the
task.

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

  + If your life is a hard drive, Christ can be your backup. +

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


Re: [HACKERS] Adding support for SE-Linux security

2009-12-05 Thread Robert Haas
On Sat, Dec 5, 2009 at 12:14 AM, Bruce Momjian  wrote:
> Robert Haas wrote:
>> Actually, we tried that already, in a previous iteration of this
>> discussion.  Someone actually materialized and commented on a few
>> things.  The problem, as I remember it, was that they didn't know much
>> about PostgreSQL, so we didn't get very far with it.  Unfortunately, I
>> can't find the relevant email thread at the moment.
>>
>> In fact, we've tried about everything with these patches.  Tom
>> reviewed them, Bruce reviewed them, Peter reviewed them, I reviewed
>> them, Stephen Frost reviewed them, Heikki took at least a brief look
>> at them, and I think there were a few other people, too.  The first
>> person who I can recall being relatively happy with any version of
>> this patch was Stephen Frost, commenting on the access control
>> framework that we suggested KaiGai try to separate from the main body
>> of the patch to break it into more managable chunks.  That patch was
>> summarily rejected by Tom for what I believe were valid reasons.  In
>> other words, in 18 months of trying we've yet to see something that is
>> close to being committable.  Contrast that with Hot Standby, which
>> Heikki made a real shot at committing during the first CommitFest to
>> which it was submitted.
>>
>> I think David Fetter summarized it pretty well here - the rest of the
>> thread is worth reading, too.
>>
>> http://archives.postgresql.org/pgsql-hackers/2009-07/msg01159.php
>>
>> I think the only chance of this ever getting committed is if a
>> committer volunteers to take ownership of it, similar to what Heikki
>> has done for Hot Standby and Streaming Replication.  Right now, we
>> don't have any volunteers, and even if Tom or Heikki were interested,
>> I suspect it would occupy their entire attention for several
>> CommitFests just as HS and SR have done for Heikki.  I suspect the
>> amount of work for SE-PostgreSQL might even be larger than for HS.  If
>> we DON'T have a committer who is willing to own this, then I don't
>> think there's a choice other than giving up.
>
> I offered to review it.  I was going to mostly review the parts that
> impacted our existing code, and I wasn't going to be able to do a
> thorough job of the SE-Linux-specific files.

Review it and commit it, after making whatever modifications are
necessary?  Or review it in part, leaving the final review and commit
to someone else?

I just read through the latest version of this patch and it does
appear to be in significantly better shape than the versions I read
back in July.  So it might not require a Herculean feat of strength to
get this in, but I still think it's going to be a big job.  There's a
lot of code here that needs to be verified and in some cases probably
cleaned up or restructured.  If you're prepared to take it on, I'm not
going to speak against that, other than to say that I think you have
your work cut out for you.

...Robert

-- 
Sent 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 more frame types in window functions (ROWS)

2009-12-05 Thread Andrew Gierth
> "Hitoshi" == Hitoshi Harada  writes:

 Hitoshi> One thing for rule test, I checked existing regression test
 Hitoshi> cases and concluded DROP VIEW is necessary, or even VIEW
 Hitoshi> test for a specific feature is not needed. I remember your
 Hitoshi> aggregate ORDER BY patch contains "rules" test
 Hitoshi> changes. However, since processing order of regression tests
 Hitoshi> is not predictable and may change AFAIK, I guess it
 Hitoshi> shouldn't add those changes in rules.out.

Actually, looking more closely, the way you have it currently works only
by chance - "rules" and "window" are running in parallel, therefore the
view creation in "window" can break the output of "rules".

The order of regression tests is set in parallel_schedule and
serial_schedule; it's unpredictable only for tests within the same
parallel group.

I think a modification of the schedule is needed here; the only other
option would be to move the view creation into a different test.

-- 
Andrew.

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


Re: [HACKERS] add more frame types in window functions (ROWS)

2009-12-05 Thread Hitoshi Harada
2009/12/5 Hitoshi Harada :

> I'm now reworking as reviewed. The last issue is whether we accept
> extension of frame types without RANGE support.

Attached is updated version. I added AggGetMemoryContext() in
executor/nodeAgg.h (though I'm not sure where to go...) and its second
argument "iswindowagg" is output parameter to know whether the call
context is Agg or WindowAgg. Your proposal of APIs to know whether the
function is called as Aggregate or not is also a candidate to be, but
it seems out of this patch scope, so it doesn't touch anything.

Fix tsearch function is also included, as well as typo phisical ->
physical. Pass false to get_rule_expr() of value in
PRECEDING/FOLLOWING.

One thing for rule test, I checked existing regression test cases and
concluded DROP VIEW is necessary, or even VIEW test for a specific
feature is not needed. I remember your aggregate ORDER BY patch
contains "rules" test changes. However, since processing order of
regression tests is not predictable and may change AFAIK, I guess it
shouldn't add those changes in rules.out.


Regards.

-- 
Hitoshi Harada


rows_frame_types.20091205.patch.gz
Description: GNU Zip compressed 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] Summary and Plan for Hot Standby

2009-12-05 Thread Simon Riggs
On Sun, 2009-11-15 at 16:07 +0200, Heikki Linnakangas wrote:

> - The assumption that b-tree vacuum records don't need conflict
> resolution because we did that with the additional cleanup-info record
> works ATM, but it hinges on the fact that we don't delete any tuples
> marked as killed while we do the vacuum. That seems like a low-hanging
> fruit that I'd actually like to do now that I spotted it, but will then
> need to fix b-tree vacuum records accordingly. 

You didn't make a change, so I wonder whether you realised no change was
required or that you still think change is necessary, but have left it
to me? Not sure.

I've investigated this but can't see any problem or need for change.

btvacuumpage() is very specific about deleting *only* index tuples that
have been collected during the VACUUM's heap scan, as identified by the
heap callback function, lazy_tid_reaped().

There is no reliance at all on the state of the index tuple. If you
ain't on the list, you ain't cleaned. I accept your observation that
some additional index tuples may be marked as killed by backends
accessing the table that is being vacuumed, since those backends could
have a RecentGlobalXmin later than the OldestXmin used by the VACUUM as
a result of the change that means GetSnapshotData() ignores lazy
vacuums. But those tuples will not be identified by the callback
function and so the "additionally killed" index tuples will not be
removed.

It is a possible future optimisation of b-tree vacuum that it cleans
these additional killed tuples while it executes, but it doesn't do it
now and so we need not worry about that for HS.

I think its important that we note this assumption though.

Comment?

-- 
 Simon Riggs   www.2ndQuadrant.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 more frame types in window functions (ROWS)

2009-12-05 Thread Hitoshi Harada
2009/12/5 Andrew Gierth :

> 1) Memory context handling for aggregate calls
>
>    aggcontext = AggGetMemoryContext(fcinfo->context);
>    if (!aggcontext)
>        ereport(...);
>
> For completeness, there should be two other functions: one to simply
> return whether we are in fact being called as an aggregate, and another
> one to return whether it's safe to scribble on the first argument
> (while it's currently the case that these two are equivalent, it would
> be good not to assume that).
>
> Comments? This is the most significant issue as I see it.

Yep, I agree. The most essential point on this is that external
functions refer to the struct members directly; we should provide
kinds of API.

> (Also, a function in contrib/tsearch2 that accesses wincontext wasn't
> updated by the patch.)

Thanks for noticing. I didn't know it.

> 2) Keywords

The discussion is:

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

and ideas are extracted from Tom's mail in the last year just before
committing the first window function patch, except for changing to
COL_NAME_KEYWORD rather than RESERVED_KEYWORD as suggested. The reason
I picked it up was only that it works. I cannot tell the side effect
of COL_NAME_KEYWORD but I guess we tend to avoid RESERVED_KEYWORD as
far as possible.

And the reason BETWEEN cannot work as function name any more is based
on bison's output shift/reduce conflict when SCONST follows BETWEEN in
frame_extent. I don't have clear example that makes this happen,
though.


> 3) Regression tests
>
> Testing that views work is OK as far as it goes, but I think that view
> definition should be left in place rather than dropped (possibly with
> even more variants) so that the deparse code gets properly tested too.
> (see the "rules" test)

OK,

> 4) Deparse output
>
> The code is forcing explicit casting on the offset expressions, i.e.
> the deparsed code looks like
>
>  ... ROWS BETWEEN 1::bigint PRECEDING AND 1::bigint FOLLOWING ...
>
> This looks a bit ugly; is it avoidable? At least for ROWS it should
> be, I think, since the type is known; even for RANGE, the type would
> be determined by the sort column.

Hmm, I'll change it as LIMIT clause does. Pass false as showimplicit
to get_rule_expr() maybe?

> 5) Documentation issues
>
> The entry for BETWEEN in the keywords appendix isn't updated.
> (Wouldn't it make more sense for this to be generated from the keyword
> list somehow?)

I heard that you don't need to send keywords appendix in the patch
because it is auto-generated, if I remember correctly.

>
> Spelling:
> -     current row. In ROWS mode this value means phisical row
> +     current row. In ROWS mode this value means physical row
> (this error appears twice)

Oops, thanks.

I'm now reworking as reviewed. The last issue is whether we accept
extension of frame types without RANGE support.

Regards,


-- 
Hitoshi Harada

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


Re: [HACKERS] [PATCH 4/4] Add tests to dblink covering use of COPY TO FUNCTION

2009-12-05 Thread Daniel Farina
On Mon, Nov 30, 2009 at 12:14 PM, Greg Smith  wrote:
> Jeff Davis wrote:
>
> COPY target FROM FUNCTION foo() WITH RECORDS;
>
>
> In what format would the records be?

As a not-quite aside, I think I have a better syntax suggestion.  I
have recently been digging around in the grammar with the changes made
in the following commit:

commit a6833fbb85cb5212a9d8085849e7281807f732a6
Author: Tom Lane 
Date:   Mon Sep 21 20:10:21 2009 +

Define a new, more extensible syntax for COPY options.

This is intentionally similar to the recently revised syntax for EXPLAIN
options, ie, (name value, ...).  The old syntax is still supported for
backwards compatibility, but we intend that any options added in future
will be provided only in the new syntax.

Robert Haas, Emmanuel Cecchet

As it turns out, the following syntax may work pretty well:

  COPY y TO FUNCTION (setup_function the_setup_function('some arg', 3, 7, 42))

Basically the func_expr reduction fits very neatly into the
copy_generic_opt_elem reduction:

copy_generic_opt_elem:
ColLabel copy_generic_opt_arg
{
$$ = (Node *) makeDefElem($1, $2);
}
| ColLabel func_expr
{
$$ = (Node *) $2;
}
;

Now we can use more or less any reasonable number of symbol names and
function calls we desire.  This makes life considerably easier, I
think...

We can also try to refactor COPY's internals to take advantage of
these features (and potentially reduce the number of mechanisms.  For
example, the legacy "COPY ... TO '/place' WITH CSV" perhaps can be
more verbosely/generically expressed as:

  COPY ... TO FUNCTION (setup_function to_file('/place'),
record_converter csv_formatter,
stream_function fwrite
end_function fclose);

We can also add auxiliary symbols for error handling behavior.  For
example, were the COPY to fail for some reason maybe it would make
sense "on_error" to call "unlink" to clean up the partially finished
file.

I also have what I think is a somewhat interesting hack.  Consider
some of the functions up there without arguments (presumably they'd be
called with a somewhat fixed contract the mechanics of COPY itself):
how does one disambiguate them?  Ideally, one could sometimes use
literal arguments (when the return value of that function is desired
to be threaded through the other specified functions) and other times
it'd be nice to disambiguate functions via type names.  That would
look something like the following:

  COPY ... TO FUNCTION (setup_function to_file('/place'),
record_converter csv_formatter(record),
stream_function fwrite(bytea),
end_function fclose(text));

I think this is possible to implement without much ambiguity, drawing
on the observation that the COPY statement does not have -- and
probably will never have -- references via Var(iable) node, unlike
normal SQL statements such as SELECT, INSERT, et al.  That means we
might be able disambiguate using the following rules when scanning the
funcexpr's arguments during the semantic analysis phase to figure out
what to do:

  * Only literal list items found: it's a function call with the types
of those literals.  Ex: my_setup_function('foo'::text, 3)

  * Only non-literal list items found: it's type specifiers.  Ex:
csv_formatter(record).

  * Both literal and non-literal values found: report an error.

This only works because no cases where a non-literal quantity could be
confused with a type name come to mind.  If one could name a type "3"
and being forced to double-quote "3" to get your type disambiguated
was just too ugly, then we are at an impasse.  But otherwise I think
this may work quite well.

Common constellations of functions could perhaps be bound together
into a DDL to reduce the amount of symbol soup going on here, but that
seems like a pretty clean transition strategy at some later time.
Most of the functionality could still be captured with this simple
approach for now...

Also note that factoring out high-performance implementations of
things like csv_formatter (and friends: pg_binary_formatter) will
probably take some time, but ultimately I think all the existing
functionality could be realized as a layer of syntactic sugar over
this mechanism.

fdr

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