Re: tracking commit timestamps

On 04/11/14 09:19, Michael Paquier wrote:
> On Sat, Nov 1, 2014 at 10:00 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com <mailto:michael(dot)paquier(at)gmail(dot)com>> wrote:
>
> More comments:
>
> Here are also more comments about the code that I found while focusing
> on committs.c:
> 1) When the GUC is not enabled, and when the transaction ID provided is
> not in a correct range, a dummy value based on InvalidTransactionId (?!)
> is returned, like that:
> + /* Return empty if module not enabled */
> + if (!commit_ts_enabled)
> + {
> + if (ts)
> + *ts = InvalidTransactionId;
> + if (data)
> + *data = (CommitExtraData) 0;
> + return;
> + }
> This leads to some incorrect results:
> =# select pg_get_transaction_committime('1');
> pg_get_transaction_committime
> -------------------------------
> 2000-01-01 09:00:00+09
> (1 row)

Oh, I had this on my TODO and somehow forgot about it, I am leaning
towards throwing an error when calling any committs "get" function while
the tracking is disabled.

> Or for example:
> =# SELECT txid_current();
> txid_current
> --------------
> 1006
> (1 row)
> =# select pg_get_transaction_committime('1006');
> pg_get_transaction_committime
> -------------------------------
> 2014-11-04 14:54:37.589381+09
> (1 row)
> =# select pg_get_transaction_committime('1007');
> pg_get_transaction_committime
> -------------------------------
> 2000-01-01 09:00:00+09
> (1 row)
> =# SELECT txid_current();
> txid_current
> --------------
> 1007
> (1 row)
> And at other times error is not that helpful for user:
> =# select pg_get_transaction_committime('10000');
> ERROR: XX000: could not access status of transaction 10000
> DETAIL: Could not read from file "pg_committs/0000" at offset 114688:
> Undefined error: 0.
> LOCATION: SlruReportIOError, slru.c:896
> I think that you should simply return an error in
> TransactionIdGetCommitTsData when xid is not in the range supported.
> That will be more transparent for the user.

Agreed.

> 5) Reading the code, TransactionTreeSetCommitTimestamp is always called
> with do_xlog = false, making that actually no timestamps are WAL'd...
> Hence WriteSetTimestampXlogRec is just dead code with the latest version
> of the patch. IMO, this should be always WAL-logged when
> track_commit_timestamp is on.

As Andres explained this is always WAL-logged when called by current
caller so we don't want it to be double logged, so that's why do_xlog =
false, but when extension will need call it, it will most likely need
do_xlog = true.

> 8) The redo and xlog routines of committs should be out in a dedicated
> file, like committsxlog.c or similar. The other rmgr do so, so let's be
> consistent.
>

Most actually don't AFAICS.

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