Re: Reviewing freeze map code

On Fri, Jun 10, 2016 at 4:55 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> 3. vacuumlazy.c includes this code:
>>
>> if (heap_prepare_freeze_tuple(tuple.t_data, FreezeLimit,
>> MultiXactCutoff, &frozen[nfrozen]))
>> frozen[nfrozen++].offset = offnum;
>> else if (heap_tuple_needs_eventual_freeze(tuple.t_data))
>> all_frozen = false;
>>
>> That's wrong, because a "true" return value from
>> heap_prepare_freeze_tuple() means only that it has done *some*
>> freezing work on the tuple, not that it's done all of the freezing
>> work that will ever need to be done. So, if the tuple's xmin can be
>> frozen and is aborted but not older than vacuum_freeze_min_age, then
>> heap_prepare_freeze_tuple() won't free xmax, but the page will still
>> be marked all-frozen, which is bad. I think it normally won't matter
>> because the xmax will probably be hinted invalid anyway, since we just
>> pruned the page which should have set hint bits everywhere, but if
>> those hint bits were lost then we'd eventually end up with an
>> accessible xmax pointing off into space.
>
> Good catch. Also consider multixact freezing: if there is a
> long-running transaction which is a lock-only member of tuple's Xmax,
> and the multixact needs freezing because it's older than the multixact
> cutoff, we set the xmax to a new multixact which includes that old
> locker. See FreezeMultiXactId.
>
>> My first thought was to just delete the "else" but that would be bad
>> because we'd fail to set all-frozen immediately in a lot of cases
>> where we should. This needs a bit more thought than I have time to
>> give it right now.
>
> How about changing the return tuple of heap_prepare_freeze_tuple to
> a bitmap? Two flags: "Freeze [not] done" and "[No] more freezing
> needed"

Yes, I think something like that sounds about right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company