Re: [HACKERS] SR fails to send existing WAL file after off-line copy
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
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
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
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
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
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
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
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 ...
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 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
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
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
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
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
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
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
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/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
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
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
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?
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?
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
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/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
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
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
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
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