Re: BUG #19686: Rolling back SET TABLESPACE

Hello

> v3 adds a paragraph about deferred copy and clarifies that catalog
> updates might not be visible within the transaction, I think it is good
> that it reflects the physical location of the relation. Reading the
> preceding
> paragraph I decided that it was worth adding a test case for partitioned
> tables.

0004 is empty, the documentation is missing.

> 2. Failure to copy the files at the end of transaction, e.g. the new
> tablespace doesn't have enough space. Will the transaction roll back
> cleanly reclaiming the space?

The patch could keep at least the creation of the storage (directory)
at command time, and leave only the data copy to the end. That won't
help against out of space errors, but it could properly handle all
cases of permission / preexisting directory issues.

> My proposal was to do the other way, push the cost to the end of the
> transaction instead, even the heap copy, so if the transaction fails it will
> do no file copies. If the transaction completes, it will do exactly the same
> copies that upstream does, just in a different order. At least that is the
> rationale, maybe during review additional cases that I missed will come up.

This is only true when nothing modifies the relation between the ALTER
TABLESPACE and the COMMIT. When something does, the deferred copy
copies the post-modification relation, so every row written after the
move is written twice, once into the old file and once into the new by
the copy. This is especially visible with wal_level=minimal, where
previously we logged nothing and with the patch we log the entire
table in this case.

This doesn't seem fixable to me, but it also doesn't seem to be a
blocker for the patch/approach to me. If a user does things the other
way around, first modifying the relation and then doing alter, master
already does the same amount of writes, so it only affects one
specific order.

And some specific comments for the patches:

1.

+ /*
+ * Cluster moves by relation id, then for each relation apply the last
+ * change.
+ */
+ list_sort(deferred_tablespace_moves, deferred_tablespace_move_compare_relid);

and

+static int
+deferred_tablespace_move_compare_relid(const ListCell *a, const ListCell *b)
+{
+ PendingTablespaceMove *ca = (PendingTablespaceMove *) a;
+ PendingTablespaceMove *cb = (PendingTablespaceMove *) b;
+ int t = pg_cmp_u32(ca->relid, cb->relid);
+
+ if (t == 0) /* compare pointers for stability */
+ t = (a < b) ? -1 : 1;
+ return t;
+}

This comparator is wrong in multiple ways, see the comment for list_sort:

* The comparator function is declared to receive arguments of type
* const ListCell *; this allows it to use lfirst() and variants
* without casting its arguments. Otherwise it behaves the same as
* the comparator function for standard qsort().
*
* Like qsort(), this provides no guarantees about sort stability
* for equal keys.

The issue is also easily verifiable with an SQL reproducer:

BEGIN;
SAVEPOINT s;
ALTER TABLE b SET TABLESPACE regress_ts1;
ALTER TABLE b SET TABLESPACE pg_default;
ROLLBACK TO s;
ALTER TABLE a SET TABLESPACE regress_ts1;
ALTER TABLE a SET TABLESPACE pg_default;
COMMIT;

Do we need sort at all here? Why can't the code walk the list, and
resolve the last entry per relid?

2. dropping a relation with a pending move now makes COMMIT fail with
an internal error

CREATE TABLESPACE regress_ts1 LOCATION '';
CREATE TABLE d1(a int primary key);
BEGIN; ALTER TABLE d1 SET TABLESPACE regress_ts1; DROP TABLE d1; COMMIT;

Skipping entries whose relation no longer exists seems safe to me.

3. tablespace_xact.sql fails with max_prepared_transaction = 0

this is a test only issue, but should be fixed, this causes a clear CI failure

Another issue with it is the naming of the tablespaces, those should
start with regress_:

+CREATE TABLESPACE xact_tblspace LOCATION '';
+CREATE TABLESPACE xact_tblspace2 LOCATION '';

See ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

4. ALTER TABLE ALL IN TABLESPACE acts on a stale catalog

This is another visible effect of the staleness question that was
already discussed.

SET allow_in_place_tablespaces = on;
CREATE TABLESPACE ts1 LOCATION '';
CREATE TABLESPACE ts2 LOCATION '';
CREATE TABLE t (a int PRIMARY KEY) TABLESPACE ts1;

BEGIN;
ALTER TABLE t SET TABLESPACE pg_default;
ALTER TABLE ALL IN TABLESPACE ts1 SET TABLESPACE ts2;
COMMIT;

SELECT coalesce(spcname, 'pg_default') AS tablespace
FROM pg_class c LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace
WHERE c.relname = 't';

And it seems to me that ALTER TABLE ALL IN TABLESPACE could follow the
intent of the transaction based on the pending list.