From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Geoghegan <pg(at)heroku(dot)com> |
Cc: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Is tuplesort_heap_siftup() a misnomer? |
Date: | 2016-09-08 19:46:46 |
Message-ID: | 24976.1473364006@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Peter Geoghegan <pg(at)heroku(dot)com> writes:
> Attached patch does it that way, then. I stuck with the reference to
> "shift down", though, since I think we all agree that that is
> unambiguous.
I dunno. What you've now got is
/*
* The tuple at state->memtuples[0] has been removed from the heap.
- * Decrement memtupcount, and sift up to maintain the heap invariant.
+ * Decrement memtupcount, and shift down to maintain the heap invariant.
*/
static void
-tuplesort_heap_siftup(Tuplesortstate *state, bool checkIndex)
+tuplesort_heap_delete_top(Tuplesortstate *state, bool checkIndex)
I don't find this clearer at all, because
(1) As noted in the comment, the heap top has *already* been removed,
we're just trying to re-establish the heap invariant. So maybe
"delete_top" isn't the best name after all.
(2) "shift down" seems a bit weird, since we're moving data closer to
the heap top, not further away from it; why isn't that direction "up"?
(3) "shift" makes it sound like it ought to be a simple memmove
kind of operation, which it surely is not.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Vladimir Gordiychuk | 2016-09-08 19:56:03 | Re: Stopping logical replication protocol |
Previous Message | Corey Huinker | 2016-09-08 19:44:45 | Re: Let file_fdw access COPY FROM PROGRAM |