Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jLgHI-0004xd-86 for pgsql-hackers@arkaria.postgresql.org; Tue, 07 Apr 2020 04:56:00 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jLgHH-0001OE-5f for pgsql-hackers@arkaria.postgresql.org; Tue, 07 Apr 2020 04:55:59 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jLgHG-0001O7-Gs for pgsql-hackers@lists.postgresql.org; Tue, 07 Apr 2020 04:55:58 +0000 Received: from mail-qk1-x741.google.com ([2607:f8b0:4864:20::741]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jLgHC-0007Fs-W1 for pgsql-hackers@postgresql.org; Tue, 07 Apr 2020 04:55:57 +0000 Received: by mail-qk1-x741.google.com with SMTP id o18so429895qko.12 for ; Mon, 06 Apr 2020 21:55:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telsasoft-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=3jR1HYI9HmlnEvoNMY7c6kk3fTOQoMY334F1qq8h1eU=; b=aA6aG7RT1nEbfuBEnOMY0o68Tff37lA26umih+i896egFwr2c3WcP6eam8HfnVdvxd DLUC+g0VzX7lr+6qpEjcI+Ofl9tRIECdGbe+TE6Qt+Pm9rNvFZg7qm6lcTi0FAtZ0Hre a1oXq9HZ8bKNGd1BMxULN4+ncFowcI/kKzXLOqvyu+xOZa5Pfj01UEGzBNQuVwOOTJJw Pyq5Q8vU9R9YG3ptnH5tgTl3urudVw5i/vbk6Ljc5OJ+xCdde5BFtUSLZ7IfW/B5Pd7/ Z5gCaNFTij+ybX64Vb+UQfzqib+wT7aQj7Wwla/ToAuz2m6jqkWOKQO7KKYOIX6edgdS bJnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=3jR1HYI9HmlnEvoNMY7c6kk3fTOQoMY334F1qq8h1eU=; b=sUvXwxfWMkyDFKiOTGl8N18PkfFlEJEmBatwm4TtWY5Kf/VcPVPm4zpf74xF11gKHK fS1wNFWCKLsRgyTLqvOPT7mbx/NHPRMhywC9yGhc5u20KjEJGXOe2JgkxvjVWv6mo6N/ PXv8QaAntGXOc8+F7dlFtePpiogjDa0bF51xl6l+oogPXJXsgGj/cuZALTWUalKOtR2P fiUpRgx9U1Y2inpDkxnlZPKZWzftdVkNpBcuKF/EGSRPWkVMgUXR3S+P1LO6iAnd6k0q jaoALp1go3jBWZqSfvc0K3Y9M2vny835ptQmZfQXDPhps40x+so9JBkximrHghImYyRP wk2g== X-Gm-Message-State: AGi0PuYcoJy3N9SmALY3itmFDQ3nZAnPyjydBngfdS8sbzrg9V1vQN95 8k4/OXWGDV14fqMeP7KBOj2+oQ== X-Google-Smtp-Source: APiQypJrGQBJi7x6vonKULMmvMIjDrdj0+drvCAXgqawN2XXYAAyVcoXESUsxCYpZirHUW7kXNV4yQ== X-Received: by 2002:a37:4b97:: with SMTP id y145mr459575qka.167.1586235353979; Mon, 06 Apr 2020 21:55:53 -0700 (PDT) Received: from pryzbyj (charmander.telsasoft.com. [50.244.222.1]) by smtp.gmail.com with ESMTPSA id t53sm16528725qth.70.2020.04.06.21.55.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Apr 2020 21:55:53 -0700 (PDT) Received: by pryzbyj (Postfix, from userid 1000) id 62DEF8007F7; Mon, 6 Apr 2020 23:55:51 -0500 (CDT) Date: Mon, 6 Apr 2020 23:55:51 -0500 From: Justin Pryzby To: Amit Kapila Cc: Masahiko Sawada , Mahendra Singh Thalor , Dilip Kumar , Sergei Kornilov , pgsql-hackers Subject: Re: doc review for parallel vacuum Message-ID: <20200407045551.GI2228@telsasoft.com> References: <20200322021801.GB2563@telsasoft.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="idY8LE8SD6/8DnRI" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --idY8LE8SD6/8DnRI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Apr 07, 2020 at 09:57:46AM +0530, Amit Kapila wrote: > On Mon, Mar 23, 2020 at 10:34 AM Amit Kapila wrote: > > On Sun, Mar 22, 2020 at 7:48 AM Justin Pryzby wrote: > > > > > > Original, long thread > > > https://www.postgresql.org/message-id/flat/CAA4eK1%2Bnw1FBK3_sDnW%2B7kB%2Bx4qbDJqetgqwYW8k2xv82RZ%2BKw%40mail.gmail.com#b1745ee853b137043e584b500b41300f > > > > > > > I have comments/questions on the patches: > > doc changes > > ------------------------- > > 1. > > > > - Perform vacuum index and cleanup index phases of VACUUM > > + Perform vacuum index and index cleanup phases of VACUUM > > > > Why is the proposed text improvement over what is already there? > > I have kept the existing text as it is for this case. Probably it should say "index vacuum and index cleanup", which is also what the comment in vacuumlazy.c says. > > 2. > > If the > > - PARALLEL option is omitted, then > > - VACUUM decides the number of workers based on number > > - of indexes that support parallel vacuum operation on the relation which > > - is further limited by > linkend="guc-max-parallel-workers-maintenance"/>. > > - The index can participate in a parallel vacuum if and only if the size > > + PARALLEL option is omitted, then the number of workers > > + is determined based on the number of indexes that support parallel vacuum > > + operation on the relation, and is further limited by > + linkend="guc-max-parallel-workers-maintenance"/>. > > + An index can participate in parallel vacuum if and only if the size > > of the index is more than > linkend="guc-min-parallel-index-scan-size"/>. > > > > Here, it is not clear to me why the proposed text is better than what > > we already have? > Changed as per your suggestion. To answer your question: "VACUUM decides" doesn't sound consistent with docs. "based on {+the+} number" => here, you're veritably missing an article... "relation which" technically means that the *relation* is "is further limited by"... > > Patch changes > > ------------------------- > > 1. > > - * and index cleanup with parallel workers unless the parallel vacuum is > > + * and index cleanup with parallel workers unless parallel vacuum is > > > > As per my understanding 'parallel vacuum' is a noun phrase, so we > > should have a determiner before it. > > Changed as per your suggestion. Thanks (I think you meant an "article"). > > - * We allow each worker to update it as and when it has incurred any cost and > > + * We allow each worker to update it AS AND WHEN it has incurred any cost and > > > > I don't see why it is necessary to make this part bold? We are using > > it at one other place in the code in the way it is used here. > > > > Kept the existing text as it is. I meant this as a question. I'm not sure what "as and when means" ? "If and when" ? > I have combined both of your patches. Let me know if you have any > more suggestions, otherwise, I am planning to push this tomorrow. In the meantime, I found some more issues, so I rebased on top of your patch so you can review it. -- Justin --idY8LE8SD6/8DnRI Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0001-Comments-and-doc-fixes-for-commit-40d964ec99.patch" From 3551aba7445b46d60d2199d068c0e06a5828c5c3 Mon Sep 17 00:00:00 2001 From: Amit Kapila Date: Tue, 7 Apr 2020 09:48:28 +0530 Subject: [PATCH v3 1/2] Comments and doc fixes for commit 40d964ec99. Reported-by: Justin Pryzby Author: Justin Pryzby, with few changes by me Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/20200322021801.GB2563@telsasoft.com --- doc/src/sgml/ref/vacuum.sgml | 16 +++++++------- src/backend/access/heap/vacuumlazy.c | 30 +++++++++++++-------------- src/backend/access/transam/parallel.c | 2 +- src/backend/commands/vacuum.c | 20 +++++++++--------- src/include/commands/vacuum.h | 2 +- 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 846056a353..b7e0a8af6b 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -234,13 +234,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ integer - background workers (for the detail of each vacuum phases, please + background workers (for the detail of each vacuum phase, please refer to ). If the - PARALLEL option is omitted, then - VACUUM decides the number of workers based on number - of indexes that support parallel vacuum operation on the relation which - is further limited by . - The index can participate in a parallel vacuum if and only if the size + PARALLEL option is omitted, then the number of workers + is determined based on the number of indexes that support parallel vacuum + operation on the relation, and is further limited by . + An index can participate in parallel vacuum if and only if the size of the index is more than . Please note that it is not guaranteed that the number of parallel workers specified in integer will @@ -248,7 +248,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ VACUUM causes a substantial increase in I/O traffic, which might cause poor performance for other active sessions. Therefore, it is sometimes advisable to use the cost-based vacuum delay feature. For - parallel vacuum, each worker sleeps proportional to the work done by that + parallel vacuum, each worker sleeps proportionally to the work done by that worker. See for details. diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index f3382d37a4..a757560996 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -208,7 +208,7 @@ typedef struct LVShared * live tuples in the index vacuum case or the new live tuples in the * index cleanup case. * - * estimated_count is true if the reltuples is an estimated value. + * estimated_count is true if reltuples is an estimated value. */ double reltuples; bool estimated_count; @@ -732,7 +732,7 @@ vacuum_log_cleanup_info(Relation rel, LVRelStats *vacrelstats) * to reclaim dead line pointers. * * If the table has at least two indexes, we execute both index vacuum - * and index cleanup with parallel workers unless the parallel vacuum is + * and index cleanup with parallel workers unless parallel vacuum is * disabled. In a parallel vacuum, we enter parallel mode and then * create both the parallel context and the DSM segment before starting * heap scan so that we can record dead tuples to the DSM segment. All @@ -809,8 +809,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, vacrelstats->latestRemovedXid = InvalidTransactionId; /* - * Initialize the state for a parallel vacuum. As of now, only one worker - * can be used for an index, so we invoke parallelism only if there are at + * Initialize state for a parallel vacuum. As of now, only one worker can + * be used for an index, so we invoke parallelism only if there are at * least two indexes on a table. */ if (params->nworkers >= 0 && vacrelstats->useindex && nindexes > 1) @@ -837,7 +837,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, } /* - * Allocate the space for dead tuples in case the parallel vacuum is not + * Allocate the space for dead tuples in case parallel vacuum is not * initialized. */ if (!ParallelVacuumIsActive(lps)) @@ -2215,7 +2215,7 @@ parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, shared_indstats = get_indstats(lvshared, idx); /* - * Skip processing indexes that doesn't participate in parallel + * Skip processing indexes that don't participate in parallel * operation */ if (shared_indstats == NULL || @@ -2328,8 +2328,8 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, shared_indstats->updated = true; /* - * Now that the stats[idx] points to the DSM segment, we don't need - * the locally allocated results. + * Now that stats[idx] points to the DSM segment, we don't need the + * locally allocated results. */ pfree(*stats); *stats = bulkdelete_res; @@ -2449,7 +2449,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, * lazy_cleanup_index() -- do post-vacuum cleanup for one index relation. * * reltuples is the number of heap tuples and estimated_count is true - * if the reltuples is an estimated value. + * if reltuples is an estimated value. */ static void lazy_cleanup_index(Relation indrel, @@ -3050,9 +3050,9 @@ heap_page_is_all_visible(Relation rel, Buffer buf, /* * Compute the number of parallel worker processes to request. Both index * vacuum and index cleanup can be executed with parallel workers. The index - * is eligible for parallel vacuum iff it's size is greater than + * is eligible for parallel vacuum iff its size is greater than * min_parallel_index_scan_size as invoking workers for very small indexes - * can hurt the performance. + * can hurt performance. * * nrequested is the number of parallel workers that user requested. If * nrequested is 0, we compute the parallel degree based on nindexes, that is @@ -3071,7 +3071,7 @@ compute_parallel_vacuum_workers(Relation *Irel, int nindexes, int nrequested, int i; /* - * We don't allow to perform parallel operation in standalone backend or + * We don't allow performing parallel operation in standalone backend or * when parallelism is disabled. */ if (!IsUnderPostmaster || max_parallel_maintenance_workers == 0) @@ -3144,7 +3144,7 @@ prepare_index_statistics(LVShared *lvshared, bool *can_parallel_vacuum, } /* - * Update index statistics in pg_class if the statistics is accurate. + * Update index statistics in pg_class if the statistics are accurate. */ static void update_index_statistics(Relation *Irel, IndexBulkDeleteResult **stats, @@ -3345,7 +3345,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats, /* * Destroy the parallel context, and end parallel mode. * - * Since writes are not allowed during the parallel mode, so we copy the + * Since writes are not allowed during parallel mode, copy the * updated index statistics from DSM in local memory and then later use that * to update the index statistics. One might think that we can exit from * parallel mode, update the index statistics and then destroy parallel @@ -3452,7 +3452,7 @@ skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared) * Perform work within a launched parallel process. * * Since parallel vacuum workers perform only index vacuum or index cleanup, - * we don't need to report the progress information. + * we don't need to report progress information. */ void parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 29057f389e..14a8690019 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -505,7 +505,7 @@ ReinitializeParallelDSM(ParallelContext *pcxt) /* * Reinitialize parallel workers for a parallel context such that we could - * launch the different number of workers. This is required for cases where + * launch a different number of workers. This is required for cases where * we need to reuse the same DSM segment, but the number of workers can * vary from run-to-run. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 3a89f8fe1e..495ac23a26 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2036,23 +2036,23 @@ vacuum_delay_point(void) /* * Computes the vacuum delay for parallel workers. * - * The basic idea of a cost-based vacuum delay for parallel vacuum is to allow - * each worker to sleep proportional to the work done by it. We achieve this + * The basic idea of a cost-based delay for parallel vacuum is to allow each + * worker to sleep in proportion to the share of work it's done. We achieve this * by allowing all parallel vacuum workers including the leader process to * have a shared view of cost related parameters (mainly VacuumCostBalance). * We allow each worker to update it as and when it has incurred any cost and * then based on that decide whether it needs to sleep. We compute the time * to sleep for a worker based on the cost it has incurred * (VacuumCostBalanceLocal) and then reduce the VacuumSharedCostBalance by - * that amount. This avoids letting the workers sleep who have done less or - * no I/O as compared to other workers and therefore can ensure that workers - * who are doing more I/O got throttled more. + * that amount. This avoids putting to sleep those workers which have done less + * I/O than other workers and therefore ensure that workers + * which are doing more I/O got throttled more. * - * We allow any worker to sleep only if it has performed the I/O above a - * certain threshold, which is calculated based on the number of active - * workers (VacuumActiveNWorkers), and the overall cost balance is more than - * VacuumCostLimit set by the system. The testing reveals that we achieve - * the required throttling if we allow a worker that has done more than 50% + * We allow a worker to sleep only if it has performed I/O above a certain + * threshold, which is calculated based on the number of active workers + * (VacuumActiveNWorkers), and the overall cost balance is more than + * VacuumCostLimit set by the system. Testing reveals that we achieve + * the required throttling if we force a worker that has done more than 50% * of its share of work to sleep. */ static double diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 2779bea5c9..a4cd721400 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -225,7 +225,7 @@ typedef struct VacuumParams /* * The number of parallel vacuum workers. 0 by default which means choose - * based on the number of indexes. -1 indicates a parallel vacuum is + * based on the number of indexes. -1 indicates parallel vacuum is * disabled. */ int nworkers; -- 2.17.0 --idY8LE8SD6/8DnRI Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0002-docs-comments-review-for-parallel-vacuum.patch" From 3c6314b72d9323dd1554deadedf1c57fe105ad6b Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sun, 19 Jan 2020 22:35:01 -0600 Subject: [PATCH v3 2/2] docs/comments review for parallel vacuum --- doc/src/sgml/ref/vacuum.sgml | 12 ++++++------ src/backend/access/heap/vacuumlazy.c | 22 +++++++++++----------- src/backend/commands/vacuum.c | 2 +- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index b7e0a8af6b..5dc0787459 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -232,9 +232,9 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ integer - background workers (for the detail of each vacuum phase, please + background workers (for the details of each vacuum phase, please refer to ). If the PARALLEL option is omitted, then the number of workers is determined based on the number of indexes that support parallel vacuum @@ -358,16 +358,16 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ for details. diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index a757560996..dfa57df7b6 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -232,8 +232,8 @@ typedef struct LVShared /* * Number of active parallel workers. This is used for computing the - * minimum threshold of the vacuum cost balance for a worker to go for the - * delay. + * minimum threshold of the vacuum cost balance before a worker sleeps + * for cost-based delay. */ pg_atomic_uint32 active_nworkers; @@ -2312,12 +2312,12 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* * Copy the index bulk-deletion result returned from ambulkdelete and - * amvacuumcleanup to the DSM segment if it's the first time to get it - * from them, because they allocate it locally and it's possible that an - * index will be vacuumed by the different vacuum process at the next - * time. The copying of the result normally happens only after the first - * time of index vacuuming. From the second time, we pass the result on - * the DSM segment so that they then update it directly. + * amvacuumcleanup to the DSM segment if it's the first time to get a result + * from a worker, because workers allocate BulkDeleteResults locally, and it's possible that an + * index will be vacuumed by a different vacuum process the next + * time. Copying the result normally happens only the first + * time an index is vacuumed. For any additional vacuums, we pass the result on + * the DSM segment so that workers then update it directly. * * Since all vacuum workers write the bulk-deletion result at different * slots we can write them without locking. @@ -3138,7 +3138,7 @@ prepare_index_statistics(LVShared *lvshared, bool *can_parallel_vacuum, if (!can_parallel_vacuum[i]) continue; - /* Set NOT NULL as this index do support parallelism */ + /* Set NOT NULL as this index does support parallelism */ lvshared->bitmap[i >> 3] |= 1 << (i & 0x07); } } @@ -3174,7 +3174,7 @@ update_index_statistics(Relation *Irel, IndexBulkDeleteResult **stats, /* * This function prepares and returns parallel vacuum state if we can launch - * even one worker. This function is responsible to enter parallel mode, + * even one worker. This function is responsible for entering parallel mode, * create a parallel context, and then initialize the DSM segment. */ static LVParallelState * @@ -3346,7 +3346,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats, * Destroy the parallel context, and end parallel mode. * * Since writes are not allowed during parallel mode, copy the - * updated index statistics from DSM in local memory and then later use that + * updated index statistics from DSM into local memory and then later use that * to update the index statistics. One might think that we can exit from * parallel mode, update the index statistics and then destroy parallel * context, but that won't be safe (see ExitParallelMode). diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 495ac23a26..351d5215a9 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2040,7 +2040,7 @@ vacuum_delay_point(void) * worker to sleep in proportion to the share of work it's done. We achieve this * by allowing all parallel vacuum workers including the leader process to * have a shared view of cost related parameters (mainly VacuumCostBalance). - * We allow each worker to update it as and when it has incurred any cost and + * We allow each worker to update it AS AND WHEN it has incurred any cost and * then based on that decide whether it needs to sleep. We compute the time * to sleep for a worker based on the cost it has incurred * (VacuumCostBalanceLocal) and then reduce the VacuumSharedCostBalance by -- 2.17.0 --idY8LE8SD6/8DnRI--