Re: ToDo: allow to get a number of processed rows by COPY statement [Review of Patch] - Mailing list pgsql-hackers

On Friday, September 21, 2012 1:23 AM Pavel Stehule wrote:

>> Basic stuff:
>> ------------
>> - Patch applies OK. but offset difference in line numbers.
>> - Compiles with errors in contrib [pg_stat_statements, sepgsql] modules
>> - Regression failed; one test-case in COPY due to incomplete test-case
>> attached patch. – same as reported by Heikki
>
>fixed patch is in attachment

After modifications:
---------------------------    - Patch applies OK    - Compiles cleanly without any errors/warnings    - Regression
testspass.  

>>
>> What it does:
>> --------------
>> Modification to get the number of processed rows evaluated via SPI. The
>> changes are to add extra parameter in ProcessUtility to get the number of
>> rows processed by COPY command.
>>
>> Code Review Comments:
>> ---------------------
>> 1. New parameter is added to ProcessUtility_hook_type function
>>    but the functions which get assigned to these functions like
>>    sepgsql_utility_command, pgss_ProcessUtility, prototype & definition is
>> not modified.
Functionality is not fixed correctly for hook functions, In function pgss_ProcessUtility
for bellow snippet of code processed parameter is passed NULL, as well as not initialized.
because of this when "pg_stat_statements" extention is utilized COPY command is giving garbage values.        if
(prev_ProcessUtility)               prev_ProcessUtility(parsetree, queryString, params,
                      dest, completionTag, context, NULL);        else
standard_ProcessUtility(parsetree,queryString, params,                                                        dest,
completionTag,context, NULL);           Testcase is attached.    In this testcase table has only 1000 records but it
showgarbage value.        postgres=# show shared_preload_libraries ;                           shared_preload_libraries
      --------------------------                                           pg_stat_statements        (1 row)
postgres=#CREATE TABLE tbl (a int);        CREATE TABLE        postgres=# INSERT INTO tbl
VALUES(generate_series(1,1000));       INSERT 0 1000        postgres=# do $$        declare r int;        begin
copy tbl to '/home/kiran/copytest.csv' csv;          get diagnostics r = row_count;          raise notice 'exported %
rows',r;          truncate tbl;          copy tbl from '/home/kiran/copytest.csv' csv;          get diagnostics r =
row_count;         raise notice 'imported % rows', r;        end;        $$ language plpgsql;        postgres$#
NOTICE: exported 13281616 rows        NOTICE:  imported 13281616 rows        DO  

>>
>> 2. Why to add the new parameter if completionTag hold the number of
>> processed tuple information; can be extracted
>>
>>    from it as follows:
>>                     _SPI_current->processed = strtoul(completionTag + 7,
>> NULL, 10);
>
>this is basic question. I prefer a natural type for counter - uint64
>instead text. And there are no simply way to get offset (7 in this
>case)
   I agree with your point, but currently in few other places we are parsing the completion tag for getting number of
tuplesprocessed. So may be in future we can change those places as well. For example 

pgss_ProcessUtility
{
..

/* parse command tag to retrieve the number of affected rows. */
if (completionTag &&                                                    sscanf(completionTag, "COPY " UINT64_FORMAT,
&rows)!= 1)          rows = 0; 

}

_SPI_execute_plan
{
..
..
if (IsA(stmt, CreateTableAsStmt))
{        Assert(strncmp(completionTag, "SELECT ", 7) == 0);        _SPI_current->processed = strtoul(completionTag + 7,
                                                                        NULL, 10); 

..
}


With Regards,
Amit Kapila.