• Tom Lane's avatar
    Avoid postgres_fdw crash for a targetlist entry that's just a Param. · 8cad5adb
    Tom Lane authored
    foreign_grouping_ok() is willing to put fairly arbitrary expressions into
    the targetlist of a remote SELECT that's doing grouping or aggregation on
    the remote side, including expressions that have no foreign component to
    them at all.  This is possibly a bit dubious from an efficiency standpoint;
    but it rises to the level of a crash-causing bug if the expression is just
    a Param or non-foreign Var.  In that case, the expression will necessarily
    also appear in the fdw_exprs list of values we need to send to the remote
    server, and then setrefs.c's set_foreignscan_references will mistakenly
    replace the fdw_exprs entry with a Var referencing the targetlist result.
    
    The root cause of this problem is bad design in commit e7cb7ee1: it put
    logic into set_foreignscan_references that IMV is postgres_fdw-specific,
    and yet this bug shows that it isn't postgres_fdw-specific enough.  The
    transformation being done on fdw_exprs assumes that fdw_exprs is to be
    evaluated with the fdw_scan_tlist as input, which is not how postgres_fdw
    uses it; yet it could be the right thing for some other FDW.  (In the
    bigger picture, setrefs.c has no business assuming this for the other
    expression fields of a ForeignScan either.)
    
    The right fix therefore would be to expand the FDW API so that the
    FDW could inform setrefs.c how it intends to evaluate these various
    expressions.  We can't change that in the back branches though, and we
    also can't just summarily change setrefs.c's behavior there, or we're
    likely to break external FDWs.
    
    As a stopgap, therefore, hack up postgres_fdw so that it won't attempt
    to send targetlist entries that look exactly like the fdw_exprs entries
    they'd produce.  In most cases this actually produces a superior plan,
    IMO, with less data needing to be transmitted and returned; so we probably
    ought to think harder about whether we should ship tlist expressions at
    all when they don't contain any foreign Vars or Aggs.  But that's an
    optimization not a bug fix so I left it for later.  One case where this
    produces an inferior plan is where the expression in question is actually
    a GROUP BY expression: then the restriction prevents us from using remote
    grouping.  It might be possible to work around that (since that would
    reduce to group-by-a-constant on the remote side); but it seems like a
    pretty unlikely corner case, so I'm not sure it's worth expending effort
    solely to improve that.  In any case the right long-term answer is to fix
    the API as sketched above, and then revert this hack.
    
    Per bug #15781 from Sean Johnston.  Back-patch to v10 where the problem
    was introduced.
    
    Discussion: https://postgr.es/m/15781-2601b1002bad087c@postgresql.org
    8cad5adb
deparse.c 93.2 KB