"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> It's easy enough to add code to EndPlan that goes through the
>> estate->es_rowMark list to close the rels that had ROW_MARK_FOR_UPDATE
>> set. But if that bit wasn't set, the above code opens the rel and then
>> forgets about it completely. Is that a bug? If not, I guess we need
> Seems its a bug though I'm not sure.
I looked over the code that works with rowmarks and decided it is a
bug. There are just two action flag bits for the executor to worry
about, ROW_MARK_FOR_UPDATE and ROW_ACL_FOR_UPDATE. The first makes
the execution-time stuff actually happen, while the second causes
a suitable permissions check to be applied before execution. In a
simple SELECT FOR UPDATE situation, both bits will be set. The only
way that the ROW_MARK_FOR_UPDATE bit can get unset is if the SELECT
FOR UPDATE command references a view --- in that case, the rewriter
clears the ROW_MARK_FOR_UPDATE bit on the view's rowmark entry,
and adds rowmark entries with only ROW_MARK_FOR_UPDATE set for the
tables referenced by the view. As far as I can see, this is correct
behavior: the permissions check should be applied to the view, not
the referenced tables, but actual execution happens in the referenced
tables and doesn't touch the view at all. Therefore, it's unnecessary
--- and perhaps actually wrong --- for InitPlan to be grabbing a
RowShareLock on the view.
So, I've rearranged the InitPlan code to not open the rel at all when
ROW_MARK_FOR_UPDATE is clear, and I've added code in EndPlan to
traverse the estate->es_rowMark list and heap_close the opened rels
(specifying NoLock, so that the RowShareLock is held till commit).
This seems to solve Oliver's problem, and the regress tests still pass,
so I committed it a little while ago.
> Is there anything wrong with inserting heap_close(relation, NoLock)
> immediately before 'continue;' ?
We can do that if it turns out my analysis is wrong and RowShareLock
should indeed be grabbed on views as well as their underlying tables.
regards, tom lane