Re: [Patch] RBTree iteration interface improvement

On 08/22/2016 01:00 PM, Aleksander Alekseev wrote:
>>> It seems clear to me that the existing arrangement is hazardous for
>>> any RBTree that hasn't got exactly one consumer.  I think
>>> Aleksander's plan to decouple the iteration state is probably a good
>>> one (NB: I've not read the patch, so this is not an endorsement of
>>> details).  I'd go so far as to say that we should remove the old API
>>> as being dangerous, rather than preserve it on
>>> backwards-compatibility grounds.  We make bigger changes than that in
>>> internal APIs all the time.
>>
>> Glad to hear it! I would like to propose a patch that removes the old
>> API. I have a few questions though.
>>
>> Would you like it to be a separate patch, or all-in-one patch is fine
>> in this case? I also would like to include unit/property-based tests in
>> this (these) patch (patches). However as I understand there are
>> currently no unit tests in PostgreSQL at all, only integration/system
>> tests. Any advice how to do it better?
>
> OK, here is a patch. I think we could call it a _replacement_ of an old API, so
> there is probably no need in two separate patches. I still don't know how to
> include unit test and whether they should be included at all. Thus for now
> unit/property-based tests could be found only on GitHub [1].
>
> As usual, if you have any questions, suggestions or notes regarding this patch,
> please let me know.

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.

Note that the API actually used to be like that. rb_begin_iterate() used
to return a palloc'd RBTreeIterator struct. It was changed in commit
0454f131, because the implementation didn't support more than one
iterator at a time, which made the separate struct pointless. But we're
fixing that problem now.

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.

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?

- Heikki