> PFA attached v6, addressing the reviews comments.
Thanks for the patches!
v6 is getting closer IMO. Here are some comments I have.
v6-0001 looks solid, but some minor comments:
1/
+ pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_WAL, true);
let's also use explicit (void) cast here
2/
+ * Timeout handler for flushing non-transactional stats.
I also noticed in v6-0005, we refer to "anytime" stats as
"non-transactional". It's better to just refer to them as "anytime"
anywhere instead of "non-transactional:.
v6-0002:
+ if (nowait && !LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
+ return true; /* failed to flush */
+
+ LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
Here the LWLock acquisition will deadlock if we are nowait, successfully
acquire the lock conditionally, and then we try to acquire it again. The
logic here should not try to acquire the lock twice.
+ -- Force anytime flush (inside transaction!)
+ select pg_stat_force_anytime_flush();
Not sure why we need pg_stat_force_anytime_flush.
A pg_sleep is sufficient, like below. right?
```
select test_custom_stats_fixed_anytime_update() from generate_series(1, 2);
select pg_sleep(1.5);
select 'anytime:'||numcalls from test_custom_stats_fixed_report();
```
v6-0003:
1/
Suggested doc changes:
<para>
- 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.
+ Sets the interval at which certain statistics, which can be
updated while a
+ transaction is in progress, are made visible. These include
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>
+ <link
linkend="monitoring-pg-stat-wal-view"><structname>pg_stat_wal</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.
+ <link
linkend="monitoring-pg-stat-io-view"><structname>pg_stat_io</structname></link>.
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.
- Other statistics are only made visible at transaction end and are not
- affected by this setting.
I removed this, because it's mentioned in the notes section later on.
2/
I don't see we have tests for other timeout based GUCs, but it would nice
to ensure that this woks correctly. Maybe as a custom_stats test where we
SET stats_flush_interval inside the transaction and make sure the stats flush
only after the new timeout has passed. Maybe?
v6-0004:
1/
NIT:
+$node_primary->append_conf('postgresql.conf', "stats_flush_interval= '1s'");
+$node_publisher->append_conf('postgresql.conf', "stats_flush_interval= '1s'");
missing space before the equal sign.
v6-0005:
1/
/* Partial flush only happens in anytime mode for FLUSH_ANYTIME stats */
is_partial_flush = (anytime_only && kind_info->flush_mode ==
FLUSH_ANYTIME);
Will this be tue at all time? Let's imagine a Kind that flushes all the fields
ANYTIME, would we not want to delete the pending entry?
+ /* if successfull non-partial flush, remove entry */
+ if (did_flush && !is_partial_flush)
pgstat_delete_pending_entry(entry_ref);
2/ indentation:
Some statistics are updated while a transaction is in progress
(for example,
<structfield>blks_read</structfield>, <structfield>blks_hit</structfield>,
<structfield>tup_returned</structfield> and
<structfield>tup_fetched</structfield>).
- Statistics that either do not depend on transactions or require
transactional
- consistency are updated only when the transaction ends.
Statistics that require
- transactional consistency include <structfield>xact_commit</structfield>,
- <structfield>xact_rollback</structfield>,
<structfield>tup_inserted</structfield>,
- <structfield>tup_updated</structfield> and
<structfield>tup_deleted</structfield>.
+ Statistics that either do not depend on transactions or require
transactional
+ consistency are updated only when the transaction ends.
Statistics that require
+ transactional consistency include <structfield>xact_commit</structfield>,
+ <structfield>xact_rollback</structfield>,
<structfield>tup_inserted</structfield>,
+ <structfield>tup_updated</structfield> and
<structfield>tup_deleted</structfield>.
</para>
</note>
--
Sami Imseih
Amazon Web Services (AWS)
