Re: [COMMITTERS] pgsql: Keep track of transaction commit timestamps - Mailing list pgsql-hackers

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:
> ir commit timestamp directly as they commit,
> or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this 
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused. 
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is 
dead code too, when a non-default node_id is not set. Please remove, or 
explain how all that's supposed to work.

* COMMIT_TS_SETTS. "SETTS" sounds like a typo (like Robert complained 
about "committs" earlier). Rename to "COMMIT_TS_SET_TIMESTAMP", or just 
"COMMIT_TS_SET".

* committsdesc.c -> commit_ts_desc.c, to be consistent with "commit_ts.c"

* In commit_ts_desc:

> +       nsubxids = ((XLogRecGetDataLen(record) - SizeOfCommitTsSet) /
> +                   sizeof(TransactionId));
> +       if (nsubxids > 0)
> +       {
> +           int     i;
> +           TransactionId *subxids;
> +
> +           subxids = palloc(sizeof(TransactionId) * nsubxids);
> +           memcpy(subxids,
> +                  XLogRecGetData(record) + SizeOfCommitTsSet,
> +                  sizeof(TransactionId) * nsubxids);
> +           for (i = 0; i < nsubxids; i++)
> +               appendStringInfo(buf, ", %u", subxids[i]);
> +           pfree(subxids);
> +       }

There's no need to memcpy() here. The subxid array in the WAL record is 
aligned.

* This seems to be a completely unrelated change.

> --- a/src/backend/libpq/hba.c
> +++ b/src/backend/libpq/hba.c
> @@ -1438,7 +1438,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
>                 ereport(LOG,
>                         (errcode(ERRCODE_CONFIG_FILE_ERROR),
>                          errmsg("client certificates can only be checked if a root certificate store is available"),
> -                        errhint("Make sure the configuration parameter \"ssl_ca_file\" is set."),
> +                        errhint("Make sure the configuration parameter \"%s\" is set.", "ssl_ca_file"),
>                          errcontext("line %d of configuration file \"%s\"",
>                                     line_num, HbaFileName)));
>                 return false;


* What is the definition of "latest commit", in 
pg_last_committed_xact()? It doesn't necessarily line up with the order 
of commit WAL records, nor with the order the proc array is updated 
(i.e. the order that the effects become visible to other backends). It 
seems confusing to add yet another notion of commit order. Perhaps 
that's the best we can do, but it needs to be documented.

- Heikki