Re: RFC: Allow EXPLAIN to Output Page Fault Information

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.