• Tom Lane's avatar
    Put back one copyObject() in rewriteTargetView(). · fd195257
    Tom Lane authored
    Commit 6f8cb1e2 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.
    fd195257
rewriteHandler.c 110 KB