Re: Fix parameters order for relation_copy_for_cluster

Hi, Japin!

On Mon, 1 Apr 2024 at 12:15, Japin Li <japinli(at)hotmail(dot)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