some pointless HeapTupleHeaderIndicatesMovedPartitions calls
Hello
Pavan Deolasee recently noted that a few of the
HeapTupleHeaderIndicatesMovedPartitions calls added by commit
5db6df0c0117 are useless, since they are done after comparing t_self
with t_ctid. That's because t_self can never be set to the magical
values that indicate that the tuple moved partition. If the first
test fails (so we know t_self equals t_ctid), necessarily the second
test will also fail.
So these checks can be removed and no harm is done.
--
�lvaro Herrera 39�49'30"S 73�17'W
Attachments:
0001-Remove-pointless-HeapTupleHeaderIndicatesMovedPartit.patchtext/x-diff; charset=us-asciiDownload
From 953eceb701ca35c5a3c731c4318ed1465f205811 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 29 Sep 2020 10:47:45 -0300
Subject: [PATCH] Remove pointless HeapTupleHeaderIndicatesMovedPartitions
calls
---
src/backend/access/heap/heapam.c | 12 ++++--------
src/backend/access/heap/heapam_visibility.c | 9 +++------
2 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 1585861a02..868ff13453 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2618,8 +2618,7 @@ l1:
HEAP_XMAX_IS_LOCKED_ONLY(tp.t_data->t_infomask) ||
HeapTupleHeaderIsOnlyLocked(tp.t_data))
result = TM_Ok;
- else if (!ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tp.t_data))
+ else if (!ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid))
result = TM_Updated;
else
result = TM_Deleted;
@@ -3248,8 +3247,7 @@ l2:
if (can_continue)
result = TM_Ok;
- else if (!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(oldtup.t_data))
+ else if (!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid))
result = TM_Updated;
else
result = TM_Deleted;
@@ -4485,8 +4483,7 @@ l3:
HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask) ||
HeapTupleHeaderIsOnlyLocked(tuple->t_data))
result = TM_Ok;
- else if (!ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple->t_data))
+ else if (!ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid))
result = TM_Updated;
else
result = TM_Deleted;
@@ -5059,8 +5056,7 @@ test_lockmode_for_conflict(MultiXactStatus status, TransactionId xid,
LOCKMODE_from_mxstatus(wantedstatus)))
{
/* bummer */
- if (!ItemPointerEquals(&tup->t_self, &tup->t_data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tup->t_data))
+ if (!ItemPointerEquals(&tup->t_self, &tup->t_data->t_ctid))
return TM_Updated;
else
return TM_Deleted;
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 80bd494076..cab6a48a5d 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -607,8 +607,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
{
if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
return TM_Ok;
- if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
return TM_Deleted; /* deleted by other */
@@ -653,8 +652,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (TransactionIdDidCommit(xmax))
{
- if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated;
else
return TM_Deleted;
@@ -714,8 +712,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
HeapTupleHeaderGetRawXmax(tuple));
- if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
return TM_Deleted; /* deleted by other */
--
2.20.1
Hi Alvaro,
On Tue, Sep 29, 2020 at 10:14 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
Hello
Pavan Deolasee recently noted that a few of the
HeapTupleHeaderIndicatesMovedPartitions calls added by commit
5db6df0c0117 are useless, since they are done after comparing t_self
with t_ctid. That's because t_self can never be set to the magical
values that indicate that the tuple moved partition. If the first
test fails (so we know t_self equals t_ctid), necessarily the second
test will also fail.So these checks can be removed and no harm is done.
The patch looks good to me. The existing coding pattern was a bit confusing
and that's how I noticed it. So +1 for fixing it.
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
I wonder, why this patch hangs on commitfest for so long.
The idea of the fix is clear, the code is correct and all tests pass, so, I move it to ReadyForCommitter status.
The new status of this patch is: Ready for Committer
On Fri, Feb 12, 2021 at 04:42:26PM +0000, Anastasia Lubennikova wrote:
I wonder, why this patch hangs on commitfest for so long.
The idea of the fix is clear, the code is correct and all tests pass, so, I move it to ReadyForCommitter status.The new status of this patch is: Ready for Committer
So that's this patch: https://commitfest.postgresql.org/32/2941/.
Alvaro is most likely going to take care of that, so let's wait for
him.
--
Michael
On Sat, Feb 13, 2021 at 10:49:26AM +0900, Michael Paquier wrote:
So that's this patch: https://commitfest.postgresql.org/32/2941/.
Alvaro is most likely going to take care of that, so let's wait for
him.
Hearing nothing, I have looked at this stuff and the simplification
makes sense. Any comments?
I can see that ItemPointerSetMovedPartitions() in itemptr.h is not
used in the code. Could it be better to change the comment in
htup_details.h to mention HeapTupleHeaderSetMovedPartitions instead?
Perhaps not worth bothering, just noticed this on the way.
--
Michael
On 2021-Feb-20, Michael Paquier wrote:
On Sat, Feb 13, 2021 at 10:49:26AM +0900, Michael Paquier wrote:
So that's this patch: https://commitfest.postgresql.org/32/2941/.
Alvaro is most likely going to take care of that, so let's wait for
him.Hearing nothing, I have looked at this stuff and the simplification
makes sense. Any comments?
No further comments ... I think the patch is simple enough. Thanks for
looking -- I'll get it pushed on Monday or so.
I can see that ItemPointerSetMovedPartitions() in itemptr.h is not
used in the code. Could it be better to change the comment in
htup_details.h to mention HeapTupleHeaderSetMovedPartitions instead?
Perhaps not worth bothering, just noticed this on the way.
Hmm. Alternatively, maybe it'd make sense to change
HeapTupleHeaderSetMovedPartition to use ItemPointerSetMovedPartitions
instead of doing ItemPointerSet directly. But that looks mostly
pointless, since the extensibility aspect of this interface has been
superseded by table AM work. I agree we should just remove the macro
and update the comment as you suggest.
--
�lvaro Herrera Valdivia, Chile
On Sat, Feb 20, 2021 at 12:25:58PM -0300, Álvaro Herrera wrote:
On 2021-Feb-20, Michael Paquier wrote:
Hearing nothing, I have looked at this stuff and the simplification
makes sense. Any comments?No further comments ... I think the patch is simple enough. Thanks for
looking -- I'll get it pushed on Monday or so.
Sounds good to me.
I can see that ItemPointerSetMovedPartitions() in itemptr.h is not
used in the code. Could it be better to change the comment in
htup_details.h to mention HeapTupleHeaderSetMovedPartitions instead?
Perhaps not worth bothering, just noticed this on the way.Hmm. Alternatively, maybe it'd make sense to change
HeapTupleHeaderSetMovedPartition to use ItemPointerSetMovedPartitions
instead of doing ItemPointerSet directly. But that looks mostly
pointless, since the extensibility aspect of this interface has been
superseded by table AM work. I agree we should just remove the macro
and update the comment as you suggest.
Thanks. So you will do this change in the same commit, right?
--
Michael
On 2021-Feb-21, Michael Paquier wrote:
On Sat, Feb 20, 2021 at 12:25:58PM -0300, �lvaro Herrera wrote:
Hmm. Alternatively, maybe it'd make sense to change
HeapTupleHeaderSetMovedPartition to use ItemPointerSetMovedPartitions
instead of doing ItemPointerSet directly. But that looks mostly
pointless, since the extensibility aspect of this interface has been
superseded by table AM work. I agree we should just remove the macro
and update the comment as you suggest.Thanks. So you will do this change in the same commit, right?
I changed my mind on this after noticing that
ItemPointerIndicatesMovedPartitions has a few callers; leaving the
interface incomplete/asymmetric would be worse. So I propose to do
this.
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 7c62852e7f..86b3622b5e 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -443,11 +443,10 @@ do { \
) #define HeapTupleHeaderIndicatesMovedPartitions(tup) \
- (ItemPointerGetOffsetNumber(&(tup)->t_ctid) == MovedPartitionsOffsetNumber && \
- ItemPointerGetBlockNumberNoCheck(&(tup)->t_ctid) == MovedPartitionsBlockNumber)
+ ItemPointerIndicatesMovedPartitions(&(tup)->t_ctid) #define HeapTupleHeaderSetMovedPartitions(tup) \
- ItemPointerSet(&(tup)->t_ctid, MovedPartitionsBlockNumber, MovedPartitionsOffsetNumber)
+ ItemPointerSetMovedPartitions(&(tup)->t_ctid) #define HeapTupleHeaderGetDatumLength(tup) \
VARSIZE(tup)
--
�lvaro Herrera Valdivia, Chile
"�C�mo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germ�n Poo)
On Mon, Feb 22, 2021 at 05:15:57PM -0300, Álvaro Herrera wrote:
I changed my mind on this after noticing that
ItemPointerIndicatesMovedPartitions has a few callers; leaving the
interface incomplete/asymmetric would be worse. So I propose to do
this.
Doing that looks fine to me as well.
--
Michael
On 2021-Feb-24, Michael Paquier wrote:
On Mon, Feb 22, 2021 at 05:15:57PM -0300, �lvaro Herrera wrote:
I changed my mind on this after noticing that
ItemPointerIndicatesMovedPartitions has a few callers; leaving the
interface incomplete/asymmetric would be worse. So I propose to do
this.Doing that looks fine to me as well.
Thanks! Pushed.
--
�lvaro Herrera Valdivia, Chile
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor da�o posible: gerencia." (El principio Dilbert)