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

<div class="WordSection1"><p class="MsoPlainText"><span style="font-size:12.0pt;font-family:"Times New
Roman","serif"">On16.08.2012 14:43, Pavel Stehule wrote:</span><p class="MsoPlainText"><span
style="font-size:12.0pt;font-family:"TimesNew Roman","serif"">> Hello</span><p class="MsoPlainText"><span
style="font-size:12.0pt;font-family:"TimesNew Roman","serif"">> </span><p class="MsoPlainText"><span
style="font-size:12.0pt;font-family:"TimesNew Roman","serif"">> here is updated patch</span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif""> </span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif"">[<b>Review of Patch</b>]</span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif""> </span><p
class="MsoNormal"><b><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif"">Basic stuff:</span></b><span
style="font-size:12.0pt;font-family:"TimesNew Roman","serif""><br /><b>------------</b><br />- Patch applies OK. but
offsetdifference in line numbers. <br />- Compiles with errors in contrib [pg_stat_statements, sepgsql] modules <br />-
Regressionfailed; one test-case in COPY due to incomplete test-case attached patch. – same as reported by Heikki<br
/><br/><br /><b>Details of compilation errors and regression failure:</b><br
/><b>------------------------------------------------------</b><br/><b>PATH :
postgres/src/contrib/pg_stat_statements</b><br/>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 <br
/>pg_stat_statements.c:In function â_PG_initâ: <br />pg_stat_statements.c:363: warning: assignment from incompatible
pointertype <br />pg_stat_statements.c: In function âpgss_ProcessUtilityâ: <br />pg_stat_statements.c:823: error: too
fewarguments to function âprev_ProcessUtilityâ <br />pg_stat_statements.c:826: error: too few arguments to function
âstandard_ProcessUtilityâ<br />pg_stat_statements.c:884: error: too few arguments to function âprev_ProcessUtilityâ <br
/>pg_stat_statements.c:887:error: too few arguments to function âstandard_ProcessUtilityâ <br />make: ***
[pg_stat_statements.o]Error 1 <br /><br /><b>PATH : postgres/src/contrib/sepgsql</b><br />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
<br/>In file included from hooks.c:26: <br />sepgsql.h:17:29: error: selinux/selinux.h: No such file or directory <br
/>sepgsql.h:18:25:error: selinux/avc.h: No such file or directory <br />In file included from hooks.c:26: <br
/>sepgsql.h:239:warning: âstruct av_decisionâ declared inside parameter list <br />sepgsql.h:239: warning: its scope is
onlythis definition or declaration, which is probably not what you want <br />hooks.c: In function
âsepgsql_utility_commandâ:<br />hooks.c:331: error: too few arguments to function ânext_ProcessUtility_hookâ <br
/>hooks.c:334:error: too few arguments to function âstandard_ProcessUtilityâ <br />hooks.c: In function â_PG_initâ: <br
/>hooks.c:365:warning: implicit declaration of function âis_selinux_enabledâ <br />hooks.c:426: warning: assignment
fromincompatible pointer type <br />make: *** [hooks.o] Error 1 <br /><br /><b>REGRESSION TEST:</b><br
/><b>------------------</b><br/>make installcheck <br />test copy                     ... FAILED <br
/>========================<br /> 1 of 132 tests failed. <br />======================== <br
/>~/postgres/src/test/regress>cat regression.diffs <br />*** /home/postgres/code/src/test/regress/expected/copy.out
   2012-09-18 19:57:02.000000000 +0530 <br />--- /home/postgres/code/src/test/regress/results/copy.out      2012-09-18
19:57:18.000000000+0530 <br />*************** <br />*** 71,73 **** <br />--- 71,80 ---- <br />  c1,"col with ,
comma","colwith "" quote" <br />  1,a,1 <br />  2,b,2 <br />+ -- copy should to return processed rows <br />+ do $$ <br
/>+<br />+ ERROR:  unterminated dollar-quoted string at or near "$$ <br />+   " <br />+ LINE 1: do $$ <br />+          
 ^<br /><br /><br /><b>What it does:</b><br /><b>--------------</b><br />Modification to get the number of processed
rowsevaluated via SPI. The changes are to add extra parameter in ProcessUtility to get the number of rows processed by
COPYcommand. </span><p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman","serif""><br /><br
/><b>CodeReview Comments:</b><br /><b>---------------------</b><br />1. New parameter is added to
ProcessUtility_hook_typefunction <br />   but the functions which get assigned to these functions like <br /> 
 sepgsql_utility_command,pgss_ProcessUtility, prototype & definition is not modified. <br /><br />2. Why to add the
newparameter if completionTag hold the number of processed tuple information; can be extracted </span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif"">   from it as follows: <br />   
               _SPI_current->processed = strtoul(completionTag + 7, NULL, 10); <br /><br />3. Why new variable added
inportal [portal->processed] this is not used in code ?<br /><br /><br /><b>Testing:</b><br /><b>------------</b><br
/>Thefollowing test is carried out on the patch. <br />1. Normal SQL command COPY FROM / TO is tested. <br />2. SQL
commandCOPY FROM / TO is tested from plpgsql. <br /><br />Test cases are attached in Testcases_Copy_Processed.txt <br
/><br/></span><p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman","serif""> </span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif"">With Regards,</span><p
class="MsoNormal"><spanstyle="font-size:12.0pt;font-family:"Times New Roman","serif"">Amit Kapila.</span><br /><br
/></div>