Re: Add parallel columns for seq scan and index scan on pg_stat_all_tables and _indexes

Hi,

Le mer. 4 sept. 2024 à 16:18, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
a écrit :

> Hi,
>
> On Wed, Sep 04, 2024 at 02:51:51PM +0200, Guillaume Lelarge wrote:
> > Hi,
> >
> > Le mer. 4 sept. 2024 à 10:47, Bertrand Drouvot <
> bertranddrouvot.pg@gmail.com>
> > a écrit :
> >
> > > What about to get rid of the pgstat_count_parallel_heap_scan and add an
> > > extra
> > > bolean parameter to pgstat_count_heap_scan to indicate if
> > > counts.parallelnumscans
> > > should be incremented too?
> > >
> > > Something like:
> > >
> > > pgstat_count_heap_scan(scan->rs_base.rs_rd, scan->rs_base.rs_parallel
> !=
> > > NULL)
> > >
> > > 3 ===
> > >
> > > Same comment for pgstat_count_index_scan (add an extra bolean
> parameter)
> > > and
> > > get rid of pgstat_count_parallel_index_scan()).
> > >
> > > I think that 2 === and 3 === would help to avoid missing increments
> should
> > > we
> > > add those call to other places in the future.
> > >
> > >
> > Oh OK, understood.  Done for both.
>
> Thanks for v2!
>
> 1 ===
>
> -#define pgstat_count_heap_scan(rel)
> +#define pgstat_count_heap_scan(rel, parallel)
>         do {
> -               if (pgstat_should_count_relation(rel))
> -                       (rel)->pgstat_info->counts.numscans++;
> +            if (pgstat_should_count_relation(rel)) {
> +                       if (!parallel)
> +                               (rel)->pgstat_info->counts.numscans++;
> +                       else
> +
>  (rel)->pgstat_info->counts.parallelnumscans++;
> +               }
>
> I think counts.numscans has to be incremented in all the cases (so even if
> "parallel" is true).
>
> Same comment for pgstat_count_index_scan().
>
>
You're right, and I've been too quick. Fixed in v3.


> > 4 ===
> > >
> > > +       if (lstats->counts.numscans || lstats->counts.parallelnumscans)
> > >
> > > Is it possible to have (lstats->counts.parallelnumscans) whithout
> having
> > > (lstats->counts.numscans) ?
> > >
> > >
> > Nope, parallel scans are included in seq/index scans, as far as I can
> tell.
> > I could remove the parallelnumscans testing but it would be less obvious
> to
> > read.
>
> 2 ===
>
> What about adding a comment instead of this extra check?
>
>
Done too in v3.


-- 
Guillaume.