On Wed, Nov 28, 2012 at 3:21 PM, Andres Freund 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 subscription:
http://
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 wrote:
> > One very minor nitpick I unfortunately just found now, not sure when
> > that changed:
> > binaryheap_replace_first()
[ Sorry for the slow response on this, Thanksgiving interfered. ]
On Wed, Nov 21, 2012 at 3:41 PM, Andres Freund 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 node
> of the root n
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 ex
On 2012-11-21 15:09:12 -0500, Robert Haas wrote:
> On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera
> 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 an
On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera
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 prefixes to struct memb
Robert Haas escribió:
> On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas 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 and value to the
On 2012-11-21 12:54:30 -0500, Robert Haas wrote:
> On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas 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);
instead of
Assert(
On Wed, Nov 21, 2012 at 9:46 AM, Robert Haas 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 pgsql-hackers mailing list
On Wed, Nov 21, 2012 at 6:08 AM, Andres Freund wrote:
> On 2012-11-20 22:55:52 -0500, Tom Lane wrote:
>> Abhijit Menon-Sen 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. Ma
On 2012-11-20 22:55:52 -0500, Tom Lane wrote:
> Abhijit Menon-Sen 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 thi
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 hea
Abhijit Menon-Sen 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
> or
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 reconside
Robert Haas 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 == b,
* and +1 iff a
On 2012-11-20 15:47:25 -0500, Robert Haas wrote:
> On Tue, Nov 20, 2012 at 3:19 PM, Andres Freund 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 of
On Tue, Nov 20, 2012 at 3:19 PM, Andres Freund 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 elements or one mor
On 2012-11-20 15:06:58 -0500, Robert Haas wrote:
> On Tue, Nov 20, 2012 at 2:29 PM, Andres Freund wrote:
> > On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
> >> On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen
> >> wrote:
> >> > [ new patch ]
> >>
> >> I went over this again today with a vie
On Tue, Nov 20, 2012 at 2:29 PM, Andres Freund wrote:
> On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
>> On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen
>> wrote:
>> > [ new patch ]
>>
>> I went over this again today with a view to getting it committed, but
>> discovered some compiler warn
On 2012-11-20 14:03:12 -0500, Robert Haas wrote:
> On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen
> 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 int
On Thu, Nov 15, 2012 at 8:56 PM, Abhijit Menon-Sen 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 be that the binary
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 alloc
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 ca
On Thu, Nov 15, 2012 at 11:50 AM, 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.
Yeah. I would vote agai
On 2012-11-15 11:37:16 -0500, Robert Haas wrote:
> On Thu, Nov 15, 2012 at 11:22 AM, Andres Freund
> 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
On Thu, Nov 15, 2012 at 11:22 AM, Andres Freund 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 seemed reasonable t
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 m
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 bina
On Thu, Nov 15, 2012 at 10:21 AM, Alvaro Herrera
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 think so. I was
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. Personall
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 wrote:
> On Wed, Nov 14,
Robert Haas escribió:
> On Wed, Nov 14, 2012 at 8:11 AM, Abhijit Menon-Sen
> 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.
Also ther
On Wed, Nov 14, 2012 at 8:11 AM, Abhijit Menon-Sen 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 == heap->space)
+
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 "simplehea
34 matches
Mail list logo
