Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/09/30 5:04, Tom Lane wrote:
>> 3. There remain some cases where the RTE says RowExclusiveLock but
>> the executor calculation indicates we only need AccessShareLock.
>> AFAICT, this happens only when we have a DO ALSO rule that results
>> in an added query that merely scans the target table.
> I've seen something like that happen for ON CONFLICT's excluded
> pseudo-relation RTE too, because the executor deems only the RTE fetched
> with result relation RT index to require RowExclusiveLock.
OK, I had not carefully inspected every case.
> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
> based on inspecting the query tree to cross-check with rellockmode? Why
> not just use rellockmode for locking?
Right, that's exactly where we want to end up. This intermediate state of
the patch is just an attempt to verify that we understand when and how
relying on rellockmode will change the behavior.
> Also, are the discrepancies like this to be
> considered bugs of the existing logic?
Mmm ... hard to say. In the DO ALSO case, it's possible that query
execution would take AccessShareLock and then RowExclusiveLock, which
at least in principle creates a lock-upgrade deadlock hazard. So I
think standardizing on taking the rellockmode will be an improvement,
but I don't know that I'd call the existing behavior a bug; I certainly
wouldn't risk trying to back-patch a change for it.
In the ROW_MARK_COPY case, it's conceivable that with the existing code
query re-execution would take only AccessShareLock on a FOR UPDATE target
table, where the parser had originally taken RowShareLock. Again, it
seems like making the behavior more consistent is an improvement, but
not something we'd try to back-patch.
regards, tom lane