Date: Wed, 21 Jan 2026 10:34:09 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org,
	Zsolt Parragi <zsolt.parragi@percona.com>
Subject: Re: Flush some statistics within running transactions
Message-ID: <aXCroXBLiUpxk0Ap@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aWTVEycKj7Qh/SXH@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0vgN8nf7yPrk_uNt-ABFtRnRuz7-9CM=qSzUSwj=EEUKw@mail.gmail.com>
 <aWjbBd5z+LJNWX1l@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0t6j0VYuUpxZ8JLq-ERoUriZ0rK=+8PCUtjRirmSmCx7A@mail.gmail.com>
 <aWojE18aF9qj0EPI@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com>
 <aW4gwqVybB4INCvu@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0s6FkEHFdgKf8J6vueZGwsH+08LvV0YPBXa4Dw_8QgtTw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="lCc1MWfC7i2uqs8g"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0s6FkEHFdgKf8J6vueZGwsH+08LvV0YPBXa4Dw_8QgtTw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aXCroXBLiUpxk0Ap%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--lCc1MWfC7i2uqs8g
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Tue, Jan 20, 2026 at 01:27:55PM -0600, Sami Imseih wrote:
> I have some more comments:

Thanks!

> -- v2-0001
> 
> #1.
> 
> +/* When to call pgstat_report_anytime_stat() again */
> +#define PGSTAT_ANYTIME_FLUSH_INTERVAL       1000
> +
> 
> We should just use PGSTAT_MIN_INTERVAL.

Okay, done. We can still switch to a dedicated one if we feel the need later on.

> #2.
> 
> instead of ".flush_behavior", maybe ".flush_mode"? "mode" in the name is better
> for configuration fields.

Sounds good.

> #3.
> 
> FLUSH_AT_TXN_BOUNDARY should be the first value in PgStat_FlushBehavior.
> Otherwise kinds ( built-in or custom ) that do not specify a flush_behavior
> will default to FLUSH_ANYTIME. I don't think this is what we want.
> FLUSH_AT_TXN_BOUNDARY should be the default.

Good point, agreed and done.

> #4. Can we add a test here? Maybe generate some wal inside a long
> running transaction and
> make sure the stats are updated after > 1 second

I'm not sure, that's also somehow the purpose of 0002 (with 039549d70f6 being
reverted).

0001 and 0002 could be merged and pushed as one commit. That said I'm not opposed
if you feel strongly about it.

> -- v2-0003
> 
> #1. Should we maybe make this a bit longer? maybe 2 or 3 seconds?
> May make the tests slightly longer, but maybe better for test stability.
> 
> ```
> +step s1_sleep: SELECT pg_sleep(1.5);
> +pg_sleep
> +--------
> ```

Not sure, we could increase if we see the test failing.

> #2.
> +       /*
> +        * Check if there are any non-transactional stats to flush. Avoid
> +        * unnecessarily locking the entry if nothing accumulated.
> +        */
> +       if (lstats->counts.numscans > 0 ||
> +               lstats->counts.tuples_returned > 0 ||
> +               lstats->counts.tuples_fetched > 0 ||
> +               lstats->counts.blocks_fetched > 0 ||
> +               lstats->counts.blocks_hit > 0)
> +               has_nontxn_stats = true;
> +
> +       if (!has_nontxn_stats)
> +               return true;

> Can we just do this without a has_nontxn_stats?

Yeah.

> #3.
> +    are updated while the transactions are in progress. This means
> that we can see
> +    those statistics being updated without having to wait until the transaction
> +    finishes.
> +   </para>
> 
> The "This means ...... " line used several times does not add value, IMO.
> "are updated while the transactions are in progress." is sufficient.

Removed.

> 
> #4.
> +  <note>
> +   <para>
> +    All the statistics are updated while the transactions are in
> progress, except
> +    for <structfield>xact_commit</structfield>,
> <structfield>xact_rollback</structfield>,
> +    <structfield>tup_inserted</structfield>,
> <structfield>tup_updated</structfield> and
> +    <structfield>tup_deleted</structfield> that are updated only when
> the transactions
> +    finish.
> +   </para>
> +  </note>
> 
> Only these 5 fields from pgstat_relation_flush_anytime_cb, so only the below are
> "All the statistics are updated while the transactions are in progress", right?
> 
> numscans
> tuples_returned
> tuples_fetched
> blocks_fetched
> blocks_hit

No, 0003 also changes the flush mode for the database KIND. All the fields that
I mentioned are inherited from relations stats and are flushed only at transaction
boundaries (so they don't appear in pg_stat_database until the transaction
finishes). Does that make sense? (if the database kind is not switched to 
flush any time then none would appear while the transaction is in progress, even
the ones inherited from relations stats).

PFA v3, also taking care of Zsolt's comment (thanks!) done up-thread.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--lCc1MWfC7i2uqs8g
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Add-pgstat_report_anytime_stat-for-periodic-stats.patch"