Autovacuum on partitioned table

On Mon, 2019-12-02 at 18:02 +0900, yuzuko wrote:

Greg reported in [1] before, autovacuum ignores partitioned tables.
That is, even if individual partitions’ statistics are updated, its parent's
statistics are not updated. This is TODO for declarative partitioning.
As Amit mentioned in [2], a way to make parent's statistics from
partitions' statistics without scanning the partitions would be nice,
but it will need a lot of modifications. So I tried to fix that using the
current analyze method.

The summary of the attached patch is as follows:
* If the relation is a partitioned table, check its children if they need
vacuum or analyze. Children need to do that are added to
a table list for autovacuuum. At least one child is added to the list,
the partitioned table is also added to the list. Then, autovacuum
runs on all the tables in the list.

That means that all partitions are vacuumed if only one of them needs it,
right? This will result in way more vacuuming than necessary.

Wouldn't it be an option to update the partitioned table's statistics
whenever one of the partitions is vacuumed?

Yours,
Laurenz Albe

Hi Laurenz,

Thanks for the comments.

On Mon, Dec 2, 2019 at 6:19 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Mon, 2019-12-02 at 18:02 +0900, yuzuko wrote:

Greg reported in [1] before, autovacuum ignores partitioned tables.
That is, even if individual partitions’ statistics are updated, its parent's
statistics are not updated. This is TODO for declarative partitioning.
As Amit mentioned in [2], a way to make parent's statistics from
partitions' statistics without scanning the partitions would be nice,
but it will need a lot of modifications. So I tried to fix that using the
current analyze method.

The summary of the attached patch is as follows:
* If the relation is a partitioned table, check its children if they need
vacuum or analyze. Children need to do that are added to
a table list for autovacuuum. At least one child is added to the list,
the partitioned table is also added to the list. Then, autovacuum
runs on all the tables in the list.

That means that all partitions are vacuumed if only one of them needs it,
right? This will result in way more vacuuming than necessary.

Autovacuum runs only partitions need vacuum/analyze, so unnecessary
partitions stats are not updated. However, to make parent's stats,
all children are scanned. It might be a waste of time.

Wouldn't it be an option to update the partitioned table's statistics
whenever one of the partitions is vacuumed?

Yours,
Laurenz Albe

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

Hi,

As Laurenz commented in this thread, I tried adding option
to update parent's statistics during Autovacuum. To do that,
I propose supporting 'autovacuum_enabled' option already
exists on partitioned tables.

In the attached patch, you can use 'autovacuum_enabled' option
on partitioned table as usual, that is, a default value of this option
is true. So if you don't need autovacuum on a partitioned table,
you have to specify the option:
CREATE TABLE p(i int) partition by range(i) with (autovacuum_enabled=0);

I'm not sure but I wonder if a suitable value as a default of
'autovacuum_enabled' for partitioned tables might be false.
Because autovacuum on *partitioned tables* requires scanning
all children to make partitioned tables' statistics.
But if the default value varies according to the relation,
is it confusing? Any thoughts?

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

On Fri, 27 Dec 2019 at 12:37, yuzuko <yuzukohosoya@gmail.com> wrote:

Hi,

As Laurenz commented in this thread, I tried adding option
to update parent's statistics during Autovacuum. To do that,
I propose supporting 'autovacuum_enabled' option already
exists on partitioned tables.

In the attached patch, you can use 'autovacuum_enabled' option
on partitioned table as usual, that is, a default value of this option
is true. So if you don't need autovacuum on a partitioned table,
you have to specify the option:
CREATE TABLE p(i int) partition by range(i) with (autovacuum_enabled=0);

I'm not sure but I wonder if a suitable value as a default of
'autovacuum_enabled' for partitioned tables might be false.
Because autovacuum on *partitioned tables* requires scanning
all children to make partitioned tables' statistics.
But if the default value varies according to the relation,
is it confusing? Any thoughts?

I don't look at the patch deeply yet but your patch seems to attempt
to vacuum on partitioned table. IIUC partitioned tables don't need to
be vacuumed and its all child tables are vacuumed instead if we pass
the partitioned table to vacuum() function. But autovacuum on child
tables is normally triggered since their statistics are updated.

I think it's a good idea to have that option but I think that doing
autovacuum on the parent table every time when autovacuum is triggered
on one of its child tables is very high cost especially when there are
a lot of child tables. Instead I thought it's more straight forward if
we compare the summation of the statistics of child tables (e.g.
n_live_tuples, n_dead_tuples etc) to vacuum thresholds when we
consider the needs of autovacuum on the parent table. What do you
think?

Regards,

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

Hello,

On Fri, Dec 27, 2019 at 2:02 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Fri, 27 Dec 2019 at 12:37, yuzuko <yuzukohosoya@gmail.com> wrote:

As Laurenz commented in this thread, I tried adding option
to update parent's statistics during Autovacuum. To do that,
I propose supporting 'autovacuum_enabled' option already
exists on partitioned tables.

In the attached patch, you can use 'autovacuum_enabled' option
on partitioned table as usual, that is, a default value of this option
is true. So if you don't need autovacuum on a partitioned table,
you have to specify the option:
CREATE TABLE p(i int) partition by range(i) with (autovacuum_enabled=0);

I'm not sure but I wonder if a suitable value as a default of
'autovacuum_enabled' for partitioned tables might be false.
Because autovacuum on *partitioned tables* requires scanning
all children to make partitioned tables' statistics.
But if the default value varies according to the relation,
is it confusing? Any thoughts?

I don't look at the patch deeply yet but your patch seems to attempt
to vacuum on partitioned table. IIUC partitioned tables don't need to
be vacuumed and its all child tables are vacuumed instead if we pass
the partitioned table to vacuum() function. But autovacuum on child
tables is normally triggered since their statistics are updated.

I think it's a good idea to have that option but I think that doing
autovacuum on the parent table every time when autovacuum is triggered
on one of its child tables is very high cost especially when there are
a lot of child tables. Instead I thought it's more straight forward if
we compare the summation of the statistics of child tables (e.g.
n_live_tuples, n_dead_tuples etc) to vacuum thresholds when we
consider the needs of autovacuum on the parent table. What do you
think?

There's this old email where Tom outlines a few ideas about triggering
auto-analyze on inheritance trees:

/messages/by-id/4823.1262132964@sss.pgh.pa.us

If I'm reading that correctly, the idea is to track only
changes_since_analyze and none of the finer-grained stats like
live/dead tuples for inheritance parents (partitioned tables) using
some new pgstat infrastrcture, an idea that Hosoya-san also seems to
be considering per an off-list discussion. Besides the complexity of
getting that infrastructure in place, an important question is whether
the current system of applying threshold and scale factor to
changes_since_analyze should be used as-is for inheritance parents
(partitioned tables), because if users set those parameters similarly
to for regular tables, autovacuum might analyze partitioned tables
more than necessary. We'll either need a different formula, or some
commentary in the documentation about how partitioned tables might
need different setting, or maybe both.

By the way, maybe I'm misunderstanding what Sawada-san wrote above,
but the only missing piece seems to be a way to trigger an *analyze*
on the parent tables -- to collect optimizer statistics for the
inheritance trees -- not vacuum, for which the existing system seems
enough.

Thanks,
Amit

On Tue, 28 Jan 2020 at 17:52, Amit Langote <amitlangote09@gmail.com> wrote:

Hello,

On Fri, Dec 27, 2019 at 2:02 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Fri, 27 Dec 2019 at 12:37, yuzuko <yuzukohosoya@gmail.com> wrote:

As Laurenz commented in this thread, I tried adding option
to update parent's statistics during Autovacuum. To do that,
I propose supporting 'autovacuum_enabled' option already
exists on partitioned tables.

In the attached patch, you can use 'autovacuum_enabled' option
on partitioned table as usual, that is, a default value of this option
is true. So if you don't need autovacuum on a partitioned table,
you have to specify the option:
CREATE TABLE p(i int) partition by range(i) with (autovacuum_enabled=0);

I'm not sure but I wonder if a suitable value as a default of
'autovacuum_enabled' for partitioned tables might be false.
Because autovacuum on *partitioned tables* requires scanning
all children to make partitioned tables' statistics.
But if the default value varies according to the relation,
is it confusing? Any thoughts?

I don't look at the patch deeply yet but your patch seems to attempt
to vacuum on partitioned table. IIUC partitioned tables don't need to
be vacuumed and its all child tables are vacuumed instead if we pass
the partitioned table to vacuum() function. But autovacuum on child
tables is normally triggered since their statistics are updated.

I think it's a good idea to have that option but I think that doing
autovacuum on the parent table every time when autovacuum is triggered
on one of its child tables is very high cost especially when there are
a lot of child tables. Instead I thought it's more straight forward if
we compare the summation of the statistics of child tables (e.g.
n_live_tuples, n_dead_tuples etc) to vacuum thresholds when we
consider the needs of autovacuum on the parent table. What do you
think?

There's this old email where Tom outlines a few ideas about triggering
auto-analyze on inheritance trees:

/messages/by-id/4823.1262132964@sss.pgh.pa.us

If I'm reading that correctly, the idea is to track only
changes_since_analyze and none of the finer-grained stats like
live/dead tuples for inheritance parents (partitioned tables) using
some new pgstat infrastrcture, an idea that Hosoya-san also seems to
be considering per an off-list discussion. Besides the complexity of
getting that infrastructure in place, an important question is whether
the current system of applying threshold and scale factor to
changes_since_analyze should be used as-is for inheritance parents
(partitioned tables), because if users set those parameters similarly
to for regular tables, autovacuum might analyze partitioned tables
more than necessary.

How are you going to track changes_since_analyze of partitioned table?
It's just an idea but we can accumulate changes_since_analyze of
partitioned table by adding child tables's value after analyzing each
child table. And compare the partitioned tables value to the threshold
that is computed by (autovacuum_analyze_threshold + total rows
including all child tables * autovacuum_analyze_scale_factor).

By the way, maybe I'm misunderstanding what Sawada-san wrote above,
but the only missing piece seems to be a way to trigger an *analyze*
on the parent tables -- to collect optimizer statistics for the
inheritance trees -- not vacuum, for which the existing system seems
enough.

Right. We need only autoanalyze on partitioned tables.

Regards,

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

Hello,

Besides the complexity of
getting that infrastructure in place, an important question is whether
the current system of applying threshold and scale factor to
changes_since_analyze should be used as-is for inheritance parents
(partitioned tables), because if users set those parameters similarly
to for regular tables, autovacuum might analyze partitioned tables
more than necessary. We'll either need a different formula, or some
commentary in the documentation about how partitioned tables might
need different setting, or maybe both.

I'm not sure but I think we need new autovacuum parameters for
partitioned tables (autovacuum, autovacuum_analyze_threshold,
autovacuum_analyze_scale_factor) because whether it's necessary
to run autovacuum on partitioned tables will depend on users.
What do you think?

How are you going to track changes_since_analyze of partitioned table?
It's just an idea but we can accumulate changes_since_analyze of
partitioned table by adding child tables's value after analyzing each
child table. And compare the partitioned tables value to the threshold
that is computed by (autovacuum_analyze_threshold + total rows
including all child tables * autovacuum_analyze_scale_factor).

The idea Sawada-san mentioned is similar to mine. Also, for tracking
changes_since_analyze, we have to make partitioned table's statistics.
To do that, we can invent a new PgStat_StatPartitionedTabEntry based
on PgStat_StatTabEntry. Through talking with Amit, I think the new structure
needs the following members:

tableid
changes_since_analyze
analyze_timestamp
analyze_count
autovac_analyze_timestamp
autovac_analyze_count

Vacuum doesn't run on partitioned tables, so I think members related to
(auto) vacuum need not be contained in the structure.

I'm still writing a patch. I'll send it this week.
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

On Wed, Jan 29, 2020 at 11:29 AM yuzuko <yuzukohosoya@gmail.com> wrote:

Besides the complexity of
getting that infrastructure in place, an important question is whether
the current system of applying threshold and scale factor to
changes_since_analyze should be used as-is for inheritance parents
(partitioned tables), because if users set those parameters similarly
to for regular tables, autovacuum might analyze partitioned tables
more than necessary. We'll either need a different formula, or some
commentary in the documentation about how partitioned tables might
need different setting, or maybe both.

I'm not sure but I think we need new autovacuum parameters for
partitioned tables (autovacuum, autovacuum_analyze_threshold,
autovacuum_analyze_scale_factor) because whether it's necessary
to run autovacuum on partitioned tables will depend on users.
What do you think?

Yes, we will need to first support those parameters on partitioned
tables. Currently, you get:

create table p (a int) partition by list (a) with
(autovacuum_analyze_scale_factor=0);
ERROR: unrecognized parameter "autovacuum_analyze_scale_factor"

How are you going to track changes_since_analyze of partitioned table?
It's just an idea but we can accumulate changes_since_analyze of
partitioned table by adding child tables's value after analyzing each
child table. And compare the partitioned tables value to the threshold
that is computed by (autovacuum_analyze_threshold + total rows
including all child tables * autovacuum_analyze_scale_factor).

The idea Sawada-san mentioned is similar to mine.

So if I understand this idea correctly, a partitioned table's analyze
will only be triggered when partitions are analyzed. That is,
inserts, updates, deletes of tuples in partitions will be tracked by
pgstat, which in turn is used by autovacuum to trigger analyze on
partitions. Then, partitions changes_since_analyze is added into the
parent's changes_since_analyze, which in turn *may* trigger analyze
parent. I said "may", because it would take multiple partition
analyzes to accumulate enough changes to trigger one on the parent.
Am I getting that right?

Also, for tracking
changes_since_analyze, we have to make partitioned table's statistics.
To do that, we can invent a new PgStat_StatPartitionedTabEntry based
on PgStat_StatTabEntry. Through talking with Amit, I think the new structure
needs the following members:

tableid
changes_since_analyze
analyze_timestamp
analyze_count
autovac_analyze_timestamp
autovac_analyze_count

Vacuum doesn't run on partitioned tables, so I think members related to
(auto) vacuum need not be contained in the structure.

On second thought, maybe we don't need a new PgStat_ struct. We can
just use what's used for regular tables and leave the fields that
don't make sense for partitioned tables set to 0, such as those that
track the counts of scans, tuples, etc. That means we don't have to
mess with interfaces of existing functions, like this one:

static void relation_needs_vacanalyze(Oid relid,
AutoVacOpts *relopts,
Form_pg_class classForm,
PgStat_StatTabEntry *tabentry, ...

Thanks,
Amit

On Wed, Jan 29, 2020 at 05:56:40PM +0900, Amit Langote wrote:

Yes, we will need to first support those parameters on partitioned
tables. Currently, you get:

create table p (a int) partition by list (a) with
(autovacuum_analyze_scale_factor=0);
ERROR: unrecognized parameter "autovacuum_analyze_scale_factor"

Worth the note: partitioned tables support zero reloptions as of now,
but there is the facility in place to allow that (see
RELOPT_KIND_PARTITIONED and partitioned_table_reloptions).
--
Michael

On Wed, 29 Jan 2020 at 17:56, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Jan 29, 2020 at 11:29 AM yuzuko <yuzukohosoya@gmail.com> wrote:

Besides the complexity of
getting that infrastructure in place, an important question is whether
the current system of applying threshold and scale factor to
changes_since_analyze should be used as-is for inheritance parents
(partitioned tables), because if users set those parameters similarly
to for regular tables, autovacuum might analyze partitioned tables
more than necessary. We'll either need a different formula, or some
commentary in the documentation about how partitioned tables might
need different setting, or maybe both.

I'm not sure but I think we need new autovacuum parameters for
partitioned tables (autovacuum, autovacuum_analyze_threshold,
autovacuum_analyze_scale_factor) because whether it's necessary
to run autovacuum on partitioned tables will depend on users.
What do you think?

Yes, we will need to first support those parameters on partitioned
tables. Currently, you get:

create table p (a int) partition by list (a) with
(autovacuum_analyze_scale_factor=0);
ERROR: unrecognized parameter "autovacuum_analyze_scale_factor"

How are you going to track changes_since_analyze of partitioned table?
It's just an idea but we can accumulate changes_since_analyze of
partitioned table by adding child tables's value after analyzing each
child table. And compare the partitioned tables value to the threshold
that is computed by (autovacuum_analyze_threshold + total rows
including all child tables * autovacuum_analyze_scale_factor).

The idea Sawada-san mentioned is similar to mine.

So if I understand this idea correctly, a partitioned table's analyze
will only be triggered when partitions are analyzed. That is,
inserts, updates, deletes of tuples in partitions will be tracked by
pgstat, which in turn is used by autovacuum to trigger analyze on
partitions. Then, partitions changes_since_analyze is added into the
parent's changes_since_analyze, which in turn *may* trigger analyze
parent. I said "may", because it would take multiple partition
analyzes to accumulate enough changes to trigger one on the parent.
Am I getting that right?

Yeah that is what I meant. In addition, adding partition's
changes_since_analyze to its parent needs to be done recursively as
the parent table could also be a partitioned table.

Also, for tracking
changes_since_analyze, we have to make partitioned table's statistics.
To do that, we can invent a new PgStat_StatPartitionedTabEntry based
on PgStat_StatTabEntry. Through talking with Amit, I think the new structure
needs the following members:

tableid
changes_since_analyze
analyze_timestamp
analyze_count
autovac_analyze_timestamp
autovac_analyze_count

Vacuum doesn't run on partitioned tables, so I think members related to
(auto) vacuum need not be contained in the structure.

On second thought, maybe we don't need a new PgStat_ struct. We can
just use what's used for regular tables and leave the fields that
don't make sense for partitioned tables set to 0, such as those that
track the counts of scans, tuples, etc. That means we don't have to
mess with interfaces of existing functions, like this one:

static void relation_needs_vacanalyze(Oid relid,
AutoVacOpts *relopts,
Form_pg_class classForm,
PgStat_StatTabEntry *tabentry, ...

+1

Regards,

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

On Sun, Feb 2, 2020 at 12:53 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Wed, 29 Jan 2020 at 17:56, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Jan 29, 2020 at 11:29 AM yuzuko <yuzukohosoya@gmail.com> wrote:

How are you going to track changes_since_analyze of partitioned table?
It's just an idea but we can accumulate changes_since_analyze of
partitioned table by adding child tables's value after analyzing each
child table. And compare the partitioned tables value to the threshold
that is computed by (autovacuum_analyze_threshold + total rows
including all child tables * autovacuum_analyze_scale_factor).

The idea Sawada-san mentioned is similar to mine.

So if I understand this idea correctly, a partitioned table's analyze
will only be triggered when partitions are analyzed. That is,
inserts, updates, deletes of tuples in partitions will be tracked by
pgstat, which in turn is used by autovacuum to trigger analyze on
partitions. Then, partitions changes_since_analyze is added into the
parent's changes_since_analyze, which in turn *may* trigger analyze
parent. I said "may", because it would take multiple partition
analyzes to accumulate enough changes to trigger one on the parent.
Am I getting that right?

Yeah that is what I meant. In addition, adding partition's
changes_since_analyze to its parent needs to be done recursively as
the parent table could also be a partitioned table.

That's a good point. So, changes_since_analyze increments are
essentially propagated from leaf partitions to all the way up to the
root table, including any intermediate partitioned tables. We'll need
to consider whether we should propagate only one level at a time (from
bottom of the tree) or update all parents up to the root, every time a
leaf partition is analyzed. If we we do the latter, that might end up
triggering analyze on all the parents at the same time causing
repeated scanning of the same child tables in close intervals,
although setting analyze threshold and scale factor of the parent
tables of respective levels wisely can help avoid any negative impact
of that.

Thanks,
Amit

Hello,

I'm sorry for the delay.
Attach the latest patch based on discussion in this thread.

Yeah that is what I meant. In addition, adding partition's
changes_since_analyze to its parent needs to be done recursively as
the parent table could also be a partitioned table.

That's a good point. So, changes_since_analyze increments are
essentially propagated from leaf partitions to all the way up to the
root table, including any intermediate partitioned tables. We'll need
to consider whether we should propagate only one level at a time (from
bottom of the tree) or update all parents up to the root, every time a
leaf partition is analyzed.

For multi-level partitioning, all parents' changes_since_analyze will be
updated whenever analyzing a leaf partition in this patch.
Could you please check the patch again?

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

Hosoya-san,

On Thu, Feb 20, 2020 at 3:34 PM yuzuko <yuzukohosoya@gmail.com> wrote:

Attach the latest patch based on discussion in this thread.

Yeah that is what I meant. In addition, adding partition's
changes_since_analyze to its parent needs to be done recursively as
the parent table could also be a partitioned table.

That's a good point. So, changes_since_analyze increments are
essentially propagated from leaf partitions to all the way up to the
root table, including any intermediate partitioned tables. We'll need
to consider whether we should propagate only one level at a time (from
bottom of the tree) or update all parents up to the root, every time a
leaf partition is analyzed.

For multi-level partitioning, all parents' changes_since_analyze will be
updated whenever analyzing a leaf partition in this patch.
Could you please check the patch again?

Thank you for the new patch.

I built and confirmed that the patch works.

Here are some comments:

* White-space noise in the diff (space used where tab is expected);
please check with git diff --check and fix.

* Names changes_tuples, m_changes_tuples should be changed_tuples and
m_changed_tuples, respectively?

* 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:

     /*
-     * Report ANALYZE to the stats collector, too.  However, if doing
-     * inherited stats we shouldn't report, because the stats collector only
-     * tracks per-table stats.  Reset the changes_since_analyze counter only
-     * if we analyzed all columns; otherwise, there is still work for
-     * auto-analyze to do.
+     * Report ANALYZE to the stats collector, too.  If the table is a
+     * partition, report changes_since_analyze of its parent because
+     * autovacuum process for partitioned tables needs it.  Reset the
+     * changes_since_analyze counter only if we analyzed all columns;
+     * otherwise, there is still work for auto-analyze to do.
      */
-    if (!inh)
-        pgstat_report_analyze(onerel, totalrows, totaldeadrows,
-                              (va_cols == NIL));
+    pgstat_report_analyze(onerel, totalrows, totaldeadrows,
+                          (va_cols == NIL));

* I may be missing something, but why doesn't do_autovacuum() fetch a
partitioned table's entry from pgstat instead of fetching that for
individual children and adding? That is, why do we need to do the
following:

+            /*
+             * If the relation is a partitioned table, we check it
using reltuples
+             * added up childrens' and changes_since_analyze tracked
by stats collector.

More later...

Thanks,
Amit

On Thu, Feb 20, 2020 at 4:50 PM Amit Langote <amitlangote09@gmail.com> wrote:

* I may be missing something, but why doesn't do_autovacuum() fetch a
partitioned table's entry from pgstat instead of fetching that for
individual children and adding? That is, why do we need to do the
following:

+            /*
+             * If the relation is a partitioned table, we check it
using reltuples
+             * added up childrens' and changes_since_analyze tracked
by stats collector.

Oh, it's only adding up children's pg_class.reltuple, not pgstat
stats. We need to do that because a partitioned table's
pg_class.reltuples is always 0 and correctly so. Sorry for not
reading the patch properly.

Thanks,
Amit

On Thu, Feb 20, 2020 at 5:32 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 20, 2020 at 4:50 PM Amit Langote <amitlangote09@gmail.com> wrote:

* I may be missing something, but why doesn't do_autovacuum() fetch a
partitioned table's entry from pgstat instead of fetching that for
individual children and adding? That is, why do we need to do the
following:

+            /*
+             * If the relation is a partitioned table, we check it
using reltuples
+             * added up childrens' and changes_since_analyze tracked
by stats collector.

Oh, it's only adding up children's pg_class.reltuple, not pgstat
stats. We need to do that because a partitioned table's
pg_class.reltuples is always 0 and correctly so. Sorry for not
reading the patch properly.

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. I
have attached a delta patch to show what I mean. Please check and
tell what you think.

Thanks,
Amit

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.

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

On Fri, 21 Feb 2020 at 15:14, yuzuko <yuzukohosoya@gmail.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

On Fri, Feb 21, 2020 at 4:47 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

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.

+1, that makes sense.

Thanks,
Amit

Hi,

Thanks for reviewing the patch.

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.

+1, that makes sense.

Yes, Indeed. I modified it not to update statistics other than
n_mod_since_analyze.
Attach the v5 patch. In this patch, pgstat_report_analyze() always reports 0 as
msg.m_live_tuples and m_dead_tuples when the relation is partitioned.

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center