On Sat, Nov 1, 2014 at 1:45 PM, Michael Paquier <michael.paqu...@gmail.com> wrote:
> I am still planning to do more extensive tests, and study a bit more > committs.c (with even more comments) as it is the core part of the feature. > More comments: - Heikki already mentioned it, but after reading the code I see little point in having the extra field implementing like that in core for many reasons even if it is *just* 4 bytes: 1) It is untested and actually there is no direct use for it in core. 2) Pushing code that we know as dead is no good, that's a feature more or less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it. 3) If you're going to re-use this API in BDR, which is a fork of Postgres. You'd better complete this API in BDR by yourself and not bother core with that. For those reasons I think that this extra field should be ripped off from the patch. - The API to get the commit timestamp is not that user-friendly, and I think it could really be improved, to something like that for example: pg_get_commit_timestamp(from_xact xid, number_of_xacts int); pg_get_commit_timestamp(from_xact xid); pg_get_commit_timestamp(); or pg_get_latest_commit_timestamp(); from_xact to NULL means latest. number_of_xacts to NULL means 1. Comment in line with the fact that extra field is well, not really useful. Regards, -- Michael
