On Wed, Nov 23, 2011 at 2:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 22, 2011 at 7:25 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On Tue, Nov 22, 2011 at 11:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>> I think this is unsafe for shared catalogs.
>>> I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
>>> is safe to do so.
>> Ah, you mean access to shared catalogs using MVCC snapshots.
>
> Yeah. This change would have the disadvantage of disabling HOT
> cleanup for shared catalogs; I'm not sure whether that's a good
> decision.
No, it disables cleanup when being read. They are still VACUUMed normally.
Note that non-MVCC snapshots never did run HOT page-level cleanup, so
this hardly changes anything.
And it effects shared catalogs only, which are all low traffic anyway.
> But now that you mention it, something seems funky about the other bit
> you mention, too:
>
> + /* MVCC snapshots ignore other databases */
> + if (!allDbs &&
> + proc->databaseId != MyDatabaseId &&
> + proc->databaseId != 0) /* always include WalSender */
> + continue;
> +
>
> It doesn't make sense for the RecentGlobalXmin calculation to depend
> on whether or not the current snapshot is an MVCC snapshot, because
> RecentGlobalXmin is a global variable not related to any particular
> snapshot. I don't believe it's safe to assume that RecentGlobalXmin
> will only ever be used in conjunction with the most-recently-taken
> snapshot.
Why would that matter exactly? RecentGlobalXmin is used in 4 places
and this works with them all.
This changes the meaning of that variable from what it was previously,
but so what? It's backend local.
The huge benefit is that we clean up data in normal tables much better
than we did before in cases where people use multiple databases, which
is a common case.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
