Hi,
Tom Lane <tgl@sss.pgh.pa.us> writes:
> I'm afraid that this patch is likely to be, if not completely broken,
> at least very much less useful than one could wish by the time we get
> done closing the holes discussed in this other thread:
>
> https://www.postgresql.org/message-id/flat/5d910e2e-0db8-ec06-dd5f-baec420513c3%40imap.cc
Thanks for the review here Tom, and for linking with the other
discussion (Álvaro did that too, thanks!). I've been reviewing it too.
I didn't think about the pg_temp_NN namespaces in my approach, and I
think it might be possible to make it work, but it's getting quite
involved now.
One idea would be that if every temp table in the session belongs to the
transaction, and their namespace too (we could check through pg_depend
that the namespace doesn't contain anything else beside the
transaction's tables), then we could dispose of the temp schema and
on-commit-drop tables at PREPARE COMMIT time.
Otherwise, as before, prevent the transaction to be a 2PC one.
> For instance, if we're going to have to reject the case where the
> session's temporary schema was created during the current transaction,
> then that puts a very weird constraint on whether this case works.
Yeah. The goal of my approach is to transparently get back temp table
support in 2PC when that makes sense, which is most use cases I've been
confronted to. We use 2PC in Citus, and it would be nice to be able to
use transaction local temp tables on worker nodes when doing data
injestion and roll-ups.
> Also, even without worrying about new problems that that discussion
> may lead to, I don't think that the patch works as-is. The function
> every_on_commit_is_on_commit_drop() does what it says, but that is
> NOT sufficient to conclude that every temp table the transaction has
> touched is on-commit-drop. This logic will successfully reject cases
> with on-commit-delete-rows temp tables, but not cases where the temp
> table(s) lack any ON COMMIT spec at all.
Thanks! I missed that the lack of ON COMMIT spec would have that impact
in the code. We could add tracking of that I suppose, and will have a
look at how to implement it provided that the other points find an
acceptable solution.
Regards,
--
dim
