Re: Support for REINDEX CONCURRENTLY

On Fri, Jun 28, 2013 at 4:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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...

Why did you remove the check of indisvalid from the --binary-upgrade SQL?
Without this check, if there is the invalid toast index, more than one rows are
returned and ExecuteSqlQueryForSingleRow() would cause the error.

+    foreach(lc, indexlist)
+        *toastidxs[i++] = index_open(lfirst_oid(lc), lock);

*toastidxs[i++] should be (*toastidxs)[i++]. Otherwise, segmentation fault can
happen.

For now I've not found any other big problem except the above.

Regards,

-- 
Fujii Masao