On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
> > Anyway, new version is attached. It is rebased in order to resolve conflicts
> > with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes
> > this small comment fix.
>
> Thanks for rebasing - I actually started to do that yesterday.
>
> I extracted the bits from your original 0001 patch which handled CLUSTER and
> VACUUM FULL. I don't think if there's any interest in combining that with
> ALTER anymore. On another thread (1), I tried to implement that, and Tom
> pointed out problem with the implementation, but also didn't like the idea.
>
> I'm including some proposed fixes, but didn't yet update the docs, errors or
> tests for that. (I'm including your v8 untouched in hopes of not messing up
> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I
> think due to moving system toast indexes.
I was able to avoid this issue by adding a call to GetNewRelFileNode, even
though that's already called by RelationSetNewRelfilenode(). Not sure if
there's a better way, or if it's worth Alexey's v3 patch which added a
tablespace param to RelationSetNewRelfilenode.
The current logic allows moving all the indexes and toast indexes, but I think
we should use IsSystemRelation() unless allow_system_table_mods, like existing
behavior of ALTER.
template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default;
ERROR: permission denied: "pg_extension_oid_index" is a system catalog
template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default;
REINDEX
Finally, I think the CLUSTER is missing permission checks. It looks like
relation_is_movable was factored out, but I don't see how that helps ?
Alexey, I'm hoping to hear back if you think these changes are ok or if you'll
publish a new version of the patch addressing the crash I reported.
Or if you're too busy, maybe someone else can adopt the patch (I can help).
--
Justin