Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> To modify a page:
>> If PD_ALL_VISIBLE flag is set, the bit in the visibility map is cleared
>> first. The heap page is kept pinned, but not locked, while the
>> visibility map is updated. We want to avoid holding a lock across I/O,
>> even though the visibility map is likely to stay in cache. After the
>> visibility map has been updated, the page is exclusively locked and
>> modified as usual, and PD_ALL_VISIBLE flag is cleared before releasing
>> the lock.
>
> So after having determined that you will modify a page, you release the
> ex lock on the buffer and then try to regain it later? Seems like a
> really bad idea from here. What if it's no longer possible to do the
> modification you intended?
In case of insert/update, you have to find a new target page. I put the
logic in RelationGetBufferForTuple(). In case of delete and update (old
page), the flag is checked and bit cleared just after pinning the
buffer, before doing anything else. (I note that that's not actually
what the patch is doing for heap_update, will fix..)
If we give up on the strict requirement that the bit in the visibility
map has to be cleared if the PD_ALL_VISIBLE flag on the page is not set,
then we could just update the visibility map after releasing the locks
on the heap pages. I think I'll do that for now, for simplicity.
>> To set the PD_ALL_VISIBLE flag, you must hold an exclusive lock on the
>> page, while you observe that all tuples on the page are visible to everyone.
>
> That doesn't sound too good from a concurrency standpoint...
Well, no, but it's only done in VACUUM. And pruning. I implemented it as
a new loop that call HeapTupleSatisfiesVacuum on each tuple, and
checking that xmin is old enough for live tuples, but come to think of
it, we're already calling HeapTupleSatisfiesVacuum for every tuple on
the page during VACUUM, so it should be possible to piggyback on that by
restructuring the code.
>> That's how the patch works right now. However, there's a small
>> performance problem with the current approach: setting the
>> PD_ALL_VISIBLE flag must be WAL-logged. Otherwise, this could happen:
>
> I'm more concerned about *clearing* the bit being WAL-logged. That's
> necessary for correctness.
Yes, clearing the PD_ALL_VISIBLE flag always needs to be WAL-logged.
There's a new boolean field in xl_heap_insert/update/delete records
indicating if the operation cleared the flag. On replay, if the flag was
cleared, the bit in the visibility map is also cleared.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com