Re: Flush some statistics within running transactions - Mailing list pgsql-hackers

> In v5 attached I changed the design so that we don't re-enable the timeout

Thanks!

> I do think we still need it. Indeed in 0004 that helps distinguish between
> anytime flush or mixed flush (with the help of the new pgstat_report_mixed_anytime
> global variable) in pgstat_prep_pending_entry().

Will address below in the comments.

v5-0001:

1/

```
                .write_to_file = true,
+               .flush_mode = FLUSH_ANYTIME,

                .snapshot_ctl_off = offsetof(PgStat_Snapshot, archiver),
```

FLUSH_ANYTIME does not have any impact on Archiver. It does not have
a .flush_static_cb registered and entries are flushed directly to
shared memory via `pgstat_report_archiver` inside the `pgarch_ArchiverCopyLoop`
loop.

FLUSH_ANYTIME needs to be described a bit more that it only applies to
kinds that have flush callbacks.

2/

I suggest simplifying the code comment to this:

```
        /*
-        * Some stats have to be updated only at transaction boundaries (such as
-        * tuples_inserted updated, deleted), so it's very important to set the
-        * right flush mode (FLUSH_AT_TXN_BOUNDARY being the default).
+        * The mode of when to flush stats. See PgStat_FlushMode for
more details.
         */
        PgStat_FlushMode flush_mode;
```

PgStat_FlushMode has sufficient description.


 3/

 we have this pattern:

```
               /* Schedule next anytime stats update timeout */
                if (IsUnderPostmaster &&
!get_timeout_active(ANYTIME_STATS_UPDATE_TIMEOUT))

enable_timeout_after(ANYTIME_STATS_UPDATE_TIMEOUT,
pgstat_flush_interval);

                /* Required for the flush of pending stats WAL data */
                pgstat_report_fixed = true;
```

to report fixed stats. I think it will be good to turn this into a public
routine, i.e. pgstat_report_fixed_entry, that can also be used by extensions
that register custom kinds with ANYTIME flush.

extensions using variable-numbered will get the timeout enabled
via pgstat_prep_pending_entry, per v5-0004.

We may want to add a test in test_custom_stats to ensure
this ANYTIME mechanism can work with custom kinds. What do you think?

v5-0002:

Overall, this GUC seems like a good idea. 10 second default and 1
second minimum are fine.


The patch overall looks solid, and I could not find any issues.

I do have a suggestion about the documentation:

```
       </term>
       <listitem>
        <para>
-        Sets the interval at which non-transactional statistics are
made visible
-        during running transactions. Non-transactional statistics include, for
-        example, WAL activity and I/O operations.
-        They become visible at that interval in monitoring views such as
-        <link linkend="monitoring-pg-stat-io-view">
<structname>pg_stat_io</structname></link>
-        and <link linkend="monitoring-pg-stat-wal-view">
<structname>pg_stat_wal</structname></link>
-        during running transactions.
-        If this value is specified without units, it is taken as milliseconds.
-        The default is 10 seconds (<literal>10s</literal>), which is probably
-        about the smallest value you would want in practice for long running
-        transactions.
+        Sets the interval at which statistics that can be updated
while a transaction is still running
+        are made visible. These include, for example, WAL activity
and I/O operations.
+        Such statistics are refreshed at the specified interval and
can be observed during active
+        transactions in monitoring views such as
+        <link linkend="monitoring-pg-stat-io-view"><structname>pg_stat_io</structname></link>
and
+        <link linkend="monitoring-pg-stat-wal-view"><structname>pg_stat_wal</structname></link>.
+        Other statistics are only made visible at transaction end and
are not affected by this setting.
+        If the value is specified without a unit, milliseconds are assumed.
+        The default is 10 seconds (<literal>10s</literal>), which is
generally the smallest practical
+        value for long-running transactions.
        </para>
        <note>
-        <para>
-         This parameter does not affect transactional statistics such as
-         <structname>pg_stat_all_tables</structname> columns (like
-         <structfield>n_tup_ins</structfield>,
<structfield>n_tup_upd</structfield>,
-         <structfield>n_tup_del</structfield>), which are always
flushed at transaction
-         boundaries to maintain consistency.
-        </para>
+        <para> This parameter does not affect statistics that are
only reported at transaction end,
+        such as the columns of <structname>pg_stat_all_tables</structname>
+        (for example, <structfield>n_tup_ins</structfield>,
<structfield>n_tup_upd</structfield>,
+        and <structfield>n_tup_del</structfield>). These statistics
are always flushed at the end of
+        a transaction. </para>
        </note>
       </listitem>
      </varlistentry>
```

specifically, I want to avoid using "non-transactional", "transaction
boundary" terms, as
they may be confusing.

v5-0003: looks straightforward. I have no comments.

v5-0004:

/1

+       FLUSH_MIXED,                            /* Mix of fields that
can be flushed anytime
+                                                                * or
only at transaction boundary */

I still don't think this is needed. I fail to see what value it adds.
When we set pgstat_report_mixed_anytime inside the pgstat_count functions,
which make it clear which of the fields are anytime fields.


2/

This is an oversight comment, it seems.

```
 extern PGDLLIMPORT bool pgstat_report_fixed;

+/* Track if mixed anytime stats need to be flushed */
+
 /* Backend-local stats state */
 extern PGDLLIMPORT PgStat_LocalState pgStatLocal;
```

3/

```
+       tabentry->numscans += lstats->counts.numscans;
+       if (lstats->counts.numscans)
+       {
+               TimestampTz t = GetCurrentTimestamp();
```

Considering the 10 second default, GetCurrentTimestamp() should not be
an issue here.

The doc does need to be updated. From:

"The time of the last sequential scan on this table, based on the most
recent transaction stop time"

To this?

"The approximate time of the last sequential scan on this table,
updated at least every STATS_FLUSH_INTERVAL"

same for `last_index_scan`

4/
nit: /non partial flush/non-partial flush/



--
Sami Imseih
Amazon Web Services (AWS)