Re: Fix parameters order for relation_copy_for_cluster

Поиск
Список
Период
Сортировка
От Pavel Borisov
Тема Re: Fix parameters order for relation_copy_for_cluster
Дата
Msg-id CALT9ZEGPcVjZNAii7QVuE_yZsADFLXPzvtZj-CTLiOdi2u-CHg@mail.gmail.com
обсуждение исходный текст
Ответ на [MASSMAIL]Fix parameters order for relation_copy_for_cluster  (Japin Li <japinli@hotmail.com>)
Ответы Re: Fix parameters order for relation_copy_for_cluster  (Pavel Borisov <pashkin.elfe@gmail.com>)
Список pgsql-hackers
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 по дате отправления:

Предыдущее
От: Alvaro Herrera
Дата:
Сообщение: Re: Popcount optimization using AVX512
Следующее
От: Ashutosh Bapat
Дата:
Сообщение: Re: Statistics Import and Export