Re: [Postgres-xc-developers] Review comments commit 5f3a271343b4754f7c28bd77db9e83e89631092c

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

Commit id 5aa7c108246079b2fee86512e025a2bd1454d87c addresses these points.
Details below.


On 5 April 2013 16:32, Ashutosh Bapat <ash...@en...>wrote:

> Hi Amit,
> This isn't your change but, The prologue of function
> SetDataRowForIntParams is using non-standard format of adding "----" at the
> start and end of prologue. Please remove those.
>

Done.


>
> sourceSlot and newSlot seem to be generic names in the context of function
> SetDataRowForIntParams() which says "Form a bind row for internal
> parameters". Since the function is going to change a bit, it will take some
> data from one slot and some from other to create data row, I think we
> should change the previous names of variables/functions to convey changed
> symantic.
>

I could not find any better names other than sourceSlot and dataSlot. I
have renames newSlot to dataSlot. There is also the context of DML in this
function, so in that context these names convey the meaning.


>
> Please add prologues for function append_val and append_junkval(). These
> functions need better names like append_paramval or append_param_junkval
> etc., better if you add prefix pgxc_ that we are doing for all xc specific
> function.
>

Done.


>
> Instead of having macro SET_PARAM_TYPES (which makes debugging difficult),
> can you please use function? In fact, if we set parameters just after
> setting paramtypes_set?
>

Done. In another commit : 1539b54bc24c85639ff0cf76042db0098a189e31. That
commit was actually for a bug I discovered in binding the parameters.


>
> I see we have added jf_xc_node_id and jf_whole_row as storage for
> attribute numbers of corresponding fields from the source tuple. Please add
> some comments specifying their usage (may be why we need these extra fields
> apart from jf_junkAttNo)
>

Done.


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