On Wed, Nov 28, 2012 at 3:21 PM, Andres Freund and...@2ndquadrant.com wrote:
Looks ready to me.
Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your
On 2012-11-27 11:56:41 -0500, Robert Haas wrote:
[ Sorry for the slow response on this, Thanksgiving interfered. ]
On Wed, Nov 21, 2012 at 3:41 PM, Andres Freund and...@2ndquadrant.com wrote:
One very minor nitpick I unfortunately just found now, not sure when
that changed:
[ Sorry for the slow response on this, Thanksgiving interfered. ]
On Wed, Nov 21, 2012 at 3:41 PM, Andres Freund and...@2ndquadrant.com wrote:
One very minor nitpick I unfortunately just found now, not sure when
that changed:
binaryheap_replace_first() hardcodes the indices for the left/right
On 2012-11-20 22:55:52 -0500, Tom Lane wrote:
Abhijit Menon-Sen a...@2ndquadrant.com writes:
While I'm thinking about it, why are the fields of a binaryheap_node
called key and value? That implies a semantics not actually used
here. Maybe value1 and value2 instead?
Yes, I discussed
On Wed, Nov 21, 2012 at 6:08 AM, Andres Freund and...@2ndquadrant.com wrote:
On 2012-11-20 22:55:52 -0500, Tom Lane wrote:
Abhijit Menon-Sen a...@2ndquadrant.com writes:
While I'm thinking about it, why are the fields of a binaryheap_node
called key and value? That implies a semantics not
On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas robertmh...@gmail.com wrote:
I guess I'll take another whack at it.
New version attached.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
binaryheap-rmh2.patch
Description: Binary data
--
Sent via
On 2012-11-21 12:54:30 -0500, Robert Haas wrote:
On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas robertmh...@gmail.com wrote:
I guess I'll take another whack at it.
New version attached.
I think the assert in replace_first should be
Assert(!binaryheap_empty(heap) heap-has_heap_property);
Robert Haas escribió:
On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas robertmh...@gmail.com wrote:
I guess I'll take another whack at it.
New version attached.
The following comments still talk about key and value, thus need
an update:
+ * binaryheap_add_unordered
+ *
+ * Adds the given key
On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
The following comments still talk about key and value, thus need
an update:
Oops.
This comment needs updated (s/comparator/compare/, and also add
has_heap_property and arg):
Fixed.
I would suggest to add
On 2012-11-21 15:09:12 -0500, Robert Haas wrote:
On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
The following comments still talk about key and value, thus need
an update:
Oops.
This comment needs updated (s/comparator/compare/, and also add
At 2012-11-21 15:09:12 -0500, robertmh...@gmail.com wrote:
The following comments still talk about key and value, thus need
an update:
Oops.
In the same vein, Returns NULL if the heap is empty no longer applies
to binaryheap_first and binaryheap_remove_first. Plus there is an extra
On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
[ new patch ]
I went over this again today with a view to getting it committed, but
discovered some compiler warnings that look like this:
warning: cast to pointer from integer of different size
The problem seems to
On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen a...@2ndquadrant.com
wrote:
[ new patch ]
I went over this again today with a view to getting it committed, but
discovered some compiler warnings that look like this:
warning: cast to
On Tue, Nov 20, 2012 at 2:29 PM, Andres Freund and...@2ndquadrant.com wrote:
On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen a...@2ndquadrant.com
wrote:
[ new patch ]
I went over this again today with a view to getting it committed, but
On 2012-11-20 15:06:58 -0500, Robert Haas wrote:
On Tue, Nov 20, 2012 at 2:29 PM, Andres Freund and...@2ndquadrant.com wrote:
On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen a...@2ndquadrant.com
wrote:
[ new patch ]
I went over
On Tue, Nov 20, 2012 at 3:19 PM, Andres Freund and...@2ndquadrant.com wrote:
I don't have a fundamental problem with it, but I don't think it's worth
the cost. If you have variable sized (from the point of the compiler)
values you either have more complex offset computations to the
individual
On 2012-11-20 15:47:25 -0500, Robert Haas wrote:
On Tue, Nov 20, 2012 at 3:19 PM, Andres Freund and...@2ndquadrant.com wrote:
I don't have a fundamental problem with it, but I don't think it's worth
the cost. If you have variable sized (from the point of the compiler)
values you either have
Robert Haas robertmh...@gmail.com writes:
Here's a revised patch that takes the approach of just changing void *
to Datum; it includes a bunch of other cleanups that I felt were
worthwhile.
The comment in binaryheap.h says
* For a max-heap, the comparator must return -1 iff a b, 0 iff a ==
Hi.
A brief response to earlier messages in this thread:
1. I agree that it's a good idea to use Datum rather than void *.
2. I don't think it's worth getting rid of binaryheap_node.
3. I agree that it makes sense to go with what we have now (after
Robert's reworking of my patch) and
Abhijit Menon-Sen a...@2ndquadrant.com writes:
While I'm thinking about it, why are the fields of a binaryheap_node
called key and value? That implies a semantics not actually used
here. Maybe value1 and value2 instead?
Yes, I discussed this with Andres earlier (and considered ptr and value
At 2012-11-20 22:55:52 -0500, t...@sss.pgh.pa.us wrote:
BTW, I probably missed some context upthread, but why do we have two
fields at all?
I would also have preferred to handle the nodeMergeAppend case using a
context pointer as you suggest, but Andres needs to store two pointers
in his heap
On Wed, Nov 14, 2012 at 8:11 AM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
Comments? Suggestions?
It could use a run through pgindent. And the header comments are
separated by a blank line from the functions to which they are
attached, which is not project style.
+ if (heap-size + 1
Robert Haas escribió:
On Wed, Nov 14, 2012 at 8:11 AM, Abhijit Menon-Sen a...@2ndquadrant.com
wrote:
Comments? Suggestions?
It could use a run through pgindent. And the header comments are
separated by a blank line from the functions to which they are
attached, which is not project
Assert(!description of error) is an idiom I've seen more than once,
although I'm not sure I understand why it's not written as Robert says with
the condition in the brackets (or as a print to STDERR followed by abort()
instead).
On 15 November 2012 15:11, Robert Haas robertmh...@gmail.com wrote:
On 11/15/2012 10:11 AM, Robert Haas wrote:
+ {
+ sift_down(heap, i);
+ }
Project style is to omit braces for a single-line body. This comes up
a few other places as well.
I thought we modified that some years ago, although my memory of it is a
bit hazy.
On Thu, Nov 15, 2012 at 10:21 AM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
Other than the coding style issues, I think this looks fine. If you
can fix that up, I believe I could go ahead and commit this, unless
anyone else objects.
Does this include the changes in nodeMergeappend.c?
I
On 2012-11-14 18:41:12 +0530, Abhijit Menon-Sen wrote:
There are two or three places in the Postgres source that implement heap
sort (e.g. tuplesort.c, nodeMergeAppend.c), and it's also needed by the
BDR code. It seemed reasonable to factor out the functionality.
pg_dump also contains a binary
Andrew Dunstan escribió:
On 11/15/2012 10:11 AM, Robert Haas wrote:
+{
+sift_down(heap, i);
+}
Project style is to omit braces for a single-line body. This comes up
a few other places as well.
I thought we modified that some years ago, although my memory of it
On Thu, Nov 15, 2012 at 11:22 AM, Andres Freund and...@2ndquadrant.com wrote:
On 2012-11-14 18:41:12 +0530, Abhijit Menon-Sen wrote:
There are two or three places in the Postgres source that implement heap
sort (e.g. tuplesort.c, nodeMergeAppend.c), and it's also needed by the
BDR code. It
On 2012-11-15 11:37:16 -0500, Robert Haas wrote:
On Thu, Nov 15, 2012 at 11:22 AM, Andres Freund and...@2ndquadrant.com
wrote:
On 2012-11-14 18:41:12 +0530, Abhijit Menon-Sen wrote:
There are two or three places in the Postgres source that implement heap
sort (e.g. tuplesort.c,
On Thu, Nov 15, 2012 at 11:50 AM, Andres Freund and...@2ndquadrant.com wrote:
Me neither. I was thinking about letting the user allocate enough
memory like:
binaryheap *heap = palloc(binaryheap_size(/*capacity*/ 10));
binaryheap_init(heap, 10, comparator);
But thats pretty ugly.
Yeah. I
At 2012-11-15 10:11:58 -0500, robertmh...@gmail.com wrote:
It could use a run through pgindent.
Done.
I think you want Assert(heap-size heap-space).
Of course. Thanks for catching that.
Project style is to omit braces for a single-line body.
I've removed most of them. In the few cases
On Thu, 2012-11-15 at 17:50 +0100, Andres Freund wrote:
Me neither. I was thinking about letting the user allocate enough
memory like:
binaryheap *heap = palloc(binaryheap_size(/*capacity*/ 10));
binaryheap_init(heap, 10, comparator);
But thats pretty ugly.
Why not pass the allocator in
On 2012-11-14 18:41:12 +0530, Abhijit Menon-Sen wrote:
There are two or three places in the Postgres source that implement heap
sort (e.g. tuplesort.c, nodeMergeAppend.c), and it's also needed by the
BDR code. It seemed reasonable to factor out the functionality.
This replaces the
Hi.
There are two or three places in the Postgres source that implement heap
sort (e.g. tuplesort.c, nodeMergeAppend.c), and it's also needed by the
BDR code. It seemed reasonable to factor out the functionality.
I've attached a patch (binaryheap.diff) that contains a straightforward
35 matches
Mail list logo
