Обсуждение: Documentation fixes for pg_visibility
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
Вложения
On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier <michael.paquier@gmail.com> 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
Вложения
On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier > <michael.paquier@gmail.com> 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
On Thu, Jun 23, 2016 at 12:53 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier >> <michael.paquier@gmail.com> 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
On Tue, Jun 28, 2016 at 6:51 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Jun 23, 2016 at 12:53 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Jun 23, 2016 at 1:46 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier >>> <michael.paquier@gmail.com> 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
On Thu, Jun 23, 2016 at 12:46 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Jun 23, 2016 at 1:42 PM, Michael Paquier > <michael.paquier@gmail.com> 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
On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier <michael.paquier@gmail.com> 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, &vmbuffer) && !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
On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier > <michael.paquier@gmail.com> 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, &vmbuffer) && !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
Вложения
On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier >> <michael.paquier@gmail.com> 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, &vmbuffer) && !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
On Fri, Jul 1, 2016 at 6:58 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier >>> <michael.paquier@gmail.com> 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, &vmbuffer) && !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
On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila <amit.kapila16@gmail.com> 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
On Fri, Jul 1, 2016 at 9:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila <amit.kapila16@gmail.com> 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
On Fri, Jul 1, 2016 at 10:20 PM, Amit Kapila <amit.kapila16@gmail.com> 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
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 <robertmhaas@gmail.com> 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
Вложения
On Tue, Jul 5, 2016 at 8:51 PM, Robert Haas <robertmhaas@gmail.com> 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