Re: Self-join elimination discards differing row lock wait policies

I wrote:
> Tender Wang <tndrwang(at)gmail(dot)com> writes:
>> The attached patch requires the row mark type, locking strength, and
>> wait policy to match before eliminating the self-join.
>> Thoughts?

> In some cases it might be possible to merge two "sufficiently similar"
> rowmark requirements, eg keep only the stronger lock type. I'm not
> sure that that wouldn't produce visible semantics changes, though.
> On the other hand, the code is already willing to merge an RTE with
> a RowMark with an RTE that has none, and I'm not convinced that that
> has no visible semantics effects either --- for example, if the one
> RowMark says "SKIP LOCKED", doesn't it represent a different set of
> rows than an RTE with no RowMark? Maybe we should tighten this even
> more and reject that case?

On closer consideration, there's no bug here.

1. We only support SJE on regular tables, not foreign tables.
Therefore, the requested lock strength is fully encoded in the
RowMarkType (see select_rowmark_type()), so checking that guarantees
that the strength field matches.

2. It doesn't matter whether the waitPolicy is the same, because
the behavior after SJE is isomorphic to a legal implementation
of the query without SJE. The kept waitPolicy determines what
actually happens for a given row, and it's mostly like what would
happen in a join query where the kept RTE scans a given row first:

LockWaitBlock: wait to acquire the lock. If we do that first,
and then the other RTE scans the row, it will be satisfied.

LockWaitSkip: skip the row. Even if the other RTE would have
acquired the lock, the row would not appear in the query result
since the equijoin would drop it. You could complain that
the other RTE had LockWaitError and would have thrown an error,
but it's legitimate for us to act as though we're doing this via
a nestloop and hence never scanned the other RTE for this row.
By the same argument, it's okay that we never acquired the lock,
even if the other RTE had LockWaitBlock.

LockWaitError: error if we don't get the lock. Does not matter
what the other RTE would have done when it got to the row.

3. By the same argument, my complaint above about two RTEs with
and without rowmarks is faulty. As long as we keep the rowmark,
dropping the RTE without rowmark is not problematic, because
each of the cases I just listed works the same if the second
RTE applies no row lock at all.

So I don't see anything that looks like a bug, although the
comments about this in analyzejoins.c are surely inadequate.

Having said that, there's still room for improvement here.
If we don't perform SJE because the lock strengths are different,
then the query's behavior depends on which order the planner
chooses to join the tables in. Which is not great, especially
since the user can't really control that, and there's a real risk
of deadlock failures due to attempted lock upgrade if we take the
weaker lock first. It would be better to perform SJE and make
sure we keep the stronger lock type, which generalizes what we're
doing now in the only-one-side-has-a-rowmark case. I think we
could also work out a rule for which waitPolicy to choose when
we do that, ensuring more consistent behavior than you get now.

regards, tom lane