> On further thought, btbuild is not that badly broken at the moment,
> because CREATE INDEX acquires ShareLock on the relation, so
> there can be no concurrent writers at the page level. Still, it
> seems like it'd be a good idea to do "LockBuffer(buffer,
BUFFER_LOCK_SHARE)"
> here, and probably also to invoke HeapTupleSatisfiesNow() via the
> HeapTupleSatisfies() macro so that infomask update is checked for.
> Vadim, what do you think?

Looks like there is no drawback in locking buffer so let's lock it.

Vadim

Tue, 3 Jul 2001 10:41:53 -0700 
"Mikheev, Vadim" <vmikheev@SECTORBASE.COM>
RE: Re: Buffer access rules, and a probable bug 
Okay, on to the next concern.  I've been thinking some more about the
restrictions needed to make the world safe for concurrent VACUUM.
I previously said:

> 5. To physically remove a tuple or compact free space on a page, one
> must hold a pin and an exclusive lock, *and* observe while holding the
> exclusive lock that the buffer's shared reference count is one (ie,
> no other backend holds a pin).  If these conditions are met then no other
> backend can perform a page scan until the exclusive lock is dropped, and
> no other backend can be holding a reference to an existing tuple that it
> might expect to examine again.  Note that another backend might pin the
> buffer (increment the refcount) while one is performing the cleanup, but
> it won't be able to to actually examine the page until it acquires shared
> or exclusive lock.

This is OK when considering a page in isolation, but it does not get the
job done when one is deleting related index tuples and heap tuples.  It
seems to me that there *must* be some cross-page coupling to make that
work safely.  Otherwise you could have this scenario:

1. Indexscanning process visits an index tuple, decides to access the
   corresponding heap tuple, drops its lock on the index buffer page.

2. VACUUMing process visits the index buffer page and marks the index
   tuple dead.  (It won't try to delete the tuple yet, since it sees
   the pin still held on the page by process #1, but it can acquire
   exclusive lock and mark the tuple dead anyway.)

3. VACUUMing process is the first to acquire pin and lock on the heap
   buffer page.  It sees no other pin, so it deletes the tuple.

4. Indexscanning process finally acquires pin and lock on the heap page,
   tries to access what is now a gone tuple.  Ooops.  (Even if we
   made that not an error condition, it could be worse: what if a
   third process already reused the line pointer for a new tuple?)

It does not help to postpone the actual cleaning of an index or heap
page until its own pin count drops to zero --- the problem here is that
an indexscanner has acquired a reference into a heap page from the
index, but does not yet hold a pin on the heap page to ensure that
the reference stays good.  So we can't just postpone the cleaning of
the index page till it has pin count zero, we have to make the related
heap page(s)' cleanup wait for that to happen too.

I can think of two ways of guaranteeing that this problem cannot happen.

One is for an indexscanning process to retain its shared lock on the
index page until it has acquired at least a pin on the heap page.
This is very bad for concurrency --- it means that we'd typically
be holding indexpage shared locks for the time needed to read in a
randomly-accessed disk page.  And it's very complicated, since we still
need all the other rules, plus the mechanism for postponing cleanups
until pin count goes to zero.  It'd cause considerable changes to the
index access method API, too.

The other is to forget about asynchronous cleaning, and instead have the
VACUUM process directly do the wait for pin count zero, then clean the
index page.  Then when it does the same for the heap page, we know for
sure there are no indexscanners in transit to the heap page.  This would
be logically a lot simpler, it seems to me.  Another advantage is that
we need only one WAL entry per cleaned page, not two (one for the
initial mark-dead step and one for the postponable compaction step),
and there's no need for an intermediate "gone but not forgotten" state
for index tuples.

We could implement this in pretty nearly the same way as the "mark for
cleanup" facility that you partially implemented awhile back:
essentially, the cleanup callback would send a signal or semaphore
increment to the waiting process, which would then try to acquire pin
and exclusive lock on the buffer.  If it succeeded in observing pin
count 1 with exclusive lock, it could proceed with cleanup, else loop
back and try again.  Eventually it'll get the lock.  (It might take
awhile, but for a background VACUUM I think that's OK.)

What I'm wondering is if you had any other intended use for "mark for
cleanup" than VACUUM.  The cheapest implementation would allow only
one process to be waiting for cleanup on a given buffer, which is OK
for VACUUM because we'll only allow one VACUUM at a time on a relation
anyway.  But if you had some other uses in mind, maybe the code needs
to support multiple waiters.

Comments?

			regards, tom lane

Tue, 03 Jul 2001 18:27:17 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: Buffer access rules, and a probable bug 
> -----Original Message-----
> From: Mikheev, Vadim [mailto:vmikheev@SECTORBASE.COM]
> 
> > On further thought, btbuild is not that badly broken at the moment,
> > because CREATE INDEX acquires ShareLock on the relation, so
> > there can be no concurrent writers at the page level. Still, it
> > seems like it'd be a good idea to do "LockBuffer(buffer,
> BUFFER_LOCK_SHARE)"
> > here, and probably also to invoke HeapTupleSatisfiesNow() via the
> > HeapTupleSatisfies() macro so that infomask update is checked for.
> > Vadim, what do you think?
> 
> Looks like there is no drawback in locking buffer so let's lock it.
> 

OK I would fix it.
As for HeapTupleSatisfies() there seems to be another choise to
let HeapTupleSatisfiesAny() be equivalent to HeapTupleSatisfiesNow()
other than always returning true.

Comments ?

regards,
Hiroshi Inoue 

Wed, 4 Jul 2001 19:23:22 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: Re: Buffer access rules, and a probable bug 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> As for HeapTupleSatisfies() there seems to be another choise to
> let HeapTupleSatisfiesAny() be equivalent to HeapTupleSatisfiesNow()
> other than always returning true.

Wouldn't that break the other uses of SnapshotAny?  I'm not sure
it's what nbtree.c wants, either, because then the heap_getnext
call wouldn't return recently-dead tuples at all.

			regards, tom lane

Wed, 04 Jul 2001 13:09:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: Buffer access rules, and a probable bug 
Tom Lane wrote:
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > As for HeapTupleSatisfies() there seems to be another choise to
> > let HeapTupleSatisfiesAny() be equivalent to HeapTupleSatisfiesNow()
> > other than always returning true.
> 
> Wouldn't that break the other uses of SnapshotAny? 

In theory no because HeapTupleSatisfies...() only touches
hint bits. What I mean is to implement a new function
HeapTupleSatisfiesAny() as

bool
HeapTupleSatisfiesAny(HeapTupleHeader tuple)
{
	HeapTupleSatisfiesNow(tuple);
	return true;
}
.

> I'm not sure
> it's what nbtree.c wants, either, because then the heap_getnext
> call wouldn't return recently-dead tuples at all.
> 

nbtree.c has to see all(including dead) tuples and judge
if the tuples are alive, dead or removable via unified
time qualification.

regards,
Hiroshi Inoue

Thu, 05 Jul 2001 08:43:24 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: Re: Buffer access rules, and a probable bug
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> What I mean is to implement a new function
> HeapTupleSatisfiesAny() as

> bool
> HeapTupleSatisfiesAny(HeapTupleHeader tuple)
> {
> 	HeapTupleSatisfiesNow(tuple);
> 	return true;
> }

Oh, I see: so that HeapTupleSatisfies would update the hint bits even
when called with snapshot = SnapShotAny.  Hmm.  This might be a good
idea on its own merits, but I don't think it simplifies nbtree.c at
all --- you'd still have to go through the full LockBuffer and hint
update procedure there.  (If the other transaction committed meanwhile,
the call from nbtree.c could try to update hint bits that hadn't been
updated during heap_fetch.)

BTW, I don't really think that this code belongs in nbtree.c at all.
If it lives there, then we need to duplicate the logic in each index
access method.  At some point we ought to fix the index build process
so that the loop that scans the source relation is outside the access-
method-specific code.

			regards, tom lane

Wed, 04 Jul 2001 21:04:59 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: Buffer access rules, and a probable bug 
> In theory no because HeapTupleSatisfies...() only touches
> hint bits. What I mean is to implement a new function
> HeapTupleSatisfiesAny() as

Can someone add comments to the Satisfies code to explain each
visibility function?  Some are documented and some are not.  Seems it
would be good to get this stuff in there for future coders.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Wed, 4 Jul 2001 23:16:18 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Re: Buffer access rules, and a probable bug
Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > What I mean is to implement a new function
> > HeapTupleSatisfiesAny() as
> 
> > bool
> > HeapTupleSatisfiesAny(HeapTupleHeader tuple)
> > {
> >       HeapTupleSatisfiesNow(tuple);
> >       return true;
> > }
> 
> Oh, I see: so that HeapTupleSatisfies would update the hint bits even
> when called with snapshot = SnapShotAny.  Hmm.  This might be a good
> idea on its own merits, but I don't think it simplifies nbtree.c at
> all --- you'd still have to go through the full LockBuffer and hint
> update procedure there.  (If the other transaction committed meanwhile,
> the call from nbtree.c could try to update hint bits that hadn't been
> updated during heap_fetch.)
> 

Dead(HEAP_XMAX_COMMITTED || HEAP_XMIN_INVALID) tuples never
revive. Live (not dead) tuples never die in Share Lock mode.
So I don't have to call HeapTupleSatisfies() again though I
seem to have to lock the buffer so as to see t_infomask and
t_xmax.

> BTW, I don't really think that this code belongs in nbtree.c at all.
> If it lives there, then we need to duplicate the logic in each index
> access method.  At some point we ought to fix the index build process
> so that the loop that scans the source relation is outside the access-
> method-specific code.
> 

Agreed. 

regards,
Hiroshi Inoue

Thu, 05 Jul 2001 13:45:13 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: Re: Buffer access rules, and a probable bug
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Dead(HEAP_XMAX_COMMITTED || HEAP_XMIN_INVALID) tuples never
> revive. Live (not dead) tuples never die in Share Lock mode.

Hmm ... so you're relying on the ShareLock to ensure that the state of
the tuple can't change between when heap_fetch checks it and when
nbtree looks at it.

Okay, but put in some comments documenting what this stuff is doing
and why.

			regards, tom lane

Thu, 05 Jul 2001 09:54:43 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: Buffer access rules, and a probable bug 