Re: [v9.2] Fix leaky-view problem, part 1

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.

> +     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?

> + }

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?

Thanks,
nm