Documentation fixes for pg_visibility

Started by Michael Paquierover 9 years ago15 messages
#1Michael Paquier
michael.paquier@gmail.com
1 attachment(s)

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

Attachments:

docs-visibility.patchinvalid/octet-stream; name=docs-visibility.patchDownload
diff --git a/doc/src/sgml/pgvisibility.sgml b/doc/src/sgml/pgvisibility.sgml
index 44e83de..c7dcd16 100644
--- a/doc/src/sgml/pgvisibility.sgml
+++ b/doc/src/sgml/pgvisibility.sgml
@@ -65,7 +65,7 @@
    </varlistentry>
 
    <varlistentry>
-    <term><function>pg_visibility_map(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean) returns record</function></term>
+    <term><function>pg_visibility_map(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean) returns setof record</function></term>
     <listitem>
      <para>
       Returns the all-visible and all-frozen bits in the visibility map for
@@ -75,7 +75,7 @@
    </varlistentry>
 
    <varlistentry>
-    <term><function>pg_visibility(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean, pd_all_visible OUT boolean) returns record</function></term>
+    <term><function>pg_visibility(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean, pd_all_visible OUT boolean) returns setof record</function></term>
 
     <listitem>
      <para>
#2Michael Paquier
michael.paquier@gmail.com
In reply to: Michael Paquier (#1)
1 attachment(s)
Re: Documentation fixes for pg_visibility

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

Attachments:

docs-visibility.patchinvalid/octet-stream; name=docs-visibility.patchDownload
diff --git a/doc/src/sgml/pgvisibility.sgml b/doc/src/sgml/pgvisibility.sgml
index 44e83de..d764eff 100644
--- a/doc/src/sgml/pgvisibility.sgml
+++ b/doc/src/sgml/pgvisibility.sgml
@@ -59,13 +59,13 @@
      <para>
       Returns the all-visible and all-frozen bits in the visibility map for
       the given block of the given relation, plus the
-      <literal>PD_ALL_VISIBILE</> bit for that block.
+      <literal>PD_ALL_VISIBLE</> bit for that block.
      </para>
     </listitem>
    </varlistentry>
 
    <varlistentry>
-    <term><function>pg_visibility_map(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean) returns record</function></term>
+    <term><function>pg_visibility_map(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean) returns setof record</function></term>
     <listitem>
      <para>
       Returns the all-visible and all-frozen bits in the visibility map for
@@ -75,7 +75,7 @@
    </varlistentry>
 
    <varlistentry>
-    <term><function>pg_visibility(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean, pd_all_visible OUT boolean) returns record</function></term>
+    <term><function>pg_visibility(regclass, blkno OUT bigint, all_visible OUT boolean, all_frozen OUT boolean, pd_all_visible OUT boolean) returns setof record</function></term>
 
     <listitem>
      <para>
#3Michael Paquier
michael.paquier@gmail.com
In reply to: Michael Paquier (#2)
Re: Documentation fixes for pg_visibility

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

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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#3)
Re: Documentation fixes for pg_visibility

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

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

#5Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#4)
Re: Documentation fixes for pg_visibility

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

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

#6Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#2)
Re: Documentation fixes for pg_visibility

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

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

#7Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#5)
Re: Documentation fixes for pg_visibility

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

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

#8Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#7)
1 attachment(s)
Re: Documentation fixes for pg_visibility

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

Attachments:

vm-all-frozen-check.patchinvalid/octet-stream; name=vm-all-frozen-check.patchDownload
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 32b6fdd..927f4a0 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1183,6 +1183,21 @@ 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 (VM_ALL_FROZEN(onerel, blkno, &vmbuffer) && !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
#9Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#8)
Re: Documentation fixes for pg_visibility

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

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

#10Amit Kapila
amit.kapila16@gmail.com
In reply to: Robert Haas (#9)
Re: Documentation fixes for pg_visibility

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

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

#11Robert Haas
robertmhaas@gmail.com
In reply to: Amit Kapila (#10)
Re: Documentation fixes for pg_visibility

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

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

#12Amit Kapila
amit.kapila16@gmail.com
In reply to: Robert Haas (#11)
Re: Documentation fixes for pg_visibility

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

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

#13Robert Haas
robertmhaas@gmail.com
In reply to: Amit Kapila (#12)
Re: Documentation fixes for pg_visibility

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

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

#14Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#13)
1 attachment(s)
Re: Documentation fixes for pg_visibility

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

Attachments:

vm-all-frozen-check-v2.patchtext/x-diff; charset=US-ASCII; name=vm-all-frozen-check-v2.patchDownload
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, &vmbuffer) &&
+				 !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
#15Amit Kapila
amit.kapila16@gmail.com
In reply to: Robert Haas (#13)
Re: Documentation fixes for pg_visibility

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

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