Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
>> On 2020-12-11 21:27, Alvaro Herrera wrote:
>>> By the way-- What did you think of the idea of explictly marking the
>>> types used for bitmasks using types bits32 and friends, instead of plain
>>> int, which is harder to spot?
>>
>> If we want to make it clearer, why not turn the thing into a struct, as in
>> the attached patch, and avoid the bit fiddling altogether.

- reindex_relation(OIDOldHeap, reindex_flags, 0);
+ reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
This is not common style in the PG code. Usually we would do that
with memset(0) or similar.

+ bool REINDEXOPT_VERBOSE; /* print progress info */
+ bool REINDEXOPT_REPORT_PROGRESS; /* report pgstat progress */
+ bool REINDEXOPT_MISSING_OK; /* skip missing relations */
+ bool REINDEXOPT_CONCURRENTLY; /* concurrent mode */
+} ReindexOptions
Neither is this one to use upper-case characters for variable names.

Now, we will need a ReindexOptions in the long-term to store all those
options and there would be much more coming that booleans here (this
thread talks about tablespaces, there is another thread about
collation filtering). Between using bits32 with some hex flags or
just a set of booleans within a structure, I would choose the former
as a matter of habit but yours has the advantage to make debugging a
no-brainer, which is good. For any approach taken, it seems to me
that the same style should be applied to ClusterOption and
Vacuum{Option,Params}.
--
Michael