Re: Reviewing freeze map code - Mailing list pgsql-hackers

On Wed, Jun 15, 2016 at 11:43 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Jun 15, 2016 at 12:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Jun 14, 2016 at 8:11 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>>> I noticed that the tuples that it reported were always offset 1 in a
>>>>> page, and that the page always had a maxoff over a couple of hundred,
>>>>> and that we called record_corrupt_item because VM_ALL_VISIBLE returned
>>>>> true but HeapTupleSatisfiesVacuum on the first tuple returned
>>>>> HEAPTUPLE_DELETE_IN_PROGRESS instead of the expected HEAPTUPLE_LIVE.
>>>>> It did that because HEAP_XMAX_COMMITTED was not set and
>>>>> TransactionIdIsInProgress returned true for xmax.
>>>>
>>>> So this seems like it might be a visibility map bug rather than a bug
>>>> in the test code, but I'm not completely sure of that.  How was it
>>>> legitimate to mark the page as all-visible if a tuple on the page
>>>> still had a live xmax?  If xmax is live and not just a locker then the
>>>> tuple is not visible to the transaction that wrote xmax, at least.
>>>
>>> Ah, wait a minute.  I see how this could happen.  Hang on, let me
>>> update the pg_visibility patch.
>>
>> The problem should be fixed in the attached revision of
>> pg_check_visible.  I think what happened is:
>>
>> 1. pg_check_visible computed an OldestXmin.
>> 2. Some transaction committed.
>> 3. VACUUM computed a newer OldestXmin and marked a page all-visible with it.
>> 4. pg_check_visible then used its older OldestXmin to check the
>> visibility of tuples on that page, and saw delete-in-progress as a
>> result.
>>
>> I added a guard against a similar scenario involving xmin in the last
>> version of this patch, but forgot that we need to protect xmax in the
>> same way.  With this version of the patch, I can no longer get any
>> TIDs to pop up out of pg_check_visible in my testing.  (I haven't run
>> your test script for lack of the proper Python environment...)
>
> I can still reproduce the problem with this new patch.  What I see is
> that the OldestXmin, the new RecomputedOldestXmin and the tuple's xmax
> are all the same.

I spent some time chasing down the exact circumstances.  I suspect
that there may be an interlocking problem in heap_update.  Using the
line numbers from cae1c788 [1], I see the following interaction
between the VACUUM, UPDATE and SELECT (pg_check_visible) backends, all
in reference to the same block number:
 [VACUUM] sets all visible bit
 [UPDATE] heapam.c:3931 HeapTupleHeaderSetXmax(oldtup.t_data, xmax_old_tuple); [UPDATE] heapam.c:3938
LockBuffer(buffer,BUFFER_LOCK_UNLOCK);
 
 [SELECT] LockBuffer(buffer, BUFFER_LOCK_SHARE); [SELECT] observes VM_ALL_VISIBLE as true [SELECT] observes tuple in
HEAPTUPLE_DELETE_IN_PROGRESSstate [SELECT] barfs
 
 [UPDATE] heapam.c:4116 visibilitymap_clear(...)

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/heap/heapam.c;hb=cae1c788b9b43887e4a4fa51a11c3a8ffa334939

-- 
Thomas Munro
http://www.enterprisedb.com