Re: The corresponding relminxid patch; try 1
От | Alvaro Herrera |
---|---|
Тема | Re: The corresponding relminxid patch; try 1 |
Дата | |
Msg-id | 20060612011612.GA25809@alvh.no-ip.org обсуждение исходный текст |
Ответ на | Re: The corresponding relminxid patch; try 1 (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | pgsql-patches |
Tom Lane wrote: > 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? Let's see -- I would need to fix all callers of LockRelation, and the problem I found in an earlier version of the patch (before the invention of the non-transaction stuff) was that some callers needed to pass that information several levels down. It's possible that this was an artifact of the fact that it was using the relcache. I'll experiment with changing stuff so that the global variable is not needed. > 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. No, actually it's correct. The point of that comment is that if the source database is frozen, then all XIDs appearing inside both databases (source and newly created) are frozen. So it's possible that the row in pg_database is frozen as well. But because we are creating a new row in pg_database, it's not really frozen any longer; so we change the pg_database fields in the new row to match. Actually, pg_database is going to be unfrozen right after that code, because it's opened with RowExclusiveLock shortly after, precisely to insert that new row we are inserting. So maybe this is not an issue. > 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? Yeah, they are extensive. I did several things there: get rid of a couple of global variables that no longer need to be global; remove the return value from vacuum_rel, since it's no longer needed (it's used to determine whether we can truncate pg_clog, but now we can do it regardless of whether this particular vacuuming took place or not); I changed some variables from the old "frozenXid" name to "minXid"; I put in a hack to make VACUUM FREEZE take a stronger lock; changed the API of vacuum_rel so that instead of taking a specific acceptable relkind, it receives whether TOAST is acceptable or not; and added the code needed to keep track of the earliest Xid found in code. But by far the most extensive change is the melding of vac_update_dbstats into vac_update_dbminxid, and the update of vac_update_relstats to cope with pg_ntclass. Maybe I should take a stab at making incremental patches instead of doing everything in one patch. This way it would be easier to review for correctness (and I'd be more confident that it is actually correct as well). > > 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? No, but I guess it must be expensive. While relminxid was still in the relcache, it was cheap because we checked the value before having to actually do anything else. That's why I was suggesting having a separate cache for non-transactional stuff. > 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. Well, one problem is that if enough transactions pass after the last update to a table, a normal VACUUM (i.e. not FREEZE) could mark a table as frozen as well; marking frozen is not an exclusive property of VACUUM FREEZE. > 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. Hmm, good point. I think it was OK (and actually, it was required) while relminxid was still on pg_class; or rather, there was a race condition the other way around, so it was required to unfreeze the table _before_ obtaining the lock. But it's certainly wrong now. I'll work on pg_class_nt and I'll be back to this soon. Thanks for the review.
В списке pgsql-patches по дате отправления: