Re: [HACKERS] Documentation fixes for pg_visibility

[Amit Kapila]

On Tue, Jul 5, 2016 at 8:51 PM, Robert Haas  wrote:
>
> So I'm a bit confused about what you are saying here.  If the page is
> marked all-frozen but actually isn't all-frozen, then we should clear
> the all-frozen bit in the VM.
>

Agreed.

>  The easiest way to do that is to clear
> both bits in the VM plus the page-level bit, as done here, because we
> don't presently have a way of clearing just one of the visibility map
> bits.
>

Okay, but due to that we are clearing the visibility information
(all-visible) even though we should not clear it based on all-frozen.
I don't know if there is much harm even if we do the way it is
proposed in patch, but why not improve it if we can.

> Now, since the heap_lock_tuple issue requires us to introduce a new
> method to clear all-visible without clearing all-frozen, we could
> possibly use that here too, once we have it.
>

makes sense.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

Robert wrote:
>  I think there should probably be a test for
> all_visible_according_to_vm at the beginning of that so that we don't
> add more visibility map checks for pages where we already know the VM
> bit can't possibly be set.

Yes, that looks like a good idea after more screening of this code.

On Wed, Jul 6, 2016 at 12:21 AM, Robert Haas  wrote:
> So I'm a bit confused about what you are saying here.  If the page is
> marked all-frozen but actually isn't all-frozen, then we should clear
> the all-frozen bit in the VM.  The easiest way to do that is to clear
> both bits in the VM plus the page-level bit, as done here, because we
> don't presently have a way of clearing just one of the visibility map
> bits.

Yes, that's my understanding as well for what is necessary: clear both
bits in the vm as well as the bit on the page itself, and mark it as
dirty. Way to go.

> Now, since the heap_lock_tuple issue requires us to introduce a new
> method to clear all-visible without clearing all-frozen, we could
> possibly use that here too, once we have it.

For 10.0, working on lower-level routine optimizations of this kind
sounds good to me. But I vote against this level of code reordering in
9.6 post-beta2 if that's what you suggest.
--
Michael
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 32b6fdd..98cf9e8 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1183,6 +1183,23 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 		}
 
 		/*
+		 * If the page contains non-frozen tuples but the visibility map is
+		 * telling the contrary, inform of the situation and take preventive
+		 * measures on the page and the visibility map. This situation should
+		 * not happen however.
+		 */
+		else if (all_visible_according_to_vm &&
+ VM_ALL_FROZEN(onerel, blkno, ) &&
+ !all_frozen)
+		{
+			elog(WARNING, "page contains non-frozen tuples but visibility map bit is set in relation \"%s\" page %u",
+ relname, blkno);
+			PageClearAllVisible(page);
+			MarkBufferDirty(buf);
+			visibilitymap_clear(onerel, blkno, vmbuffer);
+		}
+
+		/*
 		 * It's possible for the value returned by GetOldestXmin() to move
 		 * backwards, so it's not wrong for us to see tuples that appear to
 		 * not be visible to everyone yet, while PD_ALL_VISIBLE is already

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Fri, Jul 1, 2016 at 10:20 PM, Amit Kapila  wrote:
 Right, something like that.  I think Andres actually wants something
 like this in 9.6, and I'm inclined to think it might be a good idea,
 too.  I think there should probably be a test for
 all_visible_according_to_vm at the beginning of that so that we don't
 add more visibility map checks for pages where we already know the VM
 bit can't possibly be set.

 Other opinions on the concept or the patch?
>>>
>>> +1 on the idea.
>>>
>>> + PageClearAllVisible(page);
>>> + MarkBufferDirty(buf);
>>>
>>> What is the need to clear the Page level bit, if it is already
>>> cleared, doesn't '!all_frozen' indicate that?
>>
>> No, I don't think so.  I think all_frozen indicates whether we think
>> that all tuples on the page qualify as fully frozen.  I don't think it
>> tells us anything about whether PD_ALL_VISIBLE is set on the page.
>
> Then, can we decide to clear it on that basis?  Isn't it possible that
> page is marked as all_visible, even if it contains frozen tuples?In
> the other nearby code (refer below part of code), we only clear the
> page level bit after ensuring it is set.  Am I missing something?
>
> else if (PageIsAllVisible(page) && has_dead_tuples)
> {
> elog(WARNING, "page containing dead tuples is marked as all-visible in
> relation \"%s\" page %u",
> relname, blkno);
> PageClearAllVisible(page);
> MarkBufferDirty(buf);
> visibilitymap_clear(onerel, blkno, vmbuffer);
> }

So I'm a bit confused about what you are saying here.  If the page is
marked all-frozen but actually isn't all-frozen, then we should clear
the all-frozen bit in the VM.  The easiest way to do that is to clear
both bits in the VM plus the page-level bit, as done here, because we
don't presently have a way of clearing just one of the visibility map
bits.

Now, since the heap_lock_tuple issue requires us to introduce a new
method to clear all-visible without clearing all-frozen, we could
possibly use that here too, once we have it.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Amit Kapila]

On Fri, Jul 1, 2016 at 9:30 PM, Robert Haas  wrote:
> On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila  wrote:
 Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
 when scanning each block, and then to issue a WARNING and clear the
 visibility map. Indeed that's better. I guess I need to take a closer
 look at vacuumlazy.c. See attached for example, but that's perhaps not
 something to have in 9.6 as that's more a micro-optimization than
 anything else.
>>>
>>> Right, something like that.  I think Andres actually wants something
>>> like this in 9.6, and I'm inclined to think it might be a good idea,
>>> too.  I think there should probably be a test for
>>> all_visible_according_to_vm at the beginning of that so that we don't
>>> add more visibility map checks for pages where we already know the VM
>>> bit can't possibly be set.
>>>
>>> Other opinions on the concept or the patch?
>>
>> +1 on the idea.
>>
>> + PageClearAllVisible(page);
>> + MarkBufferDirty(buf);
>>
>> What is the need to clear the Page level bit, if it is already
>> cleared, doesn't '!all_frozen' indicate that?
>
> No, I don't think so.  I think all_frozen indicates whether we think
> that all tuples on the page qualify as fully frozen.  I don't think it
> tells us anything about whether PD_ALL_VISIBLE is set on the page.
>

Then, can we decide to clear it on that basis?  Isn't it possible that
page is marked as all_visible, even if it contains frozen tuples?  In
the other nearby code (refer below part of code), we only clear the
page level bit after ensuring it is set.  Am I missing something?

else if (PageIsAllVisible(page) && has_dead_tuples)
{
elog(WARNING, "page containing dead tuples is marked as all-visible in
relation \"%s\" page %u",
relname, blkno);
PageClearAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_clear(onerel, blkno, vmbuffer);
}



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila  wrote:
>>> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
>>> when scanning each block, and then to issue a WARNING and clear the
>>> visibility map. Indeed that's better. I guess I need to take a closer
>>> look at vacuumlazy.c. See attached for example, but that's perhaps not
>>> something to have in 9.6 as that's more a micro-optimization than
>>> anything else.
>>
>> Right, something like that.  I think Andres actually wants something
>> like this in 9.6, and I'm inclined to think it might be a good idea,
>> too.  I think there should probably be a test for
>> all_visible_according_to_vm at the beginning of that so that we don't
>> add more visibility map checks for pages where we already know the VM
>> bit can't possibly be set.
>>
>> Other opinions on the concept or the patch?
>
> +1 on the idea.
>
> + PageClearAllVisible(page);
> + MarkBufferDirty(buf);
>
> What is the need to clear the Page level bit, if it is already
> cleared, doesn't '!all_frozen' indicate that?

No, I don't think so.  I think all_frozen indicates whether we think
that all tuples on the page qualify as fully frozen.  I don't think it
tells us anything about whether PD_ALL_VISIBLE is set on the page.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Amit Kapila]

On Fri, Jul 1, 2016 at 6:58 PM, Robert Haas  wrote:
> On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier
>  wrote:
>> On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas  wrote:
>>> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
>>>  wrote:
> Under what circumstances would you wish to check only one page of a 
> relation?

 What I'd like to be able to do is to stop scanning the relation once
 one defective tuple has been found: if there is at least one problem,
 the whole vm needs to be rebuilt anyway. So this function could be
 wrapped in a plpgsql function for example. It is more flexible than
 directly modifying this function so as it stops at the first problem
 stopped.
>>>
>>> I think most likely the best way to handle this is teach VACUUM to do
>>> PageClearAllVisible() and visibilitymap_clear() on any page where
>>> VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
>>> well with the existing code to clear incorrectly-set visibility map
>>> bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
>>> purpose you're talking about here, but more efficiently.
>>
>> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
>> when scanning each block, and then to issue a WARNING and clear the
>> visibility map. Indeed that's better. I guess I need to take a closer
>> look at vacuumlazy.c. See attached for example, but that's perhaps not
>> something to have in 9.6 as that's more a micro-optimization than
>> anything else.
>
> Right, something like that.  I think Andres actually wants something
> like this in 9.6, and I'm inclined to think it might be a good idea,
> too.  I think there should probably be a test for
> all_visible_according_to_vm at the beginning of that so that we don't
> add more visibility map checks for pages where we already know the VM
> bit can't possibly be set.
>
> Other opinions on the concept or the patch?
>

+1 on the idea.

+ PageClearAllVisible(page);
+ MarkBufferDirty(buf);

What is the need to clear the Page level bit, if it is already
cleared, doesn't '!all_frozen' indicate that?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier
 wrote:
> On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas  wrote:
>> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
>>  wrote:
 Under what circumstances would you wish to check only one page of a 
 relation?
>>>
>>> What I'd like to be able to do is to stop scanning the relation once
>>> one defective tuple has been found: if there is at least one problem,
>>> the whole vm needs to be rebuilt anyway. So this function could be
>>> wrapped in a plpgsql function for example. It is more flexible than
>>> directly modifying this function so as it stops at the first problem
>>> stopped.
>>
>> I think most likely the best way to handle this is teach VACUUM to do
>> PageClearAllVisible() and visibilitymap_clear() on any page where
>> VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
>> well with the existing code to clear incorrectly-set visibility map
>> bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
>> purpose you're talking about here, but more efficiently.
>
> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
> when scanning each block, and then to issue a WARNING and clear the
> visibility map. Indeed that's better. I guess I need to take a closer
> look at vacuumlazy.c. See attached for example, but that's perhaps not
> something to have in 9.6 as that's more a micro-optimization than
> anything else.

Right, something like that.  I think Andres actually wants something
like this in 9.6, and I'm inclined to think it might be a good idea,
too.  I think there should probably be a test for
all_visible_according_to_vm at the beginning of that so that we don't
add more visibility map checks for pages where we already know the VM
bit can't possibly be set.

Other opinions on the concept or the patch?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas  wrote:
> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
>  wrote:
>>> Under what circumstances would you wish to check only one page of a 
>>> relation?
>>
>> What I'd like to be able to do is to stop scanning the relation once
>> one defective tuple has been found: if there is at least one problem,
>> the whole vm needs to be rebuilt anyway. So this function could be
>> wrapped in a plpgsql function for example. It is more flexible than
>> directly modifying this function so as it stops at the first problem
>> stopped.
>
> I think most likely the best way to handle this is teach VACUUM to do
> PageClearAllVisible() and visibilitymap_clear() on any page where
> VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
> well with the existing code to clear incorrectly-set visibility map
> bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
> purpose you're talking about here, but more efficiently.

Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
when scanning each block, and then to issue a WARNING and clear the
visibility map. Indeed that's better. I guess I need to take a closer
look at vacuumlazy.c. See attached for example, but that's perhaps not
something to have in 9.6 as that's more a micro-optimization than
anything else.
-- 
Michael


vm-all-frozen-check.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
 wrote:
>> Under what circumstances would you wish to check only one page of a relation?
>
> What I'd like to be able to do is to stop scanning the relation once
> one defective tuple has been found: if there is at least one problem,
> the whole vm needs to be rebuilt anyway. So this function could be
> wrapped in a plpgsql function for example. It is more flexible than
> directly modifying this function so as it stops at the first problem
> stopped.

I think most likely the best way to handle this is teach VACUUM to do
PageClearAllVisible() and visibilitymap_clear() on any page where
VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
well with the existing code to clear incorrectly-set visibility map
bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
purpose you're talking about here, but more efficiently.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Thu, Jun 23, 2016 at 12:46 AM, Michael Paquier
 wrote:
> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier
>  wrote:
>> While looking at the module I found two mistakes in the docs:
>> pg_visibility_map and pg_visibility *not* taking in input a block
>> number are SRFs, and return a set of records. The documentation is
>> just listing them with "returns record". A patch is attached.
>
> And that: s/PD_ALL_VISIBILE/PD_ALL_VISIBLE.

Committed.  Thanks for the careful proofreading.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

On Tue, Jun 28, 2016 at 6:51 AM, Robert Haas  wrote:
> On Thu, Jun 23, 2016 at 12:53 AM, Michael Paquier
>  wrote:
>> On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier
>>  wrote:
>>> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier
>>>  wrote:
 While looking at the module I found two mistakes in the docs:
 pg_visibility_map and pg_visibility *not* taking in input a block
 number are SRFs, and return a set of records. The documentation is
 just listing them with "returns record". A patch is attached.
>>>
>>> And that: s/PD_ALL_VISIBILE/PD_ALL_VISIBLE.
>>
>> And would it actually make sense to have pg_check_frozen(IN regclass,
>> IN blkno) to target only a certain page? Same for pg_check_visible. It
>> would take a long time to run those functions on large tables as they
>> scan all the pages of a relation at once..
>
> Under what circumstances would you wish to check only one page of a relation?

What I'd like to be able to do is to stop scanning the relation once
one defective tuple has been found: if there is at least one problem,
the whole vm needs to be rebuilt anyway. So this function could be
wrapped in a plpgsql function for example. It is more flexible than
directly modifying this function so as it stops at the first problem
stopped.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Thu, Jun 23, 2016 at 12:53 AM, Michael Paquier
 wrote:
> On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier
>  wrote:
>> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier
>>  wrote:
>>> While looking at the module I found two mistakes in the docs:
>>> pg_visibility_map and pg_visibility *not* taking in input a block
>>> number are SRFs, and return a set of records. The documentation is
>>> just listing them with "returns record". A patch is attached.
>>
>> And that: s/PD_ALL_VISIBILE/PD_ALL_VISIBLE.
>
> And would it actually make sense to have pg_check_frozen(IN regclass,
> IN blkno) to target only a certain page? Same for pg_check_visible. It
> would take a long time to run those functions on large tables as they
> scan all the pages of a relation at once..

Under what circumstances would you wish to check only one page of a relation?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier
 wrote:
> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier
>  wrote:
>> While looking at the module I found two mistakes in the docs:
>> pg_visibility_map and pg_visibility *not* taking in input a block
>> number are SRFs, and return a set of records. The documentation is
>> just listing them with "returns record". A patch is attached.
>
> And that: s/PD_ALL_VISIBILE/PD_ALL_VISIBLE.

And would it actually make sense to have pg_check_frozen(IN regclass,
IN blkno) to target only a certain page? Same for pg_check_visible. It
would take a long time to run those functions on large tables as they
scan all the pages of a relation at once..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier
 wrote:
> While looking at the module I found two mistakes in the docs:
> pg_visibility_map and pg_visibility *not* taking in input a block
> number are SRFs, and return a set of records. The documentation is
> just listing them with "returns record". A patch is attached.

And that: s/PD_ALL_VISIBILE/PD_ALL_VISIBLE.
-- 
Michael


docs-visibility.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Documentation fixes for pg_visibility

[Michael Paquier]

Hi,

While looking at the module I found two mistakes in the docs:
pg_visibility_map and pg_visibility *not* taking in input a block
number are SRFs, and return a set of records. The documentation is
just listing them with "returns record". A patch is attached.
Thanks,
-- 
Michael


docs-visibility.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers