On 15.07.2020 18:03, Alvaro Herrera wrote:
On 2020-Jul-15, Konstantin Knizhnik wrote:
On 15.07.2020 02:17, Alvaro Herrera wrote:
On 2020-Jul-10, Konstantin Knizhnik wrote:
@@ -3076,6 +3080,10 @@ relation_needs_vacanalyze(Oid relid,
instuples = tabentry->inserts_since_vacuum;
anltuples = tabentry->changes_since_analyze;
+ rel = RelationIdGetRelation(relid);
+ oldestXmin =
TransactionIdLimitedForOldSnapshots(GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM),
rel);
+ RelationClose(rel);
*cough*
Sorry, Alvaro.
Can you explain this *cough*
You didn't like that relation is opened just to call GetOldestXmin?
But this functions requires Relation. Do you suggest to rewrite it so that
it is possible to pass just Oid of relation?
At that point of autovacuum, you don't have a lock on the relation; the
only thing you have is a pg_class tuple (and we do it that way on
purpose as I recall). I think asking relcache for it is dangerous, and
moreover requesting relcache for it directly goes counter our normal
coding pattern. At the very least you should have a comment explaining
why you do it and why it's okay to do it, and also handle the case when
RelationIdGetRelation returns null.
However, looking at the bigger picture I wonder if it would be better to
test the getoldestxmin much later in the process to avoid this whole
issue. Just carry forward the relation until the point where vacuum is
called ... that may be cleaner? And the extra cost is not that much.
Thank you for explanation.
I have prepared new version of the patch which opens relation with care.
Concerning your suggestion to perform this check later (in vacuum_rel()
I guess?)
I tried to implement it but faced with another problem.
Right now information about autovacuum_oldest_xmin for relationis stored
in statistic (PgStat_StatTabEntry)
together with other atovacuum related fields like
autovac_vacuum_timestamp, autovac_analyze_timestamp,...)
I am not sure that it is right place for storing autovacuum_oldest_xmin
but I didn't want to organize in shared memory yet another hash table
just for keeping this field. May be you can suggest something better...
But right now it is stored here when vacuum is completed.
PgStat_StatTabEntry is obtained by get_pgstat_tabentry_relid() which
also needs pgstat_fetch_stat_dbentry(MyDatabaseId) and
pgstat_fetch_stat_dbentry(InvalidOid). I do not want to copy all this
stuff to vacuum.c.
It seems to me to be easier to open relation in
relation_needs_vacanalyze(), isn;t it?
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 1bbc4598f7..f7e17a9bbf 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -607,7 +607,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
pgstat_report_vacuum(RelationGetRelid(onerel),
onerel->rd_rel->relisshared,
new_live_tuples,
- vacrelstats->new_dead_tuples);
+ vacrelstats->new_dead_tuples,
+ OldestXmin);
pgstat_progress_end_command();
/* and log the action if appropriate */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9c7d4b0c60..00f7cb4ca1 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -92,6 +92,7 @@
#include "storage/lmgr.h"
#include "storage/pmsignal.h"
#include "storage/proc.h"
+#include "storage/procarray.h"
#include "storage/procsignal.h"
#include "storage/sinvaladt.h"
#include "storage/smgr.h"
@@ -2992,6 +2993,9 @@ relation_needs_vacanalyze(Oid relid,
TransactionId xidForceLimit;
MultiXactId multiForceLimit;
+ TransactionId oldestXmin = InvalidTransactionId;
+ Relation rel;
+
AssertArg(classForm != NULL);
AssertArg(OidIsValid(relid));
@@ -3076,6 +3080,20 @@ relation_needs_vacanalyze(Oid relid,
instuples = tabentry->inserts_since_vacuum;
anltuples = tabentry->changes_since_analyze;
+ /*
+ * We need to calculate oldestXmin for relation in the same way as it is done in vacuum.c
+ * (see comment in vacuum_set_xid_limits).
+ * This is why we have to open relation.
+ */
+ if (ConditionalLockRelationOid(relid, AccessShareLock))
+ {
+ rel = try_relation_open(relid, NoLock);
+ if (rel)
+ {
+ oldestXmin = TransactionIdLimitedForOldSnapshots(GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM), rel);
+ relation_close(rel, AccessShareLock);
+ }
+ }
vacthresh = (float4) vac_base_thresh + vac_scale_factor * reltuples;
vacinsthresh = (float4) vac_ins_base_thresh + vac_ins_scale_factor * reltuples;
anlthresh = (float4) anl_base_thresh + anl_scale_factor * reltuples;
@@ -3094,10 +3112,24 @@ relation_needs_vacanalyze(Oid relid,
NameStr(classForm->relname),
vactuples, vacthresh, anltuples, anlthresh);
- /* Determine if this table needs vacuum or analyze. */
- *dovacuum = force_vacuum || (vactuples > vacthresh) ||
- (vac_ins_base_thresh >= 0 && instuples > vacinsthresh);
- *doanalyze = (anltuples > anlthresh);
+ /*
+ * Determine if this table needs vacuum or analyze.
+ * Do not perform autovacuum if we have already did it with the same oldestXmin.
+ */
+ if (!TransactionIdIsValid(oldestXmin) || tabentry->autovac_oldest_xmin != oldestXmin)
+ {
+ *dovacuum = force_vacuum || (vactuples > vacthresh) ||
+ (vac_ins_base_thresh >= 0 && instuples > vacinsthresh);
+ *doanalyze = (anltuples > anlthresh);
+ elog(DEBUG1, "Consider relation %d tabentry=%p, tabentry->autovac_oldest_xmin=%d, oldestXmin=%d, dovacuum=%d, doanalyze=%d",
+ relid, tabentry, tabentry->autovac_oldest_xmin, oldestXmin, *dovacuum, *doanalyze);
+ }
+ else
+ {
+ elog(DEBUG1, "Skip autovacuum of relation %d", relid);
+ *dovacuum = force_vacuum;
+ *doanalyze = false;
+ }
}
else
{
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 88992c2da2..67504d1d99 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1461,7 +1461,8 @@ pgstat_report_autovac(Oid dboid)
*/
void
pgstat_report_vacuum(Oid tableoid, bool shared,
- PgStat_Counter livetuples, PgStat_Counter deadtuples)
+ PgStat_Counter livetuples, PgStat_Counter deadtuples,
+ TransactionId oldestXmin)
{
PgStat_MsgVacuum msg;
@@ -1475,6 +1476,7 @@ pgstat_report_vacuum(Oid tableoid, bool shared,
msg.m_vacuumtime = GetCurrentTimestamp();
msg.m_live_tuples = livetuples;
msg.m_dead_tuples = deadtuples;
+ msg.m_oldest_xmin = oldestXmin;
pgstat_send(&msg, sizeof(msg));
}
@@ -4838,6 +4840,7 @@ pgstat_get_tab_entry(PgStat_StatDBEntry *dbentry, Oid tableoid, bool create)
result->analyze_count = 0;
result->autovac_analyze_timestamp = 0;
result->autovac_analyze_count = 0;
+ result->autovac_oldest_xmin = InvalidTransactionId;
}
return result;
@@ -6007,6 +6010,7 @@ pgstat_recv_tabstat(PgStat_MsgTabstat *msg, int len)
tabentry->analyze_count = 0;
tabentry->autovac_analyze_timestamp = 0;
tabentry->autovac_analyze_count = 0;
+ tabentry->autovac_oldest_xmin = InvalidTransactionId;
}
else
{
@@ -6288,6 +6292,8 @@ pgstat_recv_vacuum(PgStat_MsgVacuum *msg, int len)
tabentry->n_live_tuples = msg->m_live_tuples;
tabentry->n_dead_tuples = msg->m_dead_tuples;
+ tabentry->autovac_oldest_xmin = msg->m_oldest_xmin;
+
/*
* It is quite possible that a non-aggressive VACUUM ended up skipping
* various pages, however, we'll zero the insert counter here regardless.
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 1387201382..ef92c5632f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -383,6 +383,7 @@ typedef struct PgStat_MsgVacuum
TimestampTz m_vacuumtime;
PgStat_Counter m_live_tuples;
PgStat_Counter m_dead_tuples;
+ TransactionId m_oldest_xmin;
} PgStat_MsgVacuum;
@@ -692,6 +693,8 @@ typedef struct PgStat_StatTabEntry
PgStat_Counter analyze_count;
TimestampTz autovac_analyze_timestamp; /* autovacuum initiated */
PgStat_Counter autovac_analyze_count;
+
+ TransactionId autovac_oldest_xmin;
} PgStat_StatTabEntry;
@@ -1301,7 +1304,8 @@ extern void pgstat_reset_slru_counter(const char *);
extern void pgstat_report_autovac(Oid dboid);
extern void pgstat_report_vacuum(Oid tableoid, bool shared,
- PgStat_Counter livetuples, PgStat_Counter deadtuples);
+ PgStat_Counter livetuples, PgStat_Counter deadtuples,
+ TransactionId oldestXmin);
extern void pgstat_report_analyze(Relation rel,
PgStat_Counter livetuples, PgStat_Counter deadtuples,
bool resetcounter);
