Fix comments of heap_prune_chain()

Started by Nonameover 4 years ago7 messages

ikedamsh@oss.nttdata.com

over 4 years ago

1 attachment(s)

Hi,

While I’m reading source codes related to vacuum, I found comments which
don’t seem to fit the reality. I think the commit[1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0 just forgot to fix them.
What do you think?

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Attachments:

v1-0001-fix_heap_prune_chain.patchapplication/octet-stream; name=v1-0001-fix_heap_prune_chain.patch; x-unix-mode=0644Download

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 15ca1b304a..2f89e4d525 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -488,17 +488,15 @@ heap_prune_satisfies_vacuum(PruneState *prstate, HeapTuple tup, Buffer buffer)
  * the HOT chain is pruned by removing all DEAD tuples at the start of the HOT
  * chain.  We also prune any RECENTLY_DEAD tuples preceding a DEAD tuple.
  * This is OK because a RECENTLY_DEAD tuple preceding a DEAD tuple is really
- * DEAD, the OldestXmin test is just too coarse to detect it.
+ * DEAD, the prstate->vistest is just too coarse to detect it.
  *
  * The root line pointer is redirected to the tuple immediately after the
  * latest DEAD tuple.  If all tuples in the chain are DEAD, the root line
  * pointer is marked LP_DEAD.  (This includes the case of a DEAD simple
  * tuple, which we treat as a chain of length 1.)
  *
- * OldestXmin is the cutoff XID used to identify dead tuples.
- *
  * We don't actually change the page here, except perhaps for hint-bit updates
- * caused by HeapTupleSatisfiesVacuum.  We just add entries to the arrays in
+ * caused by heap_prune_satisfies_vacuum.  We just add entries to the arrays in
  * prstate showing the changes to be made.  Items to be redirected are added
  * to the redirected[] array (two entries per redirection); items to be set to
  * LP_DEAD state are added to nowdead[]; and items to be set to LP_UNUSED
@@ -680,7 +678,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate)
 				break;
 
 			default:
-				elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result");
+				elog(ERROR, "unexpected heap_prune_satisfies_vacuum result");
 				break;
 		}

Alvaro Herrera

alvherre@alvh.no-ip.org

over 4 years ago

In reply to: Noname (#1)

Re: Fix comments of heap_prune_chain()

On 2021-Jul-12, ikedamsh@oss.nttdata.com wrote:

While I’m reading source codes related to vacuum, I found comments which
don’t seem to fit the reality. I think the commit[1] just forgot to fix them.
What do you think?

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0

That sounds believable, but can you be more specific?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Matthias van de Meent

boekewurm+postgres@gmail.com

over 4 years ago

In reply to: Noname (#1)

Re: Fix comments of heap_prune_chain()

On Mon, 12 Jul 2021 at 13:14, <ikedamsh@oss.nttdata.com> wrote:

Hi,

While I’m reading source codes related to vacuum, I found comments which
don’t seem to fit the reality. I think the commit[1] just forgot to fix

them.

What do you think?

Hmm, yes, those are indeed some leftovers.

Some comments on the suggested changes:

- * caused by HeapTupleSatisfiesVacuum.  We just add entries to the arrays
in
+ * caused by heap_prune_satisfies_vacuum.  We just add entries to the
arrays in

I think that HeapTupleSatisfiesVacuumHorizon might be an alternative
correct replacement here.

-                elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result");
+                elog(ERROR, "unexpected heap_prune_satisfies_vacuum
result");

The type of the value is HTSV_Result; where HTSV stands for
HeapTupleSatisfiesVacuum, so if we were to replace this, I'd go for
"unexpected result from heap_prune_satisfies_vacuum" as a message instead.

Kind regards,

Matthias van de Meent

Alvaro Herrera

alvherre@alvh.no-ip.org

over 4 years ago

In reply to: Alvaro Herrera (#2)

Re: Fix comments of heap_prune_chain()

On 2021-Jul-12, Alvaro Herrera wrote:

On 2021-Jul-12, ikedamsh@oss.nttdata.com wrote:

While I’m reading source codes related to vacuum, I found comments which
don’t seem to fit the reality. I think the commit[1] just forgot to fix them.
What do you think?

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0

That sounds believable, but can you be more specific?

Oh, apologies, I didn't realize there was an attachment. That seems
specific enough :-)

In my defense, the archives don't show the attachment either:
/messages/by-id/5CB29811-2B1D-4244-8DE2-B1E02495426B@oss.nttdata.com
I think we've seen this kind of problem before -- the MIME structure of
the message is quite unusual, which is why neither my MUA nor the
archives show it.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 4 years ago

In reply to: Alvaro Herrera (#4)

Re: Fix comments of heap_prune_chain()

(This is out of topic)

At Mon, 12 Jul 2021 20:17:55 -0400, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in

Oh, apologies, I didn't realize there was an attachment. That seems
specific enough :-)

In my defense, the archives don't show the attachment either:
/messages/by-id/5CB29811-2B1D-4244-8DE2-B1E02495426B@oss.nttdata.com
I think we've seen this kind of problem before -- the MIME structure of
the message is quite unusual, which is why neither my MUA nor the
archives show it.

The same for me. Multipart structure of that mail looks like odd.

multipart/laternative
text/plain - mail body quoted-printable
multipart/mixed
text/html - HTML alternative body
appliation/octet-stream - the patch
text/html - garbage

I found an issue in bugzilla about this behavior

https://bugzilla.mozilla.org/show_bug.cgi?id=1362539

The primary issue is Apple-mail's strange mime-composition.
I'm not sure whether it is avoidable by some settings.

(I don't think the alternative HTML body is useful at least for this
mailling list.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Masahiro Ikeda

ikedamsh@oss.nttdata.com

over 4 years ago

In reply to: Matthias van de Meent (#3)

1 attachment(s)

Re: Fix comments of heap_prune_chain()

On 2021/07/13 5:57, Matthias van de Meent wrote:

On Mon, 12 Jul 2021 at 13:14, <ikedamsh@oss.nttdata.com
<mailto:ikedamsh@oss.nttdata.com>> wrote:

Hi,

While I’m reading source codes related to vacuum, I found comments which
don’t seem to fit the reality. I think the commit[1] just forgot to fix them.
What do you think?

Hmm, yes, those are indeed some leftovers.

Some comments on the suggested changes:
- * caused by HeapTupleSatisfiesVacuum.  We just add entries to the arrays in
+ * caused by heap_prune_satisfies_vacuum.  We just add entries to the arrays in
I think that HeapTupleSatisfiesVacuumHorizon might be an alternative correct
replacement here.
-                elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result");
+                elog(ERROR, "unexpected heap_prune_satisfies_vacuum result");
The type of the value is HTSV_Result; where HTSV stands for
HeapTupleSatisfiesVacuum, so if we were to replace this, I'd go for
"unexpected result from heap_prune_satisfies_vacuum" as a message instead.

Thanks for your comments. I agree your suggestions.

I also updated prstate->vistest to heap_prune_satisfies_vacuum of v1 patch
because heap_prune_satisfies_vacuum() tests with not only prstate->vistest but
also prstate->old_snap_xmin. I think it's more accurate representation.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Attachments:

v2-0001-fix_heap_prune_chain.patchtext/x-patch; charset=UTF-8; name=v2-0001-fix_heap_prune_chain.patchDownload

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 15ca1b304a..d2f4c30baa 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -488,17 +488,15 @@ heap_prune_satisfies_vacuum(PruneState *prstate, HeapTuple tup, Buffer buffer)
  * the HOT chain is pruned by removing all DEAD tuples at the start of the HOT
  * chain.  We also prune any RECENTLY_DEAD tuples preceding a DEAD tuple.
  * This is OK because a RECENTLY_DEAD tuple preceding a DEAD tuple is really
- * DEAD, the OldestXmin test is just too coarse to detect it.
+ * DEAD, heap_prune_satisfies_vacuum() is just too coarse to detect it.
  *
  * The root line pointer is redirected to the tuple immediately after the
  * latest DEAD tuple.  If all tuples in the chain are DEAD, the root line
  * pointer is marked LP_DEAD.  (This includes the case of a DEAD simple
  * tuple, which we treat as a chain of length 1.)
  *
- * OldestXmin is the cutoff XID used to identify dead tuples.
- *
  * We don't actually change the page here, except perhaps for hint-bit updates
- * caused by HeapTupleSatisfiesVacuum.  We just add entries to the arrays in
+ * caused by HeapTupleSatisfiesVacuumHorizon.  We just add entries to the arrays in
  * prstate showing the changes to be made.  Items to be redirected are added
  * to the redirected[] array (two entries per redirection); items to be set to
  * LP_DEAD state are added to nowdead[]; and items to be set to LP_UNUSED
@@ -680,7 +678,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate)
 				break;
 
 			default:
-				elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result");
+				elog(ERROR, "unexpected result from heap_prune_satisfies_vacuum");
 				break;
 		}

Masahiro Ikeda

ikedamsh@oss.nttdata.com

over 4 years ago

In reply to: Kyotaro Horiguchi (#5)

Re: Fix comments of heap_prune_chain()

On 2021/07/13 10:22, Kyotaro Horiguchi wrote:

(This is out of topic)

At Mon, 12 Jul 2021 20:17:55 -0400, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in

Oh, apologies, I didn't realize there was an attachment. That seems
specific enough :-)

In my defense, the archives don't show the attachment either:
/messages/by-id/5CB29811-2B1D-4244-8DE2-B1E02495426B@oss.nttdata.com
I think we've seen this kind of problem before -- the MIME structure of
the message is quite unusual, which is why neither my MUA nor the
archives show it.

The same for me. Multipart structure of that mail looks like odd.

multipart/laternative
text/plain - mail body quoted-printable
multipart/mixed
text/html - HTML alternative body
appliation/octet-stream - the patch
text/html - garbage

I found an issue in bugzilla about this behavior

https://bugzilla.mozilla.org/show_bug.cgi?id=1362539

The primary issue is Apple-mail's strange mime-composition.
I'm not sure whether it is avoidable by some settings.

(I don't think the alternative HTML body is useful at least for this
mailling list.)

Thanks for replying and sorry for the above.
The reason is that I sent from MacBook PC as Horiguchi-san said.

I changed my email client and I confirmed that I could send an
email with a new patch. So, please check it.
/messages/by-id/1aa07e2a-b715-5649-6c62-4fff96304d18@oss.nttdata.com

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION