Re: Skytools committed without hackers discussion/review

On 10/10/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

As I said, the current module is minimal, my goal was to
have code where there is nothing to remove.

But for a data type that targets core, yes binary i/o support
should be added.

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

In queue usage the transaction that stores snapshots does not do
any other work on its own, so it does not matter, main requirement
is that txid_current()/txid_current_snapshot() be symmetric -
whatever the txid_current() outputs, the txid_current_snapshot() measures.

But I agree, supporting subtransactions makes the API more
universal.  And it wouldn't break Slony/PgQ current usage.

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

If you say so.  This code is from original xxid module and
has worked thus far. :)  If the requirement mentioned above
is not broken, the code needs to be brought in line with
current backend coding standards.

Will look into the problems and send patch tomorrow,
today has been too tiring already...

-- 
marko