From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Reviewing freeze map code |
Date: | 2016-06-10 20:55:24 |
Message-ID: | 20160610205524.GA260270@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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"
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-06-10 22:34:24 | Re: Perf Benchmarking and regression. |
Previous Message | Robert Haas | 2016-06-10 20:14:03 | Re: [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116 |