Thread: "page is not marked all-visible" warning in regression tests

"page is not marked all-visible" warning in regression tests

From
Tom Lane
Date:
I got this last night in a perfectly standard build of HEAD:

*** /home/tgl/pgsql/src/test/regress/expected/sanity_check.out    Thu Jan 12 14:06:14 2012
--- /home/tgl/pgsql/src/test/regress/results/sanity_check.out    Mon Jun  4 20:28:39 2012
***************
*** 1,4 ****
--- 1,5 ---- VACUUM;
+ WARNING:  page is not marked all-visible but visibility map bit is set in relation "pg_db_role_setting" page 0 -- --
sanitycheck, if we don't have indices the test will take years to -- complete.  But skip TOAST relations (since they
willhave varying
 

======================================================================

I didn't manage to reproduce it in a few tries, and I don't recall
having seen the like reported from the buildfarm either.  So there's
some low-probability bug in there ...
        regards, tom lane


Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Tuesday, June 05, 2012 03:32:08 PM Tom Lane wrote:
> I got this last night in a perfectly standard build of HEAD:
> 
> *** /home/tgl/pgsql/src/test/regress/expected/sanity_check.out    Thu Jan 12
> 14:06:14 2012 ---
> /home/tgl/pgsql/src/test/regress/results/sanity_check.out    Mon Jun  4
> 20:28:39 2012 ***************
> *** 1,4 ****
> --- 1,5 ----
>   VACUUM;
> + WARNING:  page is not marked all-visible but visibility map bit is set in
> relation "pg_db_role_setting" page 0 --
>   -- sanity check, if we don't have indices the test will take years to
>   -- complete.  But skip TOAST relations (since they will have varying
> 
> ======================================================================
> 
> I didn't manage to reproduce it in a few tries, and I don't recall
> having seen the like reported from the buildfarm either.  So there's
> some low-probability bug in there ...
I have seen that twice just yesterday. Couldn't reproduce it so far.
Workload was (pretty exactly):

initdb
postgres -c fsync=off
pgbench -i -s 100
CREATE TABLE data(id serial primary key, data int);
ALTER SEQUENCE data_id_seq INCREMENT 2;
VACUUM FREEZE;
normal shutdown
postgres -c fsync=on
pgbench -c 20 -j 20 -T 100
WARNING: ... pg_depend ...
WARNING: ... can't remember ...

Greetings,

Andres


Re: "page is not marked all-visible" warning in regression tests

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On Tuesday, June 05, 2012 03:32:08 PM Tom Lane wrote:
>> I got this last night in a perfectly standard build of HEAD:
>> + WARNING:  page is not marked all-visible but visibility map bit is set in
>> relation "pg_db_role_setting" page 0 --

> I have seen that twice just yesterday. Couldn't reproduce it so far.
> Workload was (pretty exactly):

> initdb
> postgres -c fsync=off
> pgbench -i -s 100
> CREATE TABLE data(id serial primary key, data int);
> ALTER SEQUENCE data_id_seq INCREMENT 2;
> VACUUM FREEZE;
> normal shutdown
> postgres -c fsync=on
> pgbench -c 20 -j 20 -T 100
> WARNING: ... pg_depend ...
> WARNING: ... can't remember ...

Hmm ... from memory, what I did was

configure/build/install from a fresh pull
initdb
start postmaster, fsync off
make installcheck
stop postmaster
apply Hanada-san's json patch, replace postgres executable
start postmaster, fsync off
make installcheck

and it was the second of these runs that failed.  Could we be missing
flushing some blocks out to disk at shutdown?  Maybe fsync off is a
contributing factor?
        regards, tom lane


Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Tuesday, June 05, 2012 04:18:44 PM Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On Tuesday, June 05, 2012 03:32:08 PM Tom Lane wrote:
> >> I got this last night in a perfectly standard build of HEAD:
> >> + WARNING:  page is not marked all-visible but visibility map bit is set
> >> in relation "pg_db_role_setting" page 0 --
> > 
> > I have seen that twice just yesterday. Couldn't reproduce it so far.
> > Workload was (pretty exactly):
> > 
> > initdb
> > postgres -c fsync=off
> > pgbench -i -s 100
> > CREATE TABLE data(id serial primary key, data int);
> > ALTER SEQUENCE data_id_seq INCREMENT 2;
> > VACUUM FREEZE;
> > normal shutdown
> > postgres -c fsync=on
> > pgbench -c 20 -j 20 -T 100
> > WARNING: ... pg_depend ...
> > WARNING: ... can't remember ...
> 
> Hmm ... from memory, what I did was
> 
> configure/build/install from a fresh pull
> initdb
> start postmaster, fsync off
> make installcheck
> stop postmaster
> apply Hanada-san's json patch, replace postgres executable
> start postmaster, fsync off
> make installcheck
> 
> and it was the second of these runs that failed.  Could we be missing
> flushing some blocks out to disk at shutdown?  Maybe fsync off is a
> contributing factor?
On a cursory lock it might just be a race condition in 
vacuumlazy.c:lazy_scan_heap. If scan_all is set, which it has to be for the 
warning to be visible, all_visible_according_to_vm is determined before we 
loop over all blocks. At the point where one specific heap block is actually 
read and locked that knowledge might be completely outdated by any concurrent 
backend. Am I missing something?

I have to say the whole visibilitymap correctness and crash-safety seems to be 
quite under documented, especially as it seems to be somewhat intricate (to 
me). E.g. not having any note why visibilitymap_test doesn't need locking. (I 
guess the theory is that a 1 byte read will always be consistent. But how does 
that ensure other backends see an up2date value?).

Andres

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Wed, Jun 6, 2012 at 1:46 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On a cursory lock it might just be a race condition in
> vacuumlazy.c:lazy_scan_heap. If scan_all is set, which it has to be for the
> warning to be visible, all_visible_according_to_vm is determined before we
> loop over all blocks. At the point where one specific heap block is actually
> read and locked that knowledge might be completely outdated by any concurrent
> backend. Am I missing something?

No, I think you're right.  I think that warning is bogus.  I added it
in place of some older warning which no longer made sense, but I think
this one doesn't make sense either.

> I have to say the whole visibilitymap correctness and crash-safety seems to be
> quite under documented, especially as it seems to be somewhat intricate (to
> me). E.g. not having any note why visibilitymap_test doesn't need locking. (I
> guess the theory is that a 1 byte read will always be consistent. But how does
> that ensure other backends see an up2date value?).

It's definitely intricate, and it's very possible that we should have
some more documentation.  I am not sure exactly what and where, but
feel free to suggest something.

visibilitymap_test() does have a comment saying that:
       /*        * We don't need to lock the page, as we're only looking at a
single bit.        */

But that's a bit unsatisfying, because, as you say, it doesn't address
the question of memory-ordering issues.  I think that there's no
situation in which it causes a problem to see the visibility map bit
as unset when in reality it has just recently been set by some other
back-end.  It would be bad if someone did something like:

if (visibilitymap_test(...))   visibilitymap_clear();

...because then memory-ordering issues could cause us to accidentally
fail to clear the bit.   No one should be doing that, though; the
relevant locking and conditional logic is built directly into
visibilitymap_clear().

On the flip side, if someone sees the visibility map bit as set when
it's actually just been cleared, that could cause a problem - most
seriously, index-only scans could return wrong answers.  For that to
happen, someone would have to insert a heap tuple onto a previously
all-visible page, clearing the visibility map bit, and then insert an
index tuple; concurrently, some other backend would need to see the
index tuple but not the fact that the visibility map bit had been
cleared.  I don't think that can happen: after inserting the heap
tuple, the inserting backend would release buffer content lock, which
acts as a full memory barrier; before reading the index tuple, the
index-only-scanning backend would have to take the content lock on the
index buffer, which also acts as a full memory barrier.  So the
inserter can't do the writes out of order, and the index-only-scanner
can't do the reads out of order, so I think it's safe.... but we
probably do need to explain that somewhere.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Wednesday, June 06, 2012 08:19:15 PM Robert Haas wrote:
> On Wed, Jun 6, 2012 at 1:46 PM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > On a cursory lock it might just be a race condition in
> > vacuumlazy.c:lazy_scan_heap. If scan_all is set, which it has to be for
> > the warning to be visible, all_visible_according_to_vm is determined
> > before we loop over all blocks. At the point where one specific heap
> > block is actually read and locked that knowledge might be completely
> > outdated by any concurrent backend. Am I missing something?
> 
> No, I think you're right.  I think that warning is bogus.  I added it
> in place of some older warning which no longer made sense, but I think
> this one doesn't make sense either.
Agreed.

It might be interesting to recheck the visibility and warn if its wrong. That 
should be infrequent enough to bearable and it does check for an actually 
dangerous case in a new code path.

> > I have to say the whole visibilitymap correctness and crash-safety seems
> > to be quite under documented, especially as it seems to be somewhat
> > intricate (to me). E.g. not having any note why visibilitymap_test
> > doesn't need locking. (I guess the theory is that a 1 byte read will
> > always be consistent. But how does that ensure other backends see an
> > up2date value?).
> 
> It's definitely intricate, and it's very possible that we should have
> some more documentation.  I am not sure exactly what and where, but
> feel free to suggest something.
I think some addition to the LOCKING part of visibilitymap.c's header 
explaining some of what you wrote in your email might be a good start.
I would also suggest explictly mentioning that its ok to have the 
visibilitymap and the page disagreeing about the visibility if its the 
visibility map that think that the page contains invisible data but not the 
other way round (I think that can currently happen).

visibilitymap_test() should explain that its results can be outdated if youre 
not holding a buffer lock.

> visibilitymap_test() does have a comment saying that:
> 
>         /*
>          * We don't need to lock the page, as we're only looking at a
> single bit.
>          */
Oh. I conveniently skipped that comment in my brain ;)

> But that's a bit unsatisfying, because, as you say, it doesn't address
> the question of memory-ordering issues.  I think that there's no
> situation in which it causes a problem to see the visibility map bit
> as unset when in reality it has just recently been set by some other
> back-end.  It would be bad if someone did something like:
> 
> if (visibilitymap_test(...))
>     visibilitymap_clear();
> 
> ...because then memory-ordering issues could cause us to accidentally
> fail to clear the bit.   No one should be doing that, though; the
> relevant locking and conditional logic is built directly into
> visibilitymap_clear().
Then _test should document that... I don't think its impossible that we will 
grow more uses of the visibilitymap logic.

> On the flip side, if someone sees the visibility map bit as set when
> it's actually just been cleared, that could cause a problem - most
> seriously, index-only scans could return wrong answers.  For that to
> happen, someone would have to insert a heap tuple onto a previously
> all-visible page, clearing the visibility map bit, and then insert an
> index tuple; concurrently, some other backend would need to see the
> index tuple but not the fact that the visibility map bit had been
> cleared.  I don't think that can happen: after inserting the heap
> tuple, the inserting backend would release buffer content lock, which
> acts as a full memory barrier; before reading the index tuple, the
> index-only-scanning backend would have to take the content lock on the
> index buffer, which also acts as a full memory barrier.  So the
> inserter can't do the writes out of order, and the index-only-scanner
> can't do the reads out of order, so I think it's safe.... but we
> probably do need to explain that somewhere.
Hm. For a short while I thought there would be an issue with heap_delete and 
IOS because the deleting transaction can commit without any barriers happening 
on the IOS side. But that only seems to be possible with non MVCC snapshots 
which are currently not allowed with index only scans.

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Wed, Jun 6, 2012 at 3:07 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hm. For a short while I thought there would be an issue with heap_delete and
> IOS because the deleting transaction can commit without any barriers happening
> on the IOS side. But that only seems to be possible with non MVCC snapshots
> which are currently not allowed with index only scans.

Well, one, commits are irrelevant; the page ceases to be all-visible
as soon as the delete happens.  And two, you can't do a delete or a
commit without a memory barrier - every LWLockAcquire() or
LWLockRelease() is a full barrier, so any operation that requires a
buffer content lock is both preceded and followed by a full barrier.

Proposed patch attached.  This adds some more comments in various
places, and implements your suggestion of retesting the visibility-map
bit when we detect a possible mismatch with the page-level bit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Thursday, June 07, 2012 03:20:50 PM Robert Haas wrote:
> On Wed, Jun 6, 2012 at 3:07 PM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > Hm. For a short while I thought there would be an issue with heap_delete
> > and IOS because the deleting transaction can commit without any barriers
> > happening on the IOS side. But that only seems to be possible with non
> > MVCC snapshots which are currently not allowed with index only scans.
> Well, one, commits are irrelevant; the page ceases to be all-visible
> as soon as the delete happens.
Its not irrelevant for the code as it stands if non-mvcc snapshots were 
allowed. Unless I miss something, even disregarding memory ordering issues, 
there is no interlocking providing protection against the index doing the 
visibilitymap_test() and some concurrent backend doing a heap_delete + commit 
directly after that. If a SnapshotNow were used this could result in different 
results between a IOS and a normal indexscan because the normal indexscan 
would lock the heap page before doing the visibility testing and thus would 
see the new visibility information. But then a SnapshotNow wouldn't be used if 
that were a problem...
For normal snapshots its not a problem because with regards to the snapshot 
everything on the page is still visible as I think that can only happen for 
deletions and HOT updates because the index page would need to get locked 
otherwise. Deletions aren't visible yet and HOT is irrelevant for the IOS by 
definition.

> And two, you can't do a delete or a commit without a memory barrier - every
> LWLockAcquire() or LWLockRelease() is a full barrier, so any operation that
> requires a buffer content lock is both preceded and followed by a full
> barrier.
The memory barrier on the deleting side is irrelevant if there is no memory 
barrier on the reading side.

> Proposed patch attached.  This adds some more comments in various
> places, and implements your suggestion of retesting the visibility-map
> bit when we detect a possible mismatch with the page-level bit.
Thanks, will look at it in a bit.

Andres

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> Well, one, commits are irrelevant; the page ceases to be all-visible
>> as soon as the delete happens.
> Its not irrelevant for the code as it stands if non-mvcc snapshots were
> allowed. Unless I miss something, even disregarding memory ordering issues,
> there is no interlocking providing protection against the index doing the
> visibilitymap_test() and some concurrent backend doing a heap_delete + commit
> directly after that.

Hmm.  Well, the backend that did the heap_delete would clear the
visibility map bit when it did the delete, before releasing the lock
on the heap buffer.  So normally we'll see that buffer as
not-all-visible as soon as the delete is performed, even if no commit
has happened yet.  But I guess there could be a memory-ordering issue
there for a SnapshotNow scan, because as you say there's no barrier on
the read side in that case.  The delete + commit would have to happen
after we finish fetching the index TID but before we test the
visibility map bit.  So I suppose that if we wanted to support
index-only scans  under SnapshotNow maybe we'd want to lock and unlock
the visibility map page when testing each bit.  But that almost seems
like overkill anyway: for it to matter, someone would have to be
relying on starting a scan, deleting a tuple in another transaction
while the scan was in progress, and having the scan see the delete
when it reached the deleted tuple.  But of course if the scan had take
a microsecond longer to reach the deleted tuple it wouldn't have seen
it as deleted anyway, so it's a bit hard to see how any such code
could be race-free anyhow.

>> Proposed patch attached.  This adds some more comments in various
>> places, and implements your suggestion of retesting the visibility-map
>> bit when we detect a possible mismatch with the page-level bit.
> Thanks, will look at it in a bit.

Thanks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Thursday, June 07, 2012 04:27:32 PM Robert Haas wrote:
> On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> >> Proposed patch attached.  This adds some more comments in various
> >> places, and implements your suggestion of retesting the visibility-map
> >> bit when we detect a possible mismatch with the page-level bit.
> > 
> > Thanks, will look at it in a bit.
I wonder if    /* mark page all-visible, if appropriate */    if (all_visible && !PageIsAllVisible(page))    {
PageSetAllVisible(page);       MarkBufferDirty(buf);        visibilitymap_set(onerel, blkno, InvalidXLogRecPtr,
vmbuffer,                         visibility_cutoff_xid);    }
 
shouldn't test    if (all_visible &&        (!PageIsAllVisible(page) || !all_visible_according_to_vm)
instead.

Commentwise I am not totally content with the emphasis on memory ordering 
because some of the stuff is more locking than memory ordering. Except that I 
think its a pretty clear improvement. I can reformulate the places where I 
find that relevant but I have the feeling that wouldn't help the legibility.
Its the big comment in vacuumlazy.c, the comment in nodeIndexonly.c and the 
one in the header of visibilitymap_test. Should be s/memory-
ordering/concurrency/ except in nodeIndexonlyscan.c

The visibilitymap_clear/PageClearAllVisible in heap_multi_insert should be 
moved into the critical section, shouldn't it?

Regards,

Andres

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Thu, Jun 7, 2012 at 11:04 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On Thursday, June 07, 2012 04:27:32 PM Robert Haas wrote:
>> On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres@2ndquadrant.com>
> wrote:
>> >> Proposed patch attached.  This adds some more comments in various
>> >> places, and implements your suggestion of retesting the visibility-map
>> >> bit when we detect a possible mismatch with the page-level bit.
>> >
>> > Thanks, will look at it in a bit.
> I wonder if
>                /* mark page all-visible, if appropriate */
>                if (all_visible && !PageIsAllVisible(page))
>                {
>                        PageSetAllVisible(page);
>                        MarkBufferDirty(buf);
>                        visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer,
>                                                          visibility_cutoff_xid);
>                }
> shouldn't test
>                if (all_visible &&
>                    (!PageIsAllVisible(page) || !all_visible_according_to_vm)
> instead.

Hmm, I think you're right.

> Commentwise I am not totally content with the emphasis on memory ordering
> because some of the stuff is more locking than memory ordering. Except that I
> think its a pretty clear improvement. I can reformulate the places where I
> find that relevant but I have the feeling that wouldn't help the legibility.
> Its the big comment in vacuumlazy.c, the comment in nodeIndexonly.c and the
> one in the header of visibilitymap_test. Should be s/memory-
> ordering/concurrency/ except in nodeIndexonlyscan.c

Hmm, I see your point.

> The visibilitymap_clear/PageClearAllVisible in heap_multi_insert should be
> moved into the critical section, shouldn't it?

Yes, it should.  I was thinking maybe we could go the other way and
have heap_insert do it before starting the critical section, but
that's no good: clearing the visibility map bit is part of the
critical data change, and we can't do it and then forget to WAL-log
it.

Updated patch attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Thursday, June 07, 2012 06:08:34 PM Robert Haas wrote:
> On Thu, Jun 7, 2012 at 11:04 AM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > On Thursday, June 07, 2012 04:27:32 PM Robert Haas wrote:
> >> On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres@2ndquadrant.com>
> > 
> > wrote:
> >> >> Proposed patch attached.  This adds some more comments in various
> >> >> places, and implements your suggestion of retesting the
> >> >> visibility-map bit when we detect a possible mismatch with the
> >> >> page-level bit.
> >> > 
> >> > Thanks, will look at it in a bit.
> > The visibilitymap_clear/PageClearAllVisible in heap_multi_insert should
> > be moved into the critical section, shouldn't it?
> 
> Yes, it should.  I was thinking maybe we could go the other way and
> have heap_insert do it before starting the critical section, but
> that's no good: clearing the visibility map bit is part of the
> critical data change, and we can't do it and then forget to WAL-log
> it.
You could do a visibilitymap_pin outside, but I don't really see the point as 
the page is already locked. There might be some slight benefit in doing so in 
multi_insert but that would be more complicated. And of doubtful benefit.

> Updated patch attached.
Looks good.

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Thu, Jun 7, 2012 at 12:19 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> You could do a visibilitymap_pin outside, but I don't really see the point as
> the page is already locked. There might be some slight benefit in doing so in
> multi_insert but that would be more complicated. And of doubtful benefit.

Doesn't RelationGetBufferForTuple() already do exactly that?

>> Updated patch attached.
> Looks good.

Thanks for the review.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: "page is not marked all-visible" warning in regression tests

From
Andres Freund
Date:
On Thursday, June 07, 2012 06:23:58 PM Robert Haas wrote:
> On Thu, Jun 7, 2012 at 12:19 PM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > You could do a visibilitymap_pin outside, but I don't really see the
> > point as the page is already locked. There might be some slight benefit
> > in doing so in multi_insert but that would be more complicated. And of
> > doubtful benefit.
> 
> Doesn't RelationGetBufferForTuple() already do exactly that?
Forget it, sorry. I mis(read|remembered) how multi_insert works and concluded 
from that on how insert works...


Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services


Re: "page is not marked all-visible" warning in regression tests

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> Updated patch attached.

The comments need a pass of copy-editing, eg here and here:

> + * so somebody else could be change the bit just after we look at it.  In fact,
^^^^^^^^^^^^^^^

> +         * got cleared after we checked it and before we got took the buffer
                 ^^^^^^^^
 

Seems reasonable beyond that.
        regards, tom lane


Re: "page is not marked all-visible" warning in regression tests

From
Robert Haas
Date:
On Thu, Jun 7, 2012 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Updated patch attached.
>
> The comments need a pass of copy-editing, eg here and here:
>
>> + * so somebody else could be change the bit just after we look at it.  In fact,
>                       ^^^^^^^^^^^^^^^
>
>> +         * got cleared after we checked it and before we got took the buffer
>                                                            ^^^^^^^^
>
> Seems reasonable beyond that.

Thanks, committed with those fixes and a correction for one more typo
I spotted elsewhere.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company