I took a quick look at this. I guess I'm disturbed by the idea
that we'd totally replace the implementation technology for only one
variant of foreign key checks. That means that there'll be a lot
of minor details that don't act the same depending on context. One
point I was just reminded of by [1] is that the SPI approach enforces
permissions checks on the table access, which I do not see being done
anywhere in your patch. Now, maybe it's fine not to have such checks,
on the grounds that the existence of the RI constraint is sufficient
permission (the creator had to have REFERENCES permission to make it).
But I'm not sure about that. Should we add SELECT permissions checks
to this code path to make it less different?
In the same vein, the existing code actually runs the query as the
table owner (cf. SetUserIdAndSecContext in ri_PerformCheck), another
nicety you haven't bothered with. Maybe that is invisible for a
pure SELECT query but I'm not sure I would bet on it. At the very
least you're betting that the index-related operators you invoke
aren't going to care, and that nobody is going to try to use this
difference to create a security exploit via a trojan-horse index.
Shall we mention RLS restrictions? If we don't worry about that,
I think REFERENCES privilege becomes a full bypass of RLS, at
least for unique-key columns.
I wonder also what happens if the referenced table isn't a plain
heap with a plain btree index. Maybe you're accessing it at the
right level of abstraction so things will just work with some
other access methods, but I'm not sure about that. (Anybody
want to try this with a partitioned table some of whose partitions
are foreign tables?)
Lastly, ri_PerformCheck is pretty careful about not only which
snapshot it uses, but which *pair* of snapshots it uses, because
sometimes it needs to worry about data changes since the start
of the transaction. You've ignored all of that complexity AFAICS.
That's okay (I think) for RI_FKey_check which was passing
detectNewRows = false, but for sure it's not okay for
ri_Check_Pk_Match. (I kind of thought we had isolation tests
that would catch that, but apparently not.)
So, this is a cute idea, and the speedup is pretty impressive,
but I don't think it's anywhere near committable. I also wonder
whether we really want ri_triggers.c having its own copy of
low-level stuff like the tuple-locking code you copied. Seems
like a likely maintenance hazard, so maybe some more refactoring
is needed.
regards, tom lane
[1]
https://www.postgresql.org/message-id/flat/16911-ca792f6bbe244754%40postgresql.org
