Re: [Postgres-xc-developers] Copy with default values (3477909)

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

On 13 April 2012 14:03, Michael Paquier <mic...@gm...> wrote:

> In the test xc_copy, perhaps pgxc_nodetype is dangerous written as is.
> The result provided by this function will be always C as due to
> self-registration, it is never empty.
> Could you modify it to fetch the type of the node directly based on the
> pgxc_node_name just for safety?
>



>
> In SQL, I would write it like below:
> postgres=# select node_type from pgxc_node where node_name =
> pgxc_node_str();
>  node_type
> -----------
>  C
> (1 row)
>

Oh, I forgot to mention about this issue I faced when I had first tried to
use similar query:

postgres=# select node_type from pgxc_node where node_name =
lower(pgxc_node_str());
 node_type
-----------
 C
(1 row)

 Strangely it is giving me C even on datanode :
postgres=# execute direct on data_node_1 $$ select * from pgxc_node $$ ;

 node_name  | node_type | node_port | node_host | nodeis_primary |
nodeis_preferred |  node_id
------------+-----------+-----------+-----------+----------------+------------------+------------
 datanode_1 | *C*         |      5432 | localhost | f              |
f                | -675012441
(1 row)


So I had written the new function pgxc_nodetype() as a workaround for the
above issue, but forgot to bring this point. Any idea why this is
occurring? Let me know what result you get for datanode query.

 I don't have any other comment, so once it is done commit is OK.
> Thanks,
>
>
> On Fri, Apr 13, 2012 at 4:15 PM, Amit Khandekar <
> ami...@en...> wrote:
>
>>
>>
>> On 13 April 2012 07:08, Michael Paquier <mic...@gm...>wrote:
>>
>>> That was fast. The content of the patch is great.
>>>
>>> I just have few comments.
>>> Your patch is not complete. serial_schedule mentions the test
>>> xc_copy_default but it is not included in the patch.
>>>
>>
>> Oh, I did git diff when the newly added files were in the commit list.
>> Added them in the attached patch.
>>
>> It may be perhaps better to rename this new test to a more generic name
>>> like xc_copy as we may include other copy-related tests in the future for
>>> XC.
>>>
>>
>> Agreed.
>>
>> You need to update parallel_schedule with this new test too, and knowing
>>> the purpose of this new test, I am pretty sure it is launchable in parallel
>>> with the other XC tests like xc_remote or xc_groupby.
>>>
>>
>> Did that.
>>
>>
>>>
>>>
>>>> Attached is a patch that contains support for both text and binary copy.
>>>>
>>>> In the coordinator, the default values are added to the data row to be
>>>> sent to the data node. This is done only if the default expression is not
>>>> shippable.
>>>>
>>> Yes this is nice, you are using the APIs of postgresql_fwd.h.
>>>
>>>> For e.g., if the expression is an immutable function, the default
>>>> values are not added. In this case, the values are evaluated and inserted
>>>> automatically in the datanodes, like how it was being done so far.
>>>>
>>>> There was an existing bug in binary format handling when there are NULL
>>>> values in the data. Each binary field starts with a field size, followed by
>>>> actual binary data. When the field size is -1, the data is NULL data.
>>>> Currently, in XC we skip the -1 field and do not pass anything to the
>>>> datanode for this field, when actually we should pass just the -1 without
>>>> data. This bug is corrected in the patch.
>>>>
>>> This is solving the XML issue mentionned in the other mail thread.
>>>
>>>  New regression test xc_copydefault.sql is added.
>>>>
>>>> copy2.sql already had serial datatype, but we seem to have ignored this
>>>> diff while creating expected/copy2_1.out. Now this file contains correct
>>>> data. I have modified copy2_1.out, and now it is passing.
>>>>
>>> OK. That is good to know.
>>>
>>> So once the new test is included and fixes are done, please go ahead and
>>> commit.
>>> I'll make modifications of the test xml on top of it.
>>>
>>> Thanks,
>>>
>>> --
>>> Michael Paquier
>>> http://michael.otacoo.com
>>>
>>
>>
>
>
> --
> Michael Paquier
> http://michael.otacoo.com
>