Date: Tue, 20 Oct 2020 10:41:14 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: David Christensen <david@endpoint.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16676: SELECT ... FETCH FIRST ROW WITH TIES FOR UPDATE SKIP
 LOCKED locks rows it doesn't return
Message-ID: <20201020134114.GA11045@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7EA65D44-7C3A-4FC2-8DE1-1F59BBB10B37@endpoint.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Oct-19, David Christensen wrote:

> > On Oct 19, 2020, at 6:59 PM, David Christensen <david@endpoint.com> wrote:
> > 
> >> On Oct 19, 2020, at 6:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 
> >> ﻿David Christensen <david@endpoint.com> writes:
> >>> Proposed fix:
> >>> Reorder Limit/LockRows nodes to prevent locking extra tuples in FETCH FIRST WITH TIES
> >> 
> >> Isn't that going to break more cases than it fixes?
> > 
> > In the case of Limit, isn’t LockRows supposed to only lock the number of actual rows returned?
> > 
> > What are the scenarios that this might break and do you have any ideas for alternate fixes?
> 
> Will now that I think about it, if the LockRows node is responsible
> for skipping locked rows, etc then my proposed solution is clearly
> wrong. 

I tried your proposed patch yesterday and found that the second query
returns no rows rather than two rows, which is not an improvement.

> Maybe splitting LockRows into two nodes, one for locking and one for
> emitting unlocked nodes then interleaving Limit in between? (Or only
> doing something along these lines for this admittedly narrow use case.)

I was having a similar idea, but the main reason I don't think it's a
good fix is that we can't backpatch such a change to pg13.

I was thinking if it was possible to modify Limit so that it would
"peek" into its child node without "actually reading".  But that's not
possible in our executor implementation AFAIK -- you only have
ExecProcNode as an interface, there's no way to affect what happens
underneath.

Maybe an approach is to forbid a FOR UPDATE clause in a query that has a
LIMIT WITH TIES clause; so we'd force the user to write a subquery for
LIMIT WITH TIES and then apply FOR UPDATE to an outer query.  However,
I'm surprised to find that we have *two* LockRows nodes when doing that:

explain select * from (select * from queue order by batch_id fetch first 2 rows with ties) t for update skip locked;
                                       QUERY PLAN                                       
────────────────────────────────────────────────────────────────────────────────────────
 LockRows  (cost=55.20..55.27 rows=2 width=40)
   ->  Subquery Scan on t  (cost=55.20..55.25 rows=2 width=40)
         ->  Limit  (cost=55.20..55.23 rows=2 width=14)
               ->  LockRows  (cost=55.20..83.45 rows=2260 width=14)
                     ->  Sort  (cost=55.20..60.85 rows=2260 width=14)
                           Sort Key: queue.batch_id
                           ->  Seq Scan on queue  (cost=0.00..32.60 rows=2260 width=14)
(7 filas)

and of course, we still have the behavior where the third row is
skipped.