On 10/1/07, Tom Lane <tgl at sss.pgh.pa.us> wrote:
> "Marko Kreen" <markokr at gmail.com> writes:
> > For reference I attached v4 of patch, with your changes except
> > sorting related applied.
>
> A few random comments:
>
> +++ b/contrib/txid/Makefile
> +
> +
> +test: install
> +       $(MAKE) installcheck || { less regression.diffs; exit 1; }
> +
> +ack:
> +       cp results/* expected/
>
> Please drop these random, unportable makefile targets.

Ok.

> +++ b/contrib/txid/README.txid
>
> Needs some spellchecking...

Some more explanations too.  But I hope we can work on README
after merge?

> +++ b/contrib/txid/txid.c
>
> +convert_xid(TransactionId xid, const TxidEpoch *state)
> +{
> +       uint64 epoch;
> +
> +       /* avoid issues with the the special meaning of 0 */
> +       if (xid == InvalidTransactionId)
> +               return MAX_TXID;
> +
> +       /* return special xid's as-is */
> +       if (xid < FirstNormalTransactionId)
> +               return xid;
>
> I'm having quite a bit of a problem with the above.  Why is
> InvalidTransactionId mapped to MAX_TXID, which presumably is part of the
> normal XID rotation and hence only larger than half of the universe,
> when the other special XIDs map as themselves?

Because InvalidTransactionId is supposed to be always invisible,
but others always visible?  And we don't want to add epoch to
any of them.

MAX_TXID - the txid's are externally signed int8-s, so it's the
largest value in our universe.  I really like to avoid creating
special unsigned type for txid's.  I don't see the need for that.

I don't expect that txid.c code ever sees special xid's, but
I added the support for then anyway, just to be safe.

> +/*
> + * helper functions to use StringInfo for TxidSnapshot creation.
> + */
>
> I'm kind of wondering where this idea came from at all --- it seems
> like the very worst sort of impedance mismatch.  StringInfo really
> is not intended for non-textual data...

Just to get rid of inline buffer management code mixed with
string parsing.  Although, as I have now factored out the buffer
handling code into separate functions, we could do it by hand
again in the special functions.  Do you want such change?

I used StringInfo as it was only buffer tool available from backend...

-- 
marko