Re: Potential GIN vacuum bug - Mailing list pgsql-hackers

From Jeff Janes
Subject Re: Potential GIN vacuum bug
Date
Msg-id CAMkU=1zTC_EWFeQdGjvRmL39AP2fpKM426MjcCOMxAw_FFSHWA@mail.gmail.com
Whole thread Raw
In response to Potential GIN vacuum bug  (Jeff Janes <jeff.janes@gmail.com>)
Responses Re: Potential GIN vacuum bug  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: Potential GIN vacuum bug  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
On Mon, Aug 17, 2015 at 6:23 AM, Jeff Janes <jeff.janes@gmail.com> wrote:


On Aug 16, 2015 11:49 PM, "Heikki Linnakangas" <hlinnaka@iki.fi> wrote:
>
> On 08/16/2015 12:58 AM, Jeff Janes wrote:
>>
>> When ginbulkdelete gets called for the first time in a  VACUUM(i.e. stats
>> == NULL), one of the first things it does is call ginInsertCleanup to get
>> rid of the pending list.  It does this in lieu of vacuuming the pending
>> list.
>>
>> This is important because if there are any dead tids still in the Pending
>> list, someone else could come along during the vacuum and post the dead
>> tids into a part of the index that VACUUM has already passed over.
>>
>> The potential bug is that ginInsertCleanup exits early (ginfast.c lines
>> 796, 860, 898) if it detects that someone else is cleaning up the pending
>> list, without waiting for that someone else to finish the job.
>>
>> Isn't this a problem?
>
>
> Yep, I think you're right. When that code runs as part of VACUUM, it should not give up like that.
>
> Hmm, I see other race conditions in that code too. Even if VACUUM wins the race you spotted, and performs all the insertions, reaches the end of the pending items list, and deletes the pending list pages, it's possible that another backend started earlier, and is still processing the same items from the pending items list. It will add them to the tree, and after it's finished with that it will see that the pending list page was already deleted, and bail out. But if there is a dead tuple in the pending items list, you have trouble. The other backend will re-insert it, and that might happen after VACUUM had already removed it from the tree.

Could the right to clean the pending list be represented by a self-conflicting heavy weight lock on the index?  Vacuum could block on it, while user back-ends could try to get it conditionally and just give up on the cleanup if it is not available.

>
> Also, ginInsertCleanup() seems to assume that if another backend has just finished cleaning up the same page, it will see the page marked as deleted. But what if the page is not only marked as deleted, but also reused for something else already?

Yeah.  Which is possible but pretty unlikely now; but would be far more likely if we added these page to the fsm more aggressively.


The attached patch takes a ShareUpdateExclusiveLock lock on the index in order to clean the pending list.  

This fixes the problem I had been seeing when testing https://commitfest.postgresql.org/6/322/ (which was probably caused by the deleted page situation you mentioned, not by tids getting revived issue I originally brought up.)

User backends attempt to take the lock conditionally, because otherwise they would cause an autovacuum already holding the lock to cancel itself, which seems quite bad.

Not that this a substantial behavior change, in that with this code the user backends which find the list already being cleaned will just add to the end of the pending list and go about their business.  So if things are added to the list faster than they can be cleaned up, the size of the pending list can increase without bound.

Under the existing code each concurrent user backend will try to clean the pending list at the same time.  The work doesn't parallelize, so doing this is just burns CPU (and possibly consuming up to maintenance_work_mem  for *each* backend) but it does server to throttle the insertion rate and so keep the list from growing without bound.

This is just a proof-of-concept patch, because I don't know if this approach is the right approach.

One potential problem is how it will interact with "create index concurrently".

Cheers,

Jeff

Attachment

pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: Re: why can the isolation tester handle only one waiting process?
Next
From: Robert Haas
Date:
Subject: Re: More WITH