Re: The corresponding relminxid patch; try 1

Alvaro Herrera <alvherre@commandprompt.com> writes:
> This is the relminxid patch corresponding to the pg_ntclass patch I just
> posted.

That disable_heap_unfreeze thing seriously sucks.  How bad are the API
changes needed to pass that as a parameter instead of having a
very-dangerous global variable?

The comment at line 328ff in dbcommands.c seems misguided, which makes
me doubt the code too.  datfrozenxid and datvacuumxid should be
considered as indicating what XIDs appear inside the database, not what
is in its pg_database row.

The changes in vacuum.c are far too extensive to review meaningfully.
What did you do, and did it really need to touch so much code?

> The thing that bothers me most about this is that it turns LockRelation
> into an operation that needs to heap_fetch() from pg_ntclass in some
> cases, and possibly update it.

Have you done any profiling to see what that actually costs?

I think we could possibly dodge the work in the normal case if we are
willing to make VACUUM FREEZE take ExclusiveLock and send out a relation
cache inval on the relation.  Then, we can cache the pg_ntclass tuple in
relcache entries (discarding it on cache inval), and if the cached value
says it's not frozen then it's not frozen.  You couldn't trust the
cached value much further than that, but that would at least take the
fetch out of the normal path in LockRelation.  The trick here is the
problem that if VACUUM FREEZE fails after modifying pg_ntclass, its
relcache inval won't be sent out.

A bigger issue here is that I'm not sure what the locking protocol is
for pg_ntclass itself.  It looks like you're not consistently taking
a RowExclusiveLock when you update it.

BTW, I think you have the order of operations wrong in LockRelation;
should it not unfreeze only *after* obtaining lock?  Consider race
condition against relation drop for instance.

            regards, tom lane