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

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

On 6 March 2013 14:16, Ashutosh Bapat <ash...@en...> wrote:
> 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()?

Done these changes. also added DELETE scenario.

>
> 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.

Done.

> 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?

Could not find any information that we can safely infer the param types from.

> 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.

Done.

> 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().

Right. Done.

> 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  * --------------------------------

Done. Also changed the names of the both internal and extern param functions.

> 6. Variable declarations in DMLParamListToDataRow() need to aligned. We
> align
>    the start of declaration and the variable names themselves.

Done. This was existing code. But corrected it.

> 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?

The remote_param_types set in create_remotedml_plan() belong to
RemoteQuery, whereas those that are set in DMLParamListToDataRow()
belong to RemoteQueryState, so they are not overwritten. But the
remote param types that are set in create_remotedml_plan are not
required. I have realized, that part is redundant, and I have removed
it. The internal params are inferred in DMLParamListToDataRow().
>
>
>
> 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