Hi Justin,
On 09.03.2020 23:04, Justin Pryzby wrote:
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.
Do you have any understanding of what exactly causes this error? I have
tried to debug it a little bit, but still cannot figure out why we need
this extra GetNewRelFileNode() call and a mechanism how it helps.
Probably you mean v4 patch. Yes, interestingly, if we do everything at
once inside RelationSetNewRelfilenode(), then there is no issue at all with:
REINDEX DATABASE template1 TABLESPACE pg_default;
It feels like I am doing a monkey coding here, so I want to understand
it better :)
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
Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in
my opinion.
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 ?
I did this relation_is_movable refactoring in order to share the same
check between REINDEX + TABLESPACE and ALTER INDEX + SET TABLESPACE.
Then I realized that REINDEX already has its own temp tables check and
does mapped relations validation in multiple places, so I just added
global tablespace checks instead. Thus, relation_is_movable seems to be
outdated right now. Probably, we have to do another refactoring here
once all proper validations will be accumulated in this patch set.
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).
Sorry for the late response, I was not going to abandon this patch, but
was a bit busy last month.
Many thanks for you review and fixups! There are some inconsistencies
like mentions of SET TABLESPACE in error messages and so on. I am going
to refactor and include your fixes 0003-0004 into 0001 and 0002, but
keep 0005 separated for now, since this part requires more understanding
IMO (and comparison with v4 implementation).
That way, I am going to prepare a more clear patch set till the middle
of the next week. I will be glad to receive more feedback from you then.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
