Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-08 Thread Leonardo Francalanci
Applied with some significant editorialization. The biggest problem I found was that the code for expression indexes didn't really work, and would leak memory like there's no tomorrow even when it did work. Sorry I couldn't write the way it was supposed to... I'll look at the differences

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-07 Thread Tom Lane
Josh Kupershmidt schmi...@gmail.com writes: So I think there are definitely cases where this patch helps, but it looks like a seq. scan is being chosen in some cases where it doesn't help. I've been poking through this patch, and have found two different ways in which it underestimates the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-07 Thread Tom Lane
Itagaki Takahiro itagaki.takah...@gmail.com writes: I re-ordered some description in the doc. Does it look better? Comments and suggestions welcome. Applied with some significant editorialization. The biggest problem I found was that the code for expression indexes didn't really work, and

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-07 Thread Tom Lane
Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes: BTW, we could have LogicalTapeReadExact() as an alias of LogicalTapeRead() and checking the result because we have many duplicated codes for unexpected end of data errors. Good idea, done. regards, tom lane --

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-07 Thread Tom Lane
Itagaki Takahiro itagaki.takah...@gmail.com writes: I wrote a patch to improve CLUSTER VERBOSE (and VACUUM FULL VERBOSE). The patch should be applied after sorted_cluster-20100721.patch . Applied with minor fixes; in particular, I think you got the effects of rewrite_heap_dead_tuple backwards.

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-07 Thread Itagaki Takahiro
On Fri, Oct 8, 2010 at 10:49 AM, Tom Lane t...@sss.pgh.pa.us wrote: Itagaki Takahiro itagaki.takah...@gmail.com writes: I wrote a patch to improve CLUSTER VERBOSE (and VACUUM FULL VERBOSE). The patch should be applied after sorted_cluster-20100721.patch . Applied with minor fixes; in

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-04 Thread Robert Haas
On Oct 1, 2010, at 8:36 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Fri, Oct 1, 2010 at 4:33 AM, Leonardo Francalanci m_li...@yahoo.it wrote: I ran a few more performance tests on this patch. Here's what I got for the tests Leonardo posted originally: * 2M rows: 22 seconds for seq.

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-04 Thread Leonardo Francalanci
It sounds like the costing model might need a bit more work before we commit this. I tried again the simple sql tests I posted a while ago, and I still get the same ratios. I've tested the applied patch on a dual opteron + disk array Solaris machine. I really don't get how a laptop hard

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-04 Thread Josh Kupershmidt
On Mon, Oct 4, 2010 at 8:42 AM, Robert Haas robertmh...@gmail.com wrote: Did you also adjust random_page_cost? No, my seq_page_cost (1) and random_page_cost (4) are from the defaults. Here are all my non-default settings: test=# SELECT name, unit, setting FROM pg_settings WHERE source !=

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-04 Thread Josh Kupershmidt
On Mon, Oct 4, 2010 at 4:47 PM, Leonardo Francalanci m_li...@yahoo.it wrote: It sounds like the costing model might need a bit more work before  we commit this. I tried again the simple sql tests I posted a while ago, and I still get the same ratios. I've tested the applied patch on a dual

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-03 Thread Itagaki Takahiro
On Sat, Oct 2, 2010 at 9:36 AM, Josh Kupershmidt schmi...@gmail.com wrote: Hrm, this is interesting. I set up a test table with 5M rows like so: Such discussions are for the planner itself, right? The sorted cluster patch uses the existing planner's costing model, so we can discuss the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-01 Thread Leonardo Francalanci
I ran a few more performance tests on this patch. Here's what I got for the tests Leonardo posted originally: * 2M rows: 22 seconds for seq. scan, 24 seconds for index scan * 5M rows: 139 seconds for seq. scan, 97 seconds for index scan * 10M rows: 256 seconds seq. scan, 611

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-01 Thread Robert Haas
On Fri, Oct 1, 2010 at 4:33 AM, Leonardo Francalanci m_li...@yahoo.it wrote: I guess that if the planner makes a wrong choice in this case (that is, seq scan + sort instead of index scan) there's no way for cluster to behave in a different way. If, on the contrary, the create table... uses the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-10-01 Thread Josh Kupershmidt
On Fri, Oct 1, 2010 at 4:33 AM, Leonardo Francalanci m_li...@yahoo.it wrote: I ran a few more performance tests on this patch. Here's what  I got for the tests Leonardo posted originally:    * 2M  rows:  22 seconds for seq. scan, 24 seconds for index scan    * 5M  rows:  139 seconds for seq.

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-30 Thread Simon Riggs
On Wed, 2010-09-29 at 08:44 -0400, Alvaro Herrera wrote: So, I think twice disk space of the sum of table and indexes would be the simplest explanation for safe margin. Agreed. Surely the peak space is x3? Old space + sort space + new space. -- Simon Riggs

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-30 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes: On Wed, 2010-09-29 at 08:44 -0400, Alvaro Herrera wrote: So, I think twice disk space of the sum of table and indexes would be the simplest explanation for safe margin. Agreed. Surely the peak space is x3? Old space + sort space + new space. The

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-30 Thread Itagaki Takahiro
Hi, Leonardo-san, On Fri, Oct 1, 2010 at 4:04 AM, Tom Lane t...@sss.pgh.pa.us wrote: The wording should be something like CLUSTER requires transient disk space equal to about twice the size of the table plus its indexes. Could you merge those discussions into the final patch? Also, please

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-30 Thread Robert Haas
On Sep 30, 2010, at 9:07 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: Hi, Leonardo-san, On Fri, Oct 1, 2010 at 4:04 AM, Tom Lane t...@sss.pgh.pa.us wrote: The wording should be something like CLUSTER requires transient disk space equal to about twice the size of the table plus its

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Leonardo Francalanci
10% is nothing. I was expecting this patch would give an order of magnitude of improvement or somethine like that in the worst cases of the current code (highly unsorted input) Yes. It should be x10 faster than ordinary method in the worst cases. Here's my post with a (very simple)

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Alvaro Herrera
Excerpts from Leonardo Francalanci's message of mié sep 29 03:17:07 -0400 2010: Here's my post with a (very simple) performance test: http://archives.postgresql.org/pgsql-hackers/2010-02/msg00766.php I think the 10M rows test is more in line with what we want (83s vs. 646). -- Álvaro

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Alvaro Herrera
Excerpts from Itagaki Takahiro's message of mié sep 29 01:25:38 -0400 2010: To be exact, It's very complex. During reconstructing tables, it requires about twice disk space of the old table (for sort tapes and the new table). After sorting the table, CLUSTER performs REINDEX. We need {same

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Robert Haas
On Wed, Sep 29, 2010 at 1:12 AM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Wed, Sep 29, 2010 at 1:27 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I see a consistent ~10% advantage for the sequential scan clusters. 10% is nothing.  I was expecting this patch would give an

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Itagaki Takahiro
On Wed, Sep 29, 2010 at 10:14 PM, Robert Haas robertmh...@gmail.com wrote: http://reorg.projects.postgresql.org/ Can you reproduce that with this patch? No, I can't use the machine anymore. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Leonardo Francalanci
Here's my post with a (very simple) performance test: http://archives.postgresql.org/pgsql-hackers/2010-02/msg00766.php I think the 10M rows test is more in line with what we want (83s vs. 646). Can someone else test the patch to see if what I found is still valid? I don't think it makes

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-29 Thread Josh Kupershmidt
On Wed, Sep 29, 2010 at 11:55 AM, Leonardo Francalanci m_li...@yahoo.it wrote: Can someone else test the patch to see if what I found is still valid? I don't think it makes much sense if I'm the only one that says this is faster :) I ran a few more performance tests on this patch. Here's what

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-28 Thread Josh Kupershmidt
On Mon, Sep 27, 2010 at 10:05 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: I re-ordered some description in the doc. Does it look better? Comments and suggestions welcome. I thought this paragraph was a little confusing: ! In the second case, a full table scan is followed by a

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-28 Thread Alvaro Herrera
Excerpts from Josh Kupershmidt's message of mar sep 28 23:53:33 -0400 2010: I started looking at the performance impact of this patch based on Leonardo's SQL file. On the 2 million row table, I see a consistent ~10% advantage for the sequential scan clusters. I'm going to try to run the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-28 Thread Itagaki Takahiro
On Wed, Sep 29, 2010 at 1:27 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I see a consistent ~10% advantage for the sequential scan clusters. 10% is nothing.  I was expecting this patch would give an order of magnitude of improvement or somethine like that in the worst cases of the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-28 Thread Itagaki Takahiro
On Wed, Sep 29, 2010 at 12:53 PM, Josh Kupershmidt schmi...@gmail.com wrote: I thought this paragraph was a little confusing: Thanks for checking. !     In the second case, a full table scan is followed by a sort operation. !     The method is faster than the first one when the table is

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-27 Thread Itagaki Takahiro
On Tue, Aug 31, 2010 at 8:04 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: I think the patch is almost ready to commit, but still have some comments for the usability and documentations. I hope native English speakers would help improving docs. I'm checking the latest patch for

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-09-01 Thread Itagaki Takahiro
On Tue, Aug 31, 2010 at 8:04 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: * How to determine which method was used?  We can get some information from trace_sort logs,  but CLUSTER VERBOSE would be better to log  which CLUSTER method was used. I wrote a patch to improve CLUSTER

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-08-31 Thread Itagaki Takahiro
Sorry for the very delayed review. On Wed, Jul 21, 2010 at 11:15 PM, Leonardo Francalanci m_li...@yahoo.it wrote: 2) what other areas can I comment more? I think the patch is almost ready to commit, but still have some comments for the usability and documentations. I hope native English

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-07-21 Thread Leonardo Francalanci
I think writetup_rawheap() and readtup_rawheap() are a little complex, but should work as long as there are no padding between t_len and t_self in HeapTupleData struct. - It might be cleaner if you write the total item length and tuple data separately. - (char *) tuple +

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-07-07 Thread Takahiro Itagaki
Leonardo F m_li...@yahoo.it wrote: I saw that you also changed the writing: (snip) Are we sure it's 100% equivalent? I think writetup_rawheap() and readtup_rawheap() are a little complex, but should work as long as there are no padding between t_len and t_self in HeapTupleData struct. - It

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-07-07 Thread Alvaro Herrera
Excerpts from Takahiro Itagaki's message of mié jul 07 04:39:38 -0400 2010: BTW, we could have LogicalTapeReadExact() as an alias of LogicalTapeRead() and checking the result because we have many duplicated codes for unexpected end of data errors. I'd just add a boolean exact required to the

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-07-06 Thread Takahiro Itagaki
Leonardo F m_li...@yahoo.it wrote: Attached the updated patch (should solve a bug) and a script. I reviewed your patch. It seems to be in good shape, and worked as expected. I suppressed a compiler warning in the patch and cleaned up whitespaces in it. Patch attached. I think we need some

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-07-06 Thread Leonardo F
I reviewed your patch. It seems to be in good shape, and worked as expected. I suppressed a compiler warning in the patch and cleaned up whitespaces in it. Patch attached. Thanks for the review! I saw that you also changed the writing: LogicalTapeWrite(state-tapeset, tapenum, tuple,

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-10 Thread Leonardo F
Hi all, while testing the seq scan + sort CLUSTER implementation, I've found a bug in write/readtup_rawheap. The functions are needed by the sort part. The code in http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php didn't copy the whole tuple, but only the HeapTuple header:

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-10 Thread Heikki Linnakangas
Leonardo F wrote: static void writetup_rawheap(Tuplesortstate *state, int tapenum, SortTuple *stup) { HeapTupletuple = (HeapTuple) stup-tuple; I think you're confusing HeapTuple and HeapTupleHeader. SortTuple-tuple field should point to a HeapTupleHeader, not a HeapTuple. The right

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-10 Thread Leonardo F
I think you're confusing HeapTuple and HeapTupleHeader. SortTuple-tuple field should point to a HeapTupleHeader, not a HeapTuple. Mmh, I don't get it: that would mean I might be using more info than required, but I still don't understand why it's not working... at the end, CLUSTER is going to

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-10 Thread Leonardo F
I think I've found the problem: tuple-t_data wasn't at HEAPTUPLESIZE distance from tuple. I guess the code makes that assumption somewhere, so I had to do: tuple-t_data = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE); Now that

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-10 Thread Leonardo F
Perhaps you could supply a .sql file containing a testcase illustrating the performance benefits you tested with your patch Sure. Attached the updated patch (should solve a bug) and a script. The sql scripts generates a 2M rows table (orig); then the table is copied and the copy clustered

I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-09 Thread Leonardo F
Not even a comment? As I said, performance results on my system were very good I know you're all very busy getting 9.0 out, but I think the results in heap scanning + sort instead of index scanning for CLUSTER are very good... I would like to know if I did something wrong/I should improve

Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch

2010-02-09 Thread Josh Kupershmidt
On Tue, Feb 9, 2010 at 5:49 AM, Leonardo F m_li...@yahoo.it wrote: Not even a comment? As I said, performance results on my system were very good Hi Leonardo, Perhaps you could supply a .sql file containing a testcase illustrating the performance benefits you tested with your patch -- as