Re: [PATCH] Add sampling statistics to autoanalyze log output - Mailing list pgsql-hackers

Hi,

Thanks for the work on this, and I agree that the logging between a manual
ANALYZE and autoanalyze should be unified. +1

I have a few comments:

1/ Wouldn’t it be better to combine the new sampling output fields into a
single struct? This is more idiomatic, similar to BufferUsage or WalUsage,
and it would also make the API easier to extend in the future. Right now,
changing the function signature breaks the AcquireSampleRowsFunc ABI,
which is acceptable for a major release, but using a struct would help
avoid future breaks if we ever add more sampling data in later releases.

```
@@ -535,11 +546,15 @@ do_analyze_rel(Relation onerel, const VacuumParams params,
        if (inh)
                numrows = acquire_inherited_sample_rows(onerel, elevel,

                         rows, targrows,
-
                         &totalrows, &totaldeadrows);
+
                         &totalrows, &totaldeadrows,
+
                         &totalpages, &scannedpages,
+
                         &sampleliverows, &sampledeadrows);
        else
                numrows = (*acquirefunc) (onerel, elevel,

rows, targrows,
-
&totalrows, &totaldeadrows);
+
&totalrows, &totaldeadrows,
+
&totalpages, &scannedpages,
+
&sampleliverows, &sampledeadrows);

```

2/ I think this patch should just focus on unifying the existing logging only.

The " estimated total rows" for a foreign table should be a separate
thread/patch, IMO.

3/ You should also run pgindent.

Regards,

--
Sami Imseih
Amazon Web Services (AWS)