On 04/11/14 09:19, Michael Paquier wrote:
On Sat, Nov 1, 2014 at 10:00 PM, Michael Paquier
<michael.paqu...@gmail.com <mailto:michael.paqu...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
