On Tue Oct 28, 2025 at 9:43 AM CET, torikoshia wrote:
> Rebased the patch again.
I took another look at this patch because I think it would be really
useful to have. Below is my review:
1.
ExplainPropertyInteger("Storage I/O Read", NULL,
usage->inblock, es);
ExplainPropertyInteger("Storage I/O Read", NULL,
usage->outblock, es);
The second one is a copy paste error and should say Storage I/O Write
instead of read.
2. I think the comment on pgStorageIOUsageParallel could be a bit
clearer on how it works, because it's different than how we
accumulate most others explain numbers. Something like:
Accumulates the I/O usage send by parallel workers to the main
process. This does not contain the I/O from the main backend process
itself because the kernel tracks that instead of us.
3. I'm not sure if I like the "times" suffix either, because it doesn't
mean that did that many reads. It probably read a bunch of blocks in
one go. Maybe just put the number without a suffix.
4. I think it would be good to use the string "Storage I/O" in the docs,
so that it's easily findable for people that try to figure out how it
works.
5. I think this debug statement can be removed:
ereport(DEBUG1,
(errmsg("Parallel worker's storage I/O times: inblock:%ld outblock:%ld",
storageiousage->inblock, storageiousage->outblock)));
6. The prepare exacute planning calculation is missing a
StorageIOUsageDiff call, so it will report the number since process
start.
7. I think it would be good for GetStorageIOUsage to stil initialize the
struct values even if pgaio workers are enabled. Let's just set them
to 0 in that case.
8. Now that worker is the default io_method the new explain fields are never
present in the explain tests anymore. So explain_1.out is
unnecessary and can be removed. That obviously also means that
there's no coverage for this feature at all in the tests currently,
which is clearly a problem. So I think it would be good to add some
actual tests for the feature using some other io_method in a Perl TAP
test.
9. The added docs about the kernel not being built with the correct
options seems a bit too much detail. I'd say remove that sentence.
And maybe shorten th
10. nit: There's "Also, When..." in the docs, when should be lower case
there.
Attached is your original patch plus a fixup patch that address:
1, 2, 3, 5, 6 and 7.
