On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > Now that the main patch is committed, I have reviewed the other two patches.
Thanks for that On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks > good to me. I have added the commit message in the patch. I realized the 0003 patch has an error in lazy_vacuum_index; it should be: - RelationGetRelationName(indrel), + vacrelstats->indname, That was maybe due to originally using a separate errinfo for each phase, with one "char *relname" and no "char *indrel". > I don't think the above change is correct. How will vacrelstats have > correct values when vacuum_one_index is called via parallel workers > (via parallel_vacuum_main)? You're right: parallel main's vacrelstats was added by this patchset and only the error context fields were initialized. I fixed it up in the attached by also setting vacrelstats->new_rel_tuples and old_live_tuples. It's not clear if this is worth it just to save an argument to two functions? -- Justin
>From 85672d7f071c91f3ec9190be7feb293f0e49cf8a Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Wed, 26 Feb 2020 19:22:55 -0600 Subject: [PATCH v39 1/2] Avoid some calls to RelationGetRelationName --- src/backend/access/heap/vacuumlazy.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 0d2e724a7d..803e7660f7 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -655,8 +655,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, } appendStringInfo(&buf, msgfmt, get_database_name(MyDatabaseId), - get_namespace_name(RelationGetNamespace(onerel)), - RelationGetRelationName(onerel), + vacrelstats->relnamespace, + vacrelstats->relname, vacrelstats->num_index_scans); appendStringInfo(&buf, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"), vacrelstats->pages_removed, @@ -827,7 +827,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, if (params->nworkers > 0) ereport(WARNING, (errmsg("disabling parallel option of vacuum on \"%s\" --- cannot vacuum temporary tables in parallel", - RelationGetRelationName(onerel)))); + vacrelstats->relname))); } else lps = begin_parallel_vacuum(RelationGetRelid(onerel), Irel, @@ -1722,7 +1722,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, if (vacuumed_pages) ereport(elevel, (errmsg("\"%s\": removed %.0f row versions in %u pages", - RelationGetRelationName(onerel), + vacrelstats->relname, tups_vacuumed, vacuumed_pages))); /* @@ -1751,7 +1751,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, ereport(elevel, (errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages", - RelationGetRelationName(onerel), + vacrelstats->relname, tups_vacuumed, num_tuples, vacrelstats->scanned_pages, nblocks), errdetail_internal("%s", buf.data))); @@ -1883,7 +1883,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats) ereport(elevel, (errmsg("\"%s\": removed %d row versions in %d pages", - RelationGetRelationName(onerel), + vacrelstats->relname, tupindex, npages), errdetail_internal("%s", pg_rusage_show(&ru0)))); @@ -2431,7 +2431,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ereport(elevel, (errmsg(msg, - RelationGetRelationName(indrel), + vacrelstats->indname, dead_tuples->num_tuples), errdetail_internal("%s", pg_rusage_show(&ru0)))); @@ -2602,7 +2602,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats) vacrelstats->lock_waiter_detected = true; ereport(elevel, (errmsg("\"%s\": stopping truncate due to conflicting lock request", - RelationGetRelationName(onerel)))); + vacrelstats->relname))); return; } @@ -2668,7 +2668,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats) ereport(elevel, (errmsg("\"%s\": truncated %u to %u pages", - RelationGetRelationName(onerel), + vacrelstats->relname, old_rel_pages, new_rel_pages), errdetail_internal("%s", pg_rusage_show(&ru0)))); @@ -2733,7 +2733,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats) { ereport(elevel, (errmsg("\"%s\": suspending truncate due to conflicting lock request", - RelationGetRelationName(onerel)))); + vacrelstats->relname))); vacrelstats->lock_waiter_detected = true; return blkno; -- 2.17.0
>From e69a3feb5dcf3860a34771c826cd3a1bdfbf9c83 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Wed, 4 Mar 2020 12:28:50 -0600 Subject: [PATCH v39 2/2] Drop reltuples --- src/backend/access/heap/vacuumlazy.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 803e7660f7..7f5e177ac4 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -331,10 +331,10 @@ static void lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, LVRelStats *vacrelstats); static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats); + bool estimated_count, LVRelStats *vacrelstats); static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer); static bool should_attempt_truncation(VacuumParams *params, @@ -1801,7 +1801,7 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, for (idx = 0; idx < nindexes; idx++) lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples, - vacrelstats->old_live_tuples, vacrelstats); + vacrelstats); } /* Increase and report the number of index scans */ @@ -2301,11 +2301,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ if (lvshared->for_cleanup) - lazy_cleanup_index(indrel, stats, lvshared->reltuples, - lvshared->estimated_count, vacrelstats); + lazy_cleanup_index(indrel, stats, lvshared->estimated_count, vacrelstats); else lazy_vacuum_index(indrel, stats, dead_tuples, - lvshared->reltuples, vacrelstats); + vacrelstats); /* * Copy the index bulk-deletion result returned from ambulkdelete and @@ -2379,7 +2378,6 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, { for (idx = 0; idx < nindexes; idx++) lazy_cleanup_index(Irel[idx], &stats[idx], - vacrelstats->new_rel_tuples, vacrelstats->tupcount_pages < vacrelstats->rel_pages, vacrelstats); } @@ -2396,7 +2394,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, */ static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, LVRelStats *vacrelstats) { IndexVacuumInfo ivinfo; const char *msg; @@ -2410,7 +2408,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ivinfo.report_progress = false; ivinfo.estimated_count = true; ivinfo.message_level = elevel; - ivinfo.num_heap_tuples = reltuples; + ivinfo.num_heap_tuples = vacrelstats->old_live_tuples; ivinfo.strategy = vac_strategy; /* Update error traceback information */ @@ -2451,7 +2449,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats) + bool estimated_count, LVRelStats *vacrelstats) { IndexVacuumInfo ivinfo; const char *msg; @@ -2466,7 +2464,7 @@ lazy_cleanup_index(Relation indrel, ivinfo.estimated_count = estimated_count; ivinfo.message_level = elevel; - ivinfo.num_heap_tuples = reltuples; + ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples; ivinfo.strategy = vac_strategy; /* Update error traceback information */ @@ -3490,14 +3488,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) if (lvshared->maintenance_work_mem_worker > 0) maintenance_work_mem = lvshared->maintenance_work_mem_worker; - /* - * Initialize vacrelstats for use as error callback arg by parallel - * worker. - */ + /* Initialize vacrelstats for use by parallel worker. */ vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); vacrelstats.relname = pstrdup(RelationGetRelationName(onerel)); vacrelstats.indname = NULL; vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ + vacrelstats.old_live_tuples = lvshared->reltuples; /* Used for vacuum phase */ + vacrelstats.new_rel_tuples = lvshared->reltuples; /* Used for cleanup phase */ + vacrelstats.tupcount_pages = lvshared->estimated_count; /* Used for cleanup phase */ /* Setup error traceback support for ereport() */ errcallback.callback = vacuum_error_callback; -- 2.17.0