Re: Eliminating SPI from RI triggers - take 2

Hi Greg,

On Tue, Mar 21, 2023 at 3:54 AM Gregory Stark (as CFM)
<stark(dot)cfm(at)gmail(dot)com> wrote:
> On Mon, 17 Oct 2022 at 14:59, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > But I think the bigger problem for this patch set is that the
> > design-level feedback from
> > https://www.postgresql.org/message-id/CA%2BTgmoaiTNj4DgQy42OT9JmTTP1NWcMV%2Bke0i%3D%2Ba7%3DVgnzqGXw%40mail.gmail.com
> > hasn't really been addressed, AFAICS. ri_LookupKeyInPkRelPlanIsValid
> > is still trivial in v7, and that still seems wrong to me. And I still
> > don't know how we're going to avoid changing the semantics in ways
> > that are undesirable, or even knowing precisely what we did change. If
> > we don't have answers to those questions, then I suspect that this
> > patch set isn't going anywhere.
>
> Amit, do you plan to work on this patch for this commitfest (and
> therefore this release?). And do you think it has a realistic chance
> of being ready for commit this month?

Unfortunately, I don't think so.

> It looks to me like you have some good feedback and can progress and
> are unlikely to finish this patch for this release. In which case
> maybe we can move it forward to the next release?

Yes, that's what I am thinking too at this point.

I agree with Robert's point that changing the implementation from an
SQL query plan to a hand-rolled C function is going to change the
semantics in some known and perhaps many unknown ways. Until I have
enumerated all those semantic changes, it's hard to judge whether the
hand-rolled implementation is correct to begin with. I had started
doing that a few months back but couldn't keep up due to some other
work.

An example I had found of a thing that would be broken by taking out
the executor out of the equation, as the patch does, is the behavior
of an update under READ COMMITTED isolation, whereby a PK tuple being
checked for existence is concurrently updated and thus needs to
rechecked whether it still satisfies the RI query's conditions. The
executor has the EvalPlanQual() mechanism to do that, but while the
hand-rolled implementation did refactor ExecLockRows() to allow doing
the tuple-locking without a PlanState, it gave no consideration to
handling rechecking under READ COMMITTED isolation.

There may be other such things and I think I'd better look for them
carefully in the next cycle than in the next couple of weeks for this
release. My apologies that I didn't withdraw the patch sooner.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com