Re: Skytools committed without hackers discussion/review

Magnus Hagander <magnus@hagander.net> writes:
> (Assuming it's technically sound - I still haven't checked the actual
> code, but I'm assuming it's Ok since Jan approved it)

I hadn't looked at it either, but here are a few things that need
review:

* Why no binary I/O support for the new datatype?  We tend to expect
that for all core types.

* Why is txid_current_snapshot() excluding subtransaction XIDs?  That
might be all right for the current uses in Slony/Skytools, but it seems
darn close to a bug for any other use.

* Why is txid_current_snapshot() reading SerializableSnapshot rather
than an actually current snap as its name suggests?  This isn't just
misleading, this will fail completely when SerializableSnapshot
goes away, as seems likely to happen in 8.4 (and no, we won't keep it
just because txid might want it).
        regards, tom lane