Обсуждение: Remove redundant code in pl_exec.c

Hi, hackers

I found there are some redundant code in pl_exec.c,
plpgsql_param_eval_generic_ro is same as plpgsql_param_eval_generic
except it invokes MakeExpandedObjectReadOnly.

IMO, we can invoke plpgsql_param_eval_generic in plpgsql_param_eval_generic_ro
to avoid the redundant.

Is there something I missed?  Any thoughts?

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 7bd2a9fff1..543419d3da 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -6673,34 +6673,7 @@ static void
 plpgsql_param_eval_generic_ro(ExprState *state, ExprEvalStep *op,
                               ExprContext *econtext)
 {
-    ParamListInfo params;
-    PLpgSQL_execstate *estate;
-    int            dno = op->d.cparam.paramid - 1;
-    PLpgSQL_datum *datum;
-    Oid            datumtype;
-    int32        datumtypmod;
-
-    /* fetch back the hook data */
-    params = econtext->ecxt_param_list_info;
-    estate = (PLpgSQL_execstate *) params->paramFetchArg;
-    Assert(dno >= 0 && dno < estate->ndatums);
-
-    /* now we can access the target datum */
-    datum = estate->datums[dno];
-
-    /* fetch datum's value */
-    exec_eval_datum(estate, datum,
-                    &datumtype, &datumtypmod,
-                    op->resvalue, op->resnull);
-
-    /* safety check -- needed for, eg, record fields */
-    if (unlikely(datumtype != op->d.cparam.paramtype))
-        ereport(ERROR,
-                (errcode(ERRCODE_DATATYPE_MISMATCH),
-                 errmsg("type of parameter %d (%s) does not match that when preparing the plan (%s)",
-                        op->d.cparam.paramid,
-                        format_type_be(datumtype),
-                        format_type_be(op->d.cparam.paramtype))));
+    plpgsql_param_eval_generic(state, op, econtext);
 
     /* force the value to read-only */
     *op->resvalue = MakeExpandedObjectReadOnly(*op->resvalue,

Japin Li <japinli@hotmail.com> writes:
> I found there are some redundant code in pl_exec.c,
> plpgsql_param_eval_generic_ro is same as plpgsql_param_eval_generic
> except it invokes MakeExpandedObjectReadOnly.

Which is exactly why it's NOT redundant.

> IMO, we can invoke plpgsql_param_eval_generic in plpgsql_param_eval_generic_ro
> to avoid the redundant.

I don't like this particularly --- it puts way too much premium on
the happenstance that the MakeExpandedObjectReadOnly call is the
very last step in the callback function.  If that needed to change,
we'd have a mess.

            regards, tom lane

On Fri, 09 Sep 2022 at 23:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Japin Li <japinli@hotmail.com> writes:
>> IMO, we can invoke plpgsql_param_eval_generic in plpgsql_param_eval_generic_ro
>> to avoid the redundant.
>
> I don't like this particularly --- it puts way too much premium on
> the happenstance that the MakeExpandedObjectReadOnly call is the
> very last step in the callback function.  If that needed to change,
> we'd have a mess.
>

Sorry, I don't get your mind.  Could you explain it more?  Thanks in advance!

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:
> On Fri, 09 Sep 2022 at 23:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't like this particularly --- it puts way too much premium on
>> the happenstance that the MakeExpandedObjectReadOnly call is the
>> very last step in the callback function.  If that needed to change,
>> we'd have a mess.

> Sorry, I don't get your mind.  Could you explain it more?  Thanks in advance!

This refactoring cannot support the situation where there is more
code to execute after MakeExpandedObjectReadOnly.

            regards, tom lane