Hi,
> I simplified the implementation to use GetCurrentTimestamp() directly
> instead of calculating the timestamp from
I don't think it will be acceptable to add GetCurrentTimestamp() at the end of
every execution. We take extra measures to avoid such overhead in core, and
I don't think we should impose this on pg_stat_statements users as well.
For example, in c037471832e which tracks lastscan:
"
To make it easier to detect the last time a relation has been scanned, track
that time in each relation's pgstat entry. To minimize overhead a) the
timestamp is updated only when the backend pending stats entry is flushed to
shared stats b) the last transaction's stop timestamp is used as the
timestamp.
"
My opinion is still that using the last_start_time will be useful for monitoring
as you demonstrated [0], albeit with some complexity involved for the
monitoring client,
will be the best way and safest way forward.
Others may disagree.
> GetCurrentStatementStartTimestamp() + total_time. The previous optimization
> was
> premature, benchmark testing proves GetCurrentTimestamp() adds no
> measurable overhead.
I am not sure if this benchmark is enough to build confidence in this approach.
Workloads with nested queries and tack. 'all' are now paying an even heavier
cost.
Also, quickly looking at v4, you probably want to call GetCurrentTimestamp()
when kind == PGSS_EXEC only.
Assert(kind == PGSS_PLAN || kind == PGSS_EXEC);
+ /*
+ * Get current timestamp before acquiring spinlock to avoid holding
+ * the lock during syscall.
+ */
+ stats_updated_at = GetCurrentTimestamp();
+
/*
[0]
https://www.postgresql.org/message-id/ema0100e49-522e-4b8f-9c25-9257af1f0793%40cybertec.at
--
Sami Imseih
Amazon Web Services (AWS)
