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