From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>, Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net> |
Subject: | Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly |
Date: | 2020-12-12 19:45:26 |
Message-ID: | 20201212194526.GE24052@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.
So I've rebased this branch on your patch.
Some thoughts:
- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I thought it's
needed to include at least one struct member like "={false}", or else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual convention is to
pass a pointer.
I also changed the errcode and detail for this one.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("incompatible TABLESPACE option"),
errdetail("TABLESPACE can only be used with VACUUM FULL.")));
--
Justin
Attachment | Content-Type | Size |
---|---|---|
v34-0001-Convert-reindex-options-to-struct.patch | text/x-diff | 18.9 KB |
v34-0002-fix.patch | text/x-diff | 18.4 KB |
v34-0003-ExecReindex-and-ReindexParams.patch | text/x-diff | 6.6 KB |
v34-0004-Allow-REINDEX-to-change-tablespace.patch | text/x-diff | 25.5 KB |
v34-0005-Refactor-and-reuse-set_rel_tablespace.patch | text/x-diff | 6.0 KB |
v34-0006-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch | text/x-diff | 23.7 KB |
v34-0007-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch | text/x-diff | 18.4 KB |
v34-0008-Avoid-enums-for-bitmasks.patch | text/x-diff | 21.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Justin Pryzby | 2020-12-12 20:20:17 | Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly |
Previous Message | Andrey Borodin | 2020-12-12 17:47:59 | Re: pglz compression performance, take two |