Re: ToDo: allow to get a number of processed rows by COPY statement [Review of Patch]

2012/9/28 Amit Kapila <amit(dot)kapila(at)huawei(dot)com>:
>> On Friday, September 28, 2012 2:28 AM Pavel Stehule wrote:
>> Hello
>>
>> I reduced this patch as just plpgsql (SPI) problem solution.
>>
>> Correct generic solution needs a discussion about enhancing our libpq
>> interface - we can take a number of returned rows, but we should to get
>> number of processed rows too. A users should to parse this number from
>> completationTag, but this problem is not too hot and usually is not
>> blocker, because anybody can process completationTag usually.
>>
>> So this issue is primary for PL/pgSQL, where this information is not
>> possible. There is precedent - CREATE AS SELECT (thanks for sample :)),
>> and COPY should to have same impact on ROW_COUNT like this statement
>> (last patch implements it).
>
> IMO now this patch is okay. I have marked it as Ready For Committer.

Thank you very much for review

Regards

Pavel

>
> With Regards,
> Amit Kapila.
>
>
>>
>>
>> 2012/9/21 Amit Kapila <amit(dot)kapila(at)huawei(dot)com>:
>> > 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 tests pass.
>> >
>> >>>
>> >>> 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 show garbage
>> 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 tuples processed. So
>> > may be in future we can change those places as well. For example
>>
>> yes, this step can be done in future - together with libpq enhancing.
>> Is not adequate change API (and break lot of extensions) for this
>> isolated problem. So I propose fix plpgsql issue, and add to ToDo -
>> "enhance libpq to return a number of processed rows as number" - but
>> this change breaking compatibility.
>>
>> Pavel
>>
>> >
>> > 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.
>> >
>> >
>> >
>> > --
>> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org) To
>> > make changes to your subscription:
>> > http://www.postgresql.org/mailpref/pgsql-hackers
>