From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix progress reporting of CLUSTER / VACUUM FULL
In-reply-to: <E1i8pqS-0003NN-7p@gemulon.postgresql.org>
References: <E1i8pqS-0003NN-7p@gemulon.postgresql.org>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Fri, 13 Sep 2019 17:58:56 -0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <19794.1568400476.1@sss.pgh.pa.us>
Date: Fri, 13 Sep 2019 14:47:56 -0400
Message-ID: <19795.1568400476@sss.pgh.pa.us>
Precedence: bulk

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