Horton

Thread

Commits

  1. Fix the parameters order for TableAmRoutine.relation_copy_for_cluster()

  2. tableam: relation creation, VACUUM FULL/CLUSTER, SET TABLESPACE.

  1. Fix parameters order for relation_copy_for_cluster

    Japin Li <japinli@hotmail.com> — 2024-04-01T08:12:45Z

    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.

--
Regards,
Japin Li

  2. Re: Fix parameters order for relation_copy_for_cluster

    Pavel Borisov <pashkin.elfe@gmail.com> — 2024-04-01T11:13:05Z

    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

  3. Re: Fix parameters order for relation_copy_for_cluster

    Pavel Borisov <pashkin.elfe@gmail.com> — 2024-04-01T12:11:10Z

    correction: so now code is not broken

>