Autovacuum on partitioned table

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Autovacuum on partitioned table

yuzuko
Hello,

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.
* If the partitioned table has foreign partitions, ignore them.

When the parent has children don't need vacuum/analyze or foreign
partitions, parent's stats are updated scanning the current data of all
children, so old stats and new are mixed within the partition tree.
Is that suitable?  Any thoughts?

[1] https://www.postgresql.org/message-id/CAM-w4HMQKC8hw7nB9TW3OV%2BhkB5OUcPtvr_U_EiSOjByoa-e4Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CA%2BHiwqEeZQ-H2OVbHZ%3Dn2RNNPF84Hygi1HC-MDwC-VnBjpA1%3DQ%40mail.gmail.com

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

v1_autovacuum_on_partitioned_table.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Laurenz Albe
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



Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
Hi Laurenz,

Thanks for the comments.

On Mon, Dec 2, 2019 at 6:19 PM Laurenz Albe <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
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

v2_autovacuum_on_partitioned_table.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Masahiko Sawada-2
On Fri, 27 Dec 2019 at 12:37, yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
Hello,

On Fri, Dec 27, 2019 at 2:02 PM Masahiko Sawada
<[hidden email]> wrote:

> On Fri, 27 Dec 2019 at 12:37, yuzuko <[hidden email]> 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:

https://www.postgresql.org/message-id/4823.1262132964%40sss.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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Masahiko Sawada-2
On Tue, 28 Jan 2020 at 17:52, Amit Langote <[hidden email]> wrote:

>
> Hello,
>
> On Fri, Dec 27, 2019 at 2:02 PM Masahiko Sawada
> <[hidden email]> wrote:
> > On Fri, 27 Dec 2019 at 12:37, yuzuko <[hidden email]> 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:
>
> https://www.postgresql.org/message-id/4823.1262132964%40sss.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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
On Wed, Jan 29, 2020 at 11:29 AM yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Masahiko Sawada-2
In reply to this post by Amit Langote
On Wed, 29 Jan 2020 at 17:56, Amit Langote <[hidden email]> wrote:

>
> On Wed, Jan 29, 2020 at 11:29 AM yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
On Sun, Feb 2, 2020 at 12:53 PM Masahiko Sawada
<[hidden email]> wrote:

> On Wed, 29 Jan 2020 at 17:56, Amit Langote <[hidden email]> wrote:
> > On Wed, Jan 29, 2020 at 11:29 AM yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
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

v3_autovacuum_on_partitioned_table.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
Hosoya-san,

On Thu, Feb 20, 2020 at 3:34 PM yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
On Thu, Feb 20, 2020 at 4:50 PM Amit Langote <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
On Thu, Feb 20, 2020 at 5:32 PM Amit Langote <[hidden email]> wrote:

> On Thu, Feb 20, 2020 at 4:50 PM Amit Langote <[hidden email]> 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

v3_amit_delta.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
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

v4_autovacuum_on_partitioned_table.patch (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Masahiko Sawada-2
On Fri, 21 Feb 2020 at 15:14, yuzuko <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

Amit Langote
On Fri, Feb 21, 2020 at 4:47 PM Masahiko Sawada
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Autovacuum on partitioned table

yuzuko
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

v5_autovacuum_on_partitioned_table.patch (22K) Download Attachment
123