Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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)
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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 signature.asc Description: PGP signature
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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)
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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 signature.asc Description: PGP signature
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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 signature.asc Description: PGP signature
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
On Fri, Feb 12, 2021 at 04:42:26PM +, 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 signature.asc Description: PGP signature
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
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
Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
Hi Alvaro, On Tue, Sep 29, 2020 at 10:14 PM Alvaro Herrera 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
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 Herrera39°49'30"S 73°17'W
>From 953eceb701ca35c5a3c731c4318ed1465f205811 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
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(_self, _data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tp.t_data))
+ else if (!ItemPointerEquals(_self, _data->t_ctid))
result = TM_Updated;
else
result = TM_Deleted;
@@ -3248,8 +3247,7 @@ l2:
if (can_continue)
result = TM_Ok;
- else if (!ItemPointerEquals(_self, _data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(oldtup.t_data))
+ else if (!ItemPointerEquals(_self, _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(>t_self, >t_data->t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple->t_data))
+ else if (!ItemPointerEquals(>t_self, >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(>t_self, >t_data->t_ctid) ||
-HeapTupleHeaderIndicatesMovedPartitions(tup->t_data))
+ if (!ItemPointerEquals(>t_self, >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(>t_self, >t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(>t_self, >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(>t_self, >t_ctid) ||
-HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(>t_self, >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(>t_self, >t_ctid) ||
- HeapTupleHeaderIndicatesMovedPartitions(tuple))
+ if (!ItemPointerEquals(>t_self, >t_ctid))
return TM_Updated; /* updated by other */
else
return TM_Deleted; /* deleted by other */
--
2.20.1
