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

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.
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