On 10/10/07, Tom Lane <[EMAIL PROTECTED]> wrote: > Magnus Hagander <[EMAIL PROTECTED]> 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 ---------------------------(end of broadcast)--------------------------- TIP 5: don't forget to increase your free space map settings
