Re: tracking commit timestamps - Mailing list pgsql-hackers

On 04/11/14 09:19, Michael Paquier wrote:
> On Sat, Nov 1, 2014 at 10:00 PM, Michael Paquier
> <michael.paquier@gmail.com <mailto:michael.paquier@gmail.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