Re: Support for REINDEX CONCURRENTLY

Hi,

On 2013-06-18 10:53:25 +0900, Michael Paquier wrote:
> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> index c381f11..3a6342c 100644
> --- a/contrib/pg_upgrade/info.c
> +++ b/contrib/pg_upgrade/info.c
> @@ -321,12 +321,17 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
>                                "INSERT INTO info_rels "
>                                "SELECT reltoastrelid "
>                                "FROM info_rels i JOIN pg_catalog.pg_class c "
> -                              "        ON i.reloid = c.oid"));
> +                              "        ON i.reloid = c.oid "
> +                              "        AND c.reltoastrelid != %u", InvalidOid));
>      PQclear(executeQueryOrDie(conn,
>                                "INSERT INTO info_rels "
> -                              "SELECT reltoastidxid "
> -                              "FROM info_rels i JOIN pg_catalog.pg_class c "
> -                              "        ON i.reloid = c.oid"));
> +                              "SELECT indexrelid "
> +                              "FROM pg_index "
> +                              "WHERE indrelid IN (SELECT reltoastrelid "
> +                              "          FROM pg_class "
> +                              "          WHERE oid >= %u "
> +                              "             AND reltoastrelid != %u)",
> +                              FirstNormalObjectId, InvalidOid));

What's the idea behind the >= here?

I think we should ignore the invalid indexes in that SELECT?


> @@ -1392,19 +1390,62 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
>      }
>  
>      /*
> -     * If we're swapping two toast tables by content, do the same for their
> -     * indexes.
> +     * If we're swapping two toast tables by content, do the same for all of
> +     * their indexes. The swap can actually be safely done only if the
> +     * relations have indexes.
>       */
>      if (swap_toast_by_content &&
> -        relform1->reltoastidxid && relform2->reltoastidxid)
> -        swap_relation_files(relform1->reltoastidxid,
> -                            relform2->reltoastidxid,
> -                            target_is_pg_class,
> -                            swap_toast_by_content,
> -                            is_internal,
> -                            InvalidTransactionId,
> -                            InvalidMultiXactId,
> -                            mapped_tables);
> +        relform1->reltoastrelid &&
> +        relform2->reltoastrelid)
> +    {
> +        Relation toastRel1, toastRel2;
> +
> +        /* Open relations */
> +        toastRel1 = heap_open(relform1->reltoastrelid, AccessExclusiveLock);
> +        toastRel2 = heap_open(relform2->reltoastrelid, AccessExclusiveLock);
> +
> +        /* Obtain index list */
> +        RelationGetIndexList(toastRel1);
> +        RelationGetIndexList(toastRel2);
> +
> +        /* Check if the swap is possible for all the toast indexes */
> +        if (list_length(toastRel1->rd_indexlist) == 1 &&
> +            list_length(toastRel2->rd_indexlist) == 1)
> +        {
> +            ListCell *lc1, *lc2;
> +
> +            /* Now swap each couple */
> +            lc2 = list_head(toastRel2->rd_indexlist);
> +            foreach(lc1, toastRel1->rd_indexlist)
> +            {
> +                Oid indexOid1 = lfirst_oid(lc1);
> +                Oid indexOid2 = lfirst_oid(lc2);
> +                swap_relation_files(indexOid1,
> +                                    indexOid2,
> +                                    target_is_pg_class,
> +                                    swap_toast_by_content,
> +                                    is_internal,
> +                                    InvalidTransactionId,
> +                                    InvalidMultiXactId,
> +                                    mapped_tables);
> +                lc2 = lnext(lc2);
> +            }

Why are you iterating over the indexlists after checking they are both
of length == 1? Looks like the code would be noticeably shorter without
that.

> +        }
> +        else
> +        {
> +            /*
> +             * As this code path is only taken by shared catalogs, who cannot
> +             * have multiple indexes on their toast relation, simply return
> +             * an error.
> +             */
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                     errmsg("cannot swap relation files of a shared catalog with multiple indexes on toast
relation")));
> +        }
> +

Absolutely minor thing, using an elog() seems to be better here since
that uses the appropriate error code for some codepath that's not
expected to be executed.

>      /* Clean up. */
>      heap_freetuple(reltup1);
> @@ -1529,12 +1570,13 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>          if (OidIsValid(newrel->rd_rel->reltoastrelid))
>          {
>              Relation    toastrel;
> -            Oid            toastidx;
>              char        NewToastName[NAMEDATALEN];
> +            ListCell   *lc;
> +            int            count = 0;
>  
>              toastrel = relation_open(newrel->rd_rel->reltoastrelid,
>                                       AccessShareLock);
> -            toastidx = toastrel->rd_rel->reltoastidxid;
> +            RelationGetIndexList(toastrel);
>              relation_close(toastrel, AccessShareLock);
>  
>              /* rename the toast table ... */
> @@ -1543,11 +1585,23 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>              RenameRelationInternal(newrel->rd_rel->reltoastrelid,
>                                     NewToastName, true);
>  
> -            /* ... and its index too */
> -            snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
> -                     OIDOldHeap);
> -            RenameRelationInternal(toastidx,
> -                                   NewToastName, true);
> +            /* ... and its indexes too */
> +            foreach(lc, toastrel->rd_indexlist)
> +            {
> +                /*
> +                 * The first index keeps the former toast name and the
> +                 * following entries have a suffix appended.
> +                 */
> +                if (count == 0)
> +                    snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
> +                             OIDOldHeap);
> +                else
> +                    snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index_%d",
> +                             OIDOldHeap, count);
> +                RenameRelationInternal(lfirst_oid(lc),
> +                                       NewToastName, true);
> +                count++;
> +            }
>          }
>          relation_close(newrel, NoLock);
>      }

Is it actually possible to get here with multiple toast indexes?


> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
> index ec956ad..ac42389 100644
> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -2781,16 +2781,16 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
>      Oid            pg_class_reltoastidxid;
>  
>      appendPQExpBuffer(upgrade_query,
> -                      "SELECT c.reltoastrelid, t.reltoastidxid "
> +                      "SELECT c.reltoastrelid, t.indexrelid "
>                        "FROM pg_catalog.pg_class c LEFT JOIN "
> -                      "pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) "
> -                      "WHERE c.oid = '%u'::pg_catalog.oid;",
> +                      "pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) "
> +                      "WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid;",
>                        pg_class_oid);

This possibly needs a version qualification due to querying
indisalid. How far back do we support pg_upgrade?

> diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
> index 8ac2549..31309ed 100644
> --- a/src/include/utils/relcache.h
> +++ b/src/include/utils/relcache.h
> @@ -29,6 +29,16 @@ typedef struct RelationData *Relation;
>  typedef Relation *RelationPtr;
>  
>  /*
> + * RelationGetIndexListIfValid
> + * Get index list of relation without recomputing it.
> + */
> +#define RelationGetIndexListIfValid(rel) \
> +do { \
> +    if (rel->rd_indexvalid == 0) \
> +        RelationGetIndexList(rel); \
> +} while(0)

Isn't this function misnamed and should be
RelationGetIndexListIfInValid?

Going to do some performance tests now.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services