Re: [Patch] RBTree iteration interface improvement

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru>
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 11:53:11
Message-ID: d3ff2dd6-beea-b247-4426-3b8e5232f7b0@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
rbtree-iteration-improvement-heikki-v1.patch application/x-patch 13.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-08-26 11:53:31 Re: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice()
Previous Message Amit Kapila 2016-08-26 11:49:18 Re: Write Ahead Logging for Hash Indexes