On 9/30/2007 4:45 PM, Marko Kreen wrote:
> On 9/30/07, Jan Wieck <JanWieck at yahoo.com> wrote:
>> Well, found some time today. I have not checked the module by using it
>> in Slony yet. I'll have some quiet 3 hours later. But I made a couple of
>> changes as follows:
>>
>>    - Adjusted Copyright to 2003-2007
> 
> Ok.
> 
>>    - Renamed USE_BSEARCH_IF_MORE to USE_BSEARCH_IF_NXIP_GREATER
> 
> Ok.
> 
>>    - Compare nxip > ...NXIP_GREATER (instead of >=)
> 
> Ok.
> 
>>    - Removed #ifndef SET_VARSIZE (must have been xxid artifact for
>>      pre-8.3 compatibility)
> 
> I actually tested the code mostly under 8.2.  But for final
> code it should be removed, yes.
> 
>>    - Removed qsort() wrapper function. Only referenced once.
> 
> Although I don't object the change, I liked the wrapper function.
> That way all the is_visible/cmp_txid/sort code was visually
> close together and easy to get overview.
> 
> Also the qsort() takes lot of parameters, having it in the middle
> of a tricky function feels messy.
> 
> You later mentioned call overhead for called-once static functions,
> but I think you are incorrect.  Any sane compiler should inline
> such functions automatically.  Even if not, this is not a hot-path
> for txid code (txid_is_visible_in_snapshot() is.)

Why not declare it inline?

> 
>>    - Don't sort the xip vector if bsearch is never used on it.
> 
> No, this is a bad idea.  That creates two binary incompatible
> on-disk formats, that cannot be converted even with dump/restore.
> (note that the input function enforces sorting thus avoiding the
> need for sort)
> 
> Also, the "if (nxip > USE_BSEARCH_IF_NXIP_GREATER)" before
> sort makes it impossible to later tune the cutoff point.
> 
> I'd like to keep to sortedness as hardwired to format, that
> also allow later move to storing only deltas from xmin, thus
> halving the storage requirements.

Makes sense.

> 
> Only check that fits there is (nxip > 1)
> 
>>    - Use $(MAKE) in Makefile to invoke installcheck (FreeBSD's make is
>>      not gmake).
> 
> Ok.  The 'test' and 'ack' targets are useful shortcuts for
> dev/testing, but they could be removed altogether as they are
> not needed for end-user.
> 
>> Question:
>>
>>    - Should we also move the txid_snapshot parsing functionality directly
>>      into the public function? Like the qsort wrapper, it is only used
>>      once throughout the code and a non-inline function call is not a
>>      zero-cost operation.
> 
> No, I'd prefer to have the code in smaller pieces.  Even if
> compiler won't inline the code automatically, the overhead is
> minor.

Same question about declaring it inline then.

> 
> Unless it's totally critical hot-path, I think possible call
> overhead should not make us write messy code, especially for
> cases where the compiler can very well avoid the overhead.
> 
> 
> For reference I attached v4 of patch, with your changes except
> sorting related applied.  Also fixed a typo in sql.in and put
> the if (nxip > 1) to sort_snapshot().  Now its not so simple
> anymore :)
> 

Good. If I can get Slony actually working with this, I will add it to 
the CVS and we can do the final polishing there.


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck at Yahoo.com #