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

2012/10/1 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>:
> On 27.09.2012 23:58, 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).
>
>
> The comment where CREATE AS SELECT does this says:
>
>> /*
>> * CREATE TABLE AS is a messy special case for historical
>> * reasons. We must set _SPI_current->processed even though
>> * the tuples weren't returned to the caller, and we must
>> * return a special result code if the statement was spelled
>> * SELECT INTO.
>> */
>
>
> That doesn't sound like a very good precedent that we should copy to COPY. I
> don't have a problem with this approach in general, though. But if we're
> going to make it normal behavior, rather than an ugly historical kluge, that
> utility statements can return a row count without returning the tuples, we
> should document that. The above comment ought to be changed, and the manual
> section about SPI_execute needs to mention the possibility that
> SPI_processed != 0, but SPI_tuptable == NULL.
>
> Regarding the patch itself:
>
>> + else if (IsA(stmt, CopyStmt))
>> + {
>> + /*
>> + * usually utility statements
>> doesn't return a number
>> + * of processed rows, but COPY
>> does it.
>> + */
>> + Assert(strncmp(completionTag,
>> "COPY ", 5) == 0);
>> + _SPI_current->processed =
>> strtoul(completionTag + 5,
>> +
>> NULL, 10);
>> + }
>> else
>> res = SPI_OK_UTILITY;
>
>
> 'res' is not set for a CopyStmt, and there's an extra space in "COPY " in
> the Assert.
>

fixed patch

Regards

Pavel Stehule

> - Heikki