At Wed, 29 Mar 2023 13:21:55 +0900 (JST), Kyotaro Horiguchi
<horikyota....@gmail.com> wrote in
> So, sorry for the noise. I'll review it while this into cnosideration.
0003:
It's not this patche's fault, but I don't like the fact that the
variables used for GUC, VacuumCostDelay and VacuumCostLimit, are
updated outside the GUC mechanism. Also I don't like the incorrect
sorting of variables, where some working variables are referred to as
GUC parameters or vise versa.
Although it's somewhat unrelated to the goal of this patch, I think we
should clean up the code tidy before proceeding. Shouldn't we separate
the actual parameters from the GUC base variables, and sort out the
all related variaghble? (something like the attached, on top of your
patch.)
I have some comments on 0003 as-is.
+ tab->at_relopt_vac_cost_limit = avopts ?
+ avopts->vacuum_cost_limit : 0;
+ tab->at_relopt_vac_cost_delay = avopts ?
+ avopts->vacuum_cost_delay : -1;
The value is not used when do_balance is false, so I don't see a
specific reason for these variables to be different when avopts is
null.
+autovac_recalculate_workers_for_balance(void)
+{
+ dlist_iter iter;
+ int orig_nworkers_for_balance;
+ int nworkers_for_balance = 0;
+
+ if (autovacuum_vac_cost_delay == 0 ||
+ (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
return;
+ if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
+ return;
+
I'm not quite sure how these conditions relate to the need to count
workers that shares the global I/O cost. (Though I still believe this
funtion might not be necessary.)
+ if (av_relopt_cost_limit > 0)
+ VacuumCostLimit = av_relopt_cost_limit;
+ else
+ {
+ av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
+ autovacuum_vac_cost_limit : VacuumCostLimit;
+
+ AutoVacuumBalanceLimit();
I think each worker should use MyWorkerInfo->wi_dobalance to identyify
whether the worker needs to use balanced cost values.
+void
+AutoVacuumBalanceLimit(void)
I'm not sure this function needs to be a separate function.
(Sorry, timed out..)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index e9b683805a..f7ef7860ac 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,8 +72,22 @@ int vacuum_multixact_freeze_min_age;
int vacuum_multixact_freeze_table_age;
int vacuum_failsafe_age;
int vacuum_multixact_failsafe_age;
+int vacuum_cost_page_hit = 1;
+int vacuum_cost_page_miss = 2;
+int vacuum_cost_page_dirty = 20;
+int vacuum_cost_limit = 200;
+double vacuum_cost_delay = 0;
+/* working state for vacuum */
+int VacuumCostBalance = 0;
+int VacuumCostInactive = 1;
+int VacuumCostLimit = 200;
+double VacuumCostDelay = 0;
+int64 VacuumPageHit = 0;
+int64 VacuumPageMiss = 0;
+int64 VacuumPageDirty = 0;
+
/* A few variables that don't seem worth passing around as parameters */
static MemoryContext vac_context = NULL;
static BufferAccessStrategy vac_strategy;
diff --git a/src/backend/postmaster/autovacuum.c
b/src/backend/postmaster/autovacuum.c
index c8dae5465a..b475db9bfe 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1832,7 +1832,7 @@ AutoVacuumUpdateLimit(void)
else
{
av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
- autovacuum_vac_cost_limit : VacuumCostLimit;
+ autovacuum_vac_cost_limit : vacuum_cost_limit;
AutoVacuumBalanceLimit();
}
@@ -1866,7 +1866,7 @@ AutoVacuumBalanceLimit(void)
Assert(nworkers_for_balance > 0);
total_cost_limit = autovacuum_vac_cost_limit > 0 ?
- autovacuum_vac_cost_limit : av_base_cost_limit;
+ autovacuum_vac_cost_limit : vacuum_cost_limit;
balanced_cost_limit = total_cost_limit / nworkers_for_balance;
@@ -1888,10 +1888,10 @@ autovac_recalculate_workers_for_balance(void)
int nworkers_for_balance = 0;
if (autovacuum_vac_cost_delay == 0 ||
- (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
+ (autovacuum_vac_cost_delay == -1 && vacuum_cost_limit == 0))
return;
- if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
+ if (autovacuum_vac_cost_limit <= 0 && vacuum_cost_limit <= 0)
return;
orig_nworkers_for_balance =
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 6d3dd26fc7..4524df23c4 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -39,6 +39,7 @@
#include "catalog/catalog.h"
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
+#include "commands/vacuum.h"
#include "executor/instrument.h"
#include "lib/binaryheap.h"
#include "miscadmin.h"
@@ -894,7 +895,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence,
ForkNumber forkNum,
VacuumPageHit++;
if (!VacuumCostInactive)
- VacuumCostBalance += VacuumCostPageHit;
+ VacuumCostBalance += vacuum_cost_page_hit;
TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
smgr->smgr_rlocator.locator.spcOid,
@@ -1099,7 +1100,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence,
ForkNumber forkNum,
VacuumPageMiss++;
if (!VacuumCostInactive)
- VacuumCostBalance += VacuumCostPageMiss;
+ VacuumCostBalance += vacuum_cost_page_miss;
TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
smgr->smgr_rlocator.locator.spcOid,
@@ -1673,7 +1674,7 @@ MarkBufferDirty(Buffer buffer)
VacuumPageDirty++;
pgBufferUsage.shared_blks_dirtied++;
if (!VacuumCostInactive)
- VacuumCostBalance += VacuumCostPageDirty;
+ VacuumCostBalance += vacuum_cost_page_dirty;
}
}
@@ -4200,7 +4201,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
VacuumPageDirty++;
pgBufferUsage.shared_blks_dirtied++;
if (!VacuumCostInactive)
- VacuumCostBalance += VacuumCostPageDirty;
+ VacuumCostBalance += vacuum_cost_page_dirty;
}
}
}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 608ebb9182..dbf463c6e1 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -138,16 +138,3 @@ int MaxConnections = 100;
int max_worker_processes = 8;
int max_parallel_workers = 8;
int MaxBackends = 0;
-
-int VacuumCostPageHit = 1; /* GUC parameters for vacuum */
-int VacuumCostPageMiss = 2;
-int VacuumCostPageDirty = 20;
-int VacuumCostLimit = 200;
-double VacuumCostDelay = 0;
-
-int64 VacuumPageHit = 0;
-int64 VacuumPageMiss = 0;
-int64 VacuumPageDirty = 0;
-
-int VacuumCostBalance = 0; /* working state for vacuum */
-int VacuumCostInactive = 1;
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 8062589efd..179f39ab9c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2379,7 +2379,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Vacuum cost for a page found in the
buffer cache."),
NULL
},
- &VacuumCostPageHit,
+ &vacuum_cost_page_hit,
1, 0, 10000,
NULL, NULL, NULL
},
@@ -2389,7 +2389,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Vacuum cost for a page not found in the
buffer cache."),
NULL
},
- &VacuumCostPageMiss,
+ &vacuum_cost_page_miss,
2, 0, 10000,
NULL, NULL, NULL
},
@@ -2399,7 +2399,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Vacuum cost for a page dirtied by
vacuum."),
NULL
},
- &VacuumCostPageDirty,
+ &vacuum_cost_page_dirty,
20, 0, 10000,
NULL, NULL, NULL
},
@@ -2409,7 +2409,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Vacuum cost amount available before
napping."),
NULL
},
- &VacuumCostLimit,
+ &vacuum_cost_limit,
200, 1, 10000,
NULL, NULL, NULL
},
@@ -3701,7 +3701,7 @@ struct config_real ConfigureNamesReal[] =
NULL,
GUC_UNIT_MS
},
- &VacuumCostDelay,
+ &vacuum_cost_delay,
0, 0, 100,
NULL, NULL, NULL
},
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5c3e250b06..54842a75f9 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -300,6 +300,11 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_min_age;
extern PGDLLIMPORT int vacuum_multixact_freeze_table_age;
extern PGDLLIMPORT int vacuum_failsafe_age;
extern PGDLLIMPORT int vacuum_multixact_failsafe_age;
+extern PGDLLIMPORT int vacuum_cost_limit;
+extern PGDLLIMPORT double vacuum_cost_delay;
+extern PGDLLIMPORT int vacuum_cost_page_hit;
+extern PGDLLIMPORT int vacuum_cost_page_miss;
+extern PGDLLIMPORT int vacuum_cost_page_dirty;
/* Variables for cost-based parallel vacuum */
@@ -312,7 +317,15 @@ typedef enum VacuumCostStatus
extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
+
extern PGDLLIMPORT int VacuumCostBalanceLocal;
+extern PGDLLIMPORT int VacuumCostBalance;
+extern PGDLLIMPORT int VacuumCostInactive;
+extern PGDLLIMPORT int VacuumCostLimit;
+extern PGDLLIMPORT double VacuumCostDelay;
+extern PGDLLIMPORT int64 VacuumPageHit;
+extern PGDLLIMPORT int64 VacuumPageMiss;
+extern PGDLLIMPORT int64 VacuumPageDirty;
/* in commands/vacuum.c */
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 33e22733ae..cf0cc919cf 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -263,18 +263,10 @@ extern PGDLLIMPORT double hash_mem_multiplier;
extern PGDLLIMPORT int maintenance_work_mem;
extern PGDLLIMPORT int max_parallel_maintenance_workers;
-extern PGDLLIMPORT int VacuumCostPageHit;
-extern PGDLLIMPORT int VacuumCostPageMiss;
-extern PGDLLIMPORT int VacuumCostPageDirty;
-extern PGDLLIMPORT int VacuumCostLimit;
-extern PGDLLIMPORT double VacuumCostDelay;
-extern PGDLLIMPORT int64 VacuumPageHit;
-extern PGDLLIMPORT int64 VacuumPageMiss;
-extern PGDLLIMPORT int64 VacuumPageDirty;
-
-extern PGDLLIMPORT int VacuumCostBalance;
-extern PGDLLIMPORT int VacuumCostInactive;
+/*****************************************************************************
+ * globals.h --
*
+ *****************************************************************************/
/* in tcop/postgres.c */
