Re: [Postgres-xc-developers] Param handling refactoring - Part 1

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Hi Amit,
The patch looks good and is not the track for parameter handling. I see
that we are relying more on the data produced by PG and standard planner
rather than infering ourselves in XC. So, this looks good improvement.

Here are my comments
Tests
-----
1. It seems testing the parameter handling for queries arising from plpgsql
   functions. The function prm_func() seems to be doing that. Can you
please add
   some comments in this function specifying what is being tested in
various sets
   of statements in function.
2. Also, it seems to be using two tables prm_emp1 and prm_emp2. The first
one is
   being used to populate the other one and a variable inside the function.
   Later only the other is being used. Can we just use a single table
instead of
   two?
3. Can we use an existing volatile function instead of a new one like
prm_volfunc()?

Code
----
1. Please use prefixes rq_ and rqs_ for the newly added members of
RemoteQuery
   and RemoteQueryState structures resp. This allows to locate the usage of
   these members easily through cscope/tag etc. As a general rule, we should
   always add a prefix for members of commonly used structures or members
which
   use very common variable names. rq_, rqs_, en_ are being used for
   RemoteQuery, RemoteQueryState and ExecNodes resp.
2. Is it possible to infer value of has_internal_params from rest of the
members
   of RemoteQuery structure? If so, can we drop this member and use
inference
   logic?
3. Following code needs more commenting in DMLParamListToDataRow()
5027     /* Set the remote param types if they are not already set */
   The code below, this comments seems to execute only the first time the
   RemoteQueryState is used. Please elaborate this in the comment, lest the
   reader is confused as to when this case can happen.
4. In code below
5098         /* copy data to the buffer */
5099         *datarow = palloc(buf.len);
5100         memcpy(*datarow, buf.data, buf.len);
5101         rq_state->paramval_len = buf.len;
5102         pfree(buf.data);
   Can we use datarow = buf.data. The memory context in both the cases will
have
   same life. We will save calls to palloc, pfree and memcpy. You can add
   comments about why this assignment is safe. We do this type of
assignment at
   other places too. See pgxc_rqplan_build_statement. Similar change is
needed
   in ExternParamListToDataRow().
5. More elaboration needed in prologue of DMLParamListToDataRow(). See some
   hints below. We need to elaborate on the purpose of such conversion.
Name of the
   function is misleading, there is not ParamList involved to convert from.
We are
   converting from TupleSlot.
5011 /* --------------------------------
5012  * DMLParamListToDataRow
5013  * Obtain a copy of <given> slot's data row <in what form?>, and copy
it into
5014  * <passed in/given> RemoteQueryState.paramval_data. Also set
remote_param_types <to what?>
5015  * The slot itself is undisturbed.
5016  * --------------------------------
6. Variable declarations in DMLParamListToDataRow() need to aligned. We
align
   the start of declaration and the variable names themselves.
7. In create_remotedml_plan(), we were using SetRemoteStatementName to have
all
   the parameter setting in one place. But you have instead set them
explicitly
   in the function itself. Can you please revert back the change? The
   remote_param_types set here are being over-written in
   DMLParamListToDataRow(). What if the param types/param numbers obtained
in both these
   functions are different? Can we add some asserts to check this?

On Tue, Feb 26, 2013 at 9:51 AM, Amit Khandekar <
ami...@en...> wrote:

> There has been errors like :
> "Cannot find parameter $4" or
> "Bind supplies 4 parameters while Prepare needs 8 parameters" that we
> have been getting for specific scenarios. These scenarios come up in
> plpgsql functions. This is the root cause:
>
> If PLpgSQL_datum.dtype is not a simple type (PLPGSQL_DTYPE_VAR), the
> parameter types (ParamExternData.ptype) for such plpgsql functions are
> not set until when the values are actually populated. Example of such
> variables is record variable without %rowtype specification. The
> ParamListInfo.paramFetch hook function is called when needed to fetch
> the such parameter types. In the XC function
> pgxc_set_remote_parameters(), we do not consider this, and we check
> only the ParamExternData.ptype to see if parameters are present, and
> end up with lesser parameters than the actual parameters, sometimes
> even ending up with 0 parameter types.
>
> During trigger support implementation, it was discovered that due to
> this issue,
> the NEW.field or OLD.field cannot be used directly in SQL statements.
>
> Actually we don't even need parameter types to be set at plan time in
> XC. We only need them at the BIND message. There, we can anyway infer
> the types from the tuple descriptor. So the attached patch removes all
> the places where parameter types are set, and derives them when the
> BIND data row is built.
>
> I have not touched the SetRemoteStatementName function in this patch.
> There can be scenarios where user calls PREPARE using parameter types,
> and in such cases it is better to use these parameters in
> SetRemoteStatementName() being called from BuildCachedPlan with
> non-NULL boundParams. Actually use of parameter types during PREPARE
> and rebuilding cached plans etc will be dealt further after this one.
> So, I haven't removed param types altogether.
>
> We also need to know whether the parameters are supplied through
> source data plan (DMLs) or they are external. So added a field
> has_internal_params in RemoteQuery to make this difference explicit.
> Data row and parameters types are built in a different manner for DMLs
> and non-DMLs.
>
> Moved the datarow generation function from execTuples.c to execRemote.c .
>
> Regressions
> -----------------
>
> There is a parameter related error in plpgsql.sql test, which does not
> occur now, so corrected the expected output. It still does not show
> the exact output because of absence of trigger support.
>
> Added new test xc_params.sql which would be further extended later.
>
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_d2d_feb
> _______________________________________________
> Postgres-xc-developers mailing list
> Pos...@li...
> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers
>
>

-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company