Re: Autovacuum on partitioned table (autoanalyze)
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/
Re: Autovacuum on partitioned table (autoanalyze)
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.ge26...@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] > > > https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.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
Re: Autovacuum on partitioned table (autoanalyze)
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.ge26...@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] > > https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.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
Re: Autovacuum on partitioned table (autoanalyze)
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.ge26...@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]
> https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.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
>From 0d0e149727d89115803b4528e15f5b3c04bd816b Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Mon, 16 Aug 2021 22:55:06 -0500
Subject: [PATCH] Report last_analyze and analyze_count of partitioned tables..
In v14, partitioned tables are included, but these fields are being reported as
zero, which is misleading.
---
src/backend/commands/analyze.c | 36 ++---
src/backend/postmaster/pgstat.c | 27 +
2 files changed, 43 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 8d7b38d170..0050df08f6 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -626,8 +626,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
PROGRESS_ANALYZE_PHASE_FINALIZE_ANALYZE);
/*
- * Update pages/tuples stats in pg_class, and report ANALYZE to the stats
- * collector ... but not if we're doing inherited stats.
+ * Update pages/tuples stats in pg_class ... but not if we're doing
+ * inherited stats.
*
* We assume that VACUUM hasn't set pg_class.reltuples already, even
* during a VACUUM ANALYZE. Although VACUUM often updates pg_class,
@@ -668,20 +668,32 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
InvalidMultiXactId,
in_outer_xact);
}
-
+ }
+ else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
/*
- * Now report ANALYZE to the stats collector.
- *
- * We deliberately don't report to the stats collector when doing
- * inherited stats, 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.
+ * Partitioned tables don't have storage, so we don't set any fields
+ * in their pg_class entries except for reltuples, which is necessary
+ * for auto-analyze to work properly, and relhasindex.
*/
+ vac_update_relstats(onerel, -1, totalrows,
+ 0, hasindex, InvalidTransactionId,
+ InvalidMultiXactId,
+ in_outer_xact);
+ }
+
+ /*
+ * 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 isn't part of VACUUM ANALYZE, let index AMs do cleanup.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a3c35bdf60..2a9673154b 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1632,21 +1632,31 @@ pgstat_report_analyze(Relation rel,
* be double-counted after commit. (This approach also ensures that the
* collector
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
> https://postgr.es/m/0794d7ca-5183-486b-9c5e-6d434867c...@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
commit ec796bd8ee2970e2eae3b3839e1bb96696393dc7
Author: Andres Freund
Date: 2021-07-30 17:20:21 -0700
tmp
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 0c9591415e4..df021215281 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -320,12 +320,12 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
PgStat_Counter startwritetime = 0;
if (inh)
- ereport(elevel,
+ ereport(LOG,
(errmsg("analyzing \"%s.%s\" inheritance tree",
get_namespace_name(RelationGetNamespace(onerel)),
RelationGetRelationName(onerel;
else
- ereport(elevel,
+ ereport(LOG,
(errmsg("analyzing \"%s.%s\"",
get_namespace_name(RelationGetNamespace(onerel)),
RelationGetRelationName(onerel;
@@ -682,6 +682,18 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
in_outer_xact);
}
+ /*
+ * Let the collector know about the ancestor tables of this partition.
+ */
+ if (!inh &&
+ (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));
+ }
+
/*
* Now report ANALYZE to the stats collector. For regular tables, we do
* it only if not doing inherited stats. For partitioned tables, we only
@@ -695,22 +707,6 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
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));
- }
-
/*
* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup.
*
@@ -1183,6 +1179,8 @@ acquire_sample_rows(Relation onerel, int elevel,
BlockSamplerData prefetch_bs;
#endif
+
Re: Autovacuum on partitioned table (autoanalyze)
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.ge26...@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] https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.com -- Justin
Re: Autovacuum on partitioned table (autoanalyze)
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 https://postgr.es/m/0794d7ca-5183-486b-9c5e-6d434867c...@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/
Re: Autovacuum on partitioned table (autoanalyze)
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")
Re: Autovacuum on partitioned table (autoanalyze)
On 2021-Aug-16, Tom Lane wrote: > =?utf-8?Q?=C3=81lvaro?= Herrera 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)
Re: Autovacuum on partitioned table (autoanalyze)
=?utf-8?Q?=C3=81lvaro?= Herrera 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
Re: Autovacuum on partitioned table (autoanalyze)
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)
>From cad5b710a531ec6eefc8856177c68d594c60ac8c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 16 Aug 2021 10:56:07 -0400
Subject: [PATCH] Revert analyze support for partitioned tables
This reverts the following commits:
1b5617eb844cd2470a334c1d2eec66cf9b39c41a Describe (auto-)analyze behavior for partitioned tables
0e69f705cc1a3df273b38c9883fb5765991e04fe Set pg_class.reltuples for partitioned tables
41badeaba8beee7648ebe7923a41c04f1f3cb302 Document ANALYZE storage parameters for partitioned tables
0827e8af70f4653ba17ed773f123a60eadd9f9c9 autovacuum: handle analyze for partitioned tables
There are efficiency issues in this code when handling databases with
large numbers of partitions, and it doesn't look like there isn't any
trivial way to handle those. There are some other issues as well. It's
now too late in the cycle for nontrivial fixes, so we'll have to let
Postgres 14 users continue to manually deal with ANALYZE their
partitioned tables, and hopefully we can fix the issues for Postgres 15.
I chose to keep [most of] be280cdad298 ("Don't reset relhasindex for
partitioned tables on ANALYZE") because while we added due to
0827e8af70f4, it is a reasonable change in its own right (since it
affects manual analyze as well as autovacuum-induced analyze) and
there's no reason to revert it.
I retained relkind 'p' in the definition of view pg_stat_user_tables,
because that change would require a catversion bump.
Also, in pg14 only, I keep a struct member that was added in
PgStat_TabStatEntry to avoid breaking compatibility with existing stat
files, because changing that would require a catversion bump.
Backpatch to 14.
Discussion: https://postgr.es/m/20210722205458.f2bug3z6qzxzp...@alap3.anarazel.de
---
doc/src/sgml/maintenance.sgml | 6 --
doc/src/sgml/perform.sgml | 3 +-
doc/src/sgml/ref/analyze.sgml | 40 +++--
doc/src/sgml/ref/create_table.sgml | 8 +-
doc/src/sgml/ref/pg_restore.sgml | 6 +-
src/backend/access/common/reloptions.c | 15 ++--
src/backend/commands/analyze.c | 52 +++-
src/backend/commands/tablecmds.c | 47 +--
src/backend/postmaster/autovacuum.c| 66 +++
src/backend/postmaster/pgstat.c| 108 +++--
src/include/pgstat.h | 26 +-
11 files changed, 57 insertions(+), 320 deletions(-)
diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml
index 998a48fc25..36f975b1e5 100644
--- a/doc/src/sgml/maintenance.sgml
+++ b/doc/src/sgml/maintenance.sgml
@@ -817,12 +817,6 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu
is compared to the total number of tuples inserted, updated, or deleted
since the last ANALYZE.
-For partitioned tables, inserts, updates and deletes on partitions
-are counted towards this threshold; however, DDL
-operations such as ATTACH, DETACH
-and DROP are not, so running a manual
-ANALYZE is recommended if the partition added or
-removed contains a statistically significant volume of data.
diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml
index ddd6c3ff3e..89ff58338e 100644
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -1767,8 +1767,7 @@ SELECT * FROM x, y, a, b, c WHERE something AND somethingelse;
Whenever you have significantly altered the distribution of data
within a table, running ANALYZE is strongly recommended. This
-includes bulk loading large amounts of data into the table as well as
-attaching, detaching or dropping partitions. Running
+includes bulk loading large amounts of data into the table. Running
ANALYZE (or VACUUM ANALYZE)
ensures that the planner has up-to-date statistics about the
table. With no statistics or obsolete statistics, the planner might
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 176c7cb225..c8fcebc161 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -250,38 +250,20 @@ ANALYZE [ VERBOSE ] [ table_and_columns
- If the table being analyzed is partitioned, ANALYZE
- will gather statistics by sampling blocks randomly from its partitions;
- in addition, it will recurse into each partition and update its statistics.
- (However, in multi-level partitioning scenarios, each leaf partition
- will only be analyzed once.)
- By contrast, if the table being analyzed has inheritance children,
- ANALYZE will gather statistics for it twice:
- once on the rows of the parent table only, and a
Re: Autovacuum on partitioned table (autoanalyze)
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)
Re: Autovacuum on partitioned table (autoanalyze)
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.From 3e904de5f15cfc69692ad2aea64c0034445d957e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 10 Aug 2021 13:05:59 -0400
Subject: [PATCH 1/2] Propagate counts up only to topmost ancestor
Ignore intermediate partitions, to avoid redundant sampling of
partitions. If needed, those intermediate partitions can be analyzed
manually.
---
src/backend/catalog/partition.c | 53 +++
src/backend/commands/analyze.c | 3 +-
src/backend/postmaster/autovacuum.c | 222 ++--
src/backend/postmaster/pgstat.c | 60
src/include/catalog/partition.h | 1 +
src/include/pgstat.h| 21 ++-
6 files changed, 211 insertions(+), 149 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 790f4ccb92..017d5ba5a2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -26,6 +26,7 @@
#include "nodes/makefuncs.h"
#include "optimizer/optimizer.h"
#include "partitioning/partbounds.h"
+#include "pgstat.h"
#include "rewrite/rewriteManip.h"
#include "utils/fmgroids.h"
#include "utils/partcache.h"
@@ -166,6 +167,58 @@ get_partition_ancestors_worker(Relation inhRel, Oid relid, List **ancestors)
get_partition_ancestors_worker(inhRel, parentOid, ancestors);
}
+/*
+ * Inform pgstats collector about the topmost ancestor of
+ * each of the given partitions.
+ */
+void
+partition_analyze_report_ancestors(List *partitions)
+{
+ List *report_parts = NIL;
+ List *ancestors = NIL;
+ Relation inhRel;
+ ListCell *lc;
+
+ inhRel = table_open(InheritsRelationId, AccessShareLock);
+
+ /*
+ * Search pg_inherits for the topmost ancestor of each given partition,
+ * and if found, store both their OIDs in lists.
+ *
+ * By the end of this loop, partitions and ancestors are lists to be
+ * read in parallel, where the i'th element of ancestors is the topmost
+ * ancestor of the i'th element of partitions.
+ */
+ foreach(lc, partitions)
+ {
+ Oid partition_id = lfirst_oid(lc);
+ Oid cur_relid;
+
+ cur_relid = partition_id;
+ for (;;)
+ {
+ bool detach_pending;
+ Oid parent_relid;
+
+ parent_relid = get_partition_parent_worker(inhRel, cur_relid,
+ _pending);
+ if ((!OidIsValid(parent_relid) || detach_pending) &&
+cur_relid != partition_id)
+ {
+report_parts = lappend_oid(report_parts, partition_id);
+ancestors = lappend_oid(ancestors, cur_relid);
+break;
+ }
+
+ cur_relid = parent_relid;
+ }
+ }
+
+ table_close(inhRel, AccessShareLock);
+
+ pgstat_report_anl_ancestors(report_parts, ancestors);
+}
+
/*
* index_get_partition
* Return the OID of index of the given partition that is a child
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 0099a04bbe..c930e3e3cd 100644
---
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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/
Re: Autovacuum on partitioned table (autoanalyze)
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)
Re: Autovacuum on partitioned table (autoanalyze)
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/
>From 064bc88bf94b6b4e1bfc16f0639e1500b17b9bf5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 10 Aug 2021 13:05:59 -0400
Subject: [PATCH] Propagate counts up only to topmost ancestor
Ignore intermediate partitions, to avoid redundant sampling of
partitions. If needed, those intermediate partitions can be analyzed
manually.
---
src/backend/postmaster/pgstat.c | 21 -
src/include/pgstat.h| 9 +++--
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 1b54ef74eb..a003966cc8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1684,6 +1684,7 @@ pgstat_report_analyze(Relation rel,
* counts from the partition to its ancestors. This is necessary so that
* other processes can decide whether to analyze the partitioned tables.
*/
+#include "utils/lsyscache.h"
void
pgstat_report_anl_ancestors(Oid relid)
{
@@ -1700,19 +1701,25 @@ pgstat_report_anl_ancestors(Oid relid)
foreach(lc, ancestors)
{
Oid ancestor = lfirst_oid(lc);
+ bool ispartition;
- msg.m_ancestors[msg.m_nancestors] = ancestor;
- if (++msg.m_nancestors >= PGSTAT_NUM_ANCESTORENTRIES)
+ ispartition = get_rel_relispartition(ancestor);
+
+ msg.m_ancestors[msg.m_nancestors].m_ancestor_id = ancestor;
+ msg.m_ancestors[msg.m_nancestors].m_propagate_up = !ispartition;
+ msg.m_nancestors++;
+
+ if (msg.m_nancestors >= PGSTAT_NUM_ANCESTORENTRIES)
{
pgstat_send(, offsetof(PgStat_MsgAnlAncestors, m_ancestors[0]) +
- msg.m_nancestors * sizeof(Oid));
+ msg.m_nancestors * sizeof(PgStat_AnlAncestor));
msg.m_nancestors = 0;
}
}
if (msg.m_nancestors > 0)
pgstat_send(, offsetof(PgStat_MsgAnlAncestors, m_ancestors[0]) +
- msg.m_nancestors * sizeof(Oid));
+ msg.m_nancestors * sizeof(PgStat_AnlAncestor));
list_free(ancestors);
}
@@ -5415,9 +5422,13 @@ pgstat_recv_anl_ancestors(PgStat_MsgAnlAncestors *msg, int len)
for (int i = 0; i < msg->m_nancestors; i++)
{
- Oid ancestor_relid = msg->m_ancestors[i];
+ Oid ancestor_relid;
PgStat_StatTabEntry *ancestor;
+ if (!msg->m_ancestors[i].m_propagate_up)
+ continue;
+
+ ancestor_relid = msg->m_ancestors[i].m_ancestor_id;
ancestor = pgstat_get_tab_entry(dbentry, ancestor_relid, true);
ancestor->changes_since_analyze +=
tabentry->changes_since_analyze - tabentry->changes_since_analyze_reported;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 2068a68a5f..46ef88e73b 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -438,9 +438,14 @@ typedef struct PgStat_MsgAnalyze
*analyze counters
* --
*/
+typedef struct PgStat_AnlAncestor
+{
+ Oid m_ancestor_id;
+ bool m_propagate_up;
+} PgStat_AnlAncestor;
#define PGSTAT_NUM_ANCESTORENTRIES\
((PGSTAT_MSG_PAYLOAD - sizeof(Oid) - sizeof(Oid) - sizeof(int)) \
- / sizeof(Oid))
+ / sizeof(PgStat_AnlAncestor))
typedef struct PgStat_MsgAnlAncestors
{
@@ -448,7 +453,7 @@ typedef struct PgStat_MsgAnlAncestors
Oid m_databaseid;
Oid m_tableoid;
int m_nancestors;
- Oid m_ancestors[PGSTAT_NUM_ANCESTORENTRIES];
+ PgStat_AnlAncestor m_ancestors[PGSTAT_NUM_ANCESTORENTRIES];
} PgStat_MsgAnlAncestors;
/* --
--
2.20.1
Re: Autovacuum on partitioned table (autoanalyze)
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)
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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/
Re: Autovacuum on partitioned table (autoanalyze)
At Thu, 29 Jul 2021 18:03:55 -0700, Andres Freund 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] > https://www.postgresql.org/message-id/20210408032235.GA6842%40alvherre.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 >From 16f7602f1b7755f288c508f1e57e0eae3c305813 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Wed, 4 Aug 2021 13:40:59 +0900 Subject: [PATCH] Fix changes_since_analyze's motion on manual analyze on partitioned tables The analyze-stats machinery assumed bottom-to-top-ordered relation scans but actually it was in the opposite order in manual ANALYZE. Addition to that the current code tries to propagate changes_since_analyze to parents after stats reporting which resets the number to propagate. As the result, when doing manual ANALYZE on a partition, changes_since_analyze vanishes instead of being propagated to parents. On the other hand when doing that on a partitioned tables, the leaf relations end up with having bogus stats values. To fix this, reverse the order relations on running manual ANALYZE and move stats-reporting after stats-propagation. --- src/backend/catalog/pg_inherits.c| 3 ++- src/backend/commands/analyze.c | 26 +- src/backend/commands/vacuum.c| 25 +++-- src/test/regress/expected/vacuum.out | 18 +- 4 files changed, 39 insertions(+), 33 deletions(-)
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
Justin Pryzby 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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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 Herrera39°49'30"S 73°17'W >From 37a829ec7b9c46acbbdb02f231288e39d22fcd04 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Thu, 8 Apr 2021 17:53:22 -0400 Subject: [PATCH] document reloptions for partitioned tables --- doc/src/sgml/ref/create_table.sgml | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 44e50620fd..3cf355cc8d 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -1369,8 +1369,8 @@ WITH ( MODULUS numeric_literal, REM If a table parameter value is set and the equivalent toast. parameter is not, the TOAST table will use the table's parameter value. -Specifying these parameters for partitioned tables is not supported, -but you may specify them for individual leaf partitions. +Most parameters are not supported on partitioned tables, with exceptions +noted below; you may specify them for individual leaf partitions. @@ -1452,6 +1452,7 @@ WITH ( MODULUS numeric_literal, REM If true, the autovacuum daemon will perform automatic VACUUM and/or ANALYZE operations on this table following the rules discussed in . + This parameter is supported on partitioned tables. If false, this table will not be autovacuumed, except to prevent transaction ID wraparound. See for more about wraparound prevention. @@ -1576,6 +1577,7 @@ WITH ( MODULUS numeric_literal, REM Per-table value for parameter. + This parameter is supported on partitioned tables. @@ -1591,6 +1593,7 @@ WITH ( MODULUS numeric_literal, REM Per-table value for parameter. + This parameter is supported on partitioned tables. -- 2.20.1
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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 Herrera39°49'30"S 73°17'W
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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)
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
commit 5ddb7c00e5f1d63eb251d334afce738919a772c0
Author: Alvaro Herrera
AuthorDate: Wed Apr 7 23:52:33 2021 -0400
CommitDate: Wed Apr 7 23:52:33 2021 -0400
set pg_class.reltuples for partitioned tables
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 0789117bb8..36e35722bc 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -656,6 +656,17 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
in_outer_xact);
}
}
+ else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /*
+ * Partitioned tables don't have storage, so we don't set any of these
+ * value in their pg_class entries. However, reltuples is necessary
+ * in order for auto-analyze to work properly, so update that.
+ */
+ vac_update_relstats(onerel, 0, totalrows,
+ 0, false, InvalidTransactionId, InvalidMultiXactId,
+ in_outer_xact);
+ }
/*
* Now report ANALYZE to the stats collector. For regular tables, we do
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 5517836be6..48c1bf048f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3208,44 +3208,7 @@ relation_needs_vacanalyze(Oid relid,
*/
if (PointerIsValid(tabentry) && AutoVacuumingActive())
{
- if (classForm->relkind != RELKIND_PARTITIONED_TABLE)
- {
- reltuples = classForm->reltuples;
- }
- else
- {
- /*
- * If the relation is a partitioned table, we must add up
- * children's reltuples.
- */
- 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;
-
-childtuple = SearchSysCache1(RELOID, ObjectIdGetDatum(childOID));
-childclass = (Form_pg_class) GETSTRUCT(childtuple);
-
-/* Skip a partitioned table and foreign partitions */
-if (RELKIND_HAS_STORAGE(childclass->relkind))
-{
- /* Sum up the child's reltuples for its parent table */
- reltuples += childclass->reltuples;
-}
-ReleaseSysCache(childtuple);
- }
-
- list_free(children);
- }
+ reltuples = classForm->reltuples;
vactuples = tabentry->n_dead_tuples;
instuples = tabentry->inserts_since_vacuum;
anltuples = tabentry->changes_since_analyze;
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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.
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d897bbec2b..5554275e64 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -108,7 +108,7 @@ static relopt_bool boolRelOpts[] =
{
"autovacuum_enabled",
"Enables autovacuum in this relation",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
true
@@ -246,7 +246,7 @@ static relopt_int intRelOpts[] =
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
- RELOPT_KIND_HEAP,
+ RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
-1, 0, INT_MAX
@@ -420,7 +420,7 @@ static relopt_real realRelOpts[] =
{
"autovacuum_analyze_scale_factor",
"Number of tuple inserts, updates or deletes prior to analyze as a fraction of reltuples",
- RELOPT_KIND_HEAP,
+ RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
-1, 0.0, 100.0
@@ -1962,12 +1962,11 @@ bytea *
partitioned_table_reloptions(Datum reloptions, bool validate)
{
/*
- * There are no options for partitioned tables yet, but this is able to do
- * some validation.
+ * autovacuum_enabled, autovacuum_analyze_threshold and
+ * autovacuum_analyze_scale_factor are supported for partitioned tables.
*/
- return (bytea *) build_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED,
- 0, NULL, 0);
+
+ return default_reloptions(reloptions, validate, RELOPT_KIND_PARTITIONED);
}
/*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 4d6b232787..a47e102f36 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -660,7 +660,7 @@ CREATE VIEW pg_stat_all_tables AS
FROM pg_class C LEFT JOIN
pg_index I ON C.oid = I.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-WHERE C.relkind IN ('r', 't', 'm')
+WHERE C.relkind IN ('r', 't', 'm', 'p')
GROUP BY C.oid, N.nspname, C.relname;
CREATE VIEW pg_stat_xact_all_tables AS
@@ -680,7 +680,7 @@ CREATE VIEW pg_stat_xact_all_tables AS
FROM pg_class C LEFT JOIN
pg_index I ON C.oid = I.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-WHERE C.relkind IN ('r', 't', 'm')
+WHERE C.relkind IN ('r', 't', 'm', 'p')
GROUP BY C.oid, N.nspname, C.relname;
CREATE VIEW pg_stat_sys_tables AS
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index f84616d3d2..0789117bb8 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -612,8 +612,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
PROGRESS_ANALYZE_PHASE_FINALIZE_ANALYZE);
/*
- * Update pages/tuples stats in pg_class, and report ANALYZE to the stats
- * collector ... but not if we're doing inherited stats.
+ * Update pages/tuples stats in pg_class ... but not if we're doing
+ * inherited stats.
*
* We assume that VACUUM hasn't set pg_class.reltuples already, even
* during a VACUUM ANALYZE. Although VACUUM often updates pg_class,
@@ -655,19 +655,33 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
InvalidMultiXactId,
in_outer_xact);
}
+ }
Re: Autovacuum on partitioned table (autoanalyze)
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 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 v15_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
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 Herrera39°49'30"S 73°17'W "The problem with the future is that it keeps turning into the present" (Hobbes)
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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 Herrera39°49'30"S 73°17'W
Re: Autovacuum on partitioned table (autoanalyze)
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,100) 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
Re: Autovacuum on partitioned table (autoanalyze)
On 2021-Apr-04, Tomas Vondra wrote: > In fact, one of the first posts in this threads links to this: > > https://www.postgresql.org/message-id/4823.1262132964%40sss.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
Re: Autovacuum on partitioned table (autoanalyze)
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: https://www.postgresql.org/message-id/4823.1262132964%40sss.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
Re: Autovacuum on partitioned table (autoanalyze)
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 Herrera39°49'30"S 73°17'W "La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d897bbec2b..5554275e64 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -108,7 +108,7 @@ static relopt_bool boolRelOpts[] =
{
"autovacuum_enabled",
"Enables autovacuum in this relation",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
true
@@ -246,7 +246,7 @@ static relopt_int intRelOpts[] =
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
- RELOPT_KIND_HEAP,
+ RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
-1, 0, INT_MAX
@@ -420,7 +420,7 @@ static relopt_real realRelOpts[] =
{
"autovacuum_analyze_scale_factor",
"Number of tuple inserts, updates or deletes prior to analyze as a fraction of reltuples",
- RELOPT_KIND_HEAP,
+ RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
ShareUpdateExclusiveLock
},
-1, 0.0, 100.0
@@ -1962,12 +1962,11 @@ bytea *
partitioned_table_reloptions(Datum reloptions, bool validate)
{
/*
- * There are no options for partitioned tables yet, but this is able to do
- * some validation.
+ * autovacuum_enabled, autovacuum_analyze_threshold and
+ * autovacuum_analyze_scale_factor are supported for partitioned tables.
*/
- return (bytea *) build_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED,
- 0, NULL, 0);
+
+ return default_reloptions(reloptions, validate, RELOPT_KIND_PARTITIONED);
}
/*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5f2541d316..fb41b06539 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -660,7 +660,7 @@ CREATE VIEW pg_stat_all_tables AS
FROM pg_class C LEFT JOIN
pg_index I ON C.oid = I.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-WHERE C.relkind IN ('r', 't', 'm')
+WHERE C.relkind IN ('r', 't', 'm', 'p')
GROUP BY C.oid, N.nspname, C.relname;
CREATE VIEW pg_stat_xact_all_tables AS
@@ -680,7 +680,7 @@ CREATE VIEW pg_stat_xact_all_tables AS
FROM pg_class C LEFT JOIN
pg_index I ON C.oid = I.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-WHERE C.relkind IN ('r', 't', 'm')
+WHERE C.relkind IN ('r', 't', 'm', 'p')
GROUP BY C.oid, N.nspname, C.relname;
CREATE VIEW pg_stat_sys_tables AS
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index f84616d3d2..35e9a2fc17 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -655,20 +655,22 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
InvalidMultiXactId,
in_outer_xact);
}
+ }
- /*
- * Now report ANALYZE to the stats collector.
- *
- * We deliberately don't report to the stats collector when doing
- * inherited stats, 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.
- */
+ /*
+ * Now report ANALYZE to the stats collector.
+ *
+ * Regarding inherited stats, we report only in the case of declarative
+ * partitioning. For partitioning based on inheritance, 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.
+ */
+ if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
pgstat_report_analyze(onerel, totalrows, totaldeadrows,
(va_cols == NIL));
- }
+
/*
* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 23ef23c13e..7ca074a800 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -74,7 +74,9 @@
#include "access/xact.h"
#include "catalog/dependency.h"
#include "catalog/namespace.h"
+#include "catalog/partition.h"
#include "catalog/pg_database.h"
+#include "catalog/pg_inherits.h"
#include "commands/dbcommands.h"
#include "commands/vacuum.h"
#include "lib/ilist.h"
@@ -350,6 +352,8
Re: Autovacuum on partitioned table (autoanalyze)
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 v13_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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].
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 = 1;
changes_since_analyze_reported = 0;
and we propagate 10k to parents:
changes_since_analyze = 1;
changes_since_analyze_reported = 1;
but we don't analyze anything, and we accumulate 5k more changes:
Re: Autovacuum on partitioned table (autoanalyze)
On 12/14/20 8:46 PM, yuzuko wrote: On Thu, Dec 3, 2020 at 10:28 PM Alvaro Herrera wrote: Attach the new patch based on his patch. What do you think? Álvaro, Justin, Kyotaro, thoughts on this latest patch? Regards, -- -David da...@pgmasters.net
Re: Autovacuum on partitioned table (autoanalyze)
Hello Alvaro, On Thu, Dec 3, 2020 at 10:28 PM Alvaro Herrera 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] would be more simple to solve the problem we are facing. Attach the new patch based on his patch. What do you think? [1] https://www.postgresql.org/message-id/20201110.203557.1420746510378864931.horikyota.ntt%40gmail.com -- Best regards, Yuzuko Hosoya NTT Open Source Software Center v12_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
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.
Re: Autovacuum on partitioned table (autoanalyze)
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;
>Oidm_databaseid;
>Oidm_tableoid;
>bool m_autovacuum;
>bool m_resetcounter;
>TimestampTzm_analyzetime;
>PgStat_Counter m_live_tuples;
>PgStat_Counter m_dead_tuples;
>intm_nancestors;
>Oidm_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
Re: Autovacuum on partitioned table (autoanalyze)
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.
Re: Autovacuum on partitioned table (autoanalyze)
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;
Oidm_databaseid;
Oidm_tableoid;
bool m_autovacuum;
bool m_resetcounter;
TimestampTzm_analyzetime;
PgStat_Counter m_live_tuples;
PgStat_Counter m_dead_tuples;
intm_nancestors;
Oidm_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.)
Re: Autovacuum on partitioned table (autoanalyze)
At Thu, 5 Nov 2020 16:03:12 +0900, yuzuko 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 diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml index 4d8ad754f8..50b55f5d01 100644 --- a/doc/src/sgml/maintenance.sgml +++ b/doc/src/sgml/maintenance.sgml @@ -816,6 +816,18 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu since the last ANALYZE. + +For declaratively partitioned tables, only analyze is supported. +The same analyze threshold defined above is used, +but the number of tuples is sum of their childrens' +pg_class.reltuples. +Also, total number of tuples inserted, updated, or deleted since the last +ANALYZE compared with the threshold is calculated by adding up +childrens' number of tuples analyzed in the previous ANALYZE. +This is because partitioned tables don't have any data. So analyze on partitioned +tables are one lap behind their children. + + Temporary tables cannot be accessed by autovacuum. Therefore, appropriate vacuum and analyze operations should be performed via diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index bc59a2d77d..d94caa4b7e 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -1323,8 +1323,6 @@ WITH ( MODULUS numeric_literal, REM If a table parameter value is set and the equivalent toast. parameter is not, the TOAST table will use the table's parameter value. -Specifying these parameters for partitioned tables is not supported, -but you may specify them for individual leaf partitions. diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 8ccc228a8c..35bc2e5bdb 100644 ---
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
Horiguchi-san,
Thank you for your comments.
On Fri, Oct 23, 2020 at 8:23 PM Kyotaro Horiguchi
wrote:
>
> Thanks you for the new version.
>
> At Fri, 23 Oct 2020 15:12:51 +0900, yuzuko 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
Re: Autovacuum on partitioned table (autoanalyze)
Thanks you for the new version.
At Fri, 23 Oct 2020 15:12:51 +0900, yuzuko 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
Re: Autovacuum on partitioned table (autoanalyze)
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 v10_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
Horiguchi-san,
Thank you for reviewing.
On Tue, Sep 15, 2020 at 7:01 PM Kyotaro Horiguchi
wrote:
>
> At Tue, 25 Aug 2020 14:28:20 +0200, Daniel Gustafsson 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 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] 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]
https://www.postgresql.org/message-id/CAKkQ50-bwFEDMBGb1JmDXffXsiU8xk-hN6kJK9CKjdBa7r%3DHdw%40mail.gmail.com
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
Re: Autovacuum on partitioned table (autoanalyze)
At Tue, 25 Aug 2020 14:28:20 +0200, Daniel Gustafsson 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 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
Re: Autovacuum on partitioned table (autoanalyze)
> On 17 Aug 2020, at 08:11, yuzuko 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
Re: Autovacuum on partitioned table (autoanalyze)
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 v9_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
> On 6 Jul 2020, at 12:35, yuzuko wrote: > >> On Wed, Jul 1, 2020 at 6:26 PM Daniel Gustafsson wrote: >> >>> On 21 Apr 2020, at 18:21, yuzuko 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
Re: Autovacuum on partitioned table (autoanalyze)
> On Wed, Jul 1, 2020 at 6:26 PM Daniel Gustafsson wrote: > > > On 21 Apr 2020, at 18:21, yuzuko 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 v8_autovacuum_on_partitioned_table.patch Description: Binary data
Re: Autovacuum on partitioned table (autoanalyze)
> On 21 Apr 2020, at 18:21, yuzuko 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
Re: Autovacuum on partitioned table (autoanalyze)
On Sat, Apr 25, 2020 at 11:13 PM Justin Pryzby 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: > https://www.postgresql.org/message-id/7363.1426537...@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
Re: Autovacuum on partitioned table (autoanalyze)
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: https://www.postgresql.org/message-id/7363.1426537...@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
Re: Autovacuum on partitioned table (autoanalyze)
Hello,
On Sat, Apr 18, 2020 at 2:08 PM Justin Pryzby 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 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
Re: Autovacuum on partitioned table (autoanalyze)
On Fri, Apr 17, 2020 at 10:09:07PM +0900, Amit Langote wrote:
> On Thu, Apr 16, 2020 at 11:19 PM Justin Pryzby 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]. It might be useful if that
part was split out into a separate 0001 patch (?).
Thanks,
--
Justin
[0] https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.com
Re: Autovacuum on partitioned table (autoanalyze)
On Thu, Apr 16, 2020 at 11:19 PM Justin Pryzby 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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
Hi Justin, Thank you for commens. On Tue, Apr 7, 2020 at 12:32 PM Justin Pryzby 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
Re: Autovacuum on partitioned table (autoanalyze)
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
Re: Autovacuum on partitioned table (autoanalyze)
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
