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

From: Noah Misch <noah(at)2ndQuadrant(dot)com>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [v9.2] Fix leaky-view problem, part 1
Date: 2011-07-09 09:36:40
Message-ID: 20110709093639.GB2142@tornado.leadboat.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
> 2011/7/9 Noah Misch <noah(at)2ndquadrant(dot)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.

Ah, yes. Sorry for the noise.

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

Okay, thanks.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message hubert depesz lubaczewski 2011-07-09 10:29:25 Enhanced psql in core?
Previous Message Kohei KaiGai 2011-07-09 08:52:33 Re: [v9.2] Fix leaky-view problem, part 1