Re: Add last_commit_lsn to pg_stat_database

On 2/19/24 07:57, Michael Paquier wrote:
> On Sun, Feb 18, 2024 at 02:28:06AM +0100, Tomas Vondra wrote:
>> Thanks for the updated patch. I don't have a clear opinion on the
>> feature and whether this is the way to implement it, but I have two
>> simple questions.
> 
> Some users I know of would be really happy to be able to get this
> information for each database in a single view, so that feels natural
> to plug the information into pg_stat_database.
> 

OK

>> 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.

> 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 :-(

>> 2) Earlier in this thread it was claimed the function returning the
>> last_commit_lsn is STABLE, but I have to admit it's not clear to me why
>> that's the case :-( All the pg_stat_get_db_* functions are marked as
>> stable, so I guess it's thanks to the pgstat system ...
> 
> The consistency of the shared stats data depends on
> stats_fetch_consistency.  The default of "cache" makes sure that the
> values don't change across a scan, until the end of a transaction.
> 
> I have not paid much attention about that until now, but note that it
> would not be the case of "none" where the data is retrieved each time
> it is requested.  So it seems to me that these functions should be
> actually volatile, not stable, because they could deliver different
> values across the same scan as an effect of the design behind
> pgstat_fetch_entry() and a non-default stats_fetch_consistency.  I may
> be missing something, of course.

Right. If I do this:

create or replace function get_db_lsn(oid) returns pg_lsn as $$
declare
  v_lsn pg_lsn;
begin
  select last_commit_lsn into v_lsn from pg_stat_database
   where datid = $1;
  return v_lsn;
end; $$ language plpgsql;

select min(l), max(l) from (select i, get_db_lsn(16384) AS l from
generate_series(1,100000) s(i)) foo;

and run this concurrently with a pgbench on the same database (with OID
16384), I get:

- 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?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company