Re: tracking commit timestamps

Hi,

thanks for review.

On 01/11/14 05:45, Michael Paquier wrote:
> Now here are a couple of comments at code level, this code seems not
> enough baked for a commit:
> 1) The following renaming should be done:
> - pg_get_transaction_committime to pg_get_transaction_commit_time
> - pg_get_transaction_extradata to pg_get_transaction_extra_data
> - pg_get_transaction_committime_data to pg_get_transaction_commit_time_data
> - pg_get_latest_transaction_committime_data to
> pg_get_latest_transaction_commit_time_data

Makes sense.

> 3) General remark: committs is not a name suited IMO (ts for
> transaction??). What if the code is changed to use commit_time instead?
> This remark counts as well for the file names committs.c and committs.h,
> and for pg_committs.

The ts is for timestamp, tx would be shorthand for transaction. Looking
at your remarks, it seems there is some general inconsistency with time
vs timestamp in this patch, we should pick one and stick with it.

> 6) To be consistent with everything, shouldn't track_commit_timestamp be
> renamed to track_commit_time

(see above)

> 9) Hm?! "oldest commit time xid", no?
> - "oldest running xid %u;
> %s",
> + "oldest CommitTs xid:
> %u; oldest running xid %u; %s",

Again, timestamp vs time.

> 12) In committs_desc(at)committsdesc(dot)c, isn't this block overthinking a bit:
> + else
> + appendStringInfo(buf, "UNKNOWN");
> It may be better to remove it, no?

Should be safe, indeed.

> 13) Isn't that 2014?
> + * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group

Hah, I forgot to update that (shows how long this patch has been waiting
:) )

> 16) make installcheck (test committs_off) fails on a server that has
> track_committs set to on. You should use an alternate output. I would

Well, it is supposed to fail, that's the whole point, the output should
be different depending on the value of the GUC.

> recommend removing as well the _off suffix in the test name. Let's use
> commit_time. Also, it should be mentioned in parallel_schedule with a
> comment that this test should always run alone and never in parallel
> with other tests. Honestly, I also think that test_committs brings no
> additional value and results in duplication code between
> src/test/regress and contrib/test_committs. So I'd just rip it off. On

Those tests are different though, one tests that the default (off) works
as expected the contrib one tests that the feature when turned on works
as expected. Since we can only set config values for contrib tests I
don't see how else to do this, but I am open to suggestions.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services