On Sat, Nov 1, 2014 at 1:45 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)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