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