From: | Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [Patch] RBTree iteration interface improvement |
Date: | 2016-08-26 13:07:44 |
Message-ID: | 20160826130743.GA9505@e733 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello, Heikki.
Thank you for your attention to this patch!
> This also seems to change the API so that instead of a single
> rb_begin_iterate()+rb_iterate() pair, there is a separate begin and
> iterate function for each traversal order. That seems like an unrelated
> change. Was there a particular reason for that? I think the old
> rb_begin_iterate()+rb_iterate() functions were fine, we just need to
> have a RBTreeIterator struct that's different from the tree itself.
I'm trying to avoid calling procedures by a pointer, an old habit. When
I started to work on this patch I just needed a RB-tree implementation
for a pet project. I took one from PostgreSQL code. Then I found this
flaw of iteration interface and decided to fix it. The idea to merge
this fix back to PostgreSQL code appeared later so I just wrote code the
way I like.
These days code performance depends on many factors like whether code
fits into cache, i.e not only on whether you call a procedure directly
or using a pointer. Until someone finds a real bottleneck here I think
current rb_begin_iterate()+rb_iterate() interface should do just fine.
> Another unrelated change in this patch is the addition of
> rb_rightmost(). It's not used for anything, so I'm not sure what the
> point is. Then again, there don't seem to be any callers of
> rb_leftmost() either.
It's just something I needed in tests to reach a good percent of code
coverage. Implementation of rb_rightmost is trivial so we probably can do
without it.
> I think we should something like in the attached patch. It seems to pass
> your test suite, but I haven't done any other testing on this. Does it
> look OK to you?
Looks good to me.
--
Best regards,
Aleksander Alekseev
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2016-08-26 13:16:15 | Re: Showing parallel status in \df+ |
Previous Message | Jürgen Purtz | 2016-08-26 13:06:42 | Unsupported feature F867: WITH TIES |