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