pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix progress reporting of CLUSTER / VACUUM FULL
7+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2019-09-20 c3c131e295 d1b0007639 Fix progress report of REINDEX INDEX
2019-09-13 82d0ae919f 00f167bed7 83c2c97114 ce5d04b646 6d50fd469a a0a40f611e Fix under-parenthesized macro definitions
* pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-13 17:58 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 7+ messages in thread
From: Alvaro Herrera @ 2019-09-13 17:58 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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(-)
^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-13 17:58 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2019-09-13 17:58 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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(-)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-13 18:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 2 replies; 7+ messages in thread
From: Tom Lane @ 2019-09-13 18:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-13 18:52 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2019-09-13 18:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-13 19:31 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2019-09-13 19:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-17 09:58 Amit Langote <amitlangote09@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 1 reply; 7+ messages in thread
From: Amit Langote @ 2019-09-17 09:58 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-committers@lists.postgresql.org
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
Attachments:
ReindexIndex-progress-option.patch (application/octet-stream, 515B) download
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d7c1e1c93f..70f9b6729a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -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);
}
/*
* Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
@ 2019-09-20 16:19 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2019-09-20 16:19 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2019-09-20 16:19 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13 17:58 pgsql: Fix progress reporting of CLUSTER / VACUUM FULL Alvaro Herrera <alvherre@alvh.no-ip.org>
2019-09-13 18:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-13 18:52 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-09-13 19:31 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-09-17 09:58 ` Amit Langote <amitlangote09@gmail.com>
2019-09-20 16:19 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-09-13 17:58 pgsql: Fix progress reporting of CLUSTER / VACUUM FULL Alvaro Herrera <alvherre@alvh.no-ip.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox