Обсуждение: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
Fix progress reporting of CLUSTER / VACUUM FULL The progress state was being clobbered once the first index completed being rebuilt, causing the final phases of the operation not show anything in the progress view. This was inadvertently broken in 03f9e5cba0ee, which added progress tracking for REINDEX. (The reason this bugfix is this small is that I had already noticed this problem when writing monitoring for CREATE INDEX, and had already worked around it, as can be seen in discussion starting at https://postgr.es/m/20190329150218.GA25010@alvherre.pgsql Fixing the problem is just a matter of fixing one place touched by the REINDEX monitoring.) Reported by: Álvaro Herrera Author: Álvaro Herrera Discussion: https://postgr.es/m/20190801184333.GA21369@alvherre.pgsql Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/6212276e4343f729b0152e81824f850d1a21d57e Modified Files -------------- src/backend/catalog/index.c | 24 +++++++++++++++--------- src/backend/commands/indexcmds.c | 4 ++-- src/include/nodes/parsenodes.h | 1 + 3 files changed, 18 insertions(+), 11 deletions(-)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Fix progress reporting of CLUSTER / VACUUM FULL
Not a new problem of this patch, exactly, but:
/* Reindex options */
#define REINDEXOPT_VERBOSE 1 << 0 /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS 1 << 1 /* report pgstat progress */
Surely these macro definitions are incredibly dangerous due to their
lack of parentheses.
I'd initially thought that we already had bugs in existing usages like
if (options & REINDEXOPT_VERBOSE)
After consulting a nearby C reference I see that we're accidentally
saved by << having higher priority than &, but this is not safely-
maintainable code. Expressions like "~REINDEXOPT_VERBOSE" would not
do what one expects.
regards, tom lane
On 2019-Sep-13, Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > Fix progress reporting of CLUSTER / VACUUM FULL > > Not a new problem of this patch, exactly, but: > > /* Reindex options */ > #define REINDEXOPT_VERBOSE 1 << 0 /* print progress info */ > +#define REINDEXOPT_REPORT_PROGRESS 1 << 1 /* report pgstat progress */ > > Surely these macro definitions are incredibly dangerous due to their > lack of parentheses. Ugh, I knew there was something odd here in the back of my mind but I was unable to see what it was :-( I'll fix those definitions. Thanks for looking, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Sep-13, Tom Lane wrote: > Not a new problem of this patch, exactly, but: > > /* Reindex options */ > #define REINDEXOPT_VERBOSE 1 << 0 /* print progress info */ > +#define REINDEXOPT_REPORT_PROGRESS 1 << 1 /* report pgstat progress */ > > Surely these macro definitions are incredibly dangerous due to their > lack of parentheses. > > I'd initially thought that we already had bugs in existing usages like > > if (options & REINDEXOPT_VERBOSE) > > After consulting a nearby C reference I see that we're accidentally > saved by << having higher priority than &, but this is not safely- > maintainable code. Expressions like "~REINDEXOPT_VERBOSE" would not > do what one expects. Fixed back to 9.5, where this macro appeared. I was unable to come up with a way to search for other occurrences of the same problem :-( Thanks, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Alvaro,
On Sat, Sep 14, 2019 at 2:59 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Fix progress reporting of CLUSTER / VACUUM FULL
>
> The progress state was being clobbered once the first index completed
> being rebuilt, causing the final phases of the operation not show
> anything in the progress view. This was inadvertently broken in
> 03f9e5cba0ee, which added progress tracking for REINDEX.
I noticed that the progress of REINDEX INDEX index_name is no longer
shown; REINDEX TABLE table_name is fine. Maybe you missed updating
ReindexIndex() to pass the option to report progress, like this:
@@ -2350,7 +2350,8 @@ ReindexIndex(RangeVar *indexRelation, int
options, bool concurrent)
if (concurrent)
ReindexRelationConcurrently(indOid, options);
else
- reindex_index(indOid, false, persistence, options);
+ reindex_index(indOid, false, persistence,
+ options | REINDEXOPT_REPORT_PROGRESS);
}
Attached a patch.
Thanks,
Amit
Вложения
On 2019-Sep-17, Amit Langote wrote: Hi > I noticed that the progress of REINDEX INDEX index_name is no longer > shown; REINDEX TABLE table_name is fine. Maybe you missed updating > ReindexIndex() to pass the option to report progress, like this: That's right, I broke that case, and your patch fixes it. Pushed, thanks. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services