Re: COPY table FROM STDIN doesn't show count tag

From: Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com>
To: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: COPY table FROM STDIN doesn't show count tag
Date: 2013-12-12 11:29:31
Message-ID: BF2827DCCE55594C8D7A8F7FFD3AB7713DDB15F8@SZXEML508-MBX.china.huawei.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 12th December 2013, Rajeev Rastogi Wrote:
>On 9th December, Amit Khandelkar wrote:

>>1. slashcopyissuev1.patch :- This patch fixes the \COPY issue.
>>You have removed the if condition in this statement, mentioning that it is always true now:
>>- if (copystream == pset.cur_cmd_source)
>>- pset.lineno++;
>>+ pset.lineno++;
>>
> >But copystream can be different than pset.cur_cmd_source , right ?

>As per the earlier code, condition result was always true. So pset.lineno was always incremented.
>In the earlier code pset.cur_cmd_source was sent as parameter to function and inside the function same parameter was used with the name copystream. So on entry of this function both will be one and same.
>I checked inside the function handleCopyIn, both of these parameters are not changing before above check. Also since pset is specific to single session, so it cannot change concurrently.
>Please let me know, if I am missing something.

>>+ FILE *copyStream; /* Stream to read/write for copy command */
>>
>>There is no tab between FILE and *copystream, hence it is not aligned.

>OK. I shall change accodingly.

I ran pgindent on settings.h file but found no issue reported. Seems tab is not mandatory in between variable declaration.
Even for other parameters also in psqlSetting structure space instead of tab is being used.
So seems no change required for this. Please confirm.

>>2. initialcopyissuev1_ontopofslashcopy.patch : Fix for "COPY table FROM STDIN/STDOUT doesn't show count tag".
>>The following header comments of ProcessResult() need to be modified:
>>* Changes its argument to point to the last PGresult of the command string,
>>* or NULL if that result was for a COPY FROM STDIN or COPY TO STDOUT.

>OK. I shall change accodingly.
I have changed it in the latest patch.

>>Regression results show all passed.
>>Other than this, the patch needs a new regression test.

>I had checked the existing regression test cases and observed that it has already got all kind of test cases. Like
>copy....stdin,
>copy....stdout,
>\copy.....stdin
>\copy.....stdout.

>But since as regression framework runs in "quite i.e. -q" mode, so it does not show any message except query output.
>So our new code change does not impact regression framework.

>Please let me know if you were expecting any other test cases?

Summary of two patches:

1. slashcopyissuev1.patch :- No change in this patch (same as earlier).

2. Initialcopyissuev2_ontopofslashcopy.patch : This patch is modified to change comment as per above review comments.

Please provide your opinion or let me know if any other changes are required.

Thanks and Regards,
Kumar Rajeev Rastogi

Attachment Content-Type Size
slashcopyissuev1.patch application/octet-stream 3.8 KB
initialcopyissuev2_ontopofslashcopy.patch application/octet-stream 6.3 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2013-12-12 12:04:55 Re: Changeset Extraction Interfaces
Previous Message Simon Riggs 2013-12-12 11:27:06 Re: Time-Delayed Standbys