Re: Autovacuum on partitioned table

On Fri, 21 Feb 2020 at 15:14, yuzuko <yuzukohosoya(at)gmail(dot)com> wrote:
>
> Hello Amit-san,
>
> Thanks for your comments.
>
> > * White-space noise in the diff (space used where tab is expected);
> > please check with git diff --check and fix.
> Fixed it.
>
> > * Names changes_tuples, m_changes_tuples should be changed_tuples and
> > m_changed_tuples, respectively?
> Yes, I modified it.
>
> > * Did you intend to make it so that we now report *all* inherited
> > stats to the stats collector, not just those for partitioned tables?
> > IOW, do did you intend the new feature to also cover traditional
> > inheritance parents? I am talking about the following diff:
> >
> I modified as follows to apply this feature to only declaretive partitioning.
>
> - if (!inh)
> - pgstat_report_analyze(onerel, totalrows, totaldeadrows,
> - (va_cols == NIL));
> + if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + pgstat_report_analyze(onerel, totalrows, totaldeadrows,
> + (va_cols == NIL));
>
>
> > Having read the relevant diffs again, I think this could be done
> > without duplicating code too much. You seem to have added the same
> > logic in two places: do_autovacuum() and table_recheck_autovac().
> > More importantly, part of the logic of relation_needs_vacanalyze() is
> > duplicated in both of the aforementioned places, which I think is
> > unnecessary and undesirable if you consider maintainability. I think
> > we could just add the logic to compute reltuples for partitioned
> > tables at the beginning of relation_needs_vacanalyze() and be done.
> >
> Yes, indeed. Partitioned tables don't need to vacuum so I added new
> checking process for partitioned tables outside relation_needs_vacanalyze().
> However, partitioned tables' tabentry->n_dead_tuples are always 0 so
> dovacuum is always false. So I think that checking both auto vacuum
> and analyze for partitioned tables doesn't matter. I merged v3_amit_delta.patch
> into the new patch and found minor bug, partitioned table's reltuples is
> overwritten with it's classForm->reltuples, so I fixed it.
>
> Also, I think partitioned tables' changes_since_analyze should be reported
> only when Autovacuum process. So I fixed it too.

Thank you for updating the patch. I tested v4 patch.

After analyze or autoanalyze on partitioned table n_live_tup and
n_dead_tup are updated. However, TRUNCATE and VACUUM on the
partitioned table don't change these values until invoking analyze or
autoanalyze whereas in normal tables these values are reset or
changed. For example, with your patch:

* Before
relname | n_live_tup | n_dead_tup | n_mod_since_analyze
---------+------------+------------+---------------------
c1 | 11 | 0 | 0
c2 | 11 | 0 | 0
c3 | 11 | 0 | 0
c4 | 11 | 0 | 0
c5 | 11 | 0 | 0
parent | 55 | 0 | 0
(6 rows)

* After 'TRUNCATE parent'
relname | n_live_tup | n_dead_tup | n_mod_since_analyze
---------+------------+------------+---------------------
c1 | 0 | 0 | 0
c2 | 0 | 0 | 0
c3 | 0 | 0 | 0
c4 | 0 | 0 | 0
c5 | 0 | 0 | 0
parent | 55 | 0 | 0
(6 rows)

* Before
relname | n_live_tup | n_dead_tup | n_mod_since_analyze
---------+------------+------------+---------------------
c1 | 0 | 11 | 0
c2 | 0 | 11 | 0
c3 | 0 | 11 | 0
c4 | 0 | 11 | 0
c5 | 0 | 11 | 0
parent | 0 | 55 | 0
(6 rows)

* After 'VACUUM parent'
relname | n_live_tup | n_dead_tup | n_mod_since_analyze
---------+------------+------------+---------------------
c1 | 0 | 0 | 0
c2 | 0 | 0 | 0
c3 | 0 | 0 | 0
c4 | 0 | 0 | 0
c5 | 0 | 0 | 0
parent | 0 | 55 | 0
(6 rows)

We can make it work correctly but I think perhaps we can skip updating
statistics values of partitioned tables other than n_mod_since_analyze
as the first step. Because if we support also n_live_tup and
n_dead_tup, user might get confused that other statistics values such
as seq_scan, seq_tup_read however are not supported.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services