On Mon, Feb 19, 2024 at 10:26:43AM +0100, Tomas Vondra wrote:
> On 2/19/24 07:57, Michael Paquier wrote:
> > On Sun, Feb 18, 2024 at 02:28:06AM +0100, Tomas Vondra wrote:
>>> 1) Do we really need to modify RecordTransactionCommitPrepared and
>>> XactLogCommitRecord to return the LSN of the commit record? Can't we
>>> just use XactLastRecEnd? It's good enough for advancing
>>> replorigin_session_origin_lsn, why wouldn't it be good enough for this?
>>
>> Or XactLastCommitEnd?
>
> But that's not set in RecordTransactionCommitPrepared (or twophase.c in
> general), and the patch seems to need that.
Hmm. Okay.
> > The changes in AtEOXact_PgStat() are not really
> > attractive for what's a static variable in all the backends.
>
> I'm sorry, I'm not sure what "changes not being attractive" means :-(
It just means that I am not much a fan of the signature changes done
for RecordTransactionCommit() and AtEOXact_PgStat_Database(), adding a
dependency to a specify LSN. Your suggestion to switching to
XactLastRecEnd should avoid that.
> - stats_fetch_consistency=cache: always the same min/max value
>
> - stats_fetch_consistency=none: min != max
>
> Which would suggest you're right and this should be VOLATILE and not
> STABLE. But then again - the existing pg_stat_get_db_* functions are all
> marked as STABLE, so (a) is that correct and (b) why should this
> function be marked different?
This can look like an oversight of 5891c7a8ed8f to me. I've skimmed
through the threads related to this commit and messages around [1]
explain why this GUC exists and why we have both behaviors, but I'm
not seeing a discussion about the volatibility of these functions.
The current situation is not bad either, the default makes these
functions stable, and I'd like to assume that users of this GUC know
what they do. Perhaps Andres or Horiguchi-san can comment on that.
https://www.postgresql.org/message-id/382908.1616343275@sss.pgh.pa.us
--
Michael
