pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedsome pointless HeapTupleHeaderIndicatesMovedPartitions calls
10+ messages / 5 participants
[nested] [flat]
* some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2020-09-29 16:44 Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 10+ messages in thread
From: Alvaro Herrera @ 2020-09-29 16:44 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Pavan Deolasee <pavan.deolasee@2ndQuadrant.com>; Andres Freund <andres@anarazel.de>
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:
[text/x-diff] 0001-Remove-pointless-HeapTupleHeaderIndicatesMovedPartit.patch (3.5K, ../../20200929164411.GA15497@alvherre.pgsql/2-0001-Remove-pointless-HeapTupleHeaderIndicatesMovedPartit.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-01-18 04:57 Pavan Deolasee <pavan.deolasee@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 10+ messages in thread
From: Pavan Deolasee @ 2021-01-18 04:57 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; Pavan Deolasee <pavan.deolasee@2ndquadrant.com>; Andres Freund <andres@anarazel.de>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-12 16:42 Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
parent: Pavan Deolasee <pavan.deolasee@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Anastasia Lubennikova @ 2021-02-12 16:42 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Álvaro Herrera <alvherre@alvh.no-ip.org>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-13 01:49 Michael Paquier <michael@paquier.xyz>
parent: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2021-02-13 01:49 UTC (permalink / raw)
To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; +Cc: pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@alvh.no-ip.org>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YCcwJhoiK%2FE06C9e@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-20 12:58 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2021-02-20 12:58 UTC (permalink / raw)
To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; +Cc: pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@alvh.no-ip.org>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YDEHXEKurDw2%2FwvK@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-20 15:25 Álvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Álvaro Herrera @ 2021-02-20 15:25 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-20 23:18 Michael Paquier <michael@paquier.xyz>
parent: Álvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2021-02-20 23:18 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YDGYzjlHgUSPn%2FuR@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-22 20:15 Álvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Álvaro Herrera @ 2021-02-22 20:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
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)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-02-24 02:27 Michael Paquier <michael@paquier.xyz>
parent: Álvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2021-02-24 02:27 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YDW5nE8tSI6XRXR5@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls
@ 2021-03-25 18:52 Álvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 10+ messages in thread
From: Álvaro Herrera @ 2021-03-25 18:52 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
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)
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2021-03-25 18:52 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 16:44 some pointless HeapTupleHeaderIndicatesMovedPartitions calls Alvaro Herrera <alvherre@2ndquadrant.com>
2021-01-18 04:57 ` Pavan Deolasee <pavan.deolasee@gmail.com>
2021-02-12 16:42 ` Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
2021-02-13 01:49 ` Michael Paquier <michael@paquier.xyz>
2021-02-20 12:58 ` Michael Paquier <michael@paquier.xyz>
2021-02-20 15:25 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
2021-02-20 23:18 ` Michael Paquier <michael@paquier.xyz>
2021-02-22 20:15 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
2021-02-24 02:27 ` Michael Paquier <michael@paquier.xyz>
2021-03-25 18:52 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox