Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
Hi Alvaro, On Sat, Sep 14, 2019 at 2:59 AM Alvaro Herrera 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 ReindexIndex-progress-option.patch Description: Binary data
Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
On 2019-Sep-13, Tom Lane wrote: > Alvaro Herrera 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
Alvaro Herrera 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
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 -- REL_12_STABLE Details --- https://git.postgresql.org/pg/commitdiff/da47e43dc32e3c5916396f0cbcfa974b371e4875 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(-)
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(-)
