I wrote a patch for the same bug before I saw this thread -- thanks to
Shihao for pointing me here. Having read #7312, I think your deferred
copy is the right approach, and better than what I had: it fixes the
root cause without touching the indexes.
> What do you think of this approach?
On performance it looks very good. On a 1M-row table with three indexes
I measured SET TABLESPACE at about 104 ms with #7312 versus about 108 ms
on master -- no measurable overhead, since the indexes are left alone.
(My own patch copies each index to a new relfilenode, which came out
around 1.7x, so #7312 is the better direction and I'm happy to drop mine.)
One thing I ran into while testing: a second SET TABLESPACE in the same
transaction, on a table with indexes, ends up in the wrong tablespace.
SET allow_in_place_tablespaces = true;
CREATE TABLESPACE ts LOCATION '';
CREATE TABLE t (a int);
CREATE INDEX ON t (a);
BEGIN;
ALTER TABLE t SET TABLESPACE ts;
ALTER TABLE t SET TABLESPACE pg_default;
COMMIT;
-- master: t ends up in pg_default; with #7312: t ends up in ts
The second ALTER calls CheckRelationTableSpaceMove() while pg_class still
shows the original tablespace (the first move is deferred), so moving
back to pg_default looks like a no-op and is dropped. The deferred move
probably needs to be visible to a later SET TABLESPACE in the same
transaction.
> 2. Failure to copy the files at the end of transaction [...]
Agreed this one deserves care -- moving the copy into PreCommit means a
full tablespace now fails at commit time rather than during the command.
On 0001: with 0003 in place I couldn't get the new "skip existing tuple"
path to fire in any of my tests, and turning the assertion into a silent
skip also drops a useful corruption check. Could it be removed now?
Happy to help test further.
Manu
