postgres-xc-developers

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

[Amit K. <ami...@en...>]

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.

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

[Ashutosh B. <ash...@en...>]

Hi Amit,
I am looking at this patch.

First I looked at the testcase added. 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. 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? Can we use an existing volatile function instead of a
new one like prm_volfunc()?

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

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

[Ashutosh B. <ash...@en...>]

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

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

[Amit K. <ami...@en...>]

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