Re: Missing CONCURRENT VACUUM (Was: Release notes for - Mailing list pgsql-hackers

On T, 2005-08-16 at 18:26 -0400, Tom Lane wrote:
> Hannu Krosing <hannu@skype.net> writes:
> > Once more: 
> > I would like to get at least some answer, why my patch for enabling
> > concurrent VACUUM was left out from 8.1.
> 
> You did not respond to this:
> http://archives.postgresql.org/pgsql-patches/2005-08/msg00238.php

Somehow this did not reach me :(

I'll answer this here:

> Bruce Momjian <pgman ( at ) candle ( dot ) pha ( dot ) pa ( dot ) us> writes:
> >> Is there any particular reason for not putting it in 8.1 ?
> 
> > I thought there was still uncertainty about the patch.  Is there?
> 
> Considerable uncertainty, in my mind.  What we've got here is some
> pretty fundamental hacking on the transaction visibility logic, and
> neither Hannu nor anyone else has produced a convincing argument
> that it's correct.  "It hasn't failed yet in my usage" isn't enough
> to give me a good feeling about it. 

Agreed.

>  Some specific concerns:
> 
> * Given that VACUUM ANALYZE does create new output tuples stamped with
> its xid, I'm unclear on what happens in pg_statistic with this code in
> place.  

Actually any VACUUM, not only VACUUM ANALYSE, updates pg_class at the end.
That's why I exclude only one of the transactions of the VACUUM command, and that 
transaction does not create any new tuples, it only removes old ones.

> It seems entirely possible that someone might conclude the
> analyze tuples are from a crashed transaction and mark them invalid
> before the analyze can commit (notice TransactionIdIsInProgress does not
> bother looking in PGPROC when the tuple xmin is less than RecentXmin).

Once more, only 2nd transaction of LAZY VACUUM is affected, and that one does 
only (heap scan + clean indexes + clean heap) and _only_ removes old tuples.

> * If the vacuum xact is older than what others think is the global xmin,
> it could have problems with other vacuums removing tuples it should
> still be able to see (presumably only in the system catalogs, so maybe
> this isn't an issue, but I'm unsure). 

The cleanup transaction does no lookups in system catalogs.

> A related scenario that I don't
> think can be dismissed is someone truncating off part of pg_subtrans or
> pg_multixact that the vacuum still needs.

In my patch I specifically exclude TruncateSUBTRANS from using the
inVacuum flag

At the time I originally submitted my patch, GetOldestXmin was only used
in VACUUM and CREATE INDEX, others had other means of getting oldest
Xmin, and these were not affected by my patch. When I reworked it before
last submit, i changed the only nwe use (in xlog.c, line 5165) to use a
new version of GetOldestXmin with an extra flag tu tell it to NOT exlude
transactions running vacuum.

The transaction running the heap scan/cleanup part of the vacuum command
only sets a new isVacuum flag, and this is only used by GetOldestXmin
functions. Other means of getting OldestXmin still get exactly the old
behaviour. GetOldestXmin() does exclude xmin's with isVacuum set only
when called by other VACUUMS. this is controlled by the (new) second
argument of GetOldestXmin().

So I think that both your concerns expressed here are _already_
addressed by the latest patch in:

http://archives.postgresql.org/pgsql-patches/2005-07/msg00086.php

Please check the actual patch and advise if anything is still missing.

-- 
Hannu Krosing <hannu@skype.net>