On Mon, Mar 7, 2016 at 12:41 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Attached latest version optimisation patch.
> I'm still consider regarding pg_upgrade regression test code, so I
> will submit that patch later.
I just spent some time looking at this and I'm a bit worried about the
following (existing) comment in vacuumlazy.c:
* Note: The value returned by visibilitymap_get_status could be slightly
* out-of-date, since we make this test before reading the corresponding
* heap page or locking the buffer. This is OK. If we mistakenly think
* that the page is all-visible when in fact the flag's just been cleared,
* we might fail to vacuum the page. But it's OK to skip pages when
* scan_all is not set, so no great harm done; the next vacuum will find
* them. If we make the reverse mistake and vacuum a page unnecessarily,
* it'll just be a no-op.
The patch makes some attempt to update the comment mechanically, but
that's not nearly enough. That comment is explaining that you *can't*
rely on the visibility map to tell you *for sure* that a page does not
require vacuuming. For current uses, that's OK, because if we miss a
page we'll pick it up later. But now we want to skip vacuuming pages
for relfrozenxid/relminmxid advancement, that rationale doesn't apply.
Missing pages that need to be frozen and advancing relfrozenxid anyway
would be _bad_.
However, after some further thought, I think we might actually be OK.
If a page goes from all-frozen to not-all-frozen while VACUUM is
running, any new XID added to the page must be newer than the
oldestXmin value computed by vacuum_set_xid_limits(), so it won't
affect the value to which we can safely set relfrozenxid. Similarly,
any MXID added to the page will be newer than GetOldestMultiXactId(),
so setting relminmxid is still safe for similar reasons.
I'd appreciate it if any other senior hackers could review that chain
of reasoning. It would be really bad to get this wrong.
On another note, I didn't really like the way you updated the
documentation. "eager freezing" doesn't seem like a great term to me,
and I think your changes were a little too localized. Here's a draft
alternative where I used the term "aggressive vacuum" to describe
freezing all of the pages except for those already known to be
all-frozen. Thoughts?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company