Re: SKIP LOCKED DATA (work in progress) - Mailing list pgsql-hackers

Hi Thomas,

Apologies for taking this long to get to reviewing this, I'd gotten a bit side tracked with my own patches during this commitfest.

I'm really glad to see this patch is back again. I think it will be very useful for processing queues. I could have made good use of it in my last work, using it for sending unsent emails which were "queued" up in a table in the database.

I've so far read over the patch and done only some brief tests of the functionality.

Here's what I've picked up on so far:

* In heap_lock_tuple() there's a few places where you test the wait_policy, but you're not always doing this in the same order. The previous code did if (nolock) first each time, but since there's now 3 values I think if (wait_policy == LockWaitError) should be first each time as likely this is the most common case.

* The following small whitespace change can be removed in gram.y:

@@ -119,7 +119,6 @@ typedef struct PrivTarget

 #define CAS_NOT_VALID 0x10

 #define CAS_NO_INHERIT 0x20

 

-

 #define parser_yyerror(msg)  scanner_yyerror(msg, yyscanner)

 #define parser_errposition(pos)  scanner_errposition(pos, yyscanner)  

 

I'm not quite sure I understand this yet, but I'm guessing the original code was there so that something like:

So it seems that with the patch as you've defined in by the order of the enum values in LockClauseWaitPolicy that SKIP LOCKED overrides FOR UPDATE. I'm wondering if this is dangerous.

Should the following really skip locked tuples?

SELECT * FROM (SELECT * FROM a FOR UPDATE SKIP LOCKED) a FOR UPDATE LIMIT 1;

But on the other hand perhaps I've missed a discussion on this, if so then I think the following comment should be updated to explain it all:

I'm also wondering about this block of code in general:

Just after this code heap_lock_tuple() is called, and if that returns HeapTupleWouldBlock, the code does a goto lnext, which then will repeat that whole block again. I'm wondering why there's 2 enums that are for the same thing? if you just had 1 then you'd not need this block of code at all, you could just pass erm->waitPolicy to heap_lock_tuple().

I have noticed the /* TODO -- work out what needs to be released here */ comments in head_lock_tuple(), and perhaps the lack of cleaning up is what is causing the following:

create table skiptest (id int primary key); insert into skiptest (id) select x.x from generate_series(1,1000000) x(x);

Session 1: begin work; select * from skiptest for update limit 999999;

Session 2: select * from skiptest for update skip locked limit 1;

WARNING:  out of shared memory

ERROR:  out of shared memory

HINT:  You might need to increase max_locks_per_transaction.

Yet if I do:

session 1: begin work; select * from skiptest where id > 1 for update;

session 2:  select * from skiptest for update skip locked limit 1;

That test makes me think your todo comments are in the right place, something is missing there for sure!

* plays about with patch for a bit *

I don't quite understand how heap_lock_tuple works, as if I debug session 2 from the above set of queries the first call to ConditionalLockTupleTuplock() (heapam.c line 4236) succeeds where I'd have thought it would fail, since session 1 should be locking this tuple? Later at line 4527 on the line if (!ConditionalXactLockTableWait(xwait)), it fails to grab the lock and returns HeapTupleWouldBlock. The above query seems to work ok if I just apply the following patch:

Running the test query again, I get:

Would you also be able to supply a rebased patch with current master? The applying the patch is a bit noisy and the isolation tests part does not seem to apply at all now, so I've not managed to look at those yet.

I've still got more to look at, but hopefully this will get things moving again.

Regards

David Rowley