Re: [GENERAL] huge RAM use in multi-command ALTER of table heirarchy

On 2018/04/29 1:00, Justin Pryzby wrote:
> On Tue, Jul 18, 2017 at 07:26:30PM -0400, Tom Lane wrote:
>>> It's probably a bit late in the v10 cycle to be taking any risks in
>>> this area, but I'd vote for ripping out RememberToFreeTupleDescAtEOX
>>> as soon as the v11 cycle opens, unless someone can show an example
>>> of non-broken coding that requires it. (And if so, there ought to
>>> be a regression test incorporating that.)

Fwiw, I too thought it should be possible by now to get rid of
RememberToFreeTupleDescAtEOX, but I was able to come up with at least one
case where its existence prevents a crash. The test case is based on one
provided by Noah Misch a while ago when the code in question was
introduced as a measure for the problem he described:

https://www.postgresql.org/message-id/20130801005351.GA325106%40tornado.leadboat.com

Suppose in get_relation_constraints() we're processing a table that has 2
constraints, one which has ccvalid=true and another which has
ccvalid=false. Further suppose a concurrent session performs a VALIDATE
CONSTRAINT on the second constraint, which requires just
ShareUpdateExclusiveLock and so it succeeds, sending out a sinval. Back
in the first session, eval_const_expressions() performed on the first
constraint would end up in RelationClearRelation being called on the
table, wherein, since the 2nd constraint is now marked by valid by the
concurrent session, would cause rd_att to be swapped. Thus the pointer
into rd_att that get_relation_constraints possesses would be a dangling
pointer, if not for the call to RememberToFreeTupleDescAtEOX() from
RelationDestroyRelation().

If however get_relation_constraints() had incremented the reference count
of rd_att to begin with, this problem wouldn't have arisen. IOW, we'd
need to replace all the direct accesses to rd_att (including those via
RelationGetDescr, of course) by something that increments rd_att's
reference count, before concluding that RememberToFreeTupleDescAtEOX() is
unnecessary. I'm afraid that's a lot of places.

Thanks,
Amit