Horton

Thread

Commits

  1. Rework HeapTupleHeader macros to reuse itemptr.h

  2. Remove pointless HeapTupleHeaderIndicatesMovedPartitions calls

  3. tableam: Add tuple_{insert, delete, update, lock} and use.

  1. some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-29T16:44:11Z

    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

  2. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Pavan Deolasee <pavan.deolasee@gmail.com> — 2021-01-18T04:57:11Z

    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

  3. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Anastasia Lubennikova <a.lubennikova@postgrespro.ru> — 2021-02-12T16:42:26Z

    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

  4. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Michael Paquier <michael@paquier.xyz> — 2021-02-13T01:49:26Z

    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

  5. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Michael Paquier <michael@paquier.xyz> — 2021-02-20T12:58:04Z

    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

  6. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-02-20T15:25:58Z

    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

  7. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Michael Paquier <michael@paquier.xyz> — 2021-02-20T23:18:38Z

    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

  8. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-02-22T20:15:57Z

    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)

  9. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Michael Paquier <michael@paquier.xyz> — 2021-02-24T02:27:40Z

    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

  10. Re: some pointless HeapTupleHeaderIndicatesMovedPartitions calls

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-25T18:52:06Z

    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)