Request a Demo
Home   >   mailing lists

Re: Fix parameters order for relation_copy_for_cluster - Mailing list pgsql-hackers

From Pavel Borisov
Subject Re: Fix parameters order for relation_copy_for_cluster
Date
Msg-id CALT9ZEGPcVjZNAii7QVuE_yZsADFLXPzvtZj-CTLiOdi2u-CHg@mail.gmail.com
Whole thread
In response to [MASSMAIL]Fix parameters order for relation_copy_for_cluster  (Japin Li <japinli@hotmail.com>)
Responses Re: Fix parameters order for relation_copy_for_cluster
List pgsql-hackers
Tree view
Hi, Japin!

On Mon, 1 Apr 2024 at 12:15, Japin Li <japinli@hotmail.com> wrote:

Hi,

When attempting to implement a new table access method, I discovered that
relation_copy_for_cluster() has the following declaration:


    void        (*relation_copy_for_cluster) (Relation NewTable,
                                              Relation OldTable,
                                              Relation OldIndex,
                                              bool use_sort,
                                              TransactionId OldestXmin,
                                              TransactionId *xid_cutoff,
                                              MultiXactId *multi_cutoff,
                                              double *num_tuples,
                                              double *tups_vacuumed,
                                              double *tups_recently_dead);

It claims that the first parameter is a new table, and the second one is an
old table.  However, the table_relation_copy_for_cluster() uses the first
parameter as the old table, and the second as a new table, see below:

static inline void
table_relation_copy_for_cluster(Relation OldTable, Relation NewTable,
                                Relation OldIndex,
                                bool use_sort,
                                TransactionId OldestXmin,
                                TransactionId *xid_cutoff,
                                MultiXactId *multi_cutoff,
                                double *num_tuples,
                                double *tups_vacuumed,
                                double *tups_recently_dead)
{
    OldTable->rd_tableam->relation_copy_for_cluster(OldTable, NewTable, OldIndex,
                                                    use_sort, OldestXmin,
                                                    xid_cutoff, multi_cutoff,
                                                    num_tuples, tups_vacuumed,
                                                    tups_recently_dead);
}

It's a bit confusing, so attach a patch to fix this.

I've looked into your patch. All callers of  *_relation_copy_for_cluster now use Relation OldTable, Relation NewTable order. It coincides to what is expected by the function, no now code is not broken. The only wrong thing is naming of arguments in declaration of this function in tableam.h I think this is a minor oversight from original commit d25f519107b 

Provided all the above I'd recommend committing this catch. This is for clarity only, no changes in code behavior. 

Thank you for finding this!

Best regards,
Pavel Borisov
Supabase

pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: Popcount optimization using AVX512
Next
From: Ashutosh Bapat
Date:
Subject: Re: Statistics Import and Export