Re: Commit Timestamp and LSN Inversion issue

On 11/6/24 21:30, Zhijie Hou (Fujitsu) wrote:
>
> Thanks for the patch! I am reading the patch and noticed few minor things.
>
> 1.
> + /*
> + * This is a local transaction. Make sure that the xact_time
> + * higher than any timestamp we have seen thus far.
> + *
> + * TODO: This is not postmaster restart safe. If the local
> + * system clock is further behind other nodes than it takes
> + * for the postmaster to restart (time between it stops
> + * accepting new transactions and time when it becomes ready
> + * to accept new transactions), local transactions will not
> + * be bumped into the future correctly.
> + */
>
> The TODO section mentions other nodes, but I believe think patch currently do
> not have the handling of timestamps for other nodes. Should we either remove
> this part or add a brief explanation to clarify the relationship with other
> nodes?

That TODO is actually obsolete. I understood from Amit Kapila that the
community does assume that NTP synchronization is good enough. And it
indeed is. Even my servers here at home are using a GPS based NTP server
connected to the LAN and are usually in sync by single-digit
microseconds. I removed it.

>
> 2.
> +/*
> + * Hook function to be called while holding the WAL insert spinlock.
> + * to adjust commit timestamps via Lamport clock if needed.
> + */
>
> The second line seems can be improved:
> "to adjust commit timestamps .." => "It adjusts commit timestamps ..."

How about

/*
* Hook function to be called while holding the WAL insert spinlock.
* It guarantees that commit timestamps are advancing in LSN order.
*/
static void EnsureMonotonicTransactionStopTimestamp(void *data);

Thank you for looking at this and your input. New patch attached.

Best Regards, Jan