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

On Wed, 26 Feb 2020 at 11:33, yuzuko <yuzukohosoya@gmail.com> wrote:

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.

Thank you for updating the patch. I'll look at it. I'd recommend to
register this patch to the next commit fest so at not to forget.

Regards,

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

Hello Yuzuko,

+	 * 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));
+	if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	pgstat_report_analyze(onerel, totalrows, totaldeadrows,
+						  (va_cols == NIL));

Hmm, I think the comment has a bug: it says "report ... of its parent"
but the report is of the same rel. (The pgstat_report_analyze line is
mis-indented also).

/*
+	 * If the relation is a partitioned table, we must add up children's
+	 * reltuples.
+	 */
+	if (classForm->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		List     *children;
+		ListCell *lc;
+
+		reltuples = 0;
+
+		/* Find all members of inheritance set taking AccessShareLock */
+		children = find_all_inheritors(relid, AccessShareLock, NULL);
+
+		foreach(lc, children)
+		{
+			Oid        childOID = lfirst_oid(lc);
+			HeapTuple  childtuple;
+			Form_pg_class childclass;
+
+			/* Ignore the parent table */
+			if (childOID == relid)
+				continue;

I think this loop counts partitioned partitions multiple times, because
we add up reltuples for all levels, no? (If I'm wrong, that is, if
a partitioned rel does not have reltuples, then why skip the parent?)

+	/*
+	 * If the table is a leaf partition, tell the stats collector its parent's
+	 * changes_since_analyze for auto analyze
+	 */
+	if (IsAutoVacuumWorkerProcess() &&
+		rel->rd_rel->relispartition &&
+		!(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))

I'm not sure I understand why we do this only on autovac. Why not all
analyzes?

+	{
+		Oid      parentoid;
+		Relation parentrel;
+		PgStat_StatDBEntry *dbentry;
+		PgStat_StatTabEntry *tabentry;
+
+		/* Get its parent table's Oid and relation */
+		parentoid = get_partition_parent(RelationGetRelid(rel));
+		parentrel = table_open(parentoid, AccessShareLock);

Climbing up the partitioning hierarchy acquiring locks on ancestor
relations opens up for deadlocks. It's better to avoid that. (As a
test, you could try what happens if you lock the topmost relation with
access-exclusive and leave a transaction open, then have autoanalyze
run). At the same time, I wonder if it's sensible to move one level up
here, and also have pgstat_report_partanalyze move more levels up.

+ * pgstat_report_partanalyze() -
+ *
+ *	Tell the collector about the parent table of which partition just analyzed.
+ *
+ * Caller must provide a child's changes_since_analyze as a parents.

I'm not sure what the last line is trying to say.

Thanks,

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hosoya-san,

Thanks for the new patch.

On Wed, Feb 26, 2020 at 11:33 AM yuzuko <yuzukohosoya@gmail.com> wrote:

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.

Some comments:

+ * PgStat_MsgPartAnalyze        Sent by the backend or autovacuum daemon
+ *                              after ANALYZE for partitioned tables

Looking at the way this message is used, it does not seem to be an
"analyze" message and also it's not sent "after ANALYZE of partitioned
tables", but really after ANALYZE of leaf partitions. Analyze (for
both partitioned tables and leaf partitions) is reported as a
PgStat_MsgAnalyze message as before. It seems that
PgStat_MsgPartAnalyze is only sent to update a leaf partition's
parent's (and recursively any grandparents') changes_since_analyze
counters, so maybe we should find a different name for it. Maybe,
PgStat_MsgPartChanges and accordingly the message type enum value.

     /*
-     * 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.
      */

The new comment says "partitions", which we typically use to refer to
a child table, but this comment really talks about parent tables. Old
comment says we don't report "inherited stats", presumably because
stats collector lacks the infrastructure to distinguish a table's
inherited stats and own stats, at least in the case of traditional
inheritance. With this patch, we are making an exception for
partitioned tables, because we are also teaching the stats collector
to maintain at least changes_since_analyze for them that accumulates
counts of changed tuples from partitions.

It seems Alvaro already reported some of the other issues I had with
the patch, such as why partanalyze messages are only sent from a
autovacuum worker.

Thanks,
Amit

On Fri, Feb 28, 2020 at 11:25 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

/*
+      * If the relation is a partitioned table, we must add up children's
+      * reltuples.
+      */
+     if (classForm->relkind == RELKIND_PARTITIONED_TABLE)
+     {
+             List     *children;
+             ListCell *lc;
+
+             reltuples = 0;
+
+             /* Find all members of inheritance set taking AccessShareLock */
+             children = find_all_inheritors(relid, AccessShareLock, NULL);
+
+             foreach(lc, children)
+             {
+                     Oid        childOID = lfirst_oid(lc);
+                     HeapTuple  childtuple;
+                     Form_pg_class childclass;
+
+                     /* Ignore the parent table */
+                     if (childOID == relid)
+                             continue;

I think this loop counts partitioned partitions multiple times, because
we add up reltuples for all levels, no? (If I'm wrong, that is, if
a partitioned rel does not have reltuples, then why skip the parent?)

+1, no need to skip partitioned tables here a their reltuples is always 0.

+     /*
+      * If the table is a leaf partition, tell the stats collector its parent's
+      * changes_since_analyze for auto analyze

Maybe write:

For a leaf partition, add its current changes_since_analyze into its
ancestors' counts. This must be done before sending the ANALYZE
message as it resets the partition's changes_since_analyze counter.

+      */
+     if (IsAutoVacuumWorkerProcess() &&
+             rel->rd_rel->relispartition &&
+             !(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))

I'm not sure I understand why we do this only on autovac. Why not all
analyzes?

+1. If there is a reason, it should at least be documented in the
comment above.

+     {
+             Oid      parentoid;
+             Relation parentrel;
+             PgStat_StatDBEntry *dbentry;
+             PgStat_StatTabEntry *tabentry;
+
+             /* Get its parent table's Oid and relation */
+             parentoid = get_partition_parent(RelationGetRelid(rel));
+             parentrel = table_open(parentoid, AccessShareLock);

Climbing up the partitioning hierarchy acquiring locks on ancestor
relations opens up for deadlocks. It's better to avoid that. (As a
test, you could try what happens if you lock the topmost relation with
access-exclusive and leave a transaction open, then have autoanalyze
run). At the same time, I wonder if it's sensible to move one level up
here, and also have pgstat_report_partanalyze move more levels up.

Maybe fetch all ancestors here and process from the top. But as we'd
have locked the leaf partition long before we got here, maybe we
should lock ancestors even before we start analyzing the leaf
partition? AccessShareLock should be enough on the ancestors because
we're not actually analyzing them.

(It appears get_partition_ancestors() returns a list where the root
parent is the last element, so need to be careful with that.)

Thanks,
Amit

Hello,

Thank you for reviewing.

+      */
+     if (IsAutoVacuumWorkerProcess() &&
+             rel->rd_rel->relispartition &&
+             !(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))

I'm not sure I understand why we do this only on autovac. Why not all
analyzes?

+1. If there is a reason, it should at least be documented in the
comment above.

When we analyze partitioned table by ANALYZE command,
all inheritors including partitioned table are analyzed
at the same time. In this case, if we call pgstat_report_partanalyze,
partitioned table's changes_since_analyze is updated
according to the number of analyzed tuples of partitions
as follows. But I think it should be 0.

\d+ p
Partitioned table "public.p"
Column | Type | Collation | Nullable | Default | Storage | Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
i | integer | | | | plain | |
Partition key: RANGE (i)
Partitions: p_1 FOR VALUES FROM (0) TO (100),
p_2 FOR VALUES FROM (100) TO (200)

insert into p select * from generate_series(0,199);
INSERT 0 200

(before analyze)
-[ RECORD 1 ]-------+------------------
relname | p
n_mod_since_analyze | 0
-[ RECORD 2 ]-------+------------------
relname | p_1
n_mod_since_analyze | 100
-[ RECORD 3 ]-------+------------------
relname | p_2
n_mod_since_analyze | 100

(after analyze)
-[ RECORD 1 ]-------+------------------
relname | p
n_mod_since_analyze | 200
-[ RECORD 2 ]-------+------------------
relname | p_1
n_mod_since_analyze | 0
-[ RECORD 3 ]-------+------------------
relname | p_2
n_mod_since_analyze | 0

I think if we analyze partition tree in order from leaf partitions
to root table, this problem can be fixed.
What do you think about it?

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

Hello,

+      */
+     if (IsAutoVacuumWorkerProcess() &&
+             rel->rd_rel->relispartition &&
+             !(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))

I'm not sure I understand why we do this only on autovac. Why not all
analyzes?

+1. If there is a reason, it should at least be documented in the
comment above.

When we analyze partitioned table by ANALYZE command,
all inheritors including partitioned table are analyzed
at the same time. In this case, if we call pgstat_report_partanalyze,
partitioned table's changes_since_analyze is updated
according to the number of analyzed tuples of partitions
as follows. But I think it should be 0.

\d+ p
Partitioned table "public.p"
Column | Type | Collation | Nullable | Default | Storage | Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
i | integer | | | | plain | |
Partition key: RANGE (i)
Partitions: p_1 FOR VALUES FROM (0) TO (100),
p_2 FOR VALUES FROM (100) TO (200)

insert into p select * from generate_series(0,199);
INSERT 0 200

(before analyze)
-[ RECORD 1 ]-------+------------------
relname | p
n_mod_since_analyze | 0
-[ RECORD 2 ]-------+------------------
relname | p_1
n_mod_since_analyze | 100
-[ RECORD 3 ]-------+------------------
relname | p_2
n_mod_since_analyze | 100

(after analyze)
-[ RECORD 1 ]-------+------------------
relname | p
n_mod_since_analyze | 200
-[ RECORD 2 ]-------+------------------
relname | p_1
n_mod_since_analyze | 0
-[ RECORD 3 ]-------+------------------
relname | p_2
n_mod_since_analyze | 0

I think if we analyze partition tree in order from leaf partitions
to root table, this problem can be fixed.
What do you think about it?

Attach the new patch fixes the above problem. Also, This patch
includes modifications accoring to all comments Alvaro and Amit
mentioned before in this thread.

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

On 2020-Mar-18, yuzuko wrote:

I think if we analyze partition tree in order from leaf partitions
to root table, this problem can be fixed.
What do you think about it?

Attach the new patch fixes the above problem.

Thanks for the new version.

I'm confused about some error messages in the regression test when a
column is mentioned twice, that changed from mentioning the table named
in the vacuum command, to mentioning the first partition. Is that
because you changed an lappend() to lcons()? I think you do this so
that the counters accumulate for the topmost parent that will be
processed at the end. I'm not sure I like that too much ... I think
that needs more thought.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Regarding this patch:

+ * the ANALYZE message as it resets the partition's changes_since_analze
=> analyze

+ * If the relation is a partitioned table, we must add up children's
childrens'

The approach in general:

I see an issue for timeseries data, where only the most recent partition is
being inserted into, and the histogram endpoint is being continuously extended
(this is our use-case). The most recent partition will be analyzed pretty
often, and I think it'll be problematic if its parent doesn't get similar
treatment. Let's say there are 12 historic, monthly children with 1e6 tuples
each, and the 13th child has 2e5 tuples (6 days into the month). It's analyzed
when it grows by 20% (1.2 days), but at that point the parent has only grown by
12x less (~2%) and won't be analyzed until 12x further into the future (14
days). Its histogram is 12x longer (geometrically), but the histogram changed
by just as much (arithmetically). That's an issue for a query over "the last
few days"; if that's past the end of the histogram bound, the query planner
will estimate about ~0 tuples, and tend to give cascades of nested loops. I'm
biased, but I'm guessing that's too common a use case to answer that the proper
fix is to set the parent's analyze_scale_factor=0.0005. I think that suggests
that the parent might sometimes need to be analyzed every time any of its
children are. In other cases (like probably any hash partitioning), that'd be
excessive, and maybe the default settings shouldn't do that, but I think that
behavior ought to be possible, and I think this patch doesn't allow that.

In the past, I think there's was talk that maybe someone would invent a clever
way to dynamically combine all the partitions' statistics, so analyzing the
parent wasn't needed. I think that's easy enough for reltuples, MCV, and I
think histogram, but ISTM that ndistinct is simultaneously important to get
right and hard to do so. It depends on whether it's the partition key, which
now can be an arbitrary expression. Extended stats further complicates it,
even if we didn't aim to dynamically compute extended stats for a parent.

While writing this, it occured to me that we could use "CREATE STATISTICS" as a
way to mark a partitioned table (or certain columns) as needing to be handled
by analyze. I understand "CREATE STATs" was intended to (eventually) allow
implementing stats on expressions without using "create index" as a hack. So
if it's excessive to automatically analyze a parent table when any of its
children are analyzed, maybe it's less excessive to only do that for parents
with a stats object, and only on the given colums. I realize this patch is
alot less useful if it requires to do anything extra/nondefault, and it's
desirable to work without creating a stats object at all. Also, using CREATE
STATs would reduces the CPU cost of re-analyzing the entire heirarchy, but
doesn't help to reduce the I/O cost, which is significant.

--
Justin

Hi Alvaro,
Thank you for your comments.

I'm confused about some error messages in the regression test when a
column is mentioned twice, that changed from mentioning the table named
in the vacuum command, to mentioning the first partition. Is that
because you changed an lappend() to lcons()? I think you do this so
that the counters accumulate for the topmost parent that will be
processed at the end. I'm not sure I like that too much ... I think
that needs more thought.

I couldn't come up with a solution that counts changes_since_analyze
precisely when analyzing partitioned trees by ANALYZE command based on
this approach (update all ancestor's changes_since_analyze according to the
number of analyzed tuples of leaf partitions).

So I tried another approach to run autovacuum on partitioned tables.
In this approach, all ancestors' changed_tuples are updated when commiting
transactions (at AtEOXact_PgStat) according to the number of inserted/updated/
deleted tuples of leaf partitions.

Attach the latest patch. What do you think?
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

Not sure if you saw my earlier message ?

I think it ought to be possible to configure this feature such that an
auto-analyze on any child partition would trigger analyze of the parent. I
think that would be important for maintaining accurate stats of the partition
key column for many cases involving RANGE-partitioned tables, which are likely
to rely on histogram rather than MCVs.

On Wed, Mar 18, 2020 at 11:30:39AM -0500, Justin Pryzby wrote:

Regarding this patch:

+ * the ANALYZE message as it resets the partition's changes_since_analze
=> analyze

+ * If the relation is a partitioned table, we must add up children's
childrens'

The approach in general:

I see an issue for timeseries data, where only the most recent partition is
being inserted into, and the histogram endpoint is being continuously extended
(this is our use-case). The most recent partition will be analyzed pretty
often, and I think it'll be problematic if its parent doesn't get similar
treatment. Let's say there are 12 historic, monthly children with 1e6 tuples
each, and the 13th child has 2e5 tuples (6 days into the month). It's analyzed
when it grows by 20% (1.2 days), but at that point the parent has only grown by
12x less (~2%) and won't be analyzed until 12x further into the future (14
days). Its histogram is 12x longer (geometrically), but the histogram changed
by just as much (arithmetically). That's an issue for a query over "the last
few days"; if that's past the end of the histogram bound, the query planner
will estimate about ~0 tuples, and tend to give cascades of nested loops. I'm
biased, but I'm guessing that's too common a use case to answer that the proper
fix is to set the parent's analyze_scale_factor=0.0005. I think that suggests
that the parent might sometimes need to be analyzed every time any of its
children are. In other cases (like probably any hash partitioning), that'd be
excessive, and maybe the default settings shouldn't do that, but I think that
behavior ought to be possible, and I think this patch doesn't allow that.

In the past, I think there's was talk that maybe someone would invent a clever
way to dynamically combine all the partitions' statistics, so analyzing the
parent wasn't needed. I think that's easy enough for reltuples, MCV, and I
think histogram, but ISTM that ndistinct is simultaneously important to get
right and hard to do so. It depends on whether it's the partition key, which
now can be an arbitrary expression. Extended stats further complicates it,
even if we didn't aim to dynamically compute extended stats for a parent.

While writing this, it occured to me that we could use "CREATE STATISTICS" as a
way to mark a partitioned table (or certain columns) as needing to be handled
by analyze. I understand "CREATE STATs" was intended to (eventually) allow
implementing stats on expressions without using "create index" as a hack. So
if it's excessive to automatically analyze a parent table when any of its
children are analyzed, maybe it's less excessive to only do that for parents
with a stats object, and only on the given colums. I realize this patch is
alot less useful if it requires to do anything extra/nondefault, and it's
desirable to work without creating a stats object at all. Also, using CREATE
STATs would reduces the CPU cost of re-analyzing the entire heirarchy, but
doesn't help to reduce the I/O cost, which is significant.

--
Justin

--
Justin Pryzby
System Administrator
Telsasoft
+1-952-707-8581

Hi Justin,

Thank you for commens.

On Tue, Apr 7, 2020 at 12:32 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

Not sure if you saw my earlier message ?

I'm sorry, I didn't notice for a while.

I think it ought to be possible to configure this feature such that an
auto-analyze on any child partition would trigger analyze of the parent. I
think that would be important for maintaining accurate stats of the partition
key column for many cases involving RANGE-partitioned tables, which are likely
to rely on histogram rather than MCVs.

I read your previous email and understand that it would be neccesary to analyze
partitioned tables automatically when any of its children are analyzed. In my
first patch, auto-analyze on partitioned tables worked like this but there were
some comments about performance of autovacuum, especially when partitioned
tables have a lot of children.

The latest patch lets users set different autovacuum configuration for
each partitioned
tables like this,
create table p3(i int) partition by range(i) with
(autovacuum_analyze_scale_factor=0.0005, autovacuum_analyze_threshold=100);
so users can configure those parameters according to partitioning strategies
and other requirements.

So I think this patch can solve problem you mentioned.

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

On Thu, Apr 16, 2020 at 06:16:45PM +0900, yuzuko wrote:

I think it ought to be possible to configure this feature such that an
auto-analyze on any child partition would trigger analyze of the parent. I
think that would be important for maintaining accurate stats of the partition
key column for many cases involving RANGE-partitioned tables, which are likely
to rely on histogram rather than MCVs.

I read your previous email and understand that it would be neccesary to analyze
partitioned tables automatically when any of its children are analyzed. In my
first patch, auto-analyze on partitioned tables worked like this but there were
some comments about performance of autovacuum, especially when partitioned
tables have a lot of children.

I reread that part. There was also confusion between autovacuum vacuum and
autovacuum analyze.

I agree that it *might* be a problem to analyze the parent every time any child
is analyzed.

But it might also be what's needed for this feature to be useful.

The latest patch lets users set different autovacuum configuration for
each partitioned
tables like this,
create table p3(i int) partition by range(i) with
(autovacuum_analyze_scale_factor=0.0005, autovacuum_analyze_threshold=100);
so users can configure those parameters according to partitioning strategies
and other requirements.

So I think this patch can solve problem you mentioned.

I don't think that adequately allows what's needed.

I think it out to be possible to get the "analyze parent whenever a child is
analyzed" behavior easily, without having to compute new thershold parameters
every time one adds partitions, detaches partitions, loades 10x more data into
one of the partitions, load only 10% as much data into the latest partition,
etc.

For example, say a new customer has bunch of partitioned tables which each
currently have only one partition (for the current month), and that's expected
to grow to at least 20+ partitions (2+ years of history). How does one set the
partitioned table's auto-analyze parameters to analyze whenever any child is
analyzed ? I don't think it should be needed to update it every month after
computing sum(child tuples).

Possibly you could allow that behavior for some special values of the
threshold. Like if autovacuum_analyze_threshold=-2, then analyze the parent
whenever any of its children are analyzed.

I think that use case and that need would be common, but I'd like to hear what
others think.

--
Justin

On Thu, Apr 16, 2020 at 11:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Apr 16, 2020 at 06:16:45PM +0900, yuzuko wrote:

The latest patch lets users set different autovacuum configuration for
each partitioned
tables like this,
create table p3(i int) partition by range(i) with
(autovacuum_analyze_scale_factor=0.0005, autovacuum_analyze_threshold=100);
so users can configure those parameters according to partitioning strategies
and other requirements.

So I think this patch can solve problem you mentioned.

I don't think that adequately allows what's needed.

I think it out to be possible to get the "analyze parent whenever a child is
analyzed" behavior easily, without having to compute new thershold parameters
every time one adds partitions, detaches partitions, loades 10x more data into
one of the partitions, load only 10% as much data into the latest partition,
etc.

For example, say a new customer has bunch of partitioned tables which each
currently have only one partition (for the current month), and that's expected
to grow to at least 20+ partitions (2+ years of history). How does one set the
partitioned table's auto-analyze parameters to analyze whenever any child is
analyzed ? I don't think it should be needed to update it every month after
computing sum(child tuples).

Possibly you could allow that behavior for some special values of the
threshold. Like if autovacuum_analyze_threshold=-2, then analyze the parent
whenever any of its children are analyzed.

I think that use case and that need would be common, but I'd like to hear what
others think.

Having to constantly pay attention to whether a parent's
analyze_threshold/scale_factor is working as intended would surely be
an annoyance, so I tend to agree that we might need more than just the
ability to set analyze_threshold/scale_factor on parent tables.
However, I think we can at least start with being able to do
*something* here. :) Maybe others think that this shouldn't be
considered committable until we figure out a good analyze threshold
calculation formula to apply to parent tables.

For the cases in which parent's tuple count grows at about the same
rate as partitions (hash mainly), I guess the existing formula more or
less works. That is, we can set the parent's threshold/scale_factor
same as partitions' and the autovacuum's existing formula will ensure
that the parent is auto-analyzed in time and not more than needed. For
time-series partitioning, the same formula won't work, as you have
detailed in your comments. Is there any other partitioning pattern for
which the current formula won't work?

Considering that, how about having, say, a
autovacuum_analyze_partition_parent_frequency, with string values
'default', 'partition'? -- 'default' assumes the same formula as
regular tables, whereas with 'partition', parent is analyzed as soon
as a partition is.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 17, 2020 at 10:09:07PM +0900, Amit Langote wrote:

On Thu, Apr 16, 2020 at 11:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Apr 16, 2020 at 06:16:45PM +0900, yuzuko wrote:
I don't think that adequately allows what's needed.

...(paragraph with my typos elided)...

For example, say a new customer has bunch of partitioned tables which each
currently have only one partition (for the current month), and that's expected
to grow to at least 20+ partitions (2+ years of history). How does one set the
partitioned table's auto-analyze parameters to analyze whenever any child is
analyzed ? I don't think it should be needed to update it every month after
computing sum(child tuples).

Possibly you could allow that behavior for some special values of the
threshold. Like if autovacuum_analyze_threshold=-2, then analyze the parent
whenever any of its children are analyzed.

I think that use case and that need would be common, but I'd like to hear what
others think.

Having to constantly pay attention to whether a parent's
analyze_threshold/scale_factor is working as intended would surely be
an annoyance, so I tend to agree that we might need more than just the
ability to set analyze_threshold/scale_factor on parent tables.
However, I think we can at least start with being able to do
*something* here. :) Maybe others think that this shouldn't be
considered committable until we figure out a good analyze threshold
calculation formula to apply to parent tables.

Considering that, how about having, say, a
autovacuum_analyze_partition_parent_frequency, with string values
'default', 'partition'? -- 'default' assumes the same formula as
regular tables, whereas with 'partition', parent is analyzed as soon
as a partition is.

I assume you mean a reloption to be applied only to partitioned tables,

Your "partition" setting would mean that the scale/threshold values would have
no effect, which seems kind of unfortunate.

I think it should be called something else, and done differently, like maybe:
autovacuum_analyze_mode = {off,sum,max,...}

The threshold would be threshold + scale*tuples, as always, but would be
compared with f(changes) as determined by the relopt.

sum(changes) would do what you called "default", comparing the sum(changes)
across all partitions to the threshold, which is itself computed using
sum(reltuples) AS reltuples.

max(changes) would compute max(changes) compared to the threshold, and the
threshold would be computed separately for each partition's reltuples:
threshold_N = parent_threshold + parent_scale * part_N_tuples. If *any*
partition exceeds that threshold, the partition itself is analyzed. This
allows what I want for time-series. Maybe this would have an alias called
"any".

I'm not sure if there's any other useful modes, like avg(changes)? I guess we
can add them later if someone thinks of a good use case.

Also, for me, the v7 patch warns:
|src/backend/postmaster/autovacuum.c:3117:70: warning: ‘reltuples’ may be used uninitialized in this function [-Wmaybe-uninitialized]
| vacinsthresh = (float4) vac_ins_base_thresh + vac_ins_scale_factor * reltuples;
..which seems to be a false positive, but easily avoided.

This patch includes partitioned tables in pg_stat_*_tables, which is great; I
complained awhile ago that they were missing [0]/messages/by-id/20180601221428.GU5164@telsasoft.com. It might be useful if that
part was split out into a separate 0001 patch (?).

Thanks,
--
Justin

[0]: /messages/by-id/20180601221428.GU5164@telsasoft.com

Hello,

On Sat, Apr 18, 2020 at 2:08 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Fri, Apr 17, 2020 at 10:09:07PM +0900, Amit Langote wrote:

On Thu, Apr 16, 2020 at 11:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Apr 16, 2020 at 06:16:45PM +0900, yuzuko wrote:
I don't think that adequately allows what's needed.

...(paragraph with my typos elided)...

For example, say a new customer has bunch of partitioned tables which each
currently have only one partition (for the current month), and that's expected
to grow to at least 20+ partitions (2+ years of history). How does one set the
partitioned table's auto-analyze parameters to analyze whenever any child is
analyzed ? I don't think it should be needed to update it every month after
computing sum(child tuples).

Possibly you could allow that behavior for some special values of the
threshold. Like if autovacuum_analyze_threshold=-2, then analyze the parent
whenever any of its children are analyzed.

I think that use case and that need would be common, but I'd like to hear what
others think.

Having to constantly pay attention to whether a parent's
analyze_threshold/scale_factor is working as intended would surely be
an annoyance, so I tend to agree that we might need more than just the
ability to set analyze_threshold/scale_factor on parent tables.
However, I think we can at least start with being able to do
*something* here. :) Maybe others think that this shouldn't be
considered committable until we figure out a good analyze threshold
calculation formula to apply to parent tables.

Considering that, how about having, say, a
autovacuum_analyze_partition_parent_frequency, with string values
'default', 'partition'? -- 'default' assumes the same formula as
regular tables, whereas with 'partition', parent is analyzed as soon
as a partition is.

I assume you mean a reloption to be applied only to partitioned tables,

Your "partition" setting would mean that the scale/threshold values would have
no effect, which seems kind of unfortunate.

I think it should be called something else, and done differently, like maybe:
autovacuum_analyze_mode = {off,sum,max,...}

The above reloption you suggested will be applied all tables?
Users might not use it for partitions, so I think we should add "parent"
to reloption's name, like Amit's suggestion.

The threshold would be threshold + scale*tuples, as always, but would be
compared with f(changes) as determined by the relopt.

sum(changes) would do what you called "default", comparing the sum(changes)
across all partitions to the threshold, which is itself computed using
sum(reltuples) AS reltuples.

max(changes) would compute max(changes) compared to the threshold, and the
threshold would be computed separately for each partition's reltuples:
threshold_N = parent_threshold + parent_scale * part_N_tuples. If *any*
partition exceeds that threshold, the partition itself is analyzed. This
allows what I want for time-series. Maybe this would have an alias called
"any".

I may be wrong but I think the fomula,

threshold_N = parent_threshold + parent_scale * part_N_tuples

would use orginary table's threshold, not parent's. If it use parent_threshold,
parent might not be analyzed even if its any partition is analyzed when
parent_threshold is larger than normal threshold. I'm worried that this case
meets requirements for time-series.

I'm not sure if there's any other useful modes, like avg(changes)? I guess we
can add them later if someone thinks of a good use case.

Also, for me, the v7 patch warns:
|src/backend/postmaster/autovacuum.c:3117:70: warning: ‘reltuples’ may be used uninitialized in this function [-Wmaybe-uninitialized]
| vacinsthresh = (float4) vac_ins_base_thresh + vac_ins_scale_factor * reltuples;
..which seems to be a false positive, but easily avoided.

Thank you for testing the patch.
I got it. I'll update the patch soon.

This patch includes partitioned tables in pg_stat_*_tables, which is great; I
complained awhile ago that they were missing [0]. It might be useful if that
part was split out into a separate 0001 patch (?).

If partitioned table's statistics is used for other purposes, I think
it would be
better to split the patch. Does anyone have any opinion?

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

On Wed, Mar 18, 2020 at 11:30:39AM -0500, Justin Pryzby wrote:

In the past, I think there's was talk that maybe someone would invent a clever
way to dynamically combine all the partitions' statistics, so analyzing the
parent wasn't needed. [...]

I happened across the thread I was referring to:
/messages/by-id/7363.1426537103@sss.pgh.pa.us

I'm not opposed to doing things the currently-proposed way (trigger analyze of
partitioned tables based on updates, same as nonpartitioned tables), but we
should think if it's worth doing something totally different, like what Tom
proposed.

Robert had concerns that it would increase planning time. I imagine that
argument is even stronger now, since PG12 has *less* planning time for large
heirarchies (428b260f8) and advertizes support for "thousands" of partitions.

Tom said:

we would automatically get statistics that account for
partitions being eliminated by constraint exclusion, because only the
non-eliminated partitions are present in the appendrel. And second,

That's a pretty strong benefit. I don't know if there's a good way to support
both(either) ways of doing things. Like maybe a reloption that allows
triggering autovacuum on partitioned tables, but if no statistics exist on a
partitioned table, then the planner would dynamically determine the selectivity
by decending into child statistics (Tom's way). I think the usual way this
would play out is that someone with a small partition heirarchies would
eventually complain about high planning time and then we'd suggest implementing
a manual ANALYZE job.

I'm not sure it's good to support two ways anyway, since 1) I think that gives
different (better) statistics Tom's way (due to excluding stats of excluded
partitions); 2) there's not a good way to put an ANALYZE job in place and then
get rid of parent stats (have to DELETE FROM pg_statistic WHERE
starelid='...'::regclass; 3) if someone implements an ANALYZE job, but they
disable it or it stops working then they have outdated stats forever;

--
Justin

On Sat, Apr 25, 2020 at 11:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Mar 18, 2020 at 11:30:39AM -0500, Justin Pryzby wrote:

In the past, I think there's was talk that maybe someone would invent a clever
way to dynamically combine all the partitions' statistics, so analyzing the
parent wasn't needed. [...]

I happened across the thread I was referring to:
/messages/by-id/7363.1426537103@sss.pgh.pa.us

I'm not opposed to doing things the currently-proposed way (trigger analyze of
partitioned tables based on updates, same as nonpartitioned tables), but we
should think if it's worth doing something totally different, like what Tom
proposed.

Robert had concerns that it would increase planning time. I imagine that
argument is even stronger now, since PG12 has *less* planning time for large
heirarchies (428b260f8) and advertizes support for "thousands" of partitions.

Tom said:

we would automatically get statistics that account for
partitions being eliminated by constraint exclusion, because only the
non-eliminated partitions are present in the appendrel. And second,

That's a pretty strong benefit. I don't know if there's a good way to support
both(either) ways of doing things. Like maybe a reloption that allows
triggering autovacuum on partitioned tables, but if no statistics exist on a
partitioned table, then the planner would dynamically determine the selectivity
by decending into child statistics (Tom's way). I think the usual way this
would play out is that someone with a small partition heirarchies would
eventually complain about high planning time and then we'd suggest implementing
a manual ANALYZE job.

I'm not sure it's good to support two ways anyway, since 1) I think that gives
different (better) statistics Tom's way (due to excluding stats of excluded
partitions); 2) there's not a good way to put an ANALYZE job in place and then
get rid of parent stats (have to DELETE FROM pg_statistic WHERE
starelid='...'::regclass; 3) if someone implements an ANALYZE job, but they
disable it or it stops working then they have outdated stats forever;

Thanks for sharing that thread, had not seen it before.

I remember discussing with Alvaro and Hosoya-san an approach of
generating the whole-tree pg_statistics entries by combining the
children's entries, not during planning as the linked thread
discusses, but inside autovacuum. The motivation for that design was
the complaint that we scan the children twice with the current method
of generating whole-tree statistics -- first to generate their own
statistics and then again to generate the parent's.

Aside from how hard it would be to actually implement, that approach
also doesn't address the concern about when to generate the whole-tree
statistics. Because the linked thread mentions getting rid of the
whole-tree statistics altogether, there is no such concern if we go
its way. Although I do agree with Robert's assertion on that thread
that making every query on a parent a bit slower would not be a good
compromise.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On 21 Apr 2020, at 18:21, yuzuko <yuzukohosoya@gmail.com> wrote:

I'll update the patch soon.

Do you have an updated version to submit? The previous patch no longer applies
to HEAD, so I'm marking this entry Waiting on Author in the meantime.

cheers ./daniel

On Wed, Jul 1, 2020 at 6:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 21 Apr 2020, at 18:21, yuzuko <yuzukohosoya@gmail.com> wrote:

I'll update the patch soon.

Do you have an updated version to submit? The previous patch no longer applies
to HEAD, so I'm marking this entry Waiting on Author in the meantime.

Thank you for letting me know.
I attach the latest patch applies to HEAD.

I think there are other approaches like Tom's idea that Justin previously
referenced, but this patch works the same way as previous patches.
(tracks updated/inserted/deleted tuples and checks whether the partitioned
tables needs auto-analyze, same as nonpartitioned tables)
Because I wanted to be able to analyze partitioned tables by autovacuum
as a first step, and I think this approach is the simplest way to do it.

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

On 6 Jul 2020, at 12:35, yuzuko <yuzukohosoya@gmail.com> wrote:

On Wed, Jul 1, 2020 at 6:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 21 Apr 2020, at 18:21, yuzuko <yuzukohosoya@gmail.com> wrote:

I'll update the patch soon.

Do you have an updated version to submit? The previous patch no longer applies
to HEAD, so I'm marking this entry Waiting on Author in the meantime.

Thank you for letting me know.
I attach the latest patch applies to HEAD.

This version seems to fail under Werror which is used in the Travis builds:

autovacuum.c: In function ‘relation_needs_vacanalyze’:
autovacuum.c:3117:59: error: ‘reltuples’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
anlthresh = (float4) anl_base_thresh + anl_scale_factor * reltuples;
^
autovacuum.c:2972:9: note: ‘reltuples’ was declared here
float4 reltuples; /* pg_class.reltuples */
^

I've moved this patch to the next commitfest, but kept the status as Waiting on
Author. Please submit a new version of the patch.

cheers ./daniel

I'm sorry for the late reply.

This version seems to fail under Werror which is used in the Travis builds:

autovacuum.c: In function ‘relation_needs_vacanalyze’:
autovacuum.c:3117:59: error: ‘reltuples’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
anlthresh = (float4) anl_base_thresh + anl_scale_factor * reltuples;
^
autovacuum.c:2972:9: note: ‘reltuples’ was declared here
float4 reltuples; /* pg_class.reltuples */
^

I attach the latest patch that solves the above Werror.
Could you please check it again?

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

On 17 Aug 2020, at 08:11, yuzuko <yuzukohosoya@gmail.com> wrote:

I'm sorry for the late reply.

This version seems to fail under Werror which is used in the Travis builds:

autovacuum.c: In function ‘relation_needs_vacanalyze’:
autovacuum.c:3117:59: error: ‘reltuples’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
anlthresh = (float4) anl_base_thresh + anl_scale_factor * reltuples;
^
autovacuum.c:2972:9: note: ‘reltuples’ was declared here
float4 reltuples; /* pg_class.reltuples */
^

I attach the latest patch that solves the above Werror.
Could you please check it again?

This version now pass the tests in the Travis pipeline as can be seen in the
link below, and is ready to be reviewed in the upcoming commitfest:

http://cfbot.cputube.org/yuzuko-hosoya.html

cheers ./daniel

At Tue, 25 Aug 2020 14:28:20 +0200, Daniel Gustafsson <daniel@yesql.se> wrote in

I attach the latest patch that solves the above Werror.
Could you please check it again?

This version now pass the tests in the Travis pipeline as can be seen in the
link below, and is ready to be reviewed in the upcoming commitfest:

http://cfbot.cputube.org/yuzuko-hosoya.html

At Mon, 6 Jul 2020 19:35:37 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in

I think there are other approaches like Tom's idea that Justin previously
referenced, but this patch works the same way as previous patches.
(tracks updated/inserted/deleted tuples and checks whether the partitioned
tables needs auto-analyze, same as nonpartitioned tables)
Because I wanted to be able to analyze partitioned tables by autovacuum
as a first step, and I think this approach is the simplest way to do it.

I'm not sure if anything bad happen if parent and children are not
agree on statistics.

The requirement suggested here seems to be:

- We want to update parent's stats when any of its children gets its
stats updated. This is curucial especially for time-series
partitioning.

- However, we don't want analyze the whole-tree every time any of the
children was analyzed.

To achieve the both, stats-merging seems to the optimal solution.

Putting that aside, I had a brief look on the latest patch.

 	/* We only count stats for things that have storage */
-	if (!RELKIND_HAS_STORAGE(relkind))
+	if (!RELKIND_HAS_STORAGE(relkind) ||
+		relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		rel->pgstat_info = NULL;

RELKIND_HAS_STORAGE(RELKIND_PARTITIONED_TABLE) is already false.
Maybe you wanted to do "&& relkind !=" instead:p

+		/*
+		 * If this relation is partitioned, we store all ancestors' oid
+		 * to propagate its changed_tuples to their parents when this
+		 * transaction is committed.
+		 */
+		if (rel->rd_rel->relispartition && pgstat_info->ancestors == NULL)

If the relation was detached then attached to another partition within
a transaction, the ancestor list would get stale and the succeeding
modification to the relation propagates into wrong ancestors.

I think vacuum time is more appropriate to modify ancestors stats. It
seems to me that what Alvalo pointed isthe list-order-susceptible
manner of collecting children's modified tuples.

+ ? 0 /* partitioned tables don't have any data, so it's 0 */

If the comment is true, we shouldn't have non-zero t_changed_tuples,
too. I think the reason for the lines is something different.

# Oops! Time's up now.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Horiguchi-san,

Thank you for reviewing.

On Tue, Sep 15, 2020 at 7:01 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 25 Aug 2020 14:28:20 +0200, Daniel Gustafsson <daniel@yesql.se> wrote in

I attach the latest patch that solves the above Werror.
Could you please check it again?

This version now pass the tests in the Travis pipeline as can be seen in the
link below, and is ready to be reviewed in the upcoming commitfest:

http://cfbot.cputube.org/yuzuko-hosoya.html

At Mon, 6 Jul 2020 19:35:37 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in

I think there are other approaches like Tom's idea that Justin previously
referenced, but this patch works the same way as previous patches.
(tracks updated/inserted/deleted tuples and checks whether the partitioned
tables needs auto-analyze, same as nonpartitioned tables)
Because I wanted to be able to analyze partitioned tables by autovacuum
as a first step, and I think this approach is the simplest way to do it.

I'm not sure if anything bad happen if parent and children are not
agree on statistics.

The requirement suggested here seems to be:

- We want to update parent's stats when any of its children gets its
stats updated. This is curucial especially for time-series
partitioning.

- However, we don't want analyze the whole-tree every time any of the
children was analyzed.

To achieve the both, stats-merging seems to the optimal solution.

Putting that aside, I had a brief look on the latest patch.

/* We only count stats for things that have storage */
-       if (!RELKIND_HAS_STORAGE(relkind))
+       if (!RELKIND_HAS_STORAGE(relkind) ||
+               relkind == RELKIND_PARTITIONED_TABLE)
{
rel->pgstat_info = NULL;

RELKIND_HAS_STORAGE(RELKIND_PARTITIONED_TABLE) is already false.
Maybe you wanted to do "&& relkind !=" instead:p

Oh, indeed. I'll fix it.

+               /*
+                * If this relation is partitioned, we store all ancestors' oid
+                * to propagate its changed_tuples to their parents when this
+                * transaction is committed.
+                */
+               if (rel->rd_rel->relispartition && pgstat_info->ancestors == NULL)

If the relation was detached then attached to another partition within
a transaction, the ancestor list would get stale and the succeeding
modification to the relation propagates into wrong ancestors.

I think vacuum time is more appropriate to modify ancestors stats. It
seems to me that what Alvalo pointed isthe list-order-susceptible
manner of collecting children's modified tuples.

I proposed a patch that modified ancestors stats when vacuuming previously.
In that time, having been pointed out by Alvaro and Amit, I tried to update the
parents' changes_since_analyze in every ANALYZE. However, in that case,
the problem mentioned in [1]/messages/by-id/CAKkQ50-bwFEDMBGb1JmDXffXsiU8xk-hN6kJK9CKjdBa7r=Hdw@mail.gmail.com -- Best regards, Yuzuko Hosoya occurred, but I could not find a way to avoid it.
I think that it can be solved by updating the parents' changes_since_analyze
only in the case of auto analyze, but what do you think?

+ ? 0 /* partitioned tables don't have any data, so it's 0 */

If the comment is true, we shouldn't have non-zero t_changed_tuples,
too. I think the reason for the lines is something different.

Yes, surely. I think updating the values of live_tuples and dead_tuples
is confusing for users. I'll consider another comment.

[1]: /messages/by-id/CAKkQ50-bwFEDMBGb1JmDXffXsiU8xk-hN6kJK9CKjdBa7r=Hdw@mail.gmail.com -- Best regards, Yuzuko Hosoya
--
Best regards,
Yuzuko Hosoya

NTT Open Source Software Center

Hello,

I reconsidered a way based on the v5 patch in line with
Horiguchi-san's comment.

This approach is as follows:
- A partitioned table is checked whether it needs analyze like a plain
table in relation_needs_vacanalyze(). To do this, we should store
partitioned table's stats (changes_since_analyze).
- Partitioned table's changes_since_analyze is updated when
analyze a leaf partition by propagating its changes_since_analyze.
In the next scheduled analyze time, it is used in the above process.
That is, the partitioned table is analyzed behind leaf partitions.
- The propagation process differs between autoanalyze or plain analyze.
In autoanalyze, a leaf partition's changes_since_analyze is propagated
to *all* ancestors. Whereas, in plain analyze on an inheritance tree,
propagates to ancestors not included the tree to avoid needless counting.

Attach the latest patch to this email.
Could you check it again please?

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

Thanks you for the new version.

At Fri, 23 Oct 2020 15:12:51 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in

Hello,

I reconsidered a way based on the v5 patch in line with
Horiguchi-san's comment.

This approach is as follows:
- A partitioned table is checked whether it needs analyze like a plain
table in relation_needs_vacanalyze(). To do this, we should store
partitioned table's stats (changes_since_analyze).
- Partitioned table's changes_since_analyze is updated when
analyze a leaf partition by propagating its changes_since_analyze.
In the next scheduled analyze time, it is used in the above process.
That is, the partitioned table is analyzed behind leaf partitions.
- The propagation process differs between autoanalyze or plain analyze.
In autoanalyze, a leaf partition's changes_since_analyze is propagated
to *all* ancestors. Whereas, in plain analyze on an inheritance tree,
propagates to ancestors not included the tree to avoid needless counting.

Attach the latest patch to this email.
Could you check it again please?

+		/*
+		 * Get its all ancestors to propagate changes_since_analyze count.
+		 * However, when ANALYZE inheritance tree, we get ancestors of
+		 * toprel_oid to avoid needless counting.
+		 */
+		if (!OidIsValid(toprel_oid))
+			ancestors = get_partition_ancestors(RelationGetRelid(rel));
+		else
+			ancestors = get_partition_ancestors(toprel_oid);

This comment doesn't explaining what the code intends but what the
code does.

The reason for the difference is that if we have a valid toprel_oid,
we analyze all descendants of the relation this time, and if we
propagate the number to the descendants of the top relation, the next
analyze on the relations could happen shortly than expected.

-	msg.m_live_tuples = livetuples;
-	msg.m_dead_tuples = deadtuples;
+	msg.m_live_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+		? 0  /* if this is a partitioned table, skip modifying */
+		: livetuples;
+	msg.m_dead_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+		? 0 /* if this is a partitioned table, skip modifying */
+		: deadtuples;

Two successive branching with the same condition looks odd. And we
need an explanation of *why* we don't set the values for partitioned
tables.

+		foreach(lc, ancestors)
+		{
+			Oid     parentOid = lfirst_oid(lc);
+			Relation parentrel;
+
+			parentrel = table_open(parentOid, AccessShareLock);

I'm not sure, but all of the ancestors always cannot be a parent (in
other words, a parent of a parent of mine is not a parent of
mine). Isn't just rel sufficient?

-	 * 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.  Reset the
+	 * changes_since_analyze counter only if we analyzed all columns;
+	 * otherwise, there is still work for auto-analyze to do.
 	 */
-	if (!inh)
+	if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		pgstat_report_analyze(onerel, totalrows, totaldeadrows,

This still rejects traditional inheritance nonleaf relations. But if
we remove the description about that completely in the comment above,
we should support traditional inheritance parents here. I think we
can do that as far as we need to propagate only per-tuple stats (that
mans not per-attribute) like changes_since_analyze.

Whichever way we take, do we need the description about the behavior
in the documentation?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Oct 23, 2020 at 03:12:51PM +0900, yuzuko wrote:

Hello,

I reconsidered a way based on the v5 patch in line with
Horiguchi-san's comment.

This approach is as follows:
- A partitioned table is checked whether it needs analyze like a plain
table in relation_needs_vacanalyze(). To do this, we should store
partitioned table's stats (changes_since_analyze).
- Partitioned table's changes_since_analyze is updated when
analyze a leaf partition by propagating its changes_since_analyze.
In the next scheduled analyze time, it is used in the above process.
That is, the partitioned table is analyzed behind leaf partitions.
- The propagation process differs between autoanalyze or plain analyze.
In autoanalyze, a leaf partition's changes_since_analyze is propagated
to *all* ancestors. Whereas, in plain analyze on an inheritance tree,
propagates to ancestors not included the tree to avoid needless counting.

+                * Get its all ancestors to propagate changes_since_analyze count.
+                * However, when ANALYZE inheritance tree, we get ancestors of
+                * toprel_oid to avoid needless counting.

=> I don't understand that comment.

+                       /* Find all members of inheritance set taking AccessShareLock */
+                       children = find_all_inheritors(relid, AccessShareLock, NULL);

=> Do you know that returns the table itself ? And in pg14dev, each
partitioned table has reltuples = -1, not zero...

+                               /* Skip foreign partitions */
+                               if (childclass->relkind == RELKIND_FOREIGN_TABLE)
+                                       continue;

=> Michael's suggrestion is to use RELKIND_HAS_STORAGE to skip both foreign and
partitioned tables.

Also, you called SearchSysCacheCopy1, but didn't free the tuple. I don't think
you need to copy it anyway - just call ReleaseSysCache().

Regarding the counters in pg_stat_all_tables: maybe some of these should be
null rather than zero ? Or else you should make an 0001 patch to fully
implement this view, with all relevant counters, not just n_mod_since_analyze,
last_*analyze, and *analyze_count. These are specifically misleading:

last_vacuum |
last_autovacuum |
n_ins_since_vacuum | 0
vacuum_count | 0
autovacuum_count | 0

--
Justin

Horiguchi-san,

Thank you for your comments.

On Fri, Oct 23, 2020 at 8:23 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Thanks you for the new version.

At Fri, 23 Oct 2020 15:12:51 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in

Hello,

I reconsidered a way based on the v5 patch in line with
Horiguchi-san's comment.

This approach is as follows:
- A partitioned table is checked whether it needs analyze like a plain
table in relation_needs_vacanalyze(). To do this, we should store
partitioned table's stats (changes_since_analyze).
- Partitioned table's changes_since_analyze is updated when
analyze a leaf partition by propagating its changes_since_analyze.
In the next scheduled analyze time, it is used in the above process.
That is, the partitioned table is analyzed behind leaf partitions.
- The propagation process differs between autoanalyze or plain analyze.
In autoanalyze, a leaf partition's changes_since_analyze is propagated
to *all* ancestors. Whereas, in plain analyze on an inheritance tree,
propagates to ancestors not included the tree to avoid needless counting.

Attach the latest patch to this email.
Could you check it again please?

+               /*
+                * Get its all ancestors to propagate changes_since_analyze count.
+                * However, when ANALYZE inheritance tree, we get ancestors of
+                * toprel_oid to avoid needless counting.
+                */
+               if (!OidIsValid(toprel_oid))
+                       ancestors = get_partition_ancestors(RelationGetRelid(rel));
+               else
+                       ancestors = get_partition_ancestors(toprel_oid);

This comment doesn't explaining what the code intends but what the
code does.

The reason for the difference is that if we have a valid toprel_oid,
we analyze all descendants of the relation this time, and if we
propagate the number to the descendants of the top relation, the next
analyze on the relations could happen shortly than expected.

I modified this comment according to your advice.

-       msg.m_live_tuples = livetuples;
-       msg.m_dead_tuples = deadtuples;
+       msg.m_live_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+               ? 0  /* if this is a partitioned table, skip modifying */
+               : livetuples;
+       msg.m_dead_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+               ? 0 /* if this is a partitioned table, skip modifying */
+               : deadtuples;

Two successive branching with the same condition looks odd. And we
need an explanation of *why* we don't set the values for partitioned
tables.

I moved this part to the previous block that livetuples and deadtuples are set.
Actually, I think the reason why those counters are set 0 when the given
relation is a partitioned table is that such a table doesn't have any data.
About changes_since_analyze counter, we should support exceptionally
in order to check whether partitioned tables need auto analyze.
I added this description to the comment of this function.

+               foreach(lc, ancestors)
+               {
+                       Oid     parentOid = lfirst_oid(lc);
+                       Relation parentrel;
+
+                       parentrel = table_open(parentOid, AccessShareLock);

I'm not sure, but all of the ancestors always cannot be a parent (in
other words, a parent of a parent of mine is not a parent of
mine). Isn't just rel sufficient?

I changed 'parentrel' to 'rel'.

-        * 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.  Reset the
+        * changes_since_analyze counter only if we analyzed all columns;
+        * otherwise, there is still work for auto-analyze to do.
*/
-       if (!inh)
+       if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
pgstat_report_analyze(onerel, totalrows, totaldeadrows,

This still rejects traditional inheritance nonleaf relations. But if
we remove the description about that completely in the comment above,
we should support traditional inheritance parents here. I think we
can do that as far as we need to propagate only per-tuple stats (that
mans not per-attribute) like changes_since_analyze.

Regarding manual ANALYZE, not auto ANALYZE, when analyzing declarative
partitioning, all children are also analyzed at the same time. However,
in the case of traditional inheritance, we need to run that command on
each child table individually. That is, they are not analyzed all together
by ANALYZE. So I tried to support auto analyze for declarative
partitioning for now.
Added that we only support declarative partitioning to that comment.

Whichever way we take, do we need the description about the behavior
in the documentation?

Added a description about this patch to the document.

I attach the latest patch to this email.
Could you please check it again?

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

Hi Justin,

Thank you for your comments.
I attached the latest patch(v11) to the previous email.

+                * Get its all ancestors to propagate changes_since_analyze count.
+                * However, when ANALYZE inheritance tree, we get ancestors of
+                * toprel_oid to avoid needless counting.

=> I don't understand that comment.

I fixed that comment.

+                       /* Find all members of inheritance set taking AccessShareLock */
+                       children = find_all_inheritors(relid, AccessShareLock, NULL);

=> Do you know that returns the table itself ? And in pg14dev, each
partitioned table has reltuples = -1, not zero...

+                               /* Skip foreign partitions */
+                               if (childclass->relkind == RELKIND_FOREIGN_TABLE)
+                                       continue;

=> Michael's suggrestion is to use RELKIND_HAS_STORAGE to skip both foreign and
partitioned tables.

I overlooked that. Revised that according to your comments.

Also, you called SearchSysCacheCopy1, but didn't free the tuple. I don't think
you need to copy it anyway - just call ReleaseSysCache().

Fixed it.

Regarding the counters in pg_stat_all_tables: maybe some of these should be
null rather than zero ? Or else you should make an 0001 patch to fully
implement this view, with all relevant counters, not just n_mod_since_analyze,
last_*analyze, and *analyze_count. These are specifically misleading:

last_vacuum |
last_autovacuum |
n_ins_since_vacuum | 0
vacuum_count | 0
autovacuum_count | 0

I haven't modified this part yet, but you meant that we should set
null to counters
about vacuum because partitioned tables are not vacuumed?

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

At Thu, 5 Nov 2020 16:03:12 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in

Hi Justin,

Thank you for your comments.
I attached the latest patch(v11) to the previous email.

+                * Get its all ancestors to propagate changes_since_analyze count.
+                * However, when ANALYZE inheritance tree, we get ancestors of
+                * toprel_oid to avoid needless counting.

=> I don't understand that comment.

I fixed that comment.

+		 * Get its all ancestors to propagate changes_since_analyze count.
+		 * However, when we have a valid toprel_oid, that is ANALYZE inheritance
+		 * tree, if we propagate the number to all ancestors, the next analyze
+		 * on partitioned tables in the tree could happen shortly expected.
+		 * So we get ancestors of toprel_oid which are not analyzed this time.

In second thought about the reason for the "toprel_oid". It is perhaps
to avoid "wrongly" propagated values to ancestors after a manual
ANALYZE on a partitioned table. But the same happens after an
autoanalyze iteration if some of the ancestors of a leaf relation are
analyzed before the leaf relation in a autoanalyze iteration. That
can trigger an unnecessary analyzing for some of the ancestors.
So we need to do a similar thing for autovacuum, However..

[1(root):analzye]-[2:DONT analyze]-[3:analyze]-[leaf]

In this case topre_oid is invalid (since it's autoanalyze) but we
should avoid propagating the count to 1 and 3 if it is processed
*before* the leaf, but should propagate to 2. toprel_oid doesn't work
in that case.

So, to propagate the count properly, we need to analyze relations
leaf-to-root order, or propagate the counter only to anscestors that
haven't been processed in the current iteration. It seems a bit too
complex to sort analyze relations in that order. The latter would be
relatively simple. See the attached for how it looks like.

Anyway, either way we take, it is not pgstat.c's responsibility to do
that since the former need to heavily reliant to what analyze does,
and the latter need to know what anlyze is doing.

Also, you called SearchSysCacheCopy1, but didn't free the tuple. I don't think
you need to copy it anyway - just call ReleaseSysCache().

Fixed it.

Mmm. Unfortunately, that fix leaks cache reference when
!RELKIND_HAS_STORAGE.

Regarding the counters in pg_stat_all_tables: maybe some of these should be
null rather than zero ? Or else you should make an 0001 patch to fully
implement this view, with all relevant counters, not just n_mod_since_analyze,
last_*analyze, and *analyze_count. These are specifically misleading:

last_vacuum |
last_autovacuum |
n_ins_since_vacuum | 0
vacuum_count | 0
autovacuum_count | 0

I haven't modified this part yet, but you meant that we should set
null to counters
about vacuum because partitioned tables are not vacuumed?

Perhaps bacause partitioned tables *cannot* be vacuumed. I'm not sure
what is the best way here. Showing null seems reasonable but I'm not
sure that doesn't break anything.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

I looked at both Yuzuko Hosoya's patch and Kyotaro Horiguchi's, and
think we're doing things in a quite complicated manner, which perhaps
could be done more easily.

Hosoya's patch has pgstat_report_analyze call pgstat_get_tab_entry() for
the table being vacuumed, then obtains the list of ancestors, and then
sends for each ancestor a new message containing the partition's
changes_since_analyze for that ancestor. When stat collector receives
that message, it adds the number to the ancestor's m_changed_tuples.

Horiguchi's doing a similar thing, only differently: it is do_analyze_rel
that reads the count from the collector (this time by calling SQL
function pg_stat_get_mod_since_analyze) and then sends number back to
the collector for each ancestor.

I suggest that a better way to do this, is to forget about the new
"partchanges" message completely. Instead, let's add an array of
ancestors to the analyze message (borrowing from PgStat_MsgFuncstat).
Something like this:

#define PGSTAT_NUM_ANCESTORENTRIES \
((PGSTAT_MSG_PAYLOAD - sizeof(Oid) - sizeof(Oid) - sizeof(bool) - \
sizeof(bool) - sizeof(TimestampTz) - sizeof(PgStat_Counter) - \
sizeof(PgStat_Counter) - sizeof(int)) / sizeof(Oid))
typedef struct PgStat_MsgAnalyze
{
PgStat_MsgHdr m_hdr;
Oid m_databaseid;
Oid m_tableoid;
bool m_autovacuum;
bool m_resetcounter;
TimestampTz m_analyzetime;
PgStat_Counter m_live_tuples;
PgStat_Counter m_dead_tuples;
int m_nancestors;
Oid m_ancestors[PGSTAT_NUM_ANCESTORENTRIES];
} PgStat_MsgAnalyze;

For non-partitions, m_nancestors would be 0, so the message would be
handled as today. For partitions, the array carries the OID of all
ancestors. When the collector receives this message, first it looks up
the pgstat entries for each ancestors in the array, and it adds the
partition's current changes_since_analyze to the ancestor's
changes_since_analyze. Then it does things as currently, including
reset the changes_since_analyze counter for the partition.

Key point in this is that we don't need to read the number from
collector into the backend executing analyze. We just *send* the data
about ancestors, and the collector knows what to do with it.

One possible complaint is: what if there are more ancestors that fit in
the message? I propose that this problem can be ignored, since in order
to hit this, you'd need to have (1000-8-4-4-1-1-8-8-8-4)/4 = 238
ancestors (if my math is right). I doubt we'll hit the need to use 238
levels of partitionings before a stat collector rewrite occurs ...

(It is possible to remove that restriction by doing more complicated
things such as sending the list of ancestor in a new type of message
that can be sent several times, prior to the analyze message itself, but
I don't think this is worth the trouble.)

On 2020-Nov-10, Kyotaro Horiguchi wrote:

In second thought about the reason for the "toprel_oid". It is perhaps
to avoid "wrongly" propagated values to ancestors after a manual
ANALYZE on a partitioned table. But the same happens after an
autoanalyze iteration if some of the ancestors of a leaf relation are
analyzed before the leaf relation in a autoanalyze iteration. That
can trigger an unnecessary analyzing for some of the ancestors.

I'm not sure I understand this point. I think we should only trigger
this on analyzes of *leaf* partitions, not intermediate partitioned
relations. That way you would never get these unnecesary analyzes.
Am I missing something?

(So with my proposal in the previous email, we would send the list of
ancestor relations after analyzing a leaf partition. Whenever we
analyze a non-leaf, then the list of ancestors is sent as an empty
list.)

Regarding the counters in pg_stat_all_tables: maybe some of these should be
null rather than zero ? Or else you should make an 0001 patch to fully
implement this view, with all relevant counters, not just n_mod_since_analyze,
last_*analyze, and *analyze_count. These are specifically misleading:

last_vacuum |
last_autovacuum |
n_ins_since_vacuum | 0
vacuum_count | 0
autovacuum_count | 0

I haven't modified this part yet, but you meant that we should set
null to counters
about vacuum because partitioned tables are not vacuumed?

Perhaps bacause partitioned tables *cannot* be vacuumed. I'm not sure
what is the best way here. Showing null seems reasonable but I'm not
sure that doesn't break anything.

I agree that showing NULLs for the vacuum columns is better. Perhaps
the most reasonable way to do this is use -1 as an indicator that NULL
ought to be returned from pg_stat_get_vacuum_count() et al, and add a
boolean in PgStat_TableCounts next to t_truncated, maybe "t_nullvacuum"
that says to store -1 instead of 0 in pgstat_recv_tabstat.

Hello Alvaro,

Thank you for your comments.

In second thought about the reason for the "toprel_oid". It is perhaps
to avoid "wrongly" propagated values to ancestors after a manual
ANALYZE on a partitioned table. But the same happens after an
autoanalyze iteration if some of the ancestors of a leaf relation are
analyzed before the leaf relation in a autoanalyze iteration. That
can trigger an unnecessary analyzing for some of the ancestors.

I'm not sure I understand this point. I think we should only trigger
this on analyzes of *leaf* partitions, not intermediate partitioned
relations. That way you would never get these unnecesary analyzes.
Am I missing something?

(So with my proposal in the previous email, we would send the list of
ancestor relations after analyzing a leaf partition. Whenever we
analyze a non-leaf, then the list of ancestors is sent as an empty
list.)

The problem Horiguchi-san mentioned is as follows:

create table p1 (i int) partition by range(i);
create table p1_1 partition of p1 for values from (0) to (500)
partition by range(i);
create table p1_1_1 partition of p1_1 for values from (0) to (300);
insert into p1 select generate_series(0,299);

-- After auto analyze (first time)
postgres=# select relname, n_mod_since_analyze, last_autoanalyze from
pg_stat_all_tables where relname in ('p1','p1_1','p1_1_1');
relname | n_mod_since_analyze | last_autoanalyze
---------+---------------------+-------------------------------
p1 | 300 |
p1_1 | 300 |
p1_1_1 | 0 | 2020-12-02 22:24:18.753574+09
(3 rows)

-- Insert more rows
postgres=# insert into p1 select generate_series(0,199);
postgres=# select relname, n_mod_since_analyze, last_autoanalyze from
pg_stat_all_tables where relname in ('p1','p1_1','p1_1_1');
relname | n_mod_since_analyze | last_autoanalyze
---------+---------------------+-------------------------------
p1 | 300 |
p1_1 | 300 |
p1_1_1 | 200 | 2020-12-02 22:24:18.753574+09
(3 rows)

-- After auto analyze (second time)
postgres=# select relname, n_mod_since_analyze, last_autoanalyze from
pg_stat_all_tables where relname in ('p1','p1_1','p1_1_1');
relname | n_mod_since_analyze | last_autoanalyze
---------+---------------------+-------------------------------
p1 | 0 | 2020-12-02 22:25:18.857248+09
p1_1 | 200 | 2020-12-02 22:25:18.661932+09
p1_1_1 | 0 | 2020-12-02 22:25:18.792078+09

After 2nd auto analyze, all relations' n_mod_since_analyze should be 0,
but p1_1's is not. This is because p1_1 was analyzed before p1_1_1.
So p1_1 will be analyzed again in the 3rd auto analyze.
That is propagating changes_since_analyze to *all ancestors* may cause
unnecessary analyzes even if we do this only when analyzing leaf partitions.
So I think we should track ancestors which are not analyzed in the current
iteration as Horiguchi-san proposed.

Regarding your idea:

typedef struct PgStat_MsgAnalyze
{
PgStat_MsgHdr m_hdr;
Oid m_databaseid;
Oid m_tableoid;
bool m_autovacuum;
bool m_resetcounter;
TimestampTz m_analyzetime;
PgStat_Counter m_live_tuples;
PgStat_Counter m_dead_tuples;
int m_nancestors;
Oid m_ancestors[PGSTAT_NUM_ANCESTORENTRIES];
} PgStat_MsgAnalyze;

I'm not sure but how about storing only ancestors that aren't analyzed
in the current
iteration in m_ancestors[PGSTAT_NUM_ANCESTORENTRIES] ?

Regarding the counters in pg_stat_all_tables: maybe some of these should be
null rather than zero ? Or else you should make an 0001 patch to fully
implement this view, with all relevant counters, not just n_mod_since_analyze,
last_*analyze, and *analyze_count. These are specifically misleading:

last_vacuum |
last_autovacuum |
n_ins_since_vacuum | 0
vacuum_count | 0
autovacuum_count | 0

I haven't modified this part yet, but you meant that we should set
null to counters
about vacuum because partitioned tables are not vacuumed?

Perhaps bacause partitioned tables *cannot* be vacuumed. I'm not sure
what is the best way here. Showing null seems reasonable but I'm not
sure that doesn't break anything.

I agree that showing NULLs for the vacuum columns is better. Perhaps
the most reasonable way to do this is use -1 as an indicator that NULL
ought to be returned from pg_stat_get_vacuum_count() et al, and add a
boolean in PgStat_TableCounts next to t_truncated, maybe "t_nullvacuum"
that says to store -1 instead of 0 in pgstat_recv_tabstat.

Thank you for the advice. I'll fix it based on this idea.

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

Hello Yuzuko,

On 2020-Dec-02, yuzuko wrote:

The problem Horiguchi-san mentioned is as follows:
[explanation]

Hmm, I see. So the problem is that if some ancestor is analyzed first,
then analyze of one of its partition will cause a redundant analyze of
the ancestor, because the number of tuples that is propagated from the
partition represents a set that had already been included in the
ancestor's analysis.

If the problem was just that, then I think it would be very simple to
solve: just make sure to sort the tables to vacuum so that all leaves
are vacuumed first, and then all ancestors, sorted from the bottom up.
Problem solved.

But I'm not sure that that's the whole story, for two reasons: one, two
workers can run simultaneously, where one analyzes the partition and the
other analyzes the ancestor. Then the order is not guaranteed (and
each process will get no effect from remembering whether it did that one
or not). Second, manual analyzes can occur in any order.

Maybe it's more useful to think about this in terms of rememebering that
partition P had changed_tuples set to N when we analyzed ancestor A.
Then, when we analyze partition P, we send the message listing A as
ancestor; on receipt of that message, we see M+N changed tuples in P,
but we know that we had already seen N, so we only record M.

I'm not sure how to implement this idea however, since on analyze of
ancestor A we don't have the list of partitions, so we can't know the N
for each partition.

Hello Alvaro,

On Thu, Dec 3, 2020 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello Yuzuko,

On 2020-Dec-02, yuzuko wrote:

The problem Horiguchi-san mentioned is as follows:
[explanation]

Hmm, I see. So the problem is that if some ancestor is analyzed first,
then analyze of one of its partition will cause a redundant analyze of
the ancestor, because the number of tuples that is propagated from the
partition represents a set that had already been included in the
ancestor's analysis.

If the problem was just that, then I think it would be very simple to
solve: just make sure to sort the tables to vacuum so that all leaves
are vacuumed first, and then all ancestors, sorted from the bottom up.
Problem solved.

Indeed. When discussed with Horiguchi-san before, He mentioned
the same way:

So, to propagate the count properly, we need to analyze relations
leaf-to-root order, or propagate the counter only to anscestors that
haven't been processed in the current iteration. It seems a bit too
complex to sort analyze relations in that order.

but we didn't select it because of its complexity as you also said.

But I'm not sure that that's the whole story, for two reasons: one, two
workers can run simultaneously, where one analyzes the partition and the
other analyzes the ancestor. Then the order is not guaranteed (and
each process will get no effect from remembering whether it did that one
or not). Second, manual analyzes can occur in any order.

Maybe it's more useful to think about this in terms of rememebering that
partition P had changed_tuples set to N when we analyzed ancestor A.
Then, when we analyze partition P, we send the message listing A as
ancestor; on receipt of that message, we see M+N changed tuples in P,
but we know that we had already seen N, so we only record M.

I'm not sure how to implement this idea however, since on analyze of
ancestor A we don't have the list of partitions, so we can't know the N
for each partition.

I thought about it for a while, but I can't come up with how to implement it.
And also I think the other way Horiguchi-san suggested in [1]/messages/by-id/20201110.203557.1420746510378864931.horikyota.ntt@gmail.com would be
more simple to solve the problem we are facing.

Attach the new patch based on his patch. What do you think?

[1]: /messages/by-id/20201110.203557.1420746510378864931.horikyota.ntt@gmail.com

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

On 12/14/20 8:46 PM, yuzuko wrote:

On Thu, Dec 3, 2020 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Attach the new patch based on his patch. What do you think?

Álvaro, Justin, Kyotaro, thoughts on this latest patch?

Regards,
--
-David
david@pgmasters.net

Hi,

I took a look at this patch. It does not apply because of 5f8727f5a67,
so a rebase is needed. But I want to talk about the general approach in
general, so it does not matter.

The thread is fairly long, both in terms of number of messages and time
(started in 2019), so let me restate my understanding of the problem and
what the patch aims to do.

The problem is that autovacuum never analyzes non-leaf relations in
partition hierarchies, because they never get modified and so the value
of changes_since_analyze remains 0. This applies both to partitioning
based on inheritance and the new fancy declarative partitioning. The
consequence is that we never have accurate statistics (MCV, histograms
and so on) for the parent, which may lead to poor query plans in cases
when we don't use the child statistics for some reason.

The patch aims for fix that by propagating the changes_since_analyze to
the parent relations, so that the autovacuum properly considers if those
non-leaf relations need analyze.

I think the goal is right, and propagating the changes_since_analyze is
the right solution, but as coded it has a couple issues that may cause
trouble in practice.

Firstly, the patch propagates the changes_since_analyze values from
do_analyze_rel, i.e. from the worker after it analyzes the relation.
That may easily lead to cases with unnecessary analyzes - consider a
partitioned with 4 child relations:

p1 [reltuples=1M, changes_since_analyze=400k]
p2 [reltuples=1M, changes_since_analyze=90k]
p3 [reltuples=1M, changes_since_analyze=90k]
p4 [reltuples=1M, changes_since_analyze=90k]

With the default analyze threshold (10%) this means autoanalyze of p1,
and then (in the next round) analyze of the whole partitioned table,
because 400k is 10% of 4M. So far so good - we're now in this state:

p1 [reltuples=1M, changes_since_analyze=0]
p2 [reltuples=1M, changes_since_analyze=90k]
p3 [reltuples=1M, changes_since_analyze=90k]
p4 [reltuples=1M, changes_since_analyze=90k]

Let's do ~310k more modifications to p2:

p1 [reltuples=1M, changes_since_analyze=0]
p2 [reltuples=1M, changes_since_analyze=400k]
p3 [reltuples=1M, changes_since_analyze=90k]
p4 [reltuples=1M, changes_since_analyze=90k]

Now p2 gets analyzed, and the value gets propagate to p, triggering the
analyze. But that's bogus - we've already seen 90k of those rows in the
last analyze, the "actual" changes_since_analyze is just 310k and that
should have not triggered the analyze.

I could invent a much more extreme examples with more partitions, and or
with multiple autovacuum workers processing the leaf rels concurrently.

This seems like a quite serious issue, because analyzes on partitioned
tables sample all the partitions, which seems rather expensive. That is
not an issue introduced by this patch, of course, but it's good to keep
that in mind and not make the matters worse.

Note: I do have some ideas about how to improve that, I've started a
separate thread about it [1]https://commitfest.postgresql.org/33/3052/.

IMHO the primary issue is the patch is trying to report the counts from
the workers, and it's done incrementally, after the fact. It tries to
prevent the issue by not propagating the counts to parents analyzed in
the same round, but that doesn't seems sufficient:

- There's no guarantee how long ago the parent was analyzed. Maybe it
was 1 second ago, or maybe it was 24h ago and there have been many new
modifications since then?

- The hash table is per worker, so who knows what did the other
autovacuum workers do?

So not really a good solution, I'm afraid.

I propose a different approach - instead of propagating the counts in
do_analyze_rel for individual leaf tables, let's do that in bulk in
relation_needs_vacanalyze. Before the (existing) first pass over
pg_class, we can add a new one, propagating counts from leaf tables to
parents. I'd imagine something like this

while ((tuple = heap_getnext(relScan, ... != NULL)
{
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(tuple);

... find all ancestors for classForm ...

pgstat_propagate_changes(classForm, ancestors);
}

The pgstat_propagate_changes() simply informs the pgstat collector that
classForm has certain ancestors, and it propagates the value to all of
them. There's a problem, though - we can't reset the value for the leaf
table, because we need to check if it needs analyze, but we also don't
want to sent it again next time. But we can add another counter,
tracking that part of changes_since_analyze we already propagated, and
propagate only the difference. That is, we now have this:

PgStat_Counter changes_since_analyze;
PgStat_Counter changes_since_analyze_reported;

So for example we start with

changes_since_analyze = 10000;
changes_since_analyze_reported = 0;

and we propagate 10k to parents:

changes_since_analyze = 10000;
changes_since_analyze_reported = 10000;

but we don't analyze anything, and we accumulate 5k more changes:

changes_since_analyze = 15000;
changes_since_analyze_reported = 10000;

so now we propagate only the 5k delta. And so on. It's not exactly
atomic change (we still do this per relation), but it's "bulk" in the
sense that we force the propagation and don't wait until after the leaf
happens to be analyzed.

It might need to reread the stats file I think, to get the incremented
values, but that seems acceptable.

We may need to "sync" the counts for individual relations in a couple
places (e.g. after the worker is done with the leaf, it should propagate
the remaining delta before resetting the values to 0). Maybe multi-level
partitioning needs some additional handling, not sure.

regards

[1]: https://commitfest.postgresql.org/33/3052/

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/30/21 4:09 AM, Tomas Vondra wrote:

Hi,

...

We may need to "sync" the counts for individual relations in a couple
places (e.g. after the worker is done with the leaf, it should propagate
the remaining delta before resetting the values to 0). Maybe multi-level
partitioning needs some additional handling, not sure.

I forgot to mention one additional thing yesterday - I wonder if we need
to do something similar after a partition is attached/detached. That can
also change the parent's statistics significantly, so maybe we should
handle all partition's rows as changes_since_analyze? Not necessarily
something this patch has to handle, but might be related.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Tomas,

Thank you for reviewing the patch.

Firstly, the patch propagates the changes_since_analyze values from
do_analyze_rel, i.e. from the worker after it analyzes the relation.
That may easily lead to cases with unnecessary analyzes - consider a
partitioned with 4 child relations:
[ explanation ]

I didn't realize that till now. Indeed, this approach increments parent's
changes_since_analyze counter according to its leaf partition's counter
when the leaf partition is analyzed, so it will cause unnecessary ANALYZE
on partitioned tables as you described.

I propose a different approach - instead of propagating the counts in
do_analyze_rel for individual leaf tables, let's do that in bulk in
relation_needs_vacanalyze. Before the (existing) first pass over
pg_class, we can add a new one, propagating counts from leaf tables to
parents.

Thank you for your suggestion. I think it could solve all the issues
you mentioned. I modified the patch based on this approach:

- Create a new counter, PgStat_Counter changes_since_analyze_reported,
to track changes_since_analyze we already propagated to ancestors.
This is used for internal processing and users may not need to know it.
So this counter is not displayed at pg_stat_all_tables view for now.

- Create a new function, pgstat_propagate_changes() which propagates
changes_since_analyze counter to all ancestors and saves
changes_since_analyze_reported. This function is called in
do_autovacuum() before relation_needs_vacanalyze().

Note: I do have some ideas about how to improve that, I've started a
separate thread about it [1].

I'm also interested in merging children's statistics for partitioned tables
because it will make ANALYZE on inheritance trees more efficient.
So I'll check it later.

I forgot to mention one additional thing yesterday - I wonder if we need
to do something similar after a partition is attached/detached. That can
also change the parent's statistics significantly, so maybe we should
handle all partition's rows as changes_since_analyze? Not necessarily
something this patch has to handle, but might be related.

Regarding attached/detached partitions, I think we should update statistics
of partitioned tables according to the new inheritance tree. The latest patch
hasn't handled this case yet, but I'll give it a try soon.

Attach the v13 patch to this email. Could you please check it again?

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

Thanks for the quick rework. I like this design much better and I think
this is pretty close to committable. Here's a rebased copy with some
small cleanups (most notably, avoid calling pgstat_propagate_changes
when the partition doesn't have a tabstat entry; also, free the lists
that are allocated in a couple of places).

I didn't actually verify that it works.

--
Álvaro Herrera Valdivia, Chile
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

On 4/3/21 9:42 PM, Alvaro Herrera wrote:

Thanks for the quick rework. I like this design much better and I think
this is pretty close to committable. Here's a rebased copy with some
small cleanups (most notably, avoid calling pgstat_propagate_changes
when the partition doesn't have a tabstat entry; also, free the lists
that are allocated in a couple of places).

I didn't actually verify that it works.

Yeah, this approach seems much simpler, I think. That being said, I
think there's a couple issues:

1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?

2) pgstat_recv_tabstat

Should it really reset changes_since_analyze_reported in both branches?
AFAICS if the "found" branch does this

tabentry->changes_since_analyze_reported = 0;

that means we lose the counter any time tabstats are received, no?
That'd be wrong, because we'd propagate the changes repeatedly.

3) pgstat_recv_analyze

Shouldn't it propagate the counters before resetting them? I understand
that for the just-analyzed relation we can't do better, but why not to
propagate the counters to parents? (Not necessarily from this place in
the stat collector, maybe the analyze process should do that.)

4) pgstat_recv_reportedchanges

I think this needs to be more careful when updating the value - the
stats collector might have received other messages modifying those
counters (including e.g. PGSTAT_MTYPE_ANALYZE with a reset), so maybe we
can get into situation with

(changes_since_analyze_reported > changes_since_analyze)

if we just blindly increment the value. I'd bet would lead to funny
stuff. So maybe this should be careful to never exceed this?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Apr-04, Tomas Vondra wrote:

1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?

I suppose the rationale is that for inheritance we have always done it
that way -- similar things have been done that way for inheritance
historically, to avoid messing with long-standing behavior. We have
done that in a bunch of places -- DDL behavior, FKs, etc. Maybe in this
case it's not justified. It *will* change behavior, in the sense that
we are going to capture stats that have never been captured before.
That might or might not affect query plans for designs using regular
inheritance. But it seems reasonable to think that those changes will
be for the good; and if it does break plans for some people and they
want to revert to the original behavior, they can just set
autovacuum_enabled to off for the parent tables.

So, I agree that we should enable this new feature for inheritance
parents too.

I can't comment on the other issues. I hope to give this a closer look
tomorrow my time; with luck Hosoya-san will have commented by then.

--
Álvaro Herrera 39°49'30"S 73°17'W
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

On 4/4/21 10:05 PM, Alvaro Herrera wrote:

On 2021-Apr-04, Tomas Vondra wrote:

1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?

I suppose the rationale is that for inheritance we have always done it
that way -- similar things have been done that way for inheritance
historically, to avoid messing with long-standing behavior. We have
done that in a bunch of places -- DDL behavior, FKs, etc. Maybe in this
case it's not justified. It *will* change behavior, in the sense that
we are going to capture stats that have never been captured before.
That might or might not affect query plans for designs using regular
inheritance. But it seems reasonable to think that those changes will
be for the good; and if it does break plans for some people and they
want to revert to the original behavior, they can just set
autovacuum_enabled to off for the parent tables.

So, I agree that we should enable this new feature for inheritance
parents too.

Not sure. AFAICS the missing stats on parents are an issue both for
inheritance and partitioning. Maybe there is a reason to maintain the
current behavior with inheritance, but I don't see it.

With the other features, I think the reason for not implementing that
for inheritance was that it'd be more complex, compared to declarative
partitioning (which has stricter limitations on the partitions, etc.).
But in this case I think there's no difference in complexity, the same
code can handle both cases.

In fact, one of the first posts in this threads links to this:

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

i.e. Tom actually proposed doing something like this back in 2009, so
presumably he though it's desirable back then.

OTOH he argued against adding another per-table counter and proposed
essentially what the patch did before, i.e. propagating the counter
after analyze. But we know that may trigger analyze too often ...

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Apr-04, Tomas Vondra wrote:

In fact, one of the first posts in this threads links to this:

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

i.e. Tom actually proposed doing something like this back in 2009, so
presumably he though it's desirable back then.

OTOH he argued against adding another per-table counter and proposed
essentially what the patch did before, i.e. propagating the counter
after analyze. But we know that may trigger analyze too often ...

Yeah, I think that's a doomed approach. The reason to avoid another
column is to avoid bloat, which is good but if we end up with an
unworkable design then we know we have to backtrack on it.

I was thinking that we could get away with having a separate pgstat
struct for partitioned tables, to avoid enlarging the struct for all
tables, but if we're going to also include legacy inheritance in the
feature clearly that doesn't work.

--
Álvaro Herrera Valdivia, Chile
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

On 4/4/21 9:08 PM, Tomas Vondra wrote:

On 4/3/21 9:42 PM, Alvaro Herrera wrote:

Thanks for the quick rework. I like this design much better and I think
this is pretty close to committable. Here's a rebased copy with some
small cleanups (most notably, avoid calling pgstat_propagate_changes
when the partition doesn't have a tabstat entry; also, free the lists
that are allocated in a couple of places).

I didn't actually verify that it works.

...

3) pgstat_recv_analyze

Shouldn't it propagate the counters before resetting them? I understand
that for the just-analyzed relation we can't do better, but why not to
propagate the counters to parents? (Not necessarily from this place in
the stat collector, maybe the analyze process should do that.)

FWIW the scenario I had in mind is something like this:

create table t (a int, b int) partition by hash (a);
create table p0 partition of t for values with (modulus 2, remainder 0);
create table p1 partition of t for values with (modulus 2, remainder 1);

insert into t select i, i from generate_series(1,1000000) s(i);

select relname, n_mod_since_analyze from pg_stat_user_tables;

test=# select relname, n_mod_since_analyze from pg_stat_user_tables;
relname | n_mod_since_analyze
---------+---------------------
t | 0
p0 | 499375
p1 | 500625
(3 rows)

test=# analyze p0, p1;
ANALYZE
test=# select relname, n_mod_since_analyze from pg_stat_user_tables;
relname | n_mod_since_analyze
---------+---------------------
t | 0
p0 | 0
p1 | 0
(3 rows)

This may seem a bit silly - who would analyze the hash partitions
directly? However, with other partitioning schemes (list, range) it's
quite plausible that people load data directly into partition. They can
analyze the parent explicitly too, but with multi-level partitioning
that probably requires analyzing all the ancestors.

The other possible scenario is about rows inserted while p0/p1 are being
processed by autoanalyze. That may actually take quite a bit of time,
depending on vacuum cost limit. So I still think we should propagate the
delta after the analyze, before we reset the counters.

I also realized relation_needs_vacanalyze is not really doing what I
suggested - it propagates the counts, but does so in the existing loop
which checks which relations need vacuum/analyze.

That means we may skip the parent table in the *current* round, because
it'll see the old (not yet updated) counts. It's likely to be processed
in the next autovacuum round, but that may actually not happen. The
trouble is the reltuples for the parent is calculated using *current*
child reltuples values, but we're comparing it to the *old* value of
changes_since_analyze. So e.g. if enough rows were inserted into the
partitions, it may still be below the analyze threshold.

What I proposed is adding a separate loop that *only* propagates the
counts, and then re-read the current stats (perhaps only if we actually
propagated anything). And then decide which relations need analyze.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Apr-04, Tomas Vondra wrote:

1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?

I think there is a good reason to treat them the same: pgstat does not
have a provision to keep stats both of the table with children, and the
table without children. It can only have one of those. For
partitioning that doesn't matter: since the table-without-children
doesn't have anything on its own (no scans, no tuples, no nothing) then
we can just use the entry to store the table-with-children data. But
for the inheritance case, the parent can have its own tuples and counts
its own scans and so on; so if we change things, we'll overwrite the
stats. Maybe in the long-term we should allow pgstat to differentiate
those cases, but that seems not in scope for this patch.

I'm working on the code to fix the other issues.

--
Álvaro Herrera 39°49'30"S 73°17'W

Hi,

On 2021-04-06 16:56:49 -0400, Alvaro Herrera wrote:

I think there is a good reason to treat them the same: pgstat does not
have a provision to keep stats both of the table with children, and the
table without children. It can only have one of those. For
partitioning that doesn't matter: since the table-without-children
doesn't have anything on its own (no scans, no tuples, no nothing) then
we can just use the entry to store the table-with-children data. But
for the inheritance case, the parent can have its own tuples and counts
its own scans and so on; so if we change things, we'll overwrite the
stats. Maybe in the long-term we should allow pgstat to differentiate
those cases, but that seems not in scope for this patch.

FWIW, I think it shouldn't be too hard to do that once the shared memory
stats patch goes in (not 14, unfortunately). The hardest part will be to
avoid exploding the number of interface functions, but I think we can
figure out a way to deal with that.

Greetings,

Andres Freund

Hello,

Thank you for reviewing.
I'm working on fixing the patch according to the comments.
I'll send it as soon as I can.

On 2021-04-06 16:56:49 -0400, Alvaro Herrera wrote:

I think there is a good reason to treat them the same: pgstat does not
have a provision to keep stats both of the table with children, and the
table without children. It can only have one of those. For
partitioning that doesn't matter: since the table-without-children
doesn't have anything on its own (no scans, no tuples, no nothing) then
we can just use the entry to store the table-with-children data. But
for the inheritance case, the parent can have its own tuples and counts
its own scans and so on; so if we change things, we'll overwrite the
stats. Maybe in the long-term we should allow pgstat to differentiate
those cases, but that seems not in scope for this patch.

FWIW, I think it shouldn't be too hard to do that once the shared memory
stats patch goes in (not 14, unfortunately). The hardest part will be to
avoid exploding the number of interface functions, but I think we can
figure out a way to deal with that.

I've been thinking about traditional inheritance, I realized that we
need additional
handling to support them because unlike declarative partitioning,
parents may have
some rows in the case of traditional inheritance as Alvaro mentioned.
So I think we should support only declarative partitioning in this
patch for now,
but what do you think? I'm not sure but if we can solve this matter
at low cost by
using the shared memory stats patch, should we wait for the patch?

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

On 2021-Apr-07, yuzuko wrote:

I'm working on fixing the patch according to the comments.
I'll send it as soon as I can.

Thanks, I've been giving it a look too.

I've been thinking about traditional inheritance, I realized that we
need additional
handling to support them because unlike declarative partitioning,
parents may have
some rows in the case of traditional inheritance as Alvaro mentioned.
So I think we should support only declarative partitioning in this
patch for now,
but what do you think?

Yeah, not fixable at present I think.

I'm not sure but if we can solve this matter at low cost by using the
shared memory stats patch, should we wait for the patch?

Let's do that for 15.

--
Álvaro Herrera 39°49'30"S 73°17'W
"The problem with the future is that it keeps turning into the present"
(Hobbes)

Hi,

I fixed the patch according to the following comments.
Attach the latest patch. It is based on v14 patch Alvaro attached before.

On Mon, Apr 5, 2021 at 4:08 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/3/21 9:42 PM, Alvaro Herrera wrote:

Thanks for the quick rework. I like this design much better and I think
this is pretty close to committable. Here's a rebased copy with some
small cleanups (most notably, avoid calling pgstat_propagate_changes
when the partition doesn't have a tabstat entry; also, free the lists
that are allocated in a couple of places).

I didn't actually verify that it works.

Yeah, this approach seems much simpler, I think. That being said, I
think there's a couple issues:

1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?

As we discussed in this thread, this patch should handle only declarative
partitioning for now.

2) pgstat_recv_tabstat

Should it really reset changes_since_analyze_reported in both branches?
AFAICS if the "found" branch does this

tabentry->changes_since_analyze_reported = 0;

that means we lose the counter any time tabstats are received, no?
That'd be wrong, because we'd propagate the changes repeatedly.

I changed the changes_since_analyze_reported counter not to reset.

3) pgstat_recv_analyze

Shouldn't it propagate the counters before resetting them? I understand
that for the just-analyzed relation we can't do better, but why not to
propagate the counters to parents? (Not necessarily from this place in
the stat collector, maybe the analyze process should do that.)

I realized that we should propagate the counters for manual ANALYZE too.
thanks to the examples you offered in another email.
I fixed that for manual ANALYZE.

4) pgstat_recv_reportedchanges

I think this needs to be more careful when updating the value - the
stats collector might have received other messages modifying those
counters (including e.g. PGSTAT_MTYPE_ANALYZE with a reset), so maybe we
can get into situation with

(changes_since_analyze_reported > changes_since_analyze)

if we just blindly increment the value. I'd bet would lead to funny
stuff. So maybe this should be careful to never exceed this?

pgstat_propagate_changes() calls pgstat_report_reportedchanges()
only if (changes_since_analyze_reported < changes_since_analyze).
So I think we won't get into the such situation

(changes_since_analyze_reported > changes_since_analyze)

but am I missing something?

I also realized relation_needs_vacanalyze is not really doing what I
suggested - it propagates the counts, but does so in the existing loop
which checks which relations need vacuum/analyze.

That means we may skip the parent table in the *current* round, because
it'll see the old (not yet updated) counts. It's likely to be processed
in the next autovacuum round, but that may actually not happen. The
trouble is the reltuples for the parent is calculated using *current*
child reltuples values, but we're comparing it to the *old* value of
changes_since_analyze. So e.g. if enough rows were inserted into the
partitions, it may still be below the analyze threshold.

Indeed, the partitioned table was not analyzed at the same timing as
its leaf partitions due to the delay of propagating counters. According
to your proposal, I added a separate loop to propagate the counters
before collecting a list of relations to vacuum/analyze.

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

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

However, I just noticed there is a huge problem, which is that the new
code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
we don't necessarily have a snapshot that lets us do that. While adding
a snapshot acquisition at that spot is a very easy fix, I hesitate to
fix it that way, because the whole idea there seems quite wasteful: we
have to look up, open and lock every single partition, on every single
autovacuum iteration through the database. That seems bad. I'm
inclined to think that a better idea may be to store reltuples for the
partitioned table in pg_class.reltuples, instead of having to add up the
reltuples of each partition. I haven't checked if this is likely to
break anything.

(Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
partition, then we repeatedly propagate the counts to the parent table,
so we would cause the parent to be analyzed more times than it should.
Sounds like we should not send the ancestor list when a column list is
given to manual analyze. I haven't verified this, however.)

--
Álvaro Herrera Valdivia, Chile
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

On 2021-Apr-07, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

Pushed with this approach. Thanks for persisting with this.

--
Álvaro Herrera Valdivia, Chile

On 2021-Apr-07, Alvaro Herrera wrote:

However, I just noticed there is a huge problem, which is that the new
code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
we don't necessarily have a snapshot that lets us do that. While adding
a snapshot acquisition at that spot is a very easy fix, I hesitate to
fix it that way, because the whole idea there seems quite wasteful: we
have to look up, open and lock every single partition, on every single
autovacuum iteration through the database. That seems bad. I'm
inclined to think that a better idea may be to store reltuples for the
partitioned table in pg_class.reltuples, instead of having to add up the
reltuples of each partition. I haven't checked if this is likely to
break anything.

I forgot to comment on this aspect. First, I was obviously mistaken
about there not being an active snapshot. I mean, it's correct that
there isn't. The issue is that it's really a bug to require that there
is one; it just hasn't failed before because partially detached
partitions aren't very common. So I patched that as a bug in a
preliminary patch.

Next, the idea of storing the number of tuples in pg_class.reltuples is
a nice one, and I think we should consider it in the long run. However,
while it can be done as a quick job (shown in the attached, which AFAICT
works fine) there are side-effects -- for example, TRUNCATE doesn't
clear the value, which is surely wrong. I suspect that if I try to
handle it in this way, it would blow up in some corner case I forgot to
consider. So, I decided not to go that way, at least for now.

--
Álvaro Herrera Valdivia, Chile

On 4/8/21 5:22 AM, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

However, I just noticed there is a huge problem, which is that the new
code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
we don't necessarily have a snapshot that lets us do that. While adding
a snapshot acquisition at that spot is a very easy fix, I hesitate to
fix it that way, because the whole idea there seems quite wasteful: we
have to look up, open and lock every single partition, on every single
autovacuum iteration through the database. That seems bad. I'm
inclined to think that a better idea may be to store reltuples for the
partitioned table in pg_class.reltuples, instead of having to add up the
reltuples of each partition. I haven't checked if this is likely to
break anything.

How would that value get updated, for the parent?

(Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
partition, then we repeatedly propagate the counts to the parent table,
so we would cause the parent to be analyzed more times than it should.
Sounds like we should not send the ancestor list when a column list is
given to manual analyze. I haven't verified this, however.)

Are you sure? I haven't tried, but shouldn't this be prevented by only
sending the delta between the current and last reported value?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Apr-08, Tomas Vondra wrote:

On 4/8/21 5:22 AM, Alvaro Herrera wrote:

However, I just noticed there is a huge problem, which is that the new
code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
we don't necessarily have a snapshot that lets us do that. While adding
a snapshot acquisition at that spot is a very easy fix, I hesitate to
fix it that way, because the whole idea there seems quite wasteful: we
have to look up, open and lock every single partition, on every single
autovacuum iteration through the database. That seems bad. I'm
inclined to think that a better idea may be to store reltuples for the
partitioned table in pg_class.reltuples, instead of having to add up the
reltuples of each partition. I haven't checked if this is likely to
break anything.

How would that value get updated, for the parent?

Same as for any other relation: ANALYZE would set it, after it's done
scanning the table. We would to make sure that nothing resets it to
empty, though, and that it doesn't cause issues elsewhere. (The patch I
sent contains the minimal change to make it work, but of course that's
missing having other pieces of code maintain it.)

(Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
partition, then we repeatedly propagate the counts to the parent table,
so we would cause the parent to be analyzed more times than it should.
Sounds like we should not send the ancestor list when a column list is
given to manual analyze. I haven't verified this, however.)

Are you sure? I haven't tried, but shouldn't this be prevented by only
sending the delta between the current and last reported value?

I did try, and yes it behaves as you say.

--
Álvaro Herrera Valdivia, Chile
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree. (Don Knuth)

On 4/8/21 5:27 PM, Alvaro Herrera wrote:

On 2021-Apr-08, Tomas Vondra wrote:

On 4/8/21 5:22 AM, Alvaro Herrera wrote:

However, I just noticed there is a huge problem, which is that the new
code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
we don't necessarily have a snapshot that lets us do that. While adding
a snapshot acquisition at that spot is a very easy fix, I hesitate to
fix it that way, because the whole idea there seems quite wasteful: we
have to look up, open and lock every single partition, on every single
autovacuum iteration through the database. That seems bad. I'm
inclined to think that a better idea may be to store reltuples for the
partitioned table in pg_class.reltuples, instead of having to add up the
reltuples of each partition. I haven't checked if this is likely to
break anything.

How would that value get updated, for the parent?

Same as for any other relation: ANALYZE would set it, after it's done
scanning the table. We would to make sure that nothing resets it to
empty, though, and that it doesn't cause issues elsewhere. (The patch I
sent contains the minimal change to make it work, but of course that's
missing having other pieces of code maintain it.)

So ANALYZE would inspect the child relations, sum the reltuples and set
it for the parent? IMO that'd be problematic because it'd mean we're
comparing the current number of changes with reltuples value which may
be arbitrarily stale (if we haven't analyzed the parent for a while).

That's essentially the issue I described when explaining why I think the
code needs to propagate the changes, reread the stats and then evaluate
which relations need vacuuming. It's similar to the issue of comparing
old changes_since_analyze vs. current reltuples, which is why the code
is rereading the stats before checking the thresholds. This time it's
the opposite direction - the reltuples might be stale.

FWIW I think the current refresh logic is not quite correct, because
autovac_refresh_stats does some throttling (STATS_READ_DELAY). It
probably needs a "force" parameter to ensure it actually reads the
current stats in this one case.

(Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
partition, then we repeatedly propagate the counts to the parent table,
so we would cause the parent to be analyzed more times than it should.
Sounds like we should not send the ancestor list when a column list is
given to manual analyze. I haven't verified this, however.)

Are you sure? I haven't tried, but shouldn't this be prevented by only
sending the delta between the current and last reported value?

I did try, and yes it behaves as you say.

OK, good.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Apr-08, Tomas Vondra wrote:

On 4/8/21 5:27 PM, Alvaro Herrera wrote:

Same as for any other relation: ANALYZE would set it, after it's done
scanning the table. We would to make sure that nothing resets it to
empty, though, and that it doesn't cause issues elsewhere. (The patch I
sent contains the minimal change to make it work, but of course that's
missing having other pieces of code maintain it.)

So ANALYZE would inspect the child relations, sum the reltuples and set
it for the parent? IMO that'd be problematic because it'd mean we're
comparing the current number of changes with reltuples value which may
be arbitrarily stale (if we haven't analyzed the parent for a while).

What? Not at all. reltuples would be set by ANALYZE on one run, and
then the value is available for the future autovacuum run. That's how
it works for regular tables too, so I'm not sure what you problem have
with that. The (possibly stale) reltuples value is multiplied by the
scale factor, and added to the analyze_threshold value, and that's
compared with the current changes_since_analyze to determine whether to
analyze or not.

That's essentially the issue I described when explaining why I think the
code needs to propagate the changes, reread the stats and then evaluate
which relations need vacuuming. It's similar to the issue of comparing
old changes_since_analyze vs. current reltuples, which is why the code
is rereading the stats before checking the thresholds. This time it's
the opposite direction - the reltuples might be stale.

Well, I don't think the issue is the same. reltuples is always stale,
even for regular tables, because that's just how it works.
changes_since_analyze is not stale for regular tables, and that's why it
makes sense to propagate it from partitions to ancestors prior to
checking the analyze condition.

FWIW I think the current refresh logic is not quite correct, because
autovac_refresh_stats does some throttling (STATS_READ_DELAY). It
probably needs a "force" parameter to ensure it actually reads the
current stats in this one case.

Hmm ... good catch, but actually that throttling only applies to the
launcher. do_autovacuum runs in the worker, so there's no throttling.

--
Álvaro Herrera 39°49'30"S 73°17'W

On Thu, Apr 08, 2021 at 01:20:14AM -0400, Alvaro Herrera wrote:

On 2021-Apr-07, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

Pushed with this approach. Thanks for persisting with this.

commit 0827e8af70f4653ba17ed773f123a60eadd9f9c9
| This also introduces necessary reloptions support for partitioned tables
| (autovacuum_enabled, autovacuum_analyze_scale_factor,
| autovacuum_analyze_threshold). It's unclear how best to document this
| aspect.

At least this part needs to be updated - see also ed62d3737.

doc/src/sgml/ref/create_table.sgml- The storage parameters currently
doc/src/sgml/ref/create_table.sgml- available for tables are listed below.
...
doc/src/sgml/ref/create_table.sgml: Specifying these parameters for partitioned tables is not supported,
doc/src/sgml/ref/create_table.sgml- but you may specify them for individual leaf partitions.

--
Justin

On 2021-Apr-08, Justin Pryzby wrote:

commit 0827e8af70f4653ba17ed773f123a60eadd9f9c9
| This also introduces necessary reloptions support for partitioned tables
| (autovacuum_enabled, autovacuum_analyze_scale_factor,
| autovacuum_analyze_threshold). It's unclear how best to document this
| aspect.

At least this part needs to be updated - see also ed62d3737.

doc/src/sgml/ref/create_table.sgml- The storage parameters currently
doc/src/sgml/ref/create_table.sgml- available for tables are listed below.
...
doc/src/sgml/ref/create_table.sgml: Specifying these parameters for partitioned tables is not supported,
doc/src/sgml/ref/create_table.sgml- but you may specify them for individual leaf partitions.

Ah, thanks for pointing it out. How about the attached?

This new bit reads weird:

+    Most parameters are not supported on partitioned tables, with exceptions
+    noted below; you may specify them for individual leaf partitions.

Maybe "Most parameters are not supported on partitioned tables, with
exceptions noted below; you may specify others for individual leaf
partitions."

--
Álvaro Herrera 39°49'30"S 73°17'W

On Thu, Apr 08, 2021 at 05:56:25PM -0400, Alvaro Herrera wrote:

On 2021-Apr-08, Justin Pryzby wrote:

commit 0827e8af70f4653ba17ed773f123a60eadd9f9c9
| This also introduces necessary reloptions support for partitioned tables
| (autovacuum_enabled, autovacuum_analyze_scale_factor,
| autovacuum_analyze_threshold). It's unclear how best to document this
| aspect.

At least this part needs to be updated - see also ed62d3737.

doc/src/sgml/ref/create_table.sgml- The storage parameters currently
doc/src/sgml/ref/create_table.sgml- available for tables are listed below.
...
doc/src/sgml/ref/create_table.sgml: Specifying these parameters for partitioned tables is not supported,
doc/src/sgml/ref/create_table.sgml- but you may specify them for individual leaf partitions.

Ah, thanks for pointing it out. How about the attached?

This new bit reads weird:

+    Most parameters are not supported on partitioned tables, with exceptions
+    noted below; you may specify them for individual leaf partitions.

"Except where noted, these parameters are not supported on partitioned tables."

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

On Thu, Apr 08, 2021 at 05:56:25PM -0400, Alvaro Herrera wrote:

This new bit reads weird:

+    Most parameters are not supported on partitioned tables, with exceptions
+    noted below; you may specify them for individual leaf partitions.

"Except where noted, these parameters are not supported on partitioned
tables."

I think what it's trying to get at is

"Except where noted, these parameters are not supported on partitioned
tables. However, you can specify them on individual leaf partitions."

regards, tom lane

Hi,

On 2021-04-08 01:20:14 -0400, Alvaro Herrera wrote:

On 2021-Apr-07, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

Pushed with this approach. Thanks for persisting with this.

I'm looking at this in the context of rebasing & polishing the shared
memory stats patch.

I have a few questions / concerns:

1) Somehow it seems like a violation to do stuff like
get_partition_ancestors() in pgstat.c. It's nothing I can't live with, but
it feels a bit off. Would likely not be too hard to address, e.g. by just
putting some of pgstat_report_anl_ancestors in partition.c instead.

2) Why does it make sense that autovacuum sends a stats message for every
partition in the system that had any chances since the last autovacuum
cycle? On a database with a good number of objects / a short naptime we'll
often end up sending messages for the same set of tables from separate
workers, because they don't yet see the concurrent
tabentry->changes_since_analyze_reported.

3) What is the goal of the autovac_refresh_stats() after the loop doing
pgstat_report_anl_ancestors()? I think it'll be common that the stats
collector hasn't even processed the incoming messages by that point, not to
speak of actually having written out a new stats file. If it took less than
10ms (PGSTAT_RETRY_DELAY) to get to autovac_refresh_stats(),
backend_read_statsfile() will not wait for a new stats file to be written
out, and we'll just re-read the state we previously did.

It's pretty expensive to re-read the stats file in some workloads, so I'm a
bit concerned that we end up significantly increasing the amount of stats
updates/reads, without actually gaining anything reliable?

4) In the shared mem stats patch I went to a fair bit of trouble to try to get
rid of pgstat_vacuum_stat() (which scales extremely poorly to larger
systems). For that to work pending stats can only be "staged" while holding
a lock on a relation that prevents the relation from being concurrently
dropped (pending stats increment a refcount for the shared stats object,
which ensures that we don't loose track of the fact that a stats object has
been dropped, even when stats only get submitted later).

I'm not yet clear on how to make this work for
pgstat_report_anl_ancestors() - but I probably can find a way. But it does
feel a bit off to issue stats stuff for tables we're not sure still exist.

I'll go and read through the thread, but my first thought is that having a
hashtable in do_autovacuum() that contains stats for partitioned tables would
be a good bit more efficient than the current approach? We already have a
hashtable for each toast table, compared to that having a hashtable for each
partitioned table doesn't seem like it'd be a problem?

With a small bit of extra work that could even avoid the need for the
additional pass through pg_class. Do the partitioned table data-gathering as
part of the "collect main tables to vacuum" pass, and then do one of

a) do the perform-analyze decision purely off the contents of that
partioned-table hash
b) fetch the RELOID syscache entry by oid and then decide based on that
c) handle partioned tableas as part of the "check TOAST tables" pass - it's
not like we gain a meaningful amount of efficiency by using a ScanKey to
filter for RELKIND_TOASTVALUE, given that there's no index, and that an
index wouldn't commonly be useful given the percentage of toast tables in
pg_class

Partitioning makes it a bigger issue that do_autovacuum() does multiple passes
through pg_class (as it makes scenarios in which pg_class is large more
common), so I don't think it's great that partitioning also increases the
number of passes through pg_class.

Greetings,

Andres Freund

Hi,

On 2021-07-22 13:54:58 -0700, Andres Freund wrote:

On 2021-04-08 01:20:14 -0400, Alvaro Herrera wrote:

On 2021-Apr-07, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

Pushed with this approach. Thanks for persisting with this.

I'm looking at this in the context of rebasing & polishing the shared
memory stats patch.

I have a few questions / concerns:

Another one, and I think this might warrant thinking about for v14:

Isn't this going to create a *lot* of redundant sampling? Especially if you
have any sort of nested partition tree. In the most absurd case a partition
with n parents will get sampled n times, solely due to changes to itself.

Look at the following example:

BEGIN;
DROP TABLE if exists p;
CREATE TABLE p (i int) partition by range(i);
CREATE TABLE p_0 PARTITION OF p FOR VALUES FROM ( 0) to (5000) partition by range(i);
CREATE TABLE p_0_0 PARTITION OF p_0 FOR VALUES FROM ( 0) to (1000);
CREATE TABLE p_0_1 PARTITION OF p_0 FOR VALUES FROM (1000) to (2000);
CREATE TABLE p_0_2 PARTITION OF p_0 FOR VALUES FROM (2000) to (3000);
CREATE TABLE p_0_3 PARTITION OF p_0 FOR VALUES FROM (3000) to (4000);
CREATE TABLE p_0_4 PARTITION OF p_0 FOR VALUES FROM (4000) to (5000);
-- create some initial data
INSERT INTO p select generate_series(0, 5000 - 1) data FROM generate_series(1, 100) reps;
COMMIT;

UPDATE p_0_4 SET i = i;

Whenever the update is executed, all partitions will be sampled at least twice
(once for p and once for p_0), with p_0_4 sampled three times.

Of course, this is an extreme example, but it's not hard to imagine cases
where v14 will cause the number of auto-analyzes increase sufficiently to bog
down autovacuum to a problematic degree.

Additionally, while analyzing all child partitions for a partitioned tables
are AccessShareLock'ed at once. If a partition hierarchy has more than one
level, it actually is likely that multiple autovacuum workers will end up
processing the ancestors separately. This seems like it might contribute to
lock exhaustion issues with larger partition hierarchies?

Greetings,

Andres Freund

Hi,

CCing RMT because I think we need to do something about this for v14.

On 2021-07-27 19:23:42 -0700, Andres Freund wrote:

On 2021-07-22 13:54:58 -0700, Andres Freund wrote:

On 2021-04-08 01:20:14 -0400, Alvaro Herrera wrote:

On 2021-Apr-07, Alvaro Herrera wrote:

OK, I bit the bullet and re-did the logic in the way I had proposed
earlier in the thread: do the propagation on the collector's side, by
sending only the list of ancestors: the collector can read the tuple
change count by itself, to add it to each ancestor. This seems less
wasteful. Attached is v16 which does it that way and seems to work
nicely under my testing.

Pushed with this approach. Thanks for persisting with this.

I'm looking at this in the context of rebasing & polishing the shared
memory stats patch.

I have a few questions / concerns:

Another one, and I think this might warrant thinking about for v14:

Isn't this going to create a *lot* of redundant sampling? Especially if you
have any sort of nested partition tree. In the most absurd case a partition
with n parents will get sampled n times, solely due to changes to itself.

Look at the following example:

BEGIN;
DROP TABLE if exists p;
CREATE TABLE p (i int) partition by range(i);
CREATE TABLE p_0 PARTITION OF p FOR VALUES FROM ( 0) to (5000) partition by range(i);
CREATE TABLE p_0_0 PARTITION OF p_0 FOR VALUES FROM ( 0) to (1000);
CREATE TABLE p_0_1 PARTITION OF p_0 FOR VALUES FROM (1000) to (2000);
CREATE TABLE p_0_2 PARTITION OF p_0 FOR VALUES FROM (2000) to (3000);
CREATE TABLE p_0_3 PARTITION OF p_0 FOR VALUES FROM (3000) to (4000);
CREATE TABLE p_0_4 PARTITION OF p_0 FOR VALUES FROM (4000) to (5000);
-- create some initial data
INSERT INTO p select generate_series(0, 5000 - 1) data FROM generate_series(1, 100) reps;
COMMIT;

UPDATE p_0_4 SET i = i;

Whenever the update is executed, all partitions will be sampled at least twice
(once for p and once for p_0), with p_0_4 sampled three times.

Of course, this is an extreme example, but it's not hard to imagine cases
where v14 will cause the number of auto-analyzes increase sufficiently to bog
down autovacuum to a problematic degree.

Additionally, while analyzing all child partitions for a partitioned tables
are AccessShareLock'ed at once. If a partition hierarchy has more than one
level, it actually is likely that multiple autovacuum workers will end up
processing the ancestors separately. This seems like it might contribute to
lock exhaustion issues with larger partition hierarchies?

I started to write a patch rejiggering autovacuum.c portion of this
change. While testing it I hit the case of manual ANALYZEs leaving
changes_since_analyze for partitioned tables in a bogus state - without a
minimally invasive way to fix that. After a bit of confused staring I realized
that the current code has a very similar problem:

Using the same setup as above:

INSERT INTO p VALUES (0,0); /* repeat as many times as desired */
ANALYZE p_0_0;

At this point the system will have lost track of the changes to p_0_0, unless
an autovacuum worker was launched between the INSERTs and the ANALYZE (which
would cause pgstat_report_anl_ancestors() to report the change count upwards).

There appears to be code trying to address that, but I don't see how it
ever does anything meaningful?

/*
* Now report ANALYZE to the stats collector. For regular tables, we do
* it only if not doing inherited stats. For partitioned tables, we only
* do it for inherited stats. (We're never called for not-inherited stats
* on partitioned tables anyway.)
*
* Reset the changes_since_analyze counter only if we analyzed all
* columns; otherwise, there is still work for auto-analyze to do.
*/
if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
pgstat_report_analyze(onerel, totalrows, totaldeadrows,
(va_cols == NIL));

/*
* If this is a manual analyze of all columns of a permanent leaf
* partition, and not doing inherited stats, also let the collector know
* about the ancestor tables of this partition. Autovacuum does the
* equivalent of this at the start of its run, so there's no reason to do
* it there.
*/
if (!inh && !IsAutoVacuumWorkerProcess() &&
(va_cols == NIL) &&
onerel->rd_rel->relispartition &&
onerel->rd_rel->relkind == RELKIND_RELATION &&
onerel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
{
pgstat_report_anl_ancestors(RelationGetRelid(onerel));
}

The pgstat_report_analyze() triggers pgstat_recv_analyze() to reset the
counter that pgstat_recv_anl_ancestors() would use to report changes
upwards:

/*
* If commanded, reset changes_since_analyze to zero. This forgets any
* changes that were committed while the ANALYZE was in progress, but we
* have no good way to estimate how many of those there were.
*/
if (msg->m_resetcounter)
{
tabentry->changes_since_analyze = 0;
tabentry->changes_since_analyze_reported = 0;
}

And if one instead inverts the order of pgstat_report_analyze() and
pgstat_report_anl_ancestors() one gets a slightly different problem: A manual
ANALYZE of the partition root results in the partition root having a non-zero
changes_since_analyze afterwards. expand_vacuum() causes child partitions to be
added to the list of relations, which *first* causes the partition root to be
analyzed, and *then* partitions. The partitions then report their
changes_since_analyze upwards.

I don't think the code as is is fit for v14. It looks like it was rewritten
with a new approach just before the freeze ([1]/messages/by-id/20210408032235.GA6842@alvherre.pgsql), and as far as I can tell the
concerns I quoted above weren't even discussed in the whole thread. Alvaro,
any comments?

Greetings,

Andres Freund

[1]: /messages/by-id/20210408032235.GA6842@alvherre.pgsql

On 7/29/21 9:03 PM, Andres Freund wrote:

Hi,

CCing RMT because I think we need to do something about this for v14.

Thanks. We are now aware of it.

[...]

I don't think the code as is is fit for v14. It looks like it was rewritten
with a new approach just before the freeze ([1]), and as far as I can tell the
concerns I quoted above weren't even discussed in the whole thread. Alvaro,
any comments?

I discussed this briefly with Alvaro late last night. He's now aware of
the issue, but I believe he's away for some days, and probably won't be
able to respond until his return.

Sorry I don't have more news, but I didn't want anyone thinking this was
being ignored.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

At Thu, 29 Jul 2021 18:03:55 -0700, Andres Freund <andres@anarazel.de> wrote in

And if one instead inverts the order of pgstat_report_analyze() and
pgstat_report_anl_ancestors() one gets a slightly different problem: A manual
ANALYZE of the partition root results in the partition root having a non-zero
changes_since_analyze afterwards. expand_vacuum() causes child partitions to be
added to the list of relations, which *first* causes the partition root to be
analyzed, and *then* partitions. The partitions then report their
changes_since_analyze upwards.

For the last behavior, as Andres suggested, the scan order need to be
reversed (or to be in the same order with autovacuum). Since
find_all_inheritors scans breadth-first so just reversing the result
works. The breadth-first is currently not in the contract of the
interface of the function. I suppose we can add such a contract?

Finally, I ended up with the attached.

- reverse the relation order within a tree
- reverse the order of pgstat_report_analyze and pgstat_report_analyze.

Inheritance expansion is performed per-tree basis so it works fine
even if multiple relations are given to vacuum().

I don't think the code as is is fit for v14. It looks like it was rewritten
with a new approach just before the freeze ([1]), and as far as I can tell the
concerns I quoted above weren't even discussed in the whole thread. Alvaro,
any comments?

Greetings,

Andres Freund

[1] /messages/by-id/20210408032235.GA6842@alvherre.pgsql

FYI: this bahaves as the follows.

CREATE TABLE p (a int) PARTITION BY RANGE (a);
CREATE TABLE c1 PARTITION OF p FOR VALUES FROM (0) TO (200) PARTITION BY RANGE(a);
CREATE TABLE c11 PARTITION OF c1 FOR VALUES FROM (0) TO (100);
CREATE TABLE c12 PARTITION OF c1 FOR VALUES FROM (100) TO (200);
CREATE TABLE c2 PARTITION OF p FOR VALUES FROM (200) TO (400) PARTITION BY RANGE(a);
CREATE TABLE c21 PARTITION OF c2 FOR VALUES FROM (200) TO (300);
CREATE TABLE c22 PARTITION OF c2 FOR VALUES FROM (300) TO (400);
INSERT INTO p (SELECT a FROM generate_series(0, 400 - 1) a, generate_series(0, 10) b);

INSERT INTO p (SELECT 200 FROM generate_series(0, 99));

SELECT relid, relname, n_mod_since_analyze FROM pg_stat_user_tables ORDER BY relid;
relid | relname | n_mod_since_analyze
-------+---------+---------------------
16426 | p | 0
16429 | c1 | 0
16432 | c11 | 0
16435 | c12 | 0
16438 | c2 | 0
16441 | c21 | 100
16444 | c22 | 0
16447 | sa | 0
(8 rows)

After "ANALYZE c21;"
relid | relname | n_mod_since_analyze
-------+---------+---------------------
16426 | p | 100
16429 | c1 | 0
16432 | c11 | 0
16435 | c12 | 0
16438 | c2 | 100
16441 | c21 | 0
16444 | c22 | 0
16447 | sa | 0

After "ANALYZE c2;"
relid | relname | n_mod_since_analyze
-------+---------+---------------------
16426 | p | 100
16429 | c1 | 0
16432 | c11 | 0
16435 | c12 | 0
16438 | c2 | 0
16441 | c21 | 0
16444 | c22 | 0
16447 | sa | 0

After "ANALYZE p;"
(all zero)

However, this gives a strange-looking side-effect, which affected
regression results.

=# VACUUM ANALYZE p(a, a);
ERROR: column "a" of relation "c22" appears more than once

(Prevously it complained about p.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi

On 2021-Jul-27, Andres Freund wrote:

Isn't this going to create a *lot* of redundant sampling? Especially if you
have any sort of nested partition tree. In the most absurd case a partition
with n parents will get sampled n times, solely due to changes to itself.

It seems to me that you're barking up the wrong tree on this point.
This problem you describe is not something that was caused by this
patch; ANALYZE has always worked like this. We have discussed the idea
of avoiding redundant sampling, but it's clear that it is not a simple
problem, and solving it was not in scope for this patch.

Additionally, while analyzing all child partitions for a partitioned tables
are AccessShareLock'ed at once. If a partition hierarchy has more than one
level, it actually is likely that multiple autovacuum workers will end up
processing the ancestors separately. This seems like it might contribute to
lock exhaustion issues with larger partition hierarchies?

I agree this seems a legitimate problem.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/

Hi,

On 2021-08-09 16:02:33 -0400, Alvaro Herrera wrote:

On 2021-Jul-27, Andres Freund wrote:

Isn't this going to create a *lot* of redundant sampling? Especially if you
have any sort of nested partition tree. In the most absurd case a partition
with n parents will get sampled n times, solely due to changes to itself.

It seems to me that you're barking up the wrong tree on this point.
This problem you describe is not something that was caused by this
patch; ANALYZE has always worked like this. We have discussed the idea
of avoiding redundant sampling, but it's clear that it is not a simple
problem, and solving it was not in scope for this patch.

I don't agree. There's a difference between this happening after a manual
ANALYZE on partition roots, and this continuously happening in production
workloads due to auto-analyzes...

Greetings,

Andres Freund

Hello,

On 2021-Jul-22, Andres Freund wrote:

1) Somehow it seems like a violation to do stuff like
get_partition_ancestors() in pgstat.c. It's nothing I can't live with, but
it feels a bit off. Would likely not be too hard to address, e.g. by just
putting some of pgstat_report_anl_ancestors in partition.c instead.

I understand the complain about this being a modularity violation -- the
point being that pgstat.c has no business accessing system catalogs at all.
Before this function, all pgstat_report_* functions were just assembling
a message from counters accumulated somewhere and sending the bytes to
the collector, and this new function is a deviation from that.

It seems that we could improve this by having a function (maybe in
partition.c as you propose), something like

static void
report_partition_ancestors(Oid relid)
{
ancestors = get_partition_ancestors( ... );
array = palloc(sizeof(Oid) * list_length(ancestors));
foreach(lc, ancestors)
{
array[i++] = lfirst_oid(lc);
}
pgstat_report_partition_ancestors(oid, array);
}

and then pgstat.c works with the given array without having to consult
system catalogs.

2) Why does it make sense that autovacuum sends a stats message for every
partition in the system that had any [changes] since the last autovacuum
cycle? On a database with a good number of objects / a short naptime we'll
often end up sending messages for the same set of tables from separate
workers, because they don't yet see the concurrent
tabentry->changes_since_analyze_reported.

The traffic could be large, yeah, and I agree it seems undesirable. If
collector kept a record of the list of ancestors of each table, then we
wouldn't need to do this (we would have to know if collector knows a
particular partition or not, though ... I have no ideas on that.)

3) What is the goal of the autovac_refresh_stats() after the loop doing
pgstat_report_anl_ancestors()? I think it'll be common that the stats
collector hasn't even processed the incoming messages by that point, not to
speak of actually having written out a new stats file. If it took less than
10ms (PGSTAT_RETRY_DELAY) to get to autovac_refresh_stats(),
backend_read_statsfile() will not wait for a new stats file to be written
out, and we'll just re-read the state we previously did.

It's pretty expensive to re-read the stats file in some workloads, so I'm a
bit concerned that we end up significantly increasing the amount of stats
updates/reads, without actually gaining anything reliable?

This is done once per autovacuum run and the point is precisely to let
the next block absorb the updates that were sent. In manual ANALYZE we
do it to inform future autovacuum runs.

Note that the PGSTAT_RETRY_DELAY limit is used by the autovac launcher
only, and this code is running in the worker; we do flush out the old
data. Yes, it's expensive, but we're not doing it once per table, just
once per worker run.

4) In the shared mem stats patch I went to a fair bit of trouble to try to get
rid of pgstat_vacuum_stat() (which scales extremely poorly to larger
systems). For that to work pending stats can only be "staged" while holding
a lock on a relation that prevents the relation from being concurrently
dropped (pending stats increment a refcount for the shared stats object,
which ensures that we don't loose track of the fact that a stats object has
been dropped, even when stats only get submitted later).

I'm not yet clear on how to make this work for
pgstat_report_anl_ancestors() - but I probably can find a way. But it does
feel a bit off to issue stats stuff for tables we're not sure still exist.

I assume you refer to locking the *partition*, right? You're not
talking about locking the ancestor mentioned in the message. I don't
know how does the shmem-collector work, but it shouldn't be a problem
that an ancestor goes away (ALTER TABLE parent DETACH; DROP TABLE
parent); as long as you've kept a lock on the partition, it should be
fine. Or am I misinterpreting what you mean?

I'll go and read through the thread, but my first thought is that having a
hashtable in do_autovacuum() that contains stats for partitioned tables would
be a good bit more efficient than the current approach? We already have a
hashtable for each toast table, compared to that having a hashtable for each
partitioned table doesn't seem like it'd be a problem?

With a small bit of extra work that could even avoid the need for the
additional pass through pg_class. Do the partitioned table data-gathering as
part of the "collect main tables to vacuum" pass, and then do one of

I'll have to re-read the thread to remember why did I make it a separate
pass. I think I did it that way because otherwise there was a
requirement on the pg_class scan order. (Some earlier version of the
patch did not have a separate pass and there was some problem or other.
Maybe you're right that a hash table is sufficient.)

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"We're here to devour each other alive" (Hobbes)

On 2021-Aug-09, Andres Freund wrote:

I don't agree. There's a difference between this happening after a manual
ANALYZE on partition roots, and this continuously happening in production
workloads due to auto-analyzes...

Hmm. That's not completely untrue.

I bring a radical proposal that may be sufficient to close this
particular hole. What if we made partition only affected their
top-level parents to become auto-analyzed, and not any intermediate
ancestors? Any intermediate partitioned partitions could be analyzed
manually if the user wished, and perhaps some reloption could enable
autovacuum to do it (with the caveat that it'd cause multiple sampling
of partitions). I don't yet have a clear picture on how to implement
this, but I'll explore it while waiting for opinions on the idea.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Nadie está tan esclavizado como el que se cree libre no siéndolo" (Goethe)

On 2021-Aug-10, Alvaro Herrera wrote:

I bring a radical proposal that may be sufficient to close this
particular hole. What if we made partition only affected their
top-level parents to become auto-analyzed, and not any intermediate
ancestors? Any intermediate partitioned partitions could be analyzed
manually if the user wished, and perhaps some reloption could enable
autovacuum to do it (with the caveat that it'd cause multiple sampling
of partitions). I don't yet have a clear picture on how to implement
this, but I'll explore it while waiting for opinions on the idea.

So, with this patch (a quick and dirty job) we no longer sample all
partitions twice; we no longer propagate the tuple counts to p_0.
We don't have stats on p_0 anymore, only on p and on the individual
partitions.

I didn't move the new #include to a more decent place because
1. that stuff is going to move to partition.c as a new function,
including the new include;
2. that new function also needs to read the reloptions for p_0 to allow
the user to enable stat acquisition for p_0 with "alter table p_0 set
(autovacuum_enabled=1)";
3. need to avoid reporting ancestors of a partition repeatedly, which
forestalls the performance objection about reading reloptions too
frequently.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On 2021-Aug-09, Alvaro Herrera wrote:

3) What is the goal of the autovac_refresh_stats() after the loop doing
pgstat_report_anl_ancestors()? I think it'll be common that the stats
collector hasn't even processed the incoming messages by that point, not to
speak of actually having written out a new stats file. If it took less than
10ms (PGSTAT_RETRY_DELAY) to get to autovac_refresh_stats(),
backend_read_statsfile() will not wait for a new stats file to be written
out, and we'll just re-read the state we previously did.

It's pretty expensive to re-read the stats file in some workloads, so I'm a
bit concerned that we end up significantly increasing the amount of stats
updates/reads, without actually gaining anything reliable?

This is done once per autovacuum run and the point is precisely to let
the next block absorb the updates that were sent. In manual ANALYZE we
do it to inform future autovacuum runs.

Note that the PGSTAT_RETRY_DELAY limit is used by the autovac launcher
only, and this code is running in the worker; we do flush out the old
data. Yes, it's expensive, but we're not doing it once per table, just
once per worker run.

I misunderstood what you were talking about here -- I thought it was
about the delay in autovac_refresh_stats (STATS_READ_DELAY, 1s). Now
that I look at this again I realize what your point is, and you're
right, there isn't sufficient time for the collector to absorb the
messages we sent before the next scan pg_class scan starts.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)

After thinking about the described issues for a while, my proposal is to
completely revamp the way this feature works. See below.

Now, the proposal seems awfully invasive, but it's *the* way I see to
avoid the pgstat traffic. For pg14, maybe we can live with it, and just
use the smaller patches that Horiguchi-san and I have posted, which
solve the other issues; also, Euler Taveira suggested that we could add
a reloption to turn the feature off completely for some tables (maybe
make it off by default and have a reloption to turn it on for specific
partition hierarchies), so that it doesn't cause unduly pain for people
with large partitioning hierarchies.

* PgStat_StatTabEntry gets a new "Oid reportAncestorOid" member. This is
the OID of a single partitioned ancestor, to which the changed-tuple
counts are propagated up.
Normally this is the topmost ancestor; but if the user wishes some
intermediate ancestor to receive the counts they can use
ALTER TABLE the_intermediate_ancestor SET (autovacuum_enabled=on).

* Corollary 1: for the normal case of single-level partitioning, the
parent partitioned table behaves as currently.

* Corollary 2: for multi-level partitioning with no especially
configured intermediate ancestors, only the leaf partitions and the
top-level partitioned table will be analyzed. Intermediate ancestors
are ignored by autovacuum.

* Corollary 3: for multi-level partitioning with some intermediate
ancestor(s) marked as autovacuum_enabled=on, that ancestor will
receive all the counts from all of its partitions, so it will get
analyzed itself; and it'll also forward those counts up to its
report-ancestor.

* On ALTER TABLE .. ATTACH PARTITION or CREATE TABLE PARTITION AS,
we send a message to collector with the analyze-ancestor OID.

* Backends running manual ANALYZE as well as autovacuum will examine
each table's "relispartition" flag and its pgstat table entry; if it
is a partition and doesn't have reportAncestorOid set, determine which
ancestor should analyze counts be reported to; include this OID in the
regular PgStat_MsgAnalyze. This fixes the situation after a crash or
other stats reset. Also, it's not unduly expensive to do, because
it's only in the rare case that the value sent by ATTACH was lost.

* Possible race condition in the previous step may cause multiple
backends to send the same info. Not a serious problem so we don't try
to handle it.

* When tuple change counts for a partition are received by
pgstat_recv_tabstat, they are propagated up to the indicated parent
table in addition to being saved in the table itself.
(Bonus points: when a table is attached or detached as a partition,
the live tuples count is propagated to the newly acquired parent.)

What do people think of this?

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/

Hi,

On 2021-08-11 18:33:07 -0400, Alvaro Herrera wrote:

After thinking about the described issues for a while, my proposal is to
completely revamp the way this feature works. See below.

Now, the proposal seems awfully invasive, but it's *the* way I see to
avoid the pgstat traffic. For pg14, maybe we can live with it, and just
use the smaller patches that Horiguchi-san and I have posted, which
solve the other issues; also, Euler Taveira suggested that we could add
a reloption to turn the feature off completely for some tables (maybe
make it off by default and have a reloption to turn it on for specific
partition hierarchies), so that it doesn't cause unduly pain for people
with large partitioning hierarchies.

I think we should revert the changes for 14 - to me the feature clearly isn't
mature enough to be released.

* PgStat_StatTabEntry gets a new "Oid reportAncestorOid" member. This is
the OID of a single partitioned ancestor, to which the changed-tuple
counts are propagated up.
Normally this is the topmost ancestor; but if the user wishes some
intermediate ancestor to receive the counts they can use
ALTER TABLE the_intermediate_ancestor SET (autovacuum_enabled=on).

* Corollary 1: for the normal case of single-level partitioning, the
parent partitioned table behaves as currently.

* Corollary 2: for multi-level partitioning with no especially
configured intermediate ancestors, only the leaf partitions and the
top-level partitioned table will be analyzed. Intermediate ancestors
are ignored by autovacuum.

* Corollary 3: for multi-level partitioning with some intermediate
ancestor(s) marked as autovacuum_enabled=on, that ancestor will
receive all the counts from all of its partitions, so it will get
analyzed itself; and it'll also forward those counts up to its
report-ancestor.

This seems awfully confusing to me.

One fundamental issue here is that we separately build stats for partitioned
tables and partitions. Can we instead tackle this by reusing the stats for
partitions for the inheritance stats? I think it's a bit easier to do that
for partitioned tables than for old school inheritance roots, because there's
no other rows in partitioned tables.

Greetings,

Andres Freund

Here is a proposal for 14. This patch has four main changes:

* The mod counts are only propagated to the topmost parent, not to each ancestor. This means that we'll only analyze the topmost partitioned table and not each intermediate partitioned table; seems a good compromise to avoid sampling all partitions multiple times per round.

* One pgstat message is sent containing many partition/parent pairs, not just one. This reduces the number of messages sent. 123 partitions fit in one message (messages are 1000 bytes). This is done once per autovacuum worker run, so it shouldn't be too bad.

* There's a sleep between sending the message and re-reading stats. It would be great to have a mechanism by which pgstat collector says "I've received and processed up to this point", but we don't have that; what we can do is sleep PGSTAT_STAT_INTERVAL and then reread the file, so we're certain that the file we read is at least as new as that time. This is far longer than it takes to process the messages. Note that if the messages do take longer than that to be processed by the collector, it's not a big loss anyway; those tables will be processed by the next autovacuum run.

* I changed vacuum_expand_rel to put the main-rel OID at the end. (This is a variation of Horiguchi-san proposed patch; instead of making the complete list be in the opposite order, it's just that one OID that appears at the other end). This has the same effect as his patch: any error reports thrown by vacuum/analyze mention the first partition rather than the main table. This part is in 0002 and I'm not totally convinced it's a sane idea.

Minor changes:
* I reduced autovacuum from three passes over pg_class to two passes, per your observation that we can acquire toast association together with processing partitions, and then use that in the second pass to collect everything.

* I moved the catalog-accessing code to partition.c, so we don't need to have pgstat.c doing it.

Some doc changes are pending, and some more commentary in parts of the code, but I think this is much more sensible. I do lament the lack of a syscache for pg_inherits.

On 2021-Aug-13, Álvaro Herrera wrote:

Some doc changes are pending, and some more commentary in parts of the
code, but I think this is much more sensible. I do lament the lack of
a syscache for pg_inherits.

Thinking about this again, this one here is the killer problem, I think;
this behaves pretty horribly if you have more than one partition level,
because it'll have to do one indexscan *per level per partition*. (For
example, five partitions two levels down mean ten index scans). There's
no cache for this, and no way to disable it. So for situations with a
lot of partitions, it could be troublesome. Granted, it only needs to
be done for partitions with DML changes since the previous autovacuum
worker run in the affected database, but still it could be significant.

Now we could perhaps have a hash table in partition_analyze_report_ancestors()
to avoid the need for repeated indexscans for partitions of the same
hierarchy (an open-coded cache to take the place of the missing
pg_inherits syscache); and perhaps even use a single seqscan of
pg_inherits to capture the whole story first and then filter down to the
partitions that we were asked to process ... (so are we building a
mini-optimizer to determine which strategy to use in each case?).

That all sounds too much to be doing in the beta.

So I'm leaning towards the idea that we need to revert the patch and
start over for pg15.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I don't follow the connection to catversion?

I agree that we probably don't want to change PgStat_StatTabEntry in
v14 at this point. But it'd be a good idea to attach a comment to
the entry saying it's unused but left there for ABI reasons.

regards, tom lane

On 2021-Aug-16, Tom Lane wrote:

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I don't follow the connection to catversion?

Sorry, I misspoke -- I mean PGSTAT_FORMAT_FILE_ID. I shouldn't just
change it, since if I do then the file is reported as corrupted and all
counters are lost. So in the posted patch I did as you suggest:

I agree that we probably don't want to change PgStat_StatTabEntry in
v14 at this point. But it'd be a good idea to attach a comment to
the entry saying it's unused but left there for ABI reasons.

It's only in branch master that I'd change the pgstat format version and
remove the field. This is what I meant with the patch being for v14 and
a tweak needed for this in master.

A catversion bump would be required to change the definition of
pg_stat_user_tables, which the patch being reverted originally changed
to include relkind 'p'. A straight revert would remove that, but in my
reversal patch I chose to keep it in place.

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)

Another possible problem is that before the revert, we accept
ALTER TABLE some_partitioned_table SET (autovacuum_enabled=on/off);
(also autovacuum_analyze_scale_factor and autovacuum_analyze_threshold)
but after the revert this is will throw a syntax error. What do people
think we should do about that?

1. Do nothing. If somebody finds in that situation, they can use
ALTER TABLE .. RESET ...
to remove the settings.

2. Silently accept the option and do nothing.
3. Accept the option and throw a warning that it's a no-op.
4. Something else

Opinions?

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

It upsets me to have reverted it, but after spending so much time trying
to correct the problems, I believe it just wasn't salvageable within the
beta-period code freeze constraints. I described the issues I ran into
in earlier messages; I think a good starting point to re-develop this is
to revert the reversal commit, then apply my patch at
/messages/by-id/0794d7ca-5183-486b-9c5e-6d434867cecd@www.fastmail.com
then do something about the remaining problems that were complained
about. (Maybe: add an "ancestor OID" member to PgStat_StatTabEntry so
that the collector knows to propagate counts from children to ancestors
when the upd/ins/del counts are received. However, consider developing
it as follow-up to Horiguchi-san's shmem pgstat rather than current
pgstat implementation.)

Thanks

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/

On Mon, Aug 16, 2021 at 05:42:48PM -0400, Álvaro Herrera wrote:

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

| I retained the addition of relkind 'p' to tables included by
| pg_stat_user_tables, because reverting that would require a catversion
| bump.

Right now, on v15dev, it shows 0, which is misleading.
Shouldn't it be null ?

analyze_count | 0

Note that having analyze_count and last_analyze would be an an independently
useful change. Since parent tables aren't analyzed automatically, I have a
script to periodically process them if they weren't processed recently. Right
now, for partitioned tables, the best I could find is to check its partitions:
| MIN(last_analyzed) FROM pg_stat_all_tables psat JOIN pg_inherits i ON psat.relid=i.inhrelid

In 20200418050815.GE26953@telsasoft.com I wrote:
|This patch includes partitioned tables in pg_stat_*_tables, which is great; I
|complained awhile ago that they were missing [0]. It might be useful if that
|part was split out into a separate 0001 patch (?).
| [0] /messages/by-id/20180601221428.GU5164@telsasoft.com

--
Justin

Hi,

On 2021-08-16 17:42:48 -0400, Álvaro Herrera wrote:

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

It upsets me to have reverted it, but after spending so much time trying
to correct the problems, I believe it just wasn't salvageable within the
beta-period code freeze constraints.

:(

I described the issues I ran into
in earlier messages; I think a good starting point to re-develop this is
to revert the reversal commit, then apply my patch at
/messages/by-id/0794d7ca-5183-486b-9c5e-6d434867cecd@www.fastmail.com
then do something about the remaining problems that were complained
about. (Maybe: add an "ancestor OID" member to PgStat_StatTabEntry so
that the collector knows to propagate counts from children to ancestors
when the upd/ins/del counts are received.

My suspicion is that it'd be a lot easier to implement this efficiently if
there were no propagation done outside of actually analyzing tables. I.e. have
do_autovacuum() build a hashtable of (parent_table_id, count) and use that to
make the analyze decisions. And then only propagate up the costs to parents of
tables when a child is analyzed (and thus looses its changes_since_analyze)
value. Then we can use hashtable_value + changes_since_analyze for
partitioning decisions of partitioned tables.

I've prototyped this, and it does seem to make do_autovacuum() cheaper. I've
attached that prototype, but note it's in a rough state.

However, unless we change the way inheritance parents are stored, it still
requires repetitive get_partition_ancestors() (or get_partition_parent())
calls in do_autovacuum(), which I think is problematic due to the index scans
you pointed out as well. The obvious way to address that would be to store
parent oids in pg_class - I suspect duplicating parents in pg_class is the
best way out, but pretty it is not.

However, consider developing it as follow-up to Horiguchi-san's shmem
pgstat rather than current pgstat implementation.)

+1

It might be worth to first tackle reusing samples from a relation's children
when building inheritance stats. Either by storing the samples somewhere (not
cheap) and reusing them, or by at least updating a partition's stats when
analyzing the parent.

Greetings,

Andres Freund

Hi,

On 2021-08-16 13:13:55 -0400, Álvaro Herrera wrote:

Another possible problem is that before the revert, we accept
ALTER TABLE some_partitioned_table SET (autovacuum_enabled=on/off);
(also autovacuum_analyze_scale_factor and autovacuum_analyze_threshold)
but after the revert this is will throw a syntax error. What do people
think we should do about that?

1. Do nothing. If somebody finds in that situation, they can use
ALTER TABLE .. RESET ...
to remove the settings.

2. Silently accept the option and do nothing.
3. Accept the option and throw a warning that it's a no-op.
4. Something else

1) seems OK to me.

Greetings,

Andres Freund

On Mon, Aug 16, 2021 at 05:28:10PM -0500, Justin Pryzby wrote:

On Mon, Aug 16, 2021 at 05:42:48PM -0400, Álvaro Herrera wrote:

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

| I retained the addition of relkind 'p' to tables included by
| pg_stat_user_tables, because reverting that would require a catversion
| bump.

Right now, on v15dev, it shows 0, which is misleading.
Shouldn't it be null ?

analyze_count | 0

Note that having analyze_count and last_analyze would be an an independently
useful change. Since parent tables aren't analyzed automatically, I have a
script to periodically process them if they weren't processed recently. Right
now, for partitioned tables, the best I could find is to check its partitions:
| MIN(last_analyzed) FROM pg_stat_all_tables psat JOIN pg_inherits i ON psat.relid=i.inhrelid

In 20200418050815.GE26953@telsasoft.com I wrote:
|This patch includes partitioned tables in pg_stat_*_tables, which is great; I
|complained awhile ago that they were missing [0]. It might be useful if that
|part was split out into a separate 0001 patch (?).
| [0] /messages/by-id/20180601221428.GU5164@telsasoft.com

I suggest the attached (which partially reverts the revert), to allow showing
correct data for analyze_count and last_analyzed.

Arguably these should be reported as null in v14 for partitioned tables, since
they're not "known to be zero", but rather "currently unpopulated".

n_mod_since_analyze | 0
n_ins_since_vacuum | 0

Justin

On Tue, Aug 17, 2021 at 06:30:18AM -0500, Justin Pryzby wrote:

On Mon, Aug 16, 2021 at 05:28:10PM -0500, Justin Pryzby wrote:

On Mon, Aug 16, 2021 at 05:42:48PM -0400, Álvaro Herrera wrote:

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

| I retained the addition of relkind 'p' to tables included by
| pg_stat_user_tables, because reverting that would require a catversion
| bump.

Right now, on v15dev, it shows 0, which is misleading.
Shouldn't it be null ?

analyze_count | 0

Note that having analyze_count and last_analyze would be an an independently
useful change. Since parent tables aren't analyzed automatically, I have a
script to periodically process them if they weren't processed recently. Right
now, for partitioned tables, the best I could find is to check its partitions:
| MIN(last_analyzed) FROM pg_stat_all_tables psat JOIN pg_inherits i ON psat.relid=i.inhrelid

In 20200418050815.GE26953@telsasoft.com I wrote:
|This patch includes partitioned tables in pg_stat_*_tables, which is great; I
|complained awhile ago that they were missing [0]. It might be useful if that
|part was split out into a separate 0001 patch (?).
| [0] /messages/by-id/20180601221428.GU5164@telsasoft.com

I suggest the attached (which partially reverts the revert), to allow showing
correct data for analyze_count and last_analyzed.

Álvaro, would you comment on this ?

To me this could be an open item, but someone else should make that
determination.

--
Justin

On Fri, Aug 20, 2021 at 07:55:13AM -0500, Justin Pryzby wrote:

On Tue, Aug 17, 2021 at 06:30:18AM -0500, Justin Pryzby wrote:

On Mon, Aug 16, 2021 at 05:28:10PM -0500, Justin Pryzby wrote:

On Mon, Aug 16, 2021 at 05:42:48PM -0400, Álvaro Herrera wrote:

On 2021-Aug-16, Álvaro Herrera wrote:

Here's the reversal patch for the 14 branch. (It applies cleanly to
master, but the unused member of PgStat_StatTabEntry needs to be
removed and catversion bumped).

I have pushed this to both branches. (I did not remove the item from
the release notes in the 14 branch.)

| I retained the addition of relkind 'p' to tables included by
| pg_stat_user_tables, because reverting that would require a catversion
| bump.

Right now, on v15dev, it shows 0, which is misleading.
Shouldn't it be null ?

analyze_count | 0

Note that having analyze_count and last_analyze would be an an independently
useful change. Since parent tables aren't analyzed automatically, I have a
script to periodically process them if they weren't processed recently. Right
now, for partitioned tables, the best I could find is to check its partitions:
| MIN(last_analyzed) FROM pg_stat_all_tables psat JOIN pg_inherits i ON psat.relid=i.inhrelid

In 20200418050815.GE26953@telsasoft.com I wrote:
|This patch includes partitioned tables in pg_stat_*_tables, which is great; I
|complained awhile ago that they were missing [0]. It might be useful if that
|part was split out into a separate 0001 patch (?).
| [0] /messages/by-id/20180601221428.GU5164@telsasoft.com

I suggest the attached (which partially reverts the revert), to allow showing
correct data for analyze_count and last_analyzed.

Álvaro, would you comment on this ?

To me this could be an open item, but someone else should make that
determination.

I added an opened item until this is discussed.
| pg_stats includes partitioned tables, but always shows analyze_count=0
| Owner: Alvaro Herrera

Possible solutions, in decreasing order of my own preference:

- partially revert the revert, as proposed, to have "analyze_count" and
"last_analyzed" work properly for partitioned tables. This doesn't suffer
from any of the problems that led to the revert, does it ?

- Update the .c code to return analyze_count=NULL for partitioned tables.

- Update the catalog definition to exclude partitioned tables, again.
Requires a catalog bumped.

- Document that analyze_count=NULL for partitioned tables. It seems to just
document a misbehavior.

--
Justin

On 2021-Aug-17, Justin Pryzby wrote:

I suggest the attached (which partially reverts the revert), to allow showing
correct data for analyze_count and last_analyzed.

Yeah, that makes sense and my keeping of the pg_stat_all_tables entries
seems pretty useless without this change. I have pushed a slightly
modified version of this to 14 and master.

Arguably these should be reported as null in v14 for partitioned tables, since
they're not "known to be zero", but rather "currently unpopulated".

n_mod_since_analyze | 0
n_ins_since_vacuum | 0

I don't disagree, but it's not easy to implement this at present. I
think almost all counters should be nulls for partitioned tables. For
some of them one could make a case that it'd be more convenient to
propagate numbers up from partitions.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/