Hi Al,
I built v2 on master (82d31451606) with --enable-cassert and checked the
two cases, plus your question.
v2 fixes both. The double SET TABLESPACE now lands right: a table moved
to ts and then back to pg_default in one transaction ends in pg_default,
where v1 left it in ts. The original recipe is still fixed: the
rollback + INSERT btree case does not trap and bt_index_check reports
nothing, against master where it fails the _bt_posting_valid assertion.
One build problem: v2-0002 does not compile with --enable-cassert.
deferred_tablespace_move_compare_relid() reads its cells with
lfirst_node(PendingTablespaceMove, ...), but PendingTablespaceMove is a
plain struct, not a Node, so T_PendingTablespaceMove is undeclared.
Without assertions castNode() is a plain cast, so the build passes, which
is why it slips through, but the cassert buildfarm animals would fail on
it. PreCommit_on_commit_actions() walks on_commits with a plain
(OnCommitItem *) lfirst() cast; the same here builds under cassert.
> I just noticed that if someone check the pg_tablespaces inside the
> transaction they will get unexpected results. (Is that something we
> need to fix?)
I can reproduce it. With v2, inside the transaction
pg_class.reltablespace for an indexed table still reads the old
tablespace until commit, since the whole move is deferred. On master
SET TABLESPACE updates the catalog at execution time, so a query in the
same transaction sees the new tablespace right away. So v2 does change
that observable behavior.
Whether it is worth fixing is your call. The one thing I'd note is that
the alternative, showing the pending tablespace in the catalog within
the transaction, would leave reltablespace pointing at a tablespace the
file has not reached until commit, so it isn't only a matter of moving
the catalog update earlier. I'll leave the design to you; I mainly
wanted to confirm the behavior is real and that it differs from master.
Regards,
Manu
