8000 Fix error-cleanup mistakes in exec_stmt_call(). · postgrespro/postgres@f26c06a · GitHub
[go: up one dir, main page]
Skip to content

    • Commit f26c06a

    Browse files
    tglsfdctglsfdc
    committed
    Fix error-cleanup mistakes in exec_stmt_call().
    Commit 15c7293 was a couple bricks shy of a load: we need to ensure that expr->plan gets reset to NULL on any error exit, if it's not supposed to be saved. Also ensure that the stmt->target calculation gets redone if needed. The easy way to exhibit a problem is to set up code that violates the writable-argument restriction and then execute it twice. But error exits out of, eg, setup_param_list() could also break it. Make the existing PG_TRY block cover all of that code to be sure. Per report from Pavel Stehule. Discussion: https://postgr.es/m/CAFj8pRAeXNTO43W2Y0Cn0YOVFPv1WpYyOqQrrzUiN6s=dn7gCg@mail.gmail.com
    1 parent fa2952d commit f26c06a

    File tree

    1 file changed

    +45
    -37
    lines changed

    1 file changed

    +45
    -37
    lines changed

    ‎src/pl/plpgsql/src/pl_exec.c

    Lines changed: 45 additions & 37 deletions
    Original file line numberDiff line numberDiff line change
    @@ -2072,39 +2072,50 @@ static int
    20722072
    exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
    20732073
    {
    20742074
    PLpgSQL_expr *expr = stmt->expr;
    2075-
    ParamListInfo paramLI;
    2076-
    LocalTransactionId before_lxid;
    2075+
    volatile LocalTransactionId before_lxid;
    20772076
    LocalTransactionId after_lxid;
    2078-
    bool pushed_active_snap = false;
    2079-
    int rc;
    2077+
    volatile bool pushed_active_snap = false;
    2078+
    volatile int rc;
    20802079

    2081-
    if (expr->plan == NULL)
    2080+
    /* PG_TRY to ensure we clear the plan link, if needed, on failure */
    2081+
    PG_TRY();
    20822082
    {
    2083-
    SPIPlanPtr plan;
    2083+
    SPIPlanPtr plan = expr->plan;
    2084+
    ParamListInfo paramLI;
    20842085

    2085-
    /*
    2086-
    * Don't save the plan if not in atomic context. Otherwise,
    2087-
    * transaction ends would cause errors about plancache leaks. XXX
    2088-
    * This would be fixable with some plancache/resowner surgery
    2089-
    * elsewhere, but for now we'll just work around this here.
    2090-
    */
    2091-
    exec_prepare_plan(estate, expr, 0, estate->atomic);
    2086+
    if (plan == NULL)
    2087+
    {
    20922088

    2093-
    /*
    2094-
    * The procedure call could end transactions, which would upset the
    2095-
    * snapshot management in SPI_execute*, so don't let it do it.
    2096-
    * Instead, we set the snapshots ourselves below.
    2097-
    */
    2098-
    plan = expr->plan;
    2099-
    plan->no_snapshots = true;
    2089+
    /*
    2090+
    * Don't save the plan if not in atomic context. Otherwise,
    2091+
    * transaction ends would cause errors about plancache leaks.
    2092+
    *
    2093+
    * XXX This would be fixable with some plancache/resowner surgery
    2094+
    * elsewhere, but for now we'll just work around this here.
    2095+
    */
    2096+
    exec_prepare_plan(estate, expr, 0, estate->atomic);
    2097+
    2098+
    /*
    2099+
    * The procedure call could end transactions, which would upset
    2100+
    * the snapshot management in SPI_execute*, so don't let it do it.
    2101+
    * Instead, we set the snapshots ourselves below.
    2102+
    */
    2103+
    plan = expr->plan;
    2104+
    plan->no_snapshots = true;
    2105+
    2106+
    /*
    2107+
    * Force target to be recalculated whenever the plan changes, in
    2108+
    * case the procedure's argument list has changed.
    2109+
    */
    2110+
    stmt->target = NULL;
    2111+
    }
    21002112

    21012113
    /*
    21022114
    * We construct a DTYPE_ROW datum representing the plpgsql variables
    21032115
    * associated with the procedure's output arguments. Then we can use
    2104-
    * exec_move_row() to do the assignments. (We do this each time the
    2105-
    * plan changes, in case the procedure's argument list has changed.)
    2116+
    * exec_move_row() to do the assignments.
    21062117
    */
    2107-
    if (stmt->is_call)
    2118+
    if (stmt->is_call && stmt->target == NULL)
    21082119
    {
    21092120
    Node *node;
    21102121
    FuncExpr *funcexpr;
    @@ -2206,24 +2217,21 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
    22062217

    22072218
    stmt->target = (PLpgSQL_variable *) row;
    22082219
    }
    2209-
    }
    22102220

    2211-
    paramLI = setup_param_list(estate, expr);
    2221+
    paramLI = setup_param_list(estate, expr);
    22122222

    2213-
    before_lxid = MyProc->lxid;
    2223+
    before_lxid = MyProc->lxid;
    22142224

    2215-
    /*
    2216-
    * Set snapshot only for non-read-only procedures, similar to SPI
    2217-
    * behavior.
    2218-
    */
    2219-
    if (!estate->readonly_func)
    2220-
    {
    2221-
    PushActiveSnapshot(GetTransactionSnapshot());
    2222-
    pushed_active_snap = true;
    2223-
    }
    2225+
    /*
    2226+
    * Set snapshot only for non-read-only procedures, similar to SPI
    2227+
    * behavior.
    2228+
    */
    2229+
    if (!estate->readonly_func)
    2230+
    {
    2231+
    PushActiveSnapshot(GetTransactionSnapshot());
    2232+
    pushed_active_snap = true;
    2233+
    }
    22242234

    2225-
    PG_TRY();
    2226-
    {
    22272235
    rc = SPI_execute_plan_with_paramlist(expr->plan, paramLI,
    22282236
    estate->readonly_func, 0);
    22292237
    }

    0 commit comments

    Comments
     (0)
    0