Re: Support for REINDEX CONCURRENTLY

On Wed, Jun 26, 2013 at 1:06 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Thanks for updating the patch!
And thanks for taking time to look at that. I updated the patch
according to your comments, except for the VACUUM FULL problem. Please
see patch attached and below for more details.

> When I ran VACUUM FULL, I got the following error.
>
> ERROR:  attempt to apply a mapping to unmapped relation 16404
> STATEMENT:  vacuum full;
This can be reproduced when doing a vacuum full on pg_proc,
pg_shdescription or pg_db_role_setting for example, or relations that
have no relfilenode (mapped catalogs), and a toast relation. I still
have no idea what is happening here but I am looking at it. As this
patch removes reltoastidxid, could that removal have effect on the
relation mapping of mapped catalogs? Does someone have an idea?

> Could you let me clear why toast_save_datum needs to update even invalid toast
> index? It's required only for REINDEX CONCURRENTLY?
Because an invalid index might be marked as indisready, so ready to
receive inserts. Yes this is a requirement for REINDEX CONCURRENTLY,
and in a more general way a requirement for a relation that includes
in rd_indexlist indexes that are live, ready but not valid. Just based
on this remark I spotted a bug in my patch for tuptoaster.c where we
could insert a new index tuple entry in toast_save_datum for an index
live but not ready. Fixed that by adding an additional check to the
flag indisready before calling index_insert.

> @@ -1573,7 +1648,7 @@ toastid_valueid_exists(Oid toastrelid, Oid valueid)
>
>         toastrel = heap_open(toastrelid, AccessShareLock);
>
> -       result = toastrel_valueid_exists(toastrel, valueid);
> +       result = toastrel_valueid_exists(toastrel, valueid, AccessShareLock);
>
> toastid_valueid_exists() is used only in toast_save_datum(). So we should use
> RowExclusiveLock here rather than AccessShareLock?
Makes sense.

> + * toast_open_indexes
> + *
> + *     Get an array of index relations associated to the given toast relation
> + *     and return as well the position of the valid index used by the toast
> + *     relation in this array. It is the responsability of the caller of this
>
> Typo: responsibility
Done.

> toast_open_indexes(Relation toastrel,
> +                                  LOCKMODE lock,
> +                                  Relation **toastidxs,
> +                                  int *num_indexes)
> +{
> +       int                     i = 0;
> +       int                     res = 0;
> +       bool            found = false;
> +       List       *indexlist;
> +       ListCell   *lc;
> +
> +       /* Get index list of relation */
> +       indexlist = RelationGetIndexList(toastrel);
>
> What about adding the assertion which checks that the return value
> of RelationGetIndexList() is not NIL?
Done.

> When I ran pg_upgrade for the upgrade from 9.2 to HEAD (with patch),
> I got the following error. Without the patch, that succeeded.
>
> command: "/dav/reindex/bin/pg_dump" --host "/dav/reindex" --port 50432
> --username "postgres" --schema-only --quote-all-identifiers
> --binary-upgrade --format=custom
> --file="pg_upgrade_dump_12270.custom" "postgres" >>
> "pg_upgrade_dump_12270.log" 2>&1
> pg_dump: query returned 0 rows instead of one: SELECT c.reltoastrelid,
> t.indexrelid FROM pg_catalog.pg_class c LEFT JOIN pg_catalog.pg_index
> t ON (c.reltoastrelid = t.indrelid) WHERE c.oid =
> '16390'::pg_catalog.oid AND t.indisvalid;
This issue is reproducible easily by having more than 1 table using
toast indexes in the cluster to be upgraded. The error was on pg_dump
side when trying to do a binary upgrade. In order to fix that, I
changed the code binary_upgrade_set_pg_class_oids:pg_dump.c to fetch
the index associated to a toast relation only if there is a toast
relation. This adds one extra step in the process for each having a
toast relation, but makes the code clearer. Note that I checked
pg_upgrade down to 8.4...
--
Michael