From: Tom Lane Date: Tue, 29 Dec 2015 21:45:47 +0000 (-0500) Subject: Put back one copyObject() in rewriteTargetView(). X-Git-Tag: REL9_6_BETA1~936 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=fd1952575618cacf7afa544d8b89ddb77be9eaee;p=postgresql Put back one copyObject() in rewriteTargetView(). Commit 6f8cb1e23485bd6d tried to centralize rewriteTargetView's copying of a target view's Query struct. However, it ignored the fact that the jointree->quals field was used twice. This only accidentally failed to fail immediately because the same ChangeVarNodes mutation is applied in both cases, so that we end up with logically identical expression trees for both uses (and, as the code stands, the second ChangeVarNodes call actually does nothing). However, we end up linking *physically* identical expression trees into both an RTE's securityQuals list and the WithCheckOption list. That's pretty dangerous, mainly because prepsecurity.c is utterly cavalier about further munging such structures without copying them first. There may be no live bug in HEAD as a consequence of the fact that we apply preprocess_expression in between here and prepsecurity.c, and that will make a copy of the tree anyway. Or it may just be that the regression tests happen to not trip over it. (I noticed this only because things fell over pretty badly when I tried to relocate the planner's call of expand_security_quals to before expression preprocessing.) In any case it's very fragile because if anyone tried to make the securityQuals and WithCheckOption trees diverge before we reach preprocess_expression, it would not work. The fact that the current code will preprocess securityQuals and WithCheckOptions lists at completely different times in different query levels does nothing to increase my trust that that can't happen. In view of the fact that 9.5.0 is almost upon us and the aforesaid commit has seen exactly zero field testing, the prudent course is to make an extra copy of the quals so that the behavior is not different from what has been in the field during beta. --- diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index bc29e1790e..315d00cfde 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -2968,6 +2968,13 @@ rewriteTargetView(Query *parsetree, Relation view) { Node *viewqual = (Node *) viewquery->jointree->quals; + /* + * Even though we copied viewquery already at the top of this + * function, we must duplicate the viewqual again here, because we may + * need to use the quals again below for a WithCheckOption clause. + */ + viewqual = copyObject(viewqual); + ChangeVarNodes(viewqual, base_rt_index, new_rt_index, 0); if (RelationIsSecurityView(view))