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
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
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
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
--
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.
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
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.
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
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 !=
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
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
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
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
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.
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
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
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
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
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)
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
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
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
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
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
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
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
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
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
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
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
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
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
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 +
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
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
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
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,
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:
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
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
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
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
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
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
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 something in the patch... I haven't tested it with index
expressions yet (but the tests
Hi all,
attached a patch to do seq scan + sorting instead of index scan
on CLUSTER (when that's supposed to be faster).
As I've already said, the patch is based on:
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php
Of course, the code isn't supposed to be ready to be merged:
Consider multi-column indexes, ie:
CREATE INDEX i_foo ON foo (length(a), length(b));
Ok, I've never thought of expression indexes that way
(in the (expr1,expr2,exprN) form): that is a good example.
Maybe you're confusing expression indexes with partial indexes?
No no, that was exactly
If we ever get another index type that supports ordered
scans, it'll be time enough to worry about cases like this.
Ok
BTW, I think you could use tuplesort_begin_index_btree() rather than
touching _bt_mkscankey_nodata directly.
well I created my own tuplesort_begin_rawheap method
Leonardo F wrote:
why is IndexInfo.ii_Expressions a list? How can an index have more than
one expression? Sorry if it's a stupid question, but I'm not familiar with
index expressions.
Consider multi-column indexes, ie:
CREATE INDEX i_foo ON foo (length(a), length(b));
Maybe you're confusing
Rule it out. Note you should be looking at pg_am.amcanorder, not
hardwiring knowledge of particular index types.
Sorry, I replied ok too fast...
I can look at pg_am.amcanorder, but I would still need the ScanKey to be used
by tuplesort; and I can't find any other way of doing it than
Leonardo F m_li...@yahoo.it writes:
Rule it out. Note you should be looking at pg_am.amcanorder, not
hardwiring knowledge of particular index types.
I can look at pg_am.amcanorder, but I would still need the ScanKey to be used
by tuplesort; and I can't find any other way of doing it than
So my proposal would be: do the test seq_scan vs sort/index_scan only for
regular btree index, and integrate that test in the planner.
Keep in mind that this patch was after the deadline for 9.0, so there
is probably not a huge rush to get this done.
That's true; I'll try to get the
On Thu, Jan 21, 2010 at 4:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:
You're poking into a data structure you shouldn't be poking into:
/* Plans are opaque structs for standard users of SPI */
typedef struct _SPI_plan *SPIPlanPtr;
I hardly think that keeping yourself at arm's length from the
So, if I'm not mistaken:
hash indexes - can't be used in CLUSTER
gin indexes - can't be used in CLUSTER
that leaves:
btree - ok
expression btree - I have to find a way to compute the expression for
each tuple: hints?
gist - how can I get something comparable by tuplesort? Or should I rule it
Leonardo F m_li...@yahoo.it writes:
gist - how can I get something comparable by tuplesort? Or should I rule it
out from the seq scan + sort path?
Rule it out. Note you should be looking at pg_am.amcanorder, not
hardwiring knowledge of particular index types.
Note you should be looking at pg_am.amcanorder, not
hardwiring knowledge of particular index types.
Ok.
Would it make sense to use
FormIndexDatum
to get the index value to be used by tuplesort?
I'm having trouble avoiding the call to _bt_mkscankey_nodata
to get the scanKeys... that
Leonardo F m_li...@yahoo.it writes:
Would it make sense to use
FormIndexDatum
to get the index value to be used by tuplesort?
That would probably work. You might want to look at the code associated
with the recently-added exclusion constraint feature; that also has a
need to reproduce index
: [HACKERS] About Our CLUSTER implementation is pessimal patch
I read the thread Our CLUSTER implementation is pessimal
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php .
I was going to try to add the proper cost_index/cost_sort calls to decide
which
path should be executed
Leonardo F m_li...@yahoo.it wrote:
Anyone? I'd like some feedback before moving on to do the seq scan + sort in
those
CLUSTER cases where use_index_scan returns false...
+1 for CLUSTER using sort.
I have a couple of comments for the current implementation:
* Do we need to disable
one idea could be to actually prepare a query using SPI for select * from
table order by cols and then peek inside to see which plan was generated.
perhaps you could do this using the existing planner hook.
you might have to watch out for the user's rules or planner hooks (though I
don't think
* Do we need to disable sort-path for tables clustered on a gist index?
Yes; as I said in a previous mail, only plain btree indexes (that is, not
custom expression indexes) would have that option (at least in a first
version...)
* I'd prefer to separate cost calculation routines from
Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes:
* I'd prefer to separate cost calculation routines from create_index_path()
and cost_sort(), rather than using a dummy planner.
Don't go that way. The cost functions have enough dependencies on
low-level planner functionality that
one idea could be to actually prepare a query using SPI for select * from
table order by cols and then peek inside
to see which plan was generated.
I like that!!!
Here's a first attempt, it looks like it's working...
(I still have to skip non-btree indexes and expression indexes, plus
add a
Leonardo F m_li...@yahoo.it writes:
one idea could be to actually prepare a query using SPI for select * from
table order by cols and then peek inside
to see which plan was generated.
I like that!!!
Here's a first attempt, it looks like it's working...
(I still have to skip non-btree
By the time you make this actually work in all cases, it's probably
going to be more of a mess than the other way;
I meant to add only ASC/DESC; I would leave all other cases
(non-btrees, custom expression btrees) to use the old index-scan method.
not to mention that it
doesn't work *at
Leonardo F m_li...@yahoo.it writes:
By the time you make this actually work in all cases, it's probably
going to be more of a mess than the other way;
I meant to add only ASC/DESC; I would leave all other cases
(non-btrees, custom expression btrees) to use the old index-scan method.
That
I meant to add only ASC/DESC; I would leave all other cases
(non-btrees, custom expression btrees) to use the old index-scan method.
That hardly seems acceptable.
Well I brought up that in an earlier post:
Leonardo F m_li...@yahoo.it writes:
I hoped that since people mostly (95%?) use plain btree indexes,
a patch that helped CLUSTER with using such indexes would be fine
(at least at first...). I guess that a patch that deals with all other types
of
indexes would be way more complicated (not at
Well, the expression cases would be more likely to cost more if
implemented as a sort, but that doesn't mean that a sort couldn't be a
win. Besides, even if you blow off the expression case, what about
nulls first/last, nondefault opclasses, etc?
Ok, let's split the problem in 2 parts:
a)
On Thu, Jan 21, 2010 at 1:28 PM, Leonardo F m_li...@yahoo.it wrote:
Well, the expression cases would be more likely to cost more if
implemented as a sort, but that doesn't mean that a sort couldn't be a
win. Besides, even if you blow off the expression case, what about
nulls first/last,
Tom Lane t...@sss.pgh.pa.us wrote:
What I do think is that the quoted code snippet has no business being
outside the planner proper. It'd be better to put it in planner.c
or someplace like that.
Ah, I see. My concern was the dummy planner approach is using internal
functions of planner. It
I read the thread Our CLUSTER implementation is pessimal
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php .
I would like to try/integrate that patch as we use CLUSTER a lot on our
system.
I was going to try to add the proper cost_index/cost_sort calls to decide
which
Hi,
I read the thread Our CLUSTER implementation is pessimal
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php .
I would like to try/integrate that patch as we use CLUSTER a lot on our system.
I was going to try to add the proper cost_index/cost_sort calls to decide which
path
Leonardo F wrote:
I read the thread Our CLUSTER implementation is pessimal
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php .
I would like to try/integrate that patch as we use CLUSTER a lot on our
system.
Great!
About that patch:
1) would it be possible to use the
Yeah, I think you could do that, I agree it feels better that way.
You'll still need new copytup and comparetup functions, though, to deal
with HeapTupleHeaders instead of MinimalTuples, or modify the existing
ones to handle both.
You meant HeapTuple, not HeapTupleHeaders, right?
Mmh,
Leonardo F wrote:
Yeah, I think you could do that, I agree it feels better that way.
You'll still need new copytup and comparetup functions, though, to deal
with HeapTupleHeaders instead of MinimalTuples, or modify the existing
ones to handle both.
You meant HeapTuple, not
76 matches
Mail list logo
