Re: tracking commit timestamps - Mailing list pgsql-hackers

On Wed, Nov 12, 2014 at 10:06 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> Brief list of changes:
>  - the commit timestamp record now stores timestamp, lsn and nodeid
Now that not only the commit timestamp is stored, calling that "commit
timestamp", "committs" or "commit_timestamp" is strange, no? If this
patch is moving toward being a more complex information provider,
calling it commit information or commit data is more adapted, no?
Documentation would need a fresh brush as well in this case.

>  - added code to disallow turning track_commit_timestamp on with too small
> pagesize
>  - the get interfaces error out when track_commit_timestamp is off
OK, that's sane.

>  - if the xid passed to get interface is out of range -infinity timestamp is
> returned (I think it's bad idea to throw errors here as the valid range is
> not static and same ID can start throwing errors between calls
> theoretically)
Already mentioned by Jim in a previous mail: this would be better as NULL.

>  - renamed the sql interfaces to pg_xact_commit_timestamp,
> pg_xact_commit_timestamp_data and pg_last_committed_xact, they don't expose
> the nodeid atm, I personally am not big fan of the "xact" but it seems more
> consistent with existing naming
pg_xact_commit_timestamp and pg_xact_commit_timestamp_data are
overlapping. What's wrong with a single function able to return the
whole set (node ID, commit timetamp, commit LSN)? Let's say
pg_xact_commit_information or pg_xact_commit_data. Already mentioned,
but I also find using a SRF able to return all the available
information from a given XID value quite useful. And this does not
conflict with what is proposed currently, you would need just to call
the function with XID + number of entries wanted to get a single one.
Comments from other folks about that?

>  - documented pg_resetxlog changes and make all the pg_resetxlog options
> alphabetically ordered
>  - added WAL logging of the track_commit_timestamp GUC
>  - added alternative expected output of the regression test so that it works
> with make installcheck when track_commit_timestamp is on
>  - added C interface to set default nodeid for current backend
>  - several minor comment and naming adjustments mostly suggested by Michael
Thanks for those adjustments.

Then more input about the latest patch:
1) This block is not needed, option -e is listed twice:   The <option>-o</>, <option>-x</>, <option>-e</>,
-   <option>-m</>, <option>-O</>,
-   and <option>-l</>
+   <option>-m</>, <option>-O</>, <option>-l</>
+   and <option>-e</>
2) Very small thing: a couple of files have no newlines at the end,
among them committs.conf and test_committs/Makefile.
3) pg_last_committed_xact and not pg_last_xact_commit_information or similar?
4) storage.sgml needs to be updated with the new folder pg_committs
5) Er.. node ID is missing in pg_last_committed_xact, no?
6) This XXX notice can be removed:
+       /*
+        * Return empty if the requested value is older than what we have or
+        * newer than newest we have.
+        *
+        * XXX: should this be error instead?
+        */
We are moving toward returning invalid information in the SQL
interface when the information is not in history instead of an error,
no? (Note that I am still a partisan of an error message to let the
caller know that commit info history does not have the information
requested).
7) Note that TransactionTreeSetCommitTsData still never sets do_xlog
at true and that WriteSetTimestampXlogRec never gets called. So no
information is WAL-logged with this patch. Wouldn't that be useful for
standbys as well? Perhaps I am missing why this is disabled? This code
should be activated IMO or it would be just untested.
8) As a more general point, the node ID stuff makes me uncomfortable
and is just added on top of the existing patch without much
thinking... So I am really skeptical about it. The need here is to
pass on demand a int8 that is a node ID that can only be set through a
C interface, so only extensions could play with it. The data passed to
a WAL record is always built and determined by the system and entirely
transparent to the user, inserting user-defined data like that
inconsistent with what we've been doing until now, no?

Also, a question particularly for BDR and Slony folks: do you
sometimes add a new node using the base backup of an existing node :)
See what I come up with: a duplication of this new node ID system with
the already present system ID, no?
Similarly, the LSN is added to the WAL record containing the commit
timestamp, but cannot the LSN of the WAL record containing the commit
timestamp itself be used as a point of reference for a better
ordering? That's not exactly the same as the LSN of the transaction
commit, still it provides a WAL-based reference.
Regards,
-- 
Michael