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-15 23:58:37 |
Message-ID: | 20201215235837.GA23967@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Dec 14, 2020 at 06:14:17PM -0600, Justin Pryzby wrote:
> 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.
> >
> > 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 think maybe Michael missed this message (?)
> I had applied some changes on top of Peter's patch.
>
> I squished those commits now, and also handled ClusterOption and VacuumOption
> in the same style.
>
> Some more thoughts:
> - should the structures be named in plural ? "ReindexOptions" etc. Since they
> define *all* the options, not just a single bit.
> - For vacuum, do we even need a separate structure, or should the members be
> directly within VacuumParams ? It's a bit odd to write
> params.options.verbose. Especially since there's also ternary options which
> are directly within params.
> - Then, for cluster, I think it should be called ClusterParams, and eventually
> include the tablespaceOid, like what we're doing for Reindex.
>
> I am awaiting feedback on these before going further since I've done too much
> rebasing with these ideas going back and forth and back.
With Alexey's agreement, I propose something like this.
I've merged some commits and kept separate the ones which are more likely to be
disputed/amended. But it may be best to read the series as a single commit,
like "git diff origin.."
--
Justin
Attachment | Content-Type | Size |
---|---|---|
0001-Convert-options-to-struct-Reindex-Cluster-Vacuum.patch | text/x-diff | 47.9 KB |
0002-Change-vacuum_open_relation-vacuum_is_relation_owner.patch | text/x-diff | 6.9 KB |
0003-Move-booleans-directly-into-VacuumParams.patch | text/x-diff | 17.9 KB |
0004-ExecReindex-and-ReindexParams.patch | text/x-diff | 6.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jeff Davis | 2020-12-15 23:59:02 | Re: Reloptions for table access methods |
Previous Message | Tom Lane | 2020-12-15 23:22:30 | Re: Sorting case branches in outfuncs.c/outNode alphabetically |