|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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 .
> 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
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?
|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|