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

Hello

>
>
> 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

>
>
> Details of compilation errors and regression failure:
> ------------------------------------------------------
> PATH : postgres/src/contrib/pg_stat_statements
> gcc -g -ggdb3 -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-strict-aliasing -fwrapv -fpic -I. -I.
> -I../../src/include -D_GNU_SOURCE -c -o pg_stat_statements.o
> pg_stat_statements.c
> pg_stat_statements.c: In function â_PG_initâ:
> pg_stat_statements.c:363: warning: assignment from incompatible pointer type
> pg_stat_statements.c: In function âpgss_ProcessUtilityâ:
> pg_stat_statements.c:823: error: too few arguments to function
> âprev_ProcessUtilityâ
> pg_stat_statements.c:826: error: too few arguments to function
> âstandard_ProcessUtilityâ
> pg_stat_statements.c:884: error: too few arguments to function
> âprev_ProcessUtilityâ
> pg_stat_statements.c:887: error: too few arguments to function
> âstandard_ProcessUtilityâ
> make: *** [pg_stat_statements.o] Error 1
>
> PATH : postgres/src/contrib/sepgsql
> gcc -g -ggdb3 -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-strict-aliasing -fwrapv -fpic -I. -I.
> -I../../src/include -D_GNU_SOURCE -c -o hooks.o hooks.c
> In file included from hooks.c:26:
> sepgsql.h:17:29: error: selinux/selinux.h: No such file or directory
> sepgsql.h:18:25: error: selinux/avc.h: No such file or directory
> In file included from hooks.c:26:
> sepgsql.h:239: warning: âstruct av_decisionâ declared inside parameter list
> sepgsql.h:239: warning: its scope is only this definition or declaration,
> which is probably not what you want
> hooks.c: In function âsepgsql_utility_commandâ:
> hooks.c:331: error: too few arguments to function ânext_ProcessUtility_hookâ
> hooks.c:334: error: too few arguments to function âstandard_ProcessUtilityâ
> hooks.c: In function â_PG_initâ:
> hooks.c:365: warning: implicit declaration of function âis_selinux_enabledâ
> hooks.c:426: warning: assignment from incompatible pointer type
> make: *** [hooks.o] Error 1
>
> REGRESSION TEST:
> ------------------
> make installcheck
> test copy ... FAILED
> ========================
> 1 of 132 tests failed.
> ========================
> ~/postgres/src/test/regress> cat regression.diffs
> *** /home/postgres/code/src/test/regress/expected/copy.out 2012-09-18
> 19:57:02.000000000 +0530
> --- /home/postgres/code/src/test/regress/results/copy.out 2012-09-18
> 19:57:18.000000000 +0530
> ***************
> *** 71,73 ****
> --- 71,80 ----
> c1,"col with , comma","col with "" quote"
> 1,a,1
> 2,b,2
> + -- copy should to return processed rows
> + do $$
> +
> + ERROR: unterminated dollar-quoted string at or near "$$
> + "
> + LINE 1: do $$
> + ^
>

fixed

>
> 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.
>
> 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)

>
> 3. Why new variable added in portal [portal->processed] this is not used in
> code ?

This value can be used anywhere instead parsing completionTag to get
returned numbers

>
>
> Testing:
> ------------
> The following test is carried out on the patch.
> 1. Normal SQL command COPY FROM / TO is tested.
> 2. SQL command COPY FROM / TO is tested from plpgsql.
>
> Test cases are attached in Testcases_Copy_Processed.txt
>

Regards

Pavel

>
>
> 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
>