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 >=3D)

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

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

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.

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

-- =

marko
-------------- next part --------------
A non-text attachment was scrubbed...
Name: txid-v4.diff.gz
Type: application/x-gzip
Size: 6662 bytes
Desc: not available
Url : http://lists.slony.info/pipermail/slony1-hackers/attachments/20070930=
/046dce35/txid-v4.diff.bin