Re: [HACKERS] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Heikki Linnakangas

On 01.11.2010 05:21, Robert Haas wrote:

There seem to be two cases in the code that can generate that error.
One, attempting to open the file returns ENOENT.  Two, after the data
has been read, the last-removed position returned by
XLogGetLastRemoved precedes the data we think we just read, implying
that it was overwritten while we were in the process of reading it.
Does your installation have debugging symbols?  Can you figure out
which case is triggering (inside XLogRead) and what the values of log,
seg, lastRemovedLog, and lastRemovedSeg are?


An easy way to find out which ereport() it is is to set 
log_error_verbosity='verbose' and re-run the test. You then get the 
file and line number of the ereport in the log.


--
  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] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Heikki Linnakangas

On 31.10.2010 23:31, Greg Smith wrote:

LOG: replication connection authorized: user=rep host=127.0.0.1 port=52571
FATAL: requested WAL segment 0001 has already been
removed

Which is confusing because that file is certainly on the master still,
and hasn't even been considered archived yet much less removed:

[mas...@pyramid pg_log]$ ls -l $PGDATA/pg_xlog
-rw--- 1 master master 16777216 Oct 31 16:29 0001
drwx-- 2 master master 4096 Oct 4 12:28 archive_status
[mas...@pyramid pg_log]$ ls -l $PGDATA/pg_xlog/archive_status/
total 0

So why isn't SR handing that data over? Is there some weird unhandled
corner case this exposes, but that wasn't encountered by the systems the
tutorial was tried out on?


Yes, indeed there is a corner-case bug when you try to stream the very 
first WAL segment, with log==seg==0. We keep track of the last removed 
WAL segment, and before a piece of WAL is sent to the standby, walsender 
checks that the requested WAL segment is  the last removed. Before any 
WAL segments have been removed since postmaster startup, the latest 
removed segment is initialized to 0/0, with the idea that 0/0 precedes 
any valid WAL segment. That's clearly not true though, it does not 
precede the very first WAL segment after initdb, 0/0.


Seems that we need to change the meaning of the last removed WAL segment 
to avoid the ambiguity of 0/0. Let's store the (last removed)+1 in the 
global variable instead.


--
  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] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Heikki Linnakangas

On 01.11.2010 09:37, Heikki Linnakangas wrote:

On 31.10.2010 23:31, Greg Smith wrote:

LOG: replication connection authorized: user=rep host=127.0.0.1
port=52571
FATAL: requested WAL segment 0001 has already been
removed

Which is confusing because that file is certainly on the master still,
and hasn't even been considered archived yet much less removed:

[mas...@pyramid pg_log]$ ls -l $PGDATA/pg_xlog
-rw--- 1 master master 16777216 Oct 31 16:29 0001
drwx-- 2 master master 4096 Oct 4 12:28 archive_status
[mas...@pyramid pg_log]$ ls -l $PGDATA/pg_xlog/archive_status/
total 0

So why isn't SR handing that data over? Is there some weird unhandled
corner case this exposes, but that wasn't encountered by the systems the
tutorial was tried out on?


Yes, indeed there is a corner-case bug when you try to stream the very
first WAL segment, with log==seg==0. We keep track of the last removed
WAL segment, and before a piece of WAL is sent to the standby, walsender
checks that the requested WAL segment is  the last removed. Before any
WAL segments have been removed since postmaster startup, the latest
removed segment is initialized to 0/0, with the idea that 0/0 precedes
any valid WAL segment. That's clearly not true though, it does not
precede the very first WAL segment after initdb, 0/0.

Seems that we need to change the meaning of the last removed WAL segment
to avoid the ambiguity of 0/0. Let's store the (last removed)+1 in the
global variable instead.


Committed that. Thanks for the report, both of you. I'm not subscribed 
to pgsql-admin which is why I didn't see Matt's original report.


--
  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] [PATCH] Custom code int(32|64) = text conversions out of performance reasons

2010-11-01 Thread Andres Freund
On Monday 01 November 2010 04:04:51 Itagaki Takahiro wrote:
 On Mon, Nov 1, 2010 at 6:41 AM, Andres Freund and...@anarazel.de wrote:
  While looking at binary COPY performance I forgot to add BINARY and was a
  bit shocked to see printf that high in the profile...
  
  A change from 9192.476ms 5309.928ms seems to be pretty good indication
  that a change in that area is waranted given integer columns are quite
  ubiquous...
 
 Good optimization. Here is the result on my machine:
 * before: 13057.190 ms, 12429.092 ms, 12622.374 ms
 * after: 8261.688 ms, 8427.024 ms, 8622.370 ms
Thanks.

  * I renamed pg_[il]toa to pg_s(16|32|64)toa - I found the names
  confusing. Not sure if its worth it.
 
 Agreed, but how about pg_i(16|32|64)toa? 'i' might be more popular than
 's'. See also
 http://msdn.microsoft.com/en-US/library/yakksftt(VS.100).aspx
I find itoa not as clear about signedness as stoa, but if you insist, I dont 
feel strongly about it.

 I have a couple of questions and comments:
 
 * Why did you change MAXINT8LEN + 1 to + 2 ?
   Are there possibility of buffer overflow in the current code?
 @@ -158,12 +159,9 @@ int8out(PG_FUNCTION_ARGS)
 - charbuf[MAXINT8LEN + 1];
 + charbuf[MAXINT8LEN + 2];
Argh. That should have never gotten into the patch. I was playing around with 
another optimization which would have needed more buffer space (but was quite a 
 
bit slower).

 * The buffer reordering seems a bit messy.
 //have to reorder the string, but not 0byte.
 I'd suggest to fill a fixed-size local buffer from right to left
 and copy it to the actual output.
Hm. 

while(bufstart  buf){
char swap = *bufstart;
*bufstart++ = *buf;
*buf-- = swap;
}

Is a bit cleaner maybe, but I dont see much point in putting it into its own 
function... But again, I don't feel strongly.


 * C++-style comments should be cleaned up.
Will clean up.

Andres

-- 
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] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Fujii Masao
On Mon, Nov 1, 2010 at 5:17 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 Committed that. Thanks for the report, both of you. I'm not subscribed to
 pgsql-admin which is why I didn't see Matt's original report.

Thanks!

Regards,

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

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


[HACKERS] Complier warnings on mingw gcc 4.5.0

2010-11-01 Thread Itagaki Takahiro
I compiled the source with mingw gcc 4.5.0, that has been released recently.
The compile was succeeded and worked well at least for simple queries,
but there were many warnings during the compile.

1. warning: 'symbol' redeclared without dllimport attribute:
previous dllimport ignored
2. warning: unknown conversion type character 'm' in format
3. warning: unknown conversion type character 'l' in format


1 is easy to fix with the attached patch.
I wonder why mingw gcc  4.5 can build codes without the fix...

For 2, we could remove __attribute__((format(printf))) on mingw, but
it also disables type checking for formatters. Are there better solutions?

I have not yet researched for 3, that might be most dangerous.

=# select version();
 version
--
 PostgreSQL 9.0.1 on i686-pc-mingw32, compiled by GCC gcc.exe (GCC)
4.5.0, 32-bit
(1 row)

OS: Windows 7 64bit


diff --git a/src/include/port/win32.h b/src/include/port/win32.h
index 988c1c9..31c877f 100644
*** a/src/include/port/win32.h
--- b/src/include/port/win32.h
***
*** 58,64 
  #define PGDLLIMPORT __declspec (dllimport)
  #endif

! #ifdef _MSC_VER
  #define PGDLLEXPORT __declspec (dllexport)
  #else
  #define PGDLLEXPORT __declspec (dllimport)
--- 58,64 
  #define PGDLLIMPORT __declspec (dllimport)
  #endif

! #if defined(_MSC_VER) || __GNUC__ = 4
  #define PGDLLEXPORT __declspec (dllexport)
  #else
  #define PGDLLEXPORT __declspec (dllimport)

-- 
Itagaki Takahiro

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


Re: [HACKERS] Tracking latest timeline in standby mode

2010-11-01 Thread Fujii Masao
On Wed, Oct 27, 2010 at 11:42 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 At the moment, when you specify recovery_target_timeline='latest', we scan
 for the latest timeline at the beginning of recovery, and pick that as the
 target. If new timelines appear during recovery, we stick to the target
 chosen in the beginning, the new timelines are ignored. That's undesirable
 if you have one master and two standby servers, and failover happens to one
 of the standbys. The other standby won't automatically start tracking the
 new TLI created by the promoted new master, it requires a restart to notice.

 This was discussed a while ago:
 http://archives.postgresql.org/pgsql-hackers/2010-10/msg00620.php

 More work needs to be done to make that work over streaming replication,
 sending history files over the wire, for example, but let's take baby steps.
 At the very minimum the startup process should notice new timelines
 appearing in the archive. The attached patch does that.

 Comments?

Currently the startup process rescans the timeline history file only
when walreceiver
is not in progress. But, if walreceiver receives that file from the
master in the future,
the startup process should rescan them even while walreceiver is in progress?

 A related issue is that we should have a check for the issue I also
 mentioned in the comments:

        /*
         * If the current timeline is not part of the history of the
         * new timeline, we cannot proceed to it.
         *
         * XXX This isn't foolproof: The new timeline might have forked
 from
         * the current one, but before the current recovery location. In
 that
         * case we will still switch to the new timeline and proceed
 replaying
         * from it even though the history doesn't match what we already
         * replayed. That's not good. We will likely notice at the next
 online
         * checkpoint, as the TLI won't match what we expected, but it's
         * not guaranteed. The admin needs to make sure that doesn't
 happen.
         */

 but that's a pre-existing and orthogonal issue, it can with the current code
 too if you restart the standby, so let's handle that as a separate patch.

I'm thinking to write the timeline switch LSN to the timeline history file, and
compare LSN with the location of the last applied WAL record when that
file is rescaned. If the timeline switch LSN is ahead, we cannot do the switch.

Currently the timeline history file contains the timeline switch WAL filename,
but it's not used at all. As a first step, what about replacing that
filename with
the switch LSN?

+   /* Switch target */
+   recoveryTargetTLI = newtarget;
+   expectedTLIs = newExpectedTLIs;

Before expectedTLIs = newExpectedTLIs, we should call
list_free_deep(expectedTLIs)?

Regards,

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

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


Re: [HACKERS] Tracking latest timeline in standby mode

2010-11-01 Thread Heikki Linnakangas

On 01.11.2010 12:32, Fujii Masao wrote:

A related issue is that we should have a check for the issue I also
mentioned in the comments:


/*
 * If the current timeline is not part of the history of the
 * new timeline, we cannot proceed to it.
 *
 * XXX This isn't foolproof: The new timeline might have forked
from
 * the current one, but before the current recovery location. In
that
 * case we will still switch to the new timeline and proceed
replaying
 * from it even though the history doesn't match what we already
 * replayed. That's not good. We will likely notice at the next
online
 * checkpoint, as the TLI won't match what we expected, but it's
 * not guaranteed. The admin needs to make sure that doesn't
happen.
 */


but that's a pre-existing and orthogonal issue, it can with the current code
too if you restart the standby, so let's handle that as a separate patch.


I'm thinking to write the timeline switch LSN to the timeline history file, and
compare LSN with the location of the last applied WAL record when that
file is rescaned. If the timeline switch LSN is ahead, we cannot do the switch.


Yeah, that's one approach. Another is to validate the TLI in the xlog 
page header, it should always match the current timeline we're on. That 
would feel more robust to me.


We're a bit fuzzy about what TLI is written in the page header when the 
timeline changing checkpoint record is written, though. If the 
checkpoint record fits in the previous page, the page will carry the old 
TLI, but if the checkpoint record begins a new WAL page, the new page is 
initialized with the new TLI. I think we should rearrange that so that 
the page header will always carry the old TLI.




+   /* Switch target */
+   recoveryTargetTLI = newtarget;
+   expectedTLIs = newExpectedTLIs;

Before expectedTLIs = newExpectedTLIs, we should call
list_free_deep(expectedTLIs)?


It's an integer list so list_free(expectedTLIs) is enough, and I doubt 
that leakage will ever be a problem in practice, but in principle you're 
right.


--
  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] plan time of MASSIVE partitioning ...

2010-11-01 Thread Tom Lane
I wrote:
 samples  %symbol name
 447433   47.1553  get_tabstat_entry
 185458   19.5456  find_all_inheritors
 53064 5.5925  SearchCatCache
 33864 3.5690  pg_strtok

 get_tabstat_entry and find_all_inheritors are both obviously O(N^2) in
 the number of tables they have to deal with.  However, the constant
 factors are small enough that you need a heck of a lot of tables
 before they become significant consumers of runtime.  I'm not convinced
 that we should be optimizing for 9000-child-table cases.

 It'd be worth fixing these if we can do it without either introducing a
 lot of complexity, or slowing things down for typical cases that have
 only a few tables.  Offhand not sure about how to do either.

I had a thought about how to make get_tabstat_entry() faster without
adding overhead: what if we just plain remove the search, and always
assume that a new entry has to be added to the tabstat array?

The existing code seems to be designed to make no assumptions about
how it's being used, but that's a bit silly.  We know that the links are
coming from the relcache, which will have only one entry per relation,
and that the relcache is designed to hang onto the links for (at least)
the life of a transaction.  So rather than optimizing for the case where
the relcache fails to remember the tabstat link, maybe we should
optimize for the case where it does remember.

The worst-case consequence AFAICS would be multiple tabstat entries for
the same relation, which seems pretty noncritical anyway.

Thoughts?

regards, tom lane

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


Re: [HACKERS] Maximum function call nesting depth for regression tests

2010-11-01 Thread Tom Lane
I wrote:
 I haven't looked to see if any of these have an excessive amount of
 local variables.

I poked through the call stack and found that the only function in
this nest that seems to have a large amount of local variables is
ExecMakeFunctionResult().  The space hog there is the local
FunctionCallInfoData struct, which requires ~500 bytes on a 32-bit
machine and ~900 bytes on a 64-bit one.  Now the interesting thing
about that is that we *also* keep a FunctionCallInfoData struct in
the FuncExprState.  The reason for this redundancy is stated to be:

/*
 * For non-set-returning functions, we just use a local-variable
 * FunctionCallInfoData.  For set-returning functions we keep the callinfo
 * record in fcache-setArgs so that it can survive across multiple
 * value-per-call invocations.  (The reason we don't just do the latter
 * all the time is that plpgsql expects to be able to use simple
 * expression trees re-entrantly.  Which might not be a good idea, but the
 * penalty for not doing so is high.)
 */

AFAICS this argument is no longer valid given the changes we made last
week to avoid collisions due to reuse of fcinfo-flinfo-fn_extra.
I'm pretty strongly tempted to get rid of the local-variable
FunctionCallInfoData and just use the one in the FuncExprState always.
That would simplify and marginally speed up the function-call code,
which seems surely worth doing regardless of any savings in stack
depth.

I'm not sure that this would save enough stack space to make the
buildfarm critters happy, but it might.  However, I wouldn't care
to risk changing this in the back branches, so we'd still need some
other plan for fixing the buildfarm failures.

Any objections?

regards, tom lane

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


Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Tom Lane
Andrew Dunstan and...@dunslane.net writes:
 On 10/31/2010 04:40 PM, Alex Hunsaker wrote:
 which happens because prodesc-result_in_func.fn_addr (flinfo) is
 NULL.  That happens because when we are a trigger we don't setup
 input/output conversion.  And with the change we get the same
 proc_desc for triggers and non triggers, so if the trigger function
 gets called first, any call to the direct function will use the same
 proc_desc with the wrong input/out conversion.

 How does that happen given that the function Oid is part of the hash key?

I think the crash is dependent on the fact that the function is created
and called in the same session.  That means the validator gets called on
it first, and the validator not unreasonably assumes istrigger = true,
and then it calls compile_plperl_function which sets up a cache entry
on that basis.  So then when the regular call comes along, it tries
to reuse the cache entry in the other style.  Kaboom.

 There is a check so that trigger functions can not be called as plain
 functions, but it only gets called when we do not have an entry in
 plperl_proc_hash.  I think just moving that up, something the like the
 attached should be enough.

 This looks like the right fix, though.

No, that is just moving a test that only needs to be done once into a
place where it has to be done every time.  You might as well argue that
we shouldn't cache any of the setup but just redo it all every time.

The fundamental issue here is that the contents of plperl_proc_desc
structs are different between the trigger and non-trigger cases.
Unless you're prepared to make them the same, and guarantee that they
always will be the same in future, I think that including the istrigger
flag in the hash key is a good safety feature.  It's also the same way
that the other three PLs do things, and I see no good excuse for plperl
to do things differently here.

IOW, it's not broke, let's not fix 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] [PATCH] More Coccinelli cleanups

2010-11-01 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Fri, Oct 29, 2010 at 7:33 PM, Marti Raudsepp ma...@juffo.org wrote:
 patch 0001 turns (a - b == 0) into (a == b) and similarly with !=
 patch 0002 applies the same to operators , =, , =
 
 I'm well aware that there's a point where code cleanups defeat their
 purpose and become a burden. So this will probably be my last one,
 I'll go to doing productive things instead. :)

 I like the 0002 patch better than the 0001 patch.

I'm not really thrilled with either of them, as I don't think they are
doing much to improve readability.  As for 0002, as you say, at least
the change in bgwriter.c is actively breaking things.  I'm not eager to
go through and see which of the other changes there might be affecting
overflow or signed-vs-unsigned comparison behavior.

regards, tom lane

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


Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Andrew Dunstan



On 11/01/2010 11:28 AM, Tom Lane wrote:

  The fundamental issue here is that the contents of plperl_proc_desc
structs are different between the trigger and non-trigger cases.
Unless you're prepared to make them the same, and guarantee that they
always will be the same in future, I think that including the istrigger
flag in the hash key is a good safety feature.  It's also the same way
that the other three PLs do things, and I see no good excuse for plperl
to do things differently here.

IOW, it's not broke, let's not fix it.


Ok, then let's make a note in the code to this effect. When the question 
was asked before about why it was there nobody seemed to have any good 
answer.


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] crash in plancache with subtransactions

2010-11-01 Thread Jim Nasby
On Oct 29, 2010, at 10:54 AM, Tom Lane wrote:
 Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Tom Lane's message of mié oct 27 18:18:06 -0300 2010:
 I spent quite a bit of time trying to deal with the memory-leakage
 problem without adding still more bookkeeping overhead.  It wasn't
 looking good, and then I had a sudden insight: if we see that the in-use
 flag is set, we can simply return FALSE from exec_eval_simple_expr.
 
 I tried the original test cases that were handed to me (quite different
 from what I submitted here) and they are fixed also.  Thanks.
 
 It'd be interesting to know if there's any noticeable slowdown on
 affected real-world cases.  (Of course, if they invariably crashed
 before, there might not be a way to measure their previous speed...)

I should be able to get Alvaro something he can use to test the performance. 
Our patch framework uses a recursive function to follow patch dependencies (of 
course that can go away in 8.4 thanks to WITH). I know we've got some other 
recursive calls but I don't think any are critical (it'd be nice if there was a 
way to find out if a function was recursive, I guess theoretically that could 
be discovered during compilation but I don't know how hairy it would be).

One question: What happens if you have multiple paths to the same function 
within another function? For example, we have an assert function that's used 
all over the place; it will definitely be called from multiple places in a call 
stack.

FWIW, I definitely run into cases where recursion makes for cleaner code than 
looping, so it'd be great to avoid making it slower than it needs to be. But 
I've always assumed that recursion is slower than looping so I avoid it for 
anything I know could be performance sensitive.

(looking at original case)... the original bug wasn't actually recursive. It's 
not clear to me how it actually got into this case. The original error report 
is:

psql:sql/code.lookup_table_dynamic.sql:23: ERROR:  buffer 2682 is not owned by 
resource owner Portal
CONTEXT:  SQL function table_schema_and_name statement 1
SQL function table_full_name statement 1
PL/pgSQL function getsert line 9 during statement block local variable 
initialization
server closed the connection unexpectedly
  This probably means the server terminated abnormally
  before or while processing the request.

Line 23 is:

SELECT code.getsert( 'test.stuffs', 'stuff' );

The functions are below. The duplicity of full_name_table and table_full_name 
is because the function was originally called full_name_table, but I decided to 
rename it after creating other table functions. In any case, I don't see any 
obvious recursion or re-entry, unless perhaps tools.table_schema_and_name ends 
up getting called twice by tools.table_full_name?

-[ RECORD 1 
]---+-
Schema  | code
Name| getsert
Result data type| void
Argument data types | p_table_name text, p_lookup text
Volatility  | volatile
Owner   | cnuadmin
Language| plpgsql
Source code | 
: DECLARE
: v_object_class text := 'getsert';
: v_function_name text := p_table_name || '__' || 
v_object_class;
: 
: v_table_full text := tools.full_name_table( 
p_table_name );
: v_schema text;
: v_table text;
: 
: BEGIN
: SELECT INTO v_schema, v_table * FROM 
tools.split_schema( v_table_full );
: 
: PERFORM code_internal.create_object( v_function_name, 
'FUNCTION', v_object_class, array[ ['schema', v_schema], ['table', v_table], 
['lookup', p_lookup] ] );
: END;
: 
Description | Creates a function that will lookup an ID based on a text 
lookup value (p_lookup). If no record exists, one will be created.
: 
: Parameters:
: p_table_name Name of the table to lookup the value in
: p_lookup Name of the field to use for the lookup value
: 
: Results:
: Creates function %p_table_name%__getsert( %p_lookup% with 
a type matching the p_lookup field in p_table_name ). The function returns an 
ID as an int.
: Revokes all on the function from public and grants 
execute to cnuapp_role.
: 

test...@workbook.local=# \df+ tools.full_name_table 
List of functions
-[ RECORD 1 ]---+---
Schema  | tools
Name| full_name_table
Result data type| text
Argument data types | 

Re: [HACKERS] Patch to add a primary key using an existing index

2010-11-01 Thread Jim Nasby
UNIQUE constraints suffer from the same behavior; feel like fixing that too? :)

On Oct 9, 2010, at 1:07 PM, Gurjeet Singh wrote:

 This is a continuation from this thread: 
 http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php
 
 The attached patch allows creating a primary key using an existing index.
 
 This capability would be helpful in situations where one wishes to 
 rebuild/reindex the primary key, but associated downtime is not desirable. It 
 also allows one to create a table and start using it, while creating a unique 
 index 'concurrently' and later adding the primary key using the concurrently 
 built index. Maybe pg_dump can also use it.
 
 The command syntax is:
 
 ALTER TABLE sometable ADD PRIMARY KEY( col1, col2 ) WITH ( INDEX = 
 'indexname' );
 
 A typical use case:
 
 CREATE INDEX CONCURRENTLY new_pkey_idx ON sometable( a, b );
 
 ALTER TABLE sometable ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
 
 - OR -
 
 ALTER TABLE sometable DROP CONSTRAINT sometable_pkey,
   ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
 
 
 Notes for the reviewers:
 
 
 Don't be scared by the size of changes to index.c :) These are mostly 
 indentation diffs. I have attached two versions of the patch: one is context 
 diff, and the other is the same except ignoring whitespace changes.
 
 The pseudocode is as follows:
 
 In ATExecAddIndex()
 If this ALTER command specifies a PRIMARY KEY
   Call get_pkey_index_oid() to perform checks.
 
 In get_pkey_index_oid()
 Look for the WITH INDEX option
 Reject
 if more than one WITH INDEX clause specified
 if the index doesn't exist or not found in table's schema
 if the index is associated with any CONSTRAINT
 if index is not ready or not valid (CONCURRENT buiild? Canceled 
 CONCURRENT?)
 if index is on some other table
 if index is not unique
 if index is an expression index
 if index is a partial index
 if index columns do not match the PRIMARY KEY clause in the command
 if index is not B-tree
 If PRIMARY KEY clause doesn't have a constraint name, assign it one. 
 (code comments explain why)
 Rename the index to match constraint name in the PRIMARY KEY clause
 
 Back in ATExecAddIndex()
 Use the index OID returned by get_pkey_index_oid() to tell DefineIndex() 
 to not create index.
 Now mark the index as having 'indisprimary' flag.
 
 In DefineIndex() and index_create() APIs
 pass an additional flag: index_exists
 Skip various actions based on this flag.
 
 
 The patch contains a few tests, and doesn't yet have a docs patch.
 
 The development branch is at 
 http://github.com/gurjeet/postgres/tree/replace_pkey_index
 
 Regards,
 -- 
 gurjeet.singh
 @ EnterpriseDB - The Enterprise Postgres Company
 http://www.EnterpriseDB.com
 
 singh.gurj...@{ gmail | yahoo }.com
 Twitter/Skype: singh_gurjeet
 
 Mail sent from my BlackLaptop device
 add_pkey_with_index.patchadd_pkey_with_index.ignore_ws.patch
 -- 
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers

--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net



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


Re: [HACKERS] revision of todo: NULL for ROW variables

2010-11-01 Thread Jim Nasby
On Oct 28, 2010, at 11:41 AM, Merlin Moncure wrote:
 On Thu, Oct 28, 2010 at 10:15 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Pavel Stehule pavel.steh...@gmail.com writes:
 I am checking PLpgSQL ToDo topics, and I am not sure if this topic
 isn't done. And if not, then I would to get some detail.
 
 I think that thread petered out because we didn't have consensus on
 what the behavior ought to be.  It goes back to whether there is
 supposed to be a difference between NULL and ROW(NULL,NULL,NULL,...)
 
 I think somewhere along the line it was noticed that SQL says you are
 supposed to treat (null, null) as null and the behavior of 'is null'
 operator was changed to reflect this while other null influenced
 behaviors were left intact (for example, coalesce()).
 
 My take on this is that we are stuck with the status quo.  If a change
 must be done, the 'is null' change should be reverted to un-standard
 behavior.  The SQL standard position on this issue is, IMNSHO, on
 mars.

As someone who's wanted this... what if we had a dedicated function to tell you 
if a row variable had been defined? I definitely don't like the though of 
creating something that effectively duplicates IS NULL, but I'd much rather 
that than continue not having the ability to tell if a row/record variable has 
been set or not.
--
Jim C. Nasby, Database Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net



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


Re: [HACKERS] crash in plancache with subtransactions

2010-11-01 Thread Tom Lane
Jim Nasby j...@nasby.net writes:
 (looking at original case)... the original bug wasn't actually
 recursive.

No, there are two different cases being dealt with here.  If the first
call of an expression results in an error, and then we come back and try
to re-use the expression state tree, we can have trouble because the
state tree contains partially-updated internal state for the called
function.  This doesn't require any recursion but it leads to pretty
much the same problems as the recursive 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] revision of todo: NULL for ROW variables

2010-11-01 Thread Jeff Davis
On Mon, 2010-11-01 at 09:44 -0500, Jim Nasby wrote:
  My take on this is that we are stuck with the status quo.  If a
 change
  must be done, the 'is null' change should be reverted to un-standard
  behavior.  The SQL standard position on this issue is, IMNSHO, on
  mars.
 
 As someone who's wanted this... what if we had a dedicated function to
 tell you if a row variable had been defined? I definitely don't like
 the though of creating something that effectively duplicates IS NULL,
 but I'd much rather that than continue not having the ability to tell
 if a row/record variable has been set or not.

If we just invent a couple more variants of NULL, it will solve all our
problems ;)

Seriously though, I think that we should stick as closely to the letter
of the standard as possible here (or, if there is ambiguity, pick one
reasonable interpretation). NULL semantics are confusing enough without
everyone making their own subtle tweaks.

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] revision of todo: NULL for ROW variables

2010-11-01 Thread Kevin Grittner
Jeff Davis pg...@j-davis.com wrote:
 
 Seriously though, I think that we should stick as closely to the
 letter of the standard as possible here (or, if there is
 ambiguity, pick one reasonable interpretation). NULL semantics are
 confusing enough without everyone making their own subtle tweaks.
 
+1
 
If the standard behavior doesn't support all the functionality we
need, we should be looking at PostgreSQL extensions which do not
conflict with standard syntax.  Supporting standard syntax with
different semantics is evil.
 
-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] Range Types, discrete and/or continuous

2010-11-01 Thread Dimitri Fontaine
Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Would you be comfortable writing that '012[3-5]' range as
 '[0123, 0126)' or something similar?  What benefits do you see to
 using a range for prefixes versus a regular expression?

Your proposed syntax would do fine, sure. Something like this is even on
the TODO list for prefix indexing, but for the internal representation,
as I think there might be some optimisation potential there. Meanwhile,
it would be easy enough to accept alternative input syntax.

I don't see what benefits I'd get from regexp alike input syntax as all
we need to support is 'foo.*', or if you prefer LIKE syntax, 'foo%'.

Now please remind that the literal is a full phone number, and the table
has the prefixes. So the table would have regular expressions and the
indexing would be about optimising searches of which regexp best fits
the input literal. It seems to me the idea of a range makes it easier
here.

Oh, and there's a meaningful overlap notion too, even if depending on
the application you can't enforce non-overlapping ranges (a carrier
might own almost all the '01234' prefix, but another one owns the
'012345' prefix). It gets very funny if you include the country code in
the prefix, too, as they run from one to 5 digits according to

  http://en.wikipedia.org/wiki/List_of_country_calling_codes

Still, we're talking about continuous ranges I think, because there's no
way to count the elements that fits in any given prefix_range.

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

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


Re: [HACKERS] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Greg Smith

Heikki Linnakangas wrote:
Yes, indeed there is a corner-case bug when you try to stream the very 
first WAL segment, with log==seg==0.


I confirmed that the bug exists in only this case by taking my problem 
install and doing this:


psql -d postgres -c checkpoint; select pg_switch_xlog();

To force it to the next xlog file.  With only that change, everything 
else then works.  So we'll just need to warn people about this issue and 
provide that workaround, as something that only trivial new installs 
without much data loaded into them are likely to run into, until 9.0.2 
ships with your fix in it.  I'll update the docs on the wiki 
accordingly, once I've recovered from this morning's flight out West.


I forgot to credit Robert Noles here for rediscovering this bug on one 
of our systems and bringing it to my attention.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services and Supportwww.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] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Greg Stark
On Mon, Nov 1, 2010 at 12:37 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 Yes, indeed there is a corner-case bug when you try to stream the very first
 WAL segment, with log==seg==0.

This smells very much like
http://article.gmane.org/gmane.comp.db.postgresql.devel.general/137052

I wonder if there's some defensive programming way to avoid bugs of this sort.

-- 
greg

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


Re: [HACKERS] Range Types, discrete and/or continuous

2010-11-01 Thread Jeff Davis
On Mon, 2010-11-01 at 20:36 +0100, Dimitri Fontaine wrote:
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
  Would you be comfortable writing that '012[3-5]' range as
  '[0123, 0126)' or something similar?  What benefits do you see to
  using a range for prefixes versus a regular expression?
 
 Your proposed syntax would do fine, sure. Something like this is even on
 the TODO list for prefix indexing, but for the internal representation,
 as I think there might be some optimisation potential there. Meanwhile,
 it would be easy enough to accept alternative input syntax.

Interesting example of a situation where the representation can be
optimized. I suspected that this was the case, but perhaps my example
wasn't as compelling:

http://archives.postgresql.org/pgsql-hackers/2010-10/msg01842.php

This suggests that there should be some way for the user to specify
their own representation function.

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] Complier warnings on mingw gcc 4.5.0

2010-11-01 Thread Hiroshi Inoue

(2010/11/01 19:24), Itagaki Takahiro wrote:

I compiled the source with mingw gcc 4.5.0, that has been released recently.
The compile was succeeded and worked well at least for simple queries,
but there were many warnings during the compile.

1. warning: 'symbol' redeclared without dllimport attribute:
previous dllimport ignored
2. warning: unknown conversion type character 'm' in format
3. warning: unknown conversion type character 'l' in format


1 is easy to fix with the attached patch.


Is it safe to put back the patch you applied in
http://archives.postgresql.org/pgsql-committers/2010-05/msg00338.php
in the case __GNUC__ =4?

regards,
Hiroshi Inoue


I wonder why mingw gcc  4.5 can build codes without the fix...




*** a/src/include/port/win32.h
--- b/src/include/port/win32.h
***
*** 58,64 
   #define PGDLLIMPORT __declspec (dllimport)
   #endWindows 7 64bit


diff --git a/src/include/port/win32.h b/src/include/port/win32.h
indexif

! #ifdef _MSC_VER
   #define PGDLLEXPORT __declspec (dllexport)
   #else
   #define PGDLLEXPORT __declspec (dllimport)
--- 58,64 
   #define PGDLLIMPORT __declspec (dllimport)
   #endif

! #if defined(_MSC_VER) || __GNUC__= 4
   #define PGDLLEXPORT __declspec (dllexport)
   #else
   #define PGDLLEXPORT __declspec (dllimport)



--
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] Hash support for arrays

2010-11-01 Thread hernan gonzalez
Hmm.  I am reminded of Knuth's famous dictum: never generate random
numbers with a method chosen at random.  Is there any actual theory
behind that algorithm, and if so what is it?  The combination of
shifting with addition (not xor) seems more likely to lead to weird
cancellations than any improvement in the hash behavior.
For what's worth: the standard way in Java is multiplying by 31.
Which 31 amounts to a 5 bit shift and a substraction.

In general, a prime number is recommended though one would like that
Knuth made some analysis here... it all seems mostly empirical.

http://computinglife.wordpress.com/2008/11/20/why-do-hash-functions-use-prime-numbers/

Perhaps the 5 bit shift is related to the spread of ascii charpoints,
as that hashcode() implementation was mainly tested and implemented
for a String. But now it's also suggested for general objects, and
it's even specified for standard collections: for example

http://download.oracle.com/javase/6/docs/api/java/util/List.html#hashCode()


Hernán J. González


Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Alex Hunsaker
On Mon, Nov 1, 2010 at 09:28, Tom Lane t...@sss.pgh.pa.us wrote:
 I think the crash is dependent on the fact that the function is created
 and called in the same session.  That means the validator gets called on
 it first, and the validator not unreasonably assumes istrigger = true,
 and then it calls compile_plperl_function which sets up a cache entry
 on that basis.  So then when the regular call comes along, it tries
 to reuse the cache entry in the other style.  Kaboom.

The other Kaboom happens if the trigger gets called as a trigger first
and then directly.

 There is a check so that trigger functions can not be called as plain
 functions... I think just moving that up...

 No, that is just moving a test that only needs to be done once into a
 place where it has to be done every time.  You might as well argue that
 we shouldn't cache any of the setup but just redo it all every time.

Huh?  I might try and argue that if the new test was more complex than
2 compares :P.  In-fact the way it stands now we uselessly grab the
functions pg_proc entry in the common case.  Would you object to
trying to clean that up across all pls?  Im thinking add an is_trigger
or context to each proc_desc, then check that in the corresponding
handlers.  While im at it get rid of at least one SysCache lookup.
Thoughts?  We can still keep the is_trigger bool in the hash key, as
you said below it is a good safety feature.  I just wish the logic was
spelled out a bit more.  Maybe im alone here.

 It's also the same way
 that the other three PLs do things, and I see no good excuse for plperl
 to do things differently here.

Im all in favor of keeping things between the pls as close as possible.

Speaking of which, pltcl stores the trigger reloid instead of a flag
(it also uses tg_reloid in the internal proname).  It seems a tad
excessive to have one function *per* trigger table.  I looked through
the history to see if there was some reason, it goes all the way back
to the initial commit.  I assume its this way because it copied
plpgsql, which needs it as the rowtype might be different per table.
pltcl should not have that issue.  Find attached a patch to clean that
up and make it match the other pls (err um plperl).  It passes its
regression tests and some additional limited testing.  Thoughts?
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
***
*** 137,143  typedef struct pltcl_query_desc
  
  /**
   * For speedy lookup, we maintain a hash table mapping from
!  * function OID + trigger OID + user OID to pltcl_proc_desc pointers.
   * The reason the pltcl_proc_desc struct isn't directly part of the hash
   * entry is to simplify recovery from errors during compile_pltcl_function.
   *
--- 137,143 
  
  /**
   * For speedy lookup, we maintain a hash table mapping from
!  * function OID + trigger flag + user OID to pltcl_proc_desc pointers.
   * The reason the pltcl_proc_desc struct isn't directly part of the hash
   * entry is to simplify recovery from errors during compile_pltcl_function.
   *
***
*** 149,155  typedef struct pltcl_query_desc
  typedef struct pltcl_proc_key
  {
  	Oid			proc_id;/* Function OID */
! 	Oid			trig_id;/* Trigger OID, or 0 if not trigger */
  	Oid			user_id;/* User calling the function, or 0 */
  } pltcl_proc_key;
  
--- 149,159 
  typedef struct pltcl_proc_key
  {
  	Oid			proc_id;/* Function OID */
! 	/*
! 	 * is_trigger is really a bool, but declare as Oid to ensure this struct
! 	 * contains no padding
! 	 */
! 	Oid			is_trigger;/* is it a trigger function? */
  	Oid			user_id;/* User calling the function, or 0 */
  } pltcl_proc_key;
  
***
*** 1172,1178  compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
  
  	/* Try to find function in pltcl_proc_htab */
  	proc_key.proc_id = fn_oid;
! 	proc_key.trig_id = tgreloid;
  	proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
  
  	proc_ptr = hash_search(pltcl_proc_htab, proc_key,
--- 1176,1182 
  
  	/* Try to find function in pltcl_proc_htab */
  	proc_key.proc_id = fn_oid;
! 	proc_key.is_trigger = OidIsValid(tgreloid);
  	proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
  
  	proc_ptr = hash_search(pltcl_proc_htab, proc_key,
***
*** 1228,1241  compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
  		int			tcl_rc;
  
  		/
! 		 * Build our internal proc name from the functions Oid + trigger Oid
  		 /
! 		if (!is_trigger)
! 			snprintf(internal_proname, sizeof(internal_proname),
! 	 __PLTcl_proc_%u, fn_oid);
! 		else
! 			snprintf(internal_proname, sizeof(internal_proname),
! 	 __PLTcl_proc_%u_trigger_%u, fn_oid, tgreloid);
  
  		

Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Alex Hunsaker
On Mon, Nov 1, 2010 at 15:24, Alex Hunsaker bada...@gmail.com wrote:
houldn't cache any of the setup but just redo it all every time.

 Huh?  I might try and argue that if the new test was more complex than
 2 compares :P.  In-fact the way it stands now we uselessly grab the
 functions pg_proc entry in the common case.

This is bogus, I missed the fact that we need it to make sure the
function is uptodate for the OR REPLACE in CREATE OR REPLACE.

-- 
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] SR fails to send existing WAL file after off-line copy

2010-11-01 Thread Tom Lane
Greg Stark gsst...@mit.edu writes:
 On Mon, Nov 1, 2010 at 12:37 AM, Heikki Linnakangas
 heikki.linnakan...@enterprisedb.com wrote:
 Yes, indeed there is a corner-case bug when you try to stream the very first
 WAL segment, with log==seg==0.

 This smells very much like
 http://article.gmane.org/gmane.comp.db.postgresql.devel.general/137052

 I wonder if there's some defensive programming way to avoid bugs of this sort.

It strikes me that it's not good if there isn't a recognizable invalid
value for WAL locations.  These bits of code show that there is reason
to have one.  Maybe we should teach initdb to start the WAL one segment
later, and then 0/0 *would* mean invalid, and we could revert these
other hacks.

regards, tom lane

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


Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Tom Lane
Alex Hunsaker bada...@gmail.com writes:
 Speaking of which, pltcl stores the trigger reloid instead of a flag
 (it also uses tg_reloid in the internal proname).  It seems a tad
 excessive to have one function *per* trigger table.  I looked through
 the history to see if there was some reason, it goes all the way back
 to the initial commit.  I assume its this way because it copied
 plpgsql, which needs it as the rowtype might be different per table.
 pltcl should not have that issue.  Find attached a patch to clean that
 up and make it match the other pls (err um plperl).  It passes its
 regression tests and some additional limited testing.  Thoughts?

Surely, removing the internal name's dependency on the istrigger flag is
wrong.  If you're going to maintain separate hash entries at the pltcl
level, why would you want to risk collisions underneath that?

regards, tom lane

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


Re: [HACKERS] revision of todo: NULL for ROW variables

2010-11-01 Thread Merlin Moncure
On Mon, Nov 1, 2010 at 2:29 PM, Kevin Grittner
kevin.gritt...@wicourts.gov wrote:
 Jeff Davis pg...@j-davis.com wrote:

 Seriously though, I think that we should stick as closely to the
 letter of the standard as possible here (or, if there is
 ambiguity, pick one reasonable interpretation). NULL semantics are
 confusing enough without everyone making their own subtle tweaks.

 +1

 If the standard behavior doesn't support all the functionality we
 need, we should be looking at PostgreSQL extensions which do not
 conflict with standard syntax.  Supporting standard syntax with
 different semantics is evil.

I have basically two gripes with sql standard treatment of null row
values. One is the backward compatibility problem (which extends all
the way up to PQgetisnull, and would affect lots of my code) and the
other is that you will lose the ability to ever usefully enforce table
check constraints over rowtypes like we do for domains (you need to
reserve rowtype := null to skirt the issue in plpgsql declarations).

merlin

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


Re: [HACKERS] Complier warnings on mingw gcc 4.5.0

2010-11-01 Thread Itagaki Takahiro
On Tue, Nov 2, 2010 at 6:02 AM, Hiroshi Inoue in...@tpf.co.jp wrote:
 1. warning: 'symbol' redeclared without dllimport attribute:
 previous dllimport ignored

 Is it safe to put back the patch you applied in
 http://archives.postgresql.org/pgsql-committers/2010-05/msg00338.php
 in the case __GNUC__ =4?

Hmmm, I didn't test compiling with gcc version between 3.5 and 4.4.
Does anyone test them? If it works only on gcc 4.5, I'll also add
_GNUC_MINOR__ = 5 for the check.

-- 
Itagaki Takahiro

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


Re: [HACKERS] Complier warnings on mingw gcc 4.5.0

2010-11-01 Thread Hiroshi Inoue

(2010/11/02 8:31), Itagaki Takahiro wrote:

On Tue, Nov 2, 2010 at 6:02 AM, Hiroshi Inouein...@tpf.co.jp  wrote:

1. warning: 'symbol' redeclared without dllimport attribute:
previous dllimport ignored


Is it safe to put back the patch you applied in
http://archives.postgresql.org/pgsql-committers/2010-05/msg00338.php
in the case __GNUC__=4?


Hmmm, I didn't test compiling with gcc version between 3.5 and 4.4.
Does anyone test them? If it works only on gcc 4.5, I'll also add
_GNUC_MINOR__= 5 for the check.


The problem which was fixed by your old patch is at runtime not
at compilation time. Is it fixed with gcc 4.5?

regards,
Hiroshi Inoue



--
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] Custom code int(32|64) = text conversions out of performance reasons

2010-11-01 Thread Andres Freund
Hi, 
On Monday 01 November 2010 10:15:01 Andres Freund wrote:
 On Monday 01 November 2010 04:04:51 Itagaki Takahiro wrote:
  On Mon, Nov 1, 2010 at 6:41 AM, Andres Freund and...@anarazel.de wrote:
   While looking at binary COPY performance I forgot to add BINARY and was
   a bit shocked to see printf that high in the profile...
   
   A change from 9192.476ms 5309.928ms seems to be pretty good indication
   that a change in that area is waranted given integer columns are quite
   ubiquous...
   * I renamed pg_[il]toa to pg_s(16|32|64)toa - I found the names
   confusing. Not sure if its worth it.
  Agreed, but how about pg_i(16|32|64)toa? 'i' might be more popular than
  's'. See also
  http://msdn.microsoft.com/en-US/library/yakksftt(VS.100).aspx
 I find itoa not as clear about signedness as stoa, but if you insist, I
 dont feel strongly about it.
Let whover commits it decide...

  * The buffer reordering seems a bit messy.
  //have to reorder the string, but not 0byte.
  I'd suggest to fill a fixed-size local buffer from right to left
  and copy it to the actual output.
 Is a bit cleaner maybe, but I dont see much point in putting it into its
 own function... But again, I don't feel strongly.
Using a seperate buffer cost nearly 500ms... So I only changed the comments 
there.

The only way I could think of to make it faster was to fill the buffer from the 
end and then return a pointer to the starting point in the buffer. The speed 
benefits are small (around 80ms) and it makes the interface more cumbersome...


Revised version attached - I will submit this to the next comittfest now.

Andres
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index c450333..5340052 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -74,7 +74,7 @@ int2out(PG_FUNCTION_ARGS)
 	int16		arg1 = PG_GETARG_INT16(0);
 	char	   *result = (char *) palloc(7);	/* sign, 5 digits, '\0' */
 
-	pg_itoa(arg1, result);
+	pg_s16toa(arg1, result);
 	PG_RETURN_CSTRING(result);
 }
 
@@ -189,7 +189,7 @@ int2vectorout(PG_FUNCTION_ARGS)
 	{
 		if (num != 0)
 			*rp++ = ' ';
-		pg_itoa(int2Array-values[num], rp);
+		pg_s16toa(int2Array-values[num], rp);
 		while (*++rp != '\0')
 			;
 	}
@@ -293,7 +293,7 @@ int4out(PG_FUNCTION_ARGS)
 	int32		arg1 = PG_GETARG_INT32(0);
 	char	   *result = (char *) palloc(12);	/* sign, 10 digits, '\0' */
 
-	pg_ltoa(arg1, result);
+	pg_s32toa(arg1, result);
 	PG_RETURN_CSTRING(result);
 }
 
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 894110d..4de2dfc 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -20,6 +20,7 @@
 #include funcapi.h
 #include libpq/pqformat.h
 #include utils/int8.h
+#include utils/builtins.h
 
 
 #define MAXINT8LEN		25
@@ -158,12 +159,9 @@ int8out(PG_FUNCTION_ARGS)
 {
 	int64		val = PG_GETARG_INT64(0);
 	char	   *result;
-	int			len;
 	char		buf[MAXINT8LEN + 1];
 
-	if ((len = snprintf(buf, MAXINT8LEN, INT64_FORMAT, val))  0)
-		elog(ERROR, could not format int8);
-
+	pg_s64toa(val, buf);
 	result = pstrdup(buf);
 	PG_RETURN_CSTRING(result);
 }
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 5f8083f..61b4728 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -3,8 +3,6 @@
  * numutils.c
  *	  utility functions for I/O of built-in numeric types.
  *
- *		integer:pg_atoi, pg_itoa, pg_ltoa
- *
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
@@ -109,27 +107,126 @@ pg_atoi(char *s, int size, int c)
 }
 
 /*
- *		pg_itoa			- converts a short int to its string represention
+ * pg_s32toa - convert a signed 16bit integer to a string representation
  *
- *		Note:
- *previously based on ~ingres/source/gutil/atoi.c
- *now uses vendor's sprintf conversion
+ * It doesnt seem worth implementing this separately.
  */
 void
-pg_itoa(int16 i, char *a)
+pg_s16toa(int16 i, char *a)
 {
-	sprintf(a, %hd, (short) i);
+	pg_s32toa((int32)i, a);
 }
 
+
 /*
- *		pg_ltoa			- converts a long int to its string represention
+ * pg_s32toa - convert a signed 32bit integer to a string representation
  *
- *		Note:
- *previously based on ~ingres/source/gutil/atoi.c
- *now uses vendor's sprintf conversion
+ * Its unfortunate to have this function twice - once for 32bit, once
+ * for 64bit, but incurring the cost of 64bit computation to 32bit
+ * platforms doesn't seem to be acceptable.
  */
 void
-pg_ltoa(int32 l, char *a)
-{
-	sprintf(a, %d, l);
+pg_s32toa(int32 value, char *buf){
+	char *bufstart = buf;
+	bool neg = false;
+
+	/*
+	 * Avoid problems with the most negative not being representable
+	 * as a positive number
+	 */
+	if(value == INT32_MIN)
+	{
+		memcpy(buf, -2147483648, 12);
+		return;
+	}
+	else if(value  0)
+	{
+		value = -value;
+		neg = true;
+	}
+
+	/* Build the string by computing the wanted string backwards. 

Re: [HACKERS] why does plperl cache functions using just a bool for is_trigger

2010-11-01 Thread Alex Hunsaker
On Mon, Nov 1, 2010 at 16:59, Tom Lane t...@sss.pgh.pa.us wrote:
 Alex Hunsaker bada...@gmail.com writes:
 Speaking of which, pltcl stores the trigger reloid instead of a flag
 (it also uses tg_reloid in the internal proname).  It seems a tad
 excessive to have one function *per* trigger table.

 Surely, removing the internal name's dependency on the istrigger flag is
 wrong.  If you're going to maintain separate hash entries at the pltcl
 level, why would you want to risk collisions underneath that?

Good catch.  I was basing it off plperl which uses the same proname
for both (sprintf(subname, %s__%u, prodesc-proname, fn_oid)).  Its
OK for plperl because when we compile we save a reference to it and
use that directly (more or less).  The name does not really matter.
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
***
*** 137,143  typedef struct pltcl_query_desc
  
  /**
   * For speedy lookup, we maintain a hash table mapping from
!  * function OID + trigger OID + user OID to pltcl_proc_desc pointers.
   * The reason the pltcl_proc_desc struct isn't directly part of the hash
   * entry is to simplify recovery from errors during compile_pltcl_function.
   *
--- 137,143 
  
  /**
   * For speedy lookup, we maintain a hash table mapping from
!  * function OID + trigger flag + user OID to pltcl_proc_desc pointers.
   * The reason the pltcl_proc_desc struct isn't directly part of the hash
   * entry is to simplify recovery from errors during compile_pltcl_function.
   *
***
*** 149,155  typedef struct pltcl_query_desc
  typedef struct pltcl_proc_key
  {
  	Oid			proc_id;/* Function OID */
! 	Oid			trig_id;/* Trigger OID, or 0 if not trigger */
  	Oid			user_id;/* User calling the function, or 0 */
  } pltcl_proc_key;
  
--- 149,159 
  typedef struct pltcl_proc_key
  {
  	Oid			proc_id;/* Function OID */
! 	/*
! 	 * is_trigger is really a bool, but declare as Oid to ensure this struct
! 	 * contains no padding
! 	 */
! 	Oid			is_trigger;/* is it a trigger function? */
  	Oid			user_id;/* User calling the function, or 0 */
  } pltcl_proc_key;
  
***
*** 1172,1178  compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
  
  	/* Try to find function in pltcl_proc_htab */
  	proc_key.proc_id = fn_oid;
! 	proc_key.trig_id = tgreloid;
  	proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
  
  	proc_ptr = hash_search(pltcl_proc_htab, proc_key,
--- 1176,1182 
  
  	/* Try to find function in pltcl_proc_htab */
  	proc_key.proc_id = fn_oid;
! 	proc_key.is_trigger = OidIsValid(tgreloid);
  	proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
  
  	proc_ptr = hash_search(pltcl_proc_htab, proc_key,
***
*** 1228,1241  compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
  		int			tcl_rc;
  
  		/
! 		 * Build our internal proc name from the functions Oid + trigger Oid
! 		 /
! 		if (!is_trigger)
! 			snprintf(internal_proname, sizeof(internal_proname),
! 	 __PLTcl_proc_%u, fn_oid);
! 		else
! 			snprintf(internal_proname, sizeof(internal_proname),
! 	 __PLTcl_proc_%u_trigger_%u, fn_oid, tgreloid);
  
  		/
  		 * Allocate a new procedure description block
--- 1232,1248 
  		int			tcl_rc;
  
  		/
! 		* Build our internal proc name from the functions Oid.  Append
! 		* trigger when appropriate in case they try to manually call
! 		* the trigger and vice versa. (otherwise we might overwrite the
! 		* trigger procedure in TCL's namespace)
! 		/
! 	   if (!is_trigger)
! 		   snprintf(internal_proname, sizeof(internal_proname),
! 	__PLTcl_proc_%u, fn_oid);
! 	   else
! 		   snprintf(internal_proname, sizeof(internal_proname),
! 	__PLTcl_proc_%u_trigger, fn_oid);
  
  		/
  		 * Allocate a new procedure description block

-- 
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] Custom code int(32|64) = text conversions out of performance reasons

2010-11-01 Thread Andres Freund
On Tuesday 02 November 2010 01:37:43 Andres Freund wrote:
 Revised version attached - I will submit this to the next comittfest now.
Context diff attached this time...
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index c450333..5340052 100644
*** a/src/backend/utils/adt/int.c
--- b/src/backend/utils/adt/int.c
*** int2out(PG_FUNCTION_ARGS)
*** 74,80 
  	int16		arg1 = PG_GETARG_INT16(0);
  	char	   *result = (char *) palloc(7);	/* sign, 5 digits, '\0' */
  
! 	pg_itoa(arg1, result);
  	PG_RETURN_CSTRING(result);
  }
  
--- 74,80 
  	int16		arg1 = PG_GETARG_INT16(0);
  	char	   *result = (char *) palloc(7);	/* sign, 5 digits, '\0' */
  
! 	pg_s16toa(arg1, result);
  	PG_RETURN_CSTRING(result);
  }
  
*** int2vectorout(PG_FUNCTION_ARGS)
*** 189,195 
  	{
  		if (num != 0)
  			*rp++ = ' ';
! 		pg_itoa(int2Array-values[num], rp);
  		while (*++rp != '\0')
  			;
  	}
--- 189,195 
  	{
  		if (num != 0)
  			*rp++ = ' ';
! 		pg_s16toa(int2Array-values[num], rp);
  		while (*++rp != '\0')
  			;
  	}
*** int4out(PG_FUNCTION_ARGS)
*** 293,299 
  	int32		arg1 = PG_GETARG_INT32(0);
  	char	   *result = (char *) palloc(12);	/* sign, 10 digits, '\0' */
  
! 	pg_ltoa(arg1, result);
  	PG_RETURN_CSTRING(result);
  }
  
--- 293,299 
  	int32		arg1 = PG_GETARG_INT32(0);
  	char	   *result = (char *) palloc(12);	/* sign, 10 digits, '\0' */
  
! 	pg_s32toa(arg1, result);
  	PG_RETURN_CSTRING(result);
  }
  
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 894110d..4de2dfc 100644
*** a/src/backend/utils/adt/int8.c
--- b/src/backend/utils/adt/int8.c
***
*** 20,25 
--- 20,26 
  #include funcapi.h
  #include libpq/pqformat.h
  #include utils/int8.h
+ #include utils/builtins.h
  
  
  #define MAXINT8LEN		25
*** int8out(PG_FUNCTION_ARGS)
*** 158,169 
  {
  	int64		val = PG_GETARG_INT64(0);
  	char	   *result;
- 	int			len;
  	char		buf[MAXINT8LEN + 1];
  
! 	if ((len = snprintf(buf, MAXINT8LEN, INT64_FORMAT, val))  0)
! 		elog(ERROR, could not format int8);
! 
  	result = pstrdup(buf);
  	PG_RETURN_CSTRING(result);
  }
--- 159,167 
  {
  	int64		val = PG_GETARG_INT64(0);
  	char	   *result;
  	char		buf[MAXINT8LEN + 1];
  
! 	pg_s64toa(val, buf);
  	result = pstrdup(buf);
  	PG_RETURN_CSTRING(result);
  }
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 5f8083f..61b4728 100644
*** a/src/backend/utils/adt/numutils.c
--- b/src/backend/utils/adt/numutils.c
***
*** 3,10 
   * numutils.c
   *	  utility functions for I/O of built-in numeric types.
   *
-  *		integer:pg_atoi, pg_itoa, pg_ltoa
-  *
   * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
   *
--- 3,8 
*** pg_atoi(char *s, int size, int c)
*** 109,135 
  }
  
  /*
!  *		pg_itoa			- converts a short int to its string represention
   *
!  *		Note:
!  *previously based on ~ingres/source/gutil/atoi.c
!  *now uses vendor's sprintf conversion
   */
  void
! pg_itoa(int16 i, char *a)
  {
! 	sprintf(a, %hd, (short) i);
  }
  
  /*
!  *		pg_ltoa			- converts a long int to its string represention
   *
!  *		Note:
!  *previously based on ~ingres/source/gutil/atoi.c
!  *now uses vendor's sprintf conversion
   */
  void
! pg_ltoa(int32 l, char *a)
! {
! 	sprintf(a, %d, l);
  }
--- 107,232 
  }
  
  /*
!  * pg_s32toa - convert a signed 16bit integer to a string representation
   *
!  * It doesnt seem worth implementing this separately.
   */
  void
! pg_s16toa(int16 i, char *a)
  {
! 	pg_s32toa((int32)i, a);
  }
  
+ 
  /*
!  * pg_s32toa - convert a signed 32bit integer to a string representation
   *
!  * Its unfortunate to have this function twice - once for 32bit, once
!  * for 64bit, but incurring the cost of 64bit computation to 32bit
!  * platforms doesn't seem to be acceptable.
   */
  void
! pg_s32toa(int32 value, char *buf){
! 	char *bufstart = buf;
! 	bool neg = false;
! 
! 	/*
! 	 * Avoid problems with the most negative not being representable
! 	 * as a positive number
! 	 */
! 	if(value == INT32_MIN)
! 	{
! 		memcpy(buf, -2147483648, 12);
! 		return;
! 	}
! 	else if(value  0)
! 	{
! 		value = -value;
! 		neg = true;
! 	}
! 
! 	/* Build the string by computing the wanted string backwards. */
! 	do
! 	{
! 		int32 remainder;
! 		int32 oldval = value;
! 		/*
! 		 * division by constants can be optimized by some modern
! 		 * compilers (including gcc). We could add the concrete,
! 		 * optimized, calculatation here to be fast at -O0 and/or
! 		 * other compilers... Not sure if its worth doing.
! 		 */
! 		value /= 10;
! 		remainder = oldval - value * 10;
! 		*buf++ = '0' + remainder;
! 	}
! 	while(value != 0);
! 
! 	if(neg)
! 		*buf++ = '-';
! 
! 	/* have to reorder the string, but not 0 byte */
! 	*buf-- = 0;
! 
! 	/* reverse string */

[HACKERS] Sort is actually PlanState?

2010-11-01 Thread Hitoshi Harada
I wonder why SortState is a ScanState. As far as I know ScanState
means the node may need projection and/or qualification, or it scans
some relation, but Sort actually doesn't do such things. I also tried
to modify SortState as PlanState as in the attached patch and
regression test passed. Do I misunderstand ScanState?

Regards,

-- 
Hitoshi Harada


sort-plan.20101102.patch
Description: Binary data

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


Re: [HACKERS] Sort is actually PlanState?

2010-11-01 Thread Tom Lane
Hitoshi Harada umi.tan...@gmail.com writes:
 I wonder why SortState is a ScanState. As far as I know ScanState
 means the node may need projection and/or qualification, or it scans
 some relation, but Sort actually doesn't do such things.

No, not really.  Per the comment for ScanState:

 *ScanState extends PlanState for node types that represent
 *scans of an underlying relation.  It can also be used for nodes
 *that scan the output of an underlying plan node --- in that case,
 *only ScanTupleSlot is actually useful, and it refers to the tuple
 *retrieved from the subplan.

It might be that we don't actually need ScanTupleSlot right now in the
implementation of Sort, but I don't see a good reason to remove the
field.  We might just have to put it back later.

BTW, Sort is not the only node type like this --- I see at least
Material that's not projection-capable but has a ScanState.

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] Complier warnings on mingw gcc 4.5.0

2010-11-01 Thread Itagaki Takahiro
On Tue, Nov 2, 2010 at 8:43 AM, Hiroshi Inoue in...@tpf.co.jp wrote:
 The problem which was fixed by your old patch is at runtime not
 at compilation time. Is it fixed with gcc 4.5?

Now it works as far as simple test, including core functions and
dynamic modules. So, I think the fix for dllexport is safe enough
for mingw gcc 4.5.


BTW, with out without the above fix, regression test failed with
weird error if the server is built with gcc 4.5. However, server
can run if I started it manually with PGPORT or -o --port=X.
We might need another fix for the issue.

LOG:  database system was shut down at 2010-11-02 10:32:13 JST
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
FATAL:  parameter port cannot be changed without restarting the server
(repeated)


-- 
Itagaki Takahiro

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


Re: [HACKERS] Sort is actually PlanState?

2010-11-01 Thread Hitoshi Harada
2010/11/2 Tom Lane t...@sss.pgh.pa.us:
 Hitoshi Harada umi.tan...@gmail.com writes:
 I wonder why SortState is a ScanState. As far as I know ScanState
 means the node may need projection and/or qualification, or it scans
 some relation, but Sort actually doesn't do such things.

 No, not really.  Per the comment for ScanState:

  *        ScanState extends PlanState for node types that represent
  *        scans of an underlying relation.  It can also be used for nodes
  *        that scan the output of an underlying plan node --- in that case,
  *        only ScanTupleSlot is actually useful, and it refers to the tuple
  *        retrieved from the subplan.

 It might be that we don't actually need ScanTupleSlot right now in the
 implementation of Sort, but I don't see a good reason to remove the
 field.  We might just have to put it back later.

It might reduce a few cycle used in initializing and cleaning of
ScanTupleSlot, but I basically agree it's not good reason to do it.

 BTW, Sort is not the only node type like this --- I see at least
 Material that's not projection-capable but has a ScanState.

Yes, during designing DtScan which is coming in the writeable CTEs I
came up with the question.

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


[HACKERS] Improving planner's handling of min/max aggregate optimization

2010-11-01 Thread Tom Lane
Now that we have MergeAppend in place, it strikes me that the current
support for implementing MIN() and MAX() via indexscan+LIMIT subplans
could be generalized to work on inheritance trees: a MergeAppend
plan plus LIMIT would just about do it.

However, extending the existing implementation in planagg.c to create
this type of plan looks quite unappetizing.  It's basically duplicating
the key parts of indexpath generation, and we'd have to duplicate a
whole lot more logic in order to do MergeAppends in the same fashion.
I think it's time to rethink the idea of having that code be mostly
independent of the rest of the planner logic.

With the recent hacking on equivalence classes fresh in mind, it seems
to me that the right way to refactor planagg.c is like this:

1. Do the initial processing (checking to see if all aggregates are
MIN/MAX) early in grouping_planner().  Perhaps it'd be sensible to
merge it into count_agg_clauses(), although for the moment I'd be
satisfied with doing a second pass over the tree for this purpose.

2. If we find that the aggregates are potentially optimizable,
then push EquivalenceClauses for their arguments into the EC machinery.

3. As a result of #2, the path generation code will automatically try to
build indexscan paths with sort orders matching the aggregate arguments.
If there are any inherited tables, it'll try to build MergeAppend paths
for those, too.  (Well, actually, we'll likely have to mess with the
useful pathkeys logic just a tad for this to work.  But it won't be
hard.)

4. The final step is still the same as now, and done at the same place:
compare costs and see if we should build an alternative plan.  But
instead of constructing paths the hard way, we'll just grab the cheapest
suitably-sorted path from the existing path collection.

This will make the min/max optimization code more visible to the rest of
the planner in a couple of ways: aside from being called at two places
not one, it will have some intermediate state that'll have to be kept in
PlannerInfo, and the useful pathkeys logic will have to be complicit
in letting paths that match the aggregates' requirements survive.  But
it's not real bad, and it seems a lot better than continuing with the
fully-at-arms-length approach.

Comments?

regards, tom lane

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


[HACKERS] Starting off with the development

2010-11-01 Thread Vaibhav Kaushal
Hello,

I think that I want to get indulged in development. Have gone through the
code a bit, Unable to understand so might need some help over there. I hope
I will recieve help.

What version should I start from? I guess postgresql 9.1 alpha would be
good.

Please suggest some other dev version (if they are in progress) if you think
I should start off with. My main work concerns the planner and executor.

Thanks a lot.

-Vaibhav


[HACKERS] improved parallel make support

2010-11-01 Thread Peter Eisentraut
I have worked on some improvements on how we handle recursive make in
our makefiles.  Most places uses for loops, which has some
disadvantages: parallel make doesn't work across directories, make -k
doesn't work, and make -q doesn't work.  Instead, I went with the
approach that we already use in the src/backend directory, where we call
the subordinate makes as target prerequisites.

Note that because with this, parallel make really works, the rule
dependencies must be correct.  This has always been the case, but now it
really shows up.  A frequent issue is that this sort of thing no longer
works:

all: submake-libpgport zic

zic: $(ZICOBJS)

because this relies on the all target to execute its prerequisites in
order.  Instead, you need to write it like this:

all: zic

zic: $(ZICOBJS) | submake-libpgport

(The bar is necessary so that zic isn't considered constantly out of
date because it depends on a phony target.)

This patch requires GNU make 3.80, because of the above | feature and
the $(eval) function.  Version 3.80 is dated October 2002, so it should
be no problem, but I do occasionally read of make 3.79 around here;
maybe it's time to get rid of that.  I did put in a check that makes the
build fail right away if a wrong version of make is used.

diff --git a/GNUmakefile.in b/GNUmakefile.in
index 57f5813..9d4c4ce 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -8,57 +8,39 @@ subdir =
 top_builddir = .
 include $(top_builddir)/src/Makefile.global
 
+$(call recurse,all install,src config)
+
 all:
-	$(MAKE) -C src all
-	$(MAKE) -C config all
-	@echo All of PostgreSQL successfully made. Ready to install.
+	+...@echo All of PostgreSQL successfully made. Ready to install.
 
 docs:
 	$(MAKE) -C doc all
 
-world:
-	$(MAKE) -C doc all
-	$(MAKE) -C src all
-	$(MAKE) -C config all
-	$(MAKE) -C contrib all
-	@echo PostgreSQL, contrib, and documentation successfully made. Ready to install.
+$(call recurse,world,contrib,all)
+world: all docs
+	+...@echo PostgreSQL, contrib, and documentation successfully made. Ready to install.
 
 html man:
 	$(MAKE) -C doc $@
 
 install:
-	$(MAKE) -C src $@
-	$(MAKE) -C config $@
-	@echo PostgreSQL installation complete.
+	+...@echo PostgreSQL installation complete.
 
 install-docs:
 	$(MAKE) -C doc install
 
-install-world:
-	$(MAKE) -C doc install
-	$(MAKE) -C src install
-	$(MAKE) -C config install
-	$(MAKE) -C contrib install
-	@echo PostgreSQL, contrib, and documentation installation complete.
+$(call recurse,install-world,contrib,install)
+install-world: install install-docs
+	+...@echo PostgreSQL, contrib, and documentation installation complete.
 
-installdirs uninstall coverage:
-	$(MAKE) -C doc $@
-	$(MAKE) -C src $@
-	$(MAKE) -C config $@
+$(call recurse,installdirs uninstall coverage,doc src config)
 
-distprep:
-	$(MAKE) -C doc $@
-	$(MAKE) -C src $@
-	$(MAKE) -C config $@
-	$(MAKE) -C contrib $@
+$(call recurse,distprep,doc src config contrib)
 
 # clean, distclean, etc should apply to contrib too, even though
 # it's not built by default
+$(call recurse,clean,doc contrib src config)
 clean:
-	$(MAKE) -C doc $@
-	$(MAKE) -C contrib $@
-	$(MAKE) -C src $@
-	$(MAKE) -C config $@
 # Garbage from autoconf:
 	@rm -rf autom4te.cache/
 
@@ -78,11 +60,7 @@ check: all
 check installcheck installcheck-parallel:
 	$(MAKE) -C src/test $@
 
-installcheck-world:
-	$(MAKE) -C src/test installcheck
-	$(MAKE) -C src/pl installcheck
-	$(MAKE) -C src/interfaces/ecpg installcheck
-	$(MAKE) -C contrib installcheck
+$(call recurse,installcheck-world,src/test src/interfaces/ecpg contrib,installcheck)
 
 GNUmakefile: GNUmakefile.in $(top_builddir)/config.status
 	./config.status $@
@@ -143,4 +121,4 @@ distcheck: dist
 	rm -rf $(distdir) $(dummy)
 	@echo Distribution integrity checks out.
 
-.PHONY: dist distdir distcheck docs install-docs
+.PHONY: dist distdir distcheck docs install-docs world install-world installcheck-world
diff --git a/contrib/Makefile b/contrib/Makefile
index b777325..e1f2a84 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -63,14 +63,4 @@ endif
 #		start-scripts	\ (does not have a makefile)
 
 
-all install installdirs uninstall distprep clean distclean maintainer-clean:
-	@for dir in $(SUBDIRS); do \
-		$(MAKE) -C $$dir $@ || exit; \
-	done
-
-# We'd like check operations to run all the subtests before failing.
-check installcheck:
-	@CHECKERR=0; for dir in $(SUBDIRS); do \
-		$(MAKE) -C $$dir $@ || CHECKERR=$$?; \
-	done; \
-	exit $$CHECKERR
+$(recurse)
diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile
index 148961e..cc59128 100644
--- a/contrib/dblink/Makefile
+++ b/contrib/dblink/Makefile
@@ -4,6 +4,7 @@ MODULE_big = dblink
 PG_CPPFLAGS = -I$(libpq_srcdir)
 OBJS	= dblink.o
 SHLIB_LINK = $(libpq)
+SHLIB_PREREQS = submake-libpq
 
 DATA_built = dblink.sql 
 DATA = uninstall_dblink.sql 
diff --git a/src/Makefile b/src/Makefile
index 0e1e431..0d4a6ee 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -12,20 +12,21 @@ subdir = src
 top_builddir = 

Re: [HACKERS] Tracking latest timeline in standby mode

2010-11-01 Thread Fujii Masao
On Mon, Nov 1, 2010 at 8:32 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 Yeah, that's one approach. Another is to validate the TLI in the xlog page
 header, it should always match the current timeline we're on. That would
 feel more robust to me.

Yeah, that seems better.

 We're a bit fuzzy about what TLI is written in the page header when the
 timeline changing checkpoint record is written, though. If the checkpoint
 record fits in the previous page, the page will carry the old TLI, but if
 the checkpoint record begins a new WAL page, the new page is initialized
 with the new TLI. I think we should rearrange that so that the page header
 will always carry the old TLI.

Or after rescanning the timeline history files, what about refetching the last
applied record and checking whether the TLI in the xlog page header is the
same as the previous TLI? IOW, what about using the header of the xlog page
including the last applied record instead of the following checkpoint record?

Anyway ISTM we should also check that the min recovery point is not ahead
of the TLI switch location. So we need to fetch the record in the min recovery
point and validate the TLI of the xlog page header. Otherwise, the database
might get corrupted. This can happen, for example, when you remove all the
WAL files in pg_xlog directory and restart the standby.

Regards,

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

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