On Fri, Aug 11, 2023 at 05:48:12PM -0700, Andres Freund wrote:
> I was basically still wondering whether I defused your startup process
> concerns or not.
My apologies for dropping the ball for so long here.
FWIW, I am still confused about what the long-term plan should be for
the startup process for these database-level stats, and how
significant it would be to get access to them (mostly for pg_stat_io,
right? Still what does it mean for the startup process?). I
understand that we should not undo what e3cb1a58 has tried to improve
post-feature freeze, still the POC patch of this thread does not touch
standby.c. So would it be better to just undo e3cb1a58? Or rework
the stat structures and track pgStatBlockWriteTime for each database,
studying cheaper methods to achieve that? If we do so, what's there
to gain in terms of user experience?
+ /*
+ * If we haven't connected to a database yet, don't attribute time to
+ * "shared state" (InvalidOid is used to track stats for shared relations
+ * etc).
+ */
At least this commit is incorrect because pgstat_update_dbstats() can
be called via pgstat_report_stat() with an invalid MyDatabaseId on
purpose? And I assume that standby.c should perhaps document a bit
better that this does not affect database-level stats.
Perhaps the top-comment of pgstat_report_stat() should document the
startup process part, but I am not completely sure how helpful such an
addition would be on its own.
At the end, after looking again at the patch and the thread, I'm OK
with pgstat_update_dbstats() doing nothing if we don't have
MyDatabaseId, because there is nothing to do in this case based on the
current structure of the database-level stats, but the expectations
surrounding pgstat_update_dbstats() need some clarifications. Take it
as a "I'm OK with the logic of the patch, but it can be improved" ;)
--
Michael