Sorry for the delayed response. The patch looks good to me and your explanation makes sense.
I just re-ran my original test sql script on an official docker image of postgres (which obviously has asserts off) and didn't get any errors + the correct row is deleted.
I can investigate further if you think it's worthwhile.
Thanks for all your help.
-Mat
I wrote:
> I've not tried yet to create an actual test case for the chgParam-based
> failure. It's definitely possible that that problem is only hypothetical
> at the moment because cursor plans that would satisfy search_plan_tree
> would be too simple to contain any such flags. But I think we'd better
> add that logic anyway.
After fooling with that for awhile longer, indeed there doesn't seem to
be a way to cause such a failure today. The closest thing I was able
to find was that an InitPlan's output params could get marked for
recalculation, resulting in chgParam getting set on nodes that reference
the InitPlan. A cursor query could certainly contain an InitPlan ---
but there doesn't seem to be any code path that would result in forcing
recalculation of one, except with things that couldn't be in a simple
cursor plan, such as NestLoop joins. Still, it's not real hard to
conceive of future changes that would render this a live issue, so
I still feel that the search_plan_tree change is a good idea.
So, I've added a test case based on your example and pushed the patch.
BTW, I don't understand your report that just removing the Assert made
it work for you. That would've resulted in returning an invalid TID
to nodeTidscan.c, which AFAICS should've ended up with an ereport about
a bad block number in heap_fetch.
regards, tom lane