Re: [v9.2] Fix leaky-view problem, part 1 - Mailing list pgsql-hackers

2011/7/9 Noah Misch <noah@2ndquadrant.com>:
> On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
>> The attached patch is a revised version according to the approach that updates
>> pg_class system catalog before AlterTableInternal().
>> It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
>> null on the pg_class.reloptions of the view and increments command counter.
>
>> + /*
>> +  * ResetViewOptions
>> +  *
>> +  * It clears all the reloptions prior to replacing
>> +  */
>> + static void
>> + ResetViewOptions(Oid viewOid)
>> + {
>> +     Relation        pg_class;
>> +     HeapTuple       oldtup;
>> +     HeapTuple       newtup;
>> +     Datum           values[Natts_pg_class];
>> +     bool            nulls[Natts_pg_class];
>> +     bool            replaces[Natts_pg_class];
>> +
>> +     pg_class = heap_open(RelationRelationId, RowExclusiveLock);
>> +
>> +     oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
>
> Use SearchSysCacheCopy1, since you're modifying the tuple.
>
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.

>> +     if (!HeapTupleIsValid(oldtup))
>> +             elog(ERROR, "cache lookup failed for relation %u", viewOid);
>> +
>> +     memset(values, 0, sizeof(values));
>> +     memset(nulls, false, sizeof(nulls));
>> +     memset(replaces, false, sizeof(replaces));
>> +
>> +     replaces[Anum_pg_class_reloptions - 1] = true;
>> +     nulls[Anum_pg_class_reloptions - 1] = true;
>> +
>> +     newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
>> +                                                        values, nulls, replaces);
>> +     simple_heap_update(pg_class, &newtup->t_self, newtup);
>> +
>> +     CatalogUpdateIndexes(pg_class, newtup);
>> +
>> +     ReleaseSysCache(oldtup);
>> +
>> +     heap_close(pg_class, RowExclusiveLock);
>> +
>> +     CommandCounterIncrement();
>
> Why is a CCI necessary?
>
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.

>> + }
>
> In any event, we seem to be converging on a version of parts 0 and 1 that are
> ready for committer.  However, Robert contends that this will not be committed
> separately from part 2.  Unless someone wishes to contest that, I suggest we
> mark this Returned with Feedback and let the CF entry for part 2 subsume its
> future development.  Does that sound reasonable?
>
At least, it seems to me we don't need to tackle to this matter from
the beginning
on the next commit fest again.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>