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 |
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 |