Thread: Missing test of SPI copy functionality

Hackers,

While working on cleaning up the SPI interface, I found that one of the 
SPI error codes, SPI_ERROR_COPY, is never encountered in any test case 
when running `make check-world`.  This case is certainly reachable by a 
user, as is shown in the attached patch.  Is this tested from some other 
infrastructure?

To verify that SPI_ERROR_COPY is not tested, before and after applying 
the patch, try this modification, and notice before the patch that the 
fatal error is never encountered:

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 2c0ae395ba..ced38abbf6 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2245,6 +2245,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo 
paramLI,

    if (cstmt->filename == NULL)
    {
+        elog(FATAL, "SPI_ERROR_COPY tested");
        my_res = SPI_ERROR_COPY;
        goto fail;
    }

I am submitting this patch separately from other patches related to SPI, 
since (a) it does not touch any of the SPI code, (b) it fixes missing 
test coverage to do with COPY and PL/pgSQL, only indirectly to do with 
SPI, and (c) it should be possible to commit this patch even if other 
SPI patches are rejected.

-- 
Mark Dilger

On Wed, Nov 06, 2019 at 04:16:14PM -0800, Mark Dilger wrote:
> While working on cleaning up the SPI interface, I found that one of the SPI
> error codes, SPI_ERROR_COPY, is never encountered in any test case when
> running `make check-world`.  This case is certainly reachable by a user, as
> is shown in the attached patch.  Is this tested from some other
> infrastructure?

Hard to say, but I think that it would be good to test that part
independently anyway.  The transaction part close by is actually
getting stressed with plpgsql_transaction, so the split done in your
patch looks fine.  I'll look at it again in a couple of days to
double-check for missing spots, and commit it if there are no
objections.

> To verify that SPI_ERROR_COPY is not tested, before and after applying the
> patch, try this modification, and notice before the patch that the fatal
> error is never encountered:

If you use "Assert(false)", you would get in bonus the call stack.  I
use this trick from time to time.

> I am submitting this patch separately from other patches related to SPI,
> since (a) it does not touch any of the SPI code, (b) it fixes missing test
> coverage to do with COPY and PL/pgSQL, only indirectly to do with SPI, and
> (c) it should be possible to commit this patch even if other SPI patches are
> rejected.

Thanks for doing so.  I can see that it has been added to the CF app:
https://commitfest.postgresql.org/26/2350/
--
Michael

On 11/6/19 6:27 PM, Michael Paquier wrote:
> On Wed, Nov 06, 2019 at 04:16:14PM -0800, Mark Dilger wrote:
>> While working on cleaning up the SPI interface, I found that one of the SPI
>> error codes, SPI_ERROR_COPY, is never encountered in any test case when
>> running `make check-world`.  This case is certainly reachable by a user, as
>> is shown in the attached patch.  Is this tested from some other
>> infrastructure?
> 
> Hard to say, but I think that it would be good to test that part
> independently anyway.  The transaction part close by is actually
> getting stressed with plpgsql_transaction, so the split done in your
> patch looks fine.  I'll look at it again in a couple of days to
> double-check for missing spots, and commit it if there are no
> objections.

Thanks for reviewing!


-- 
Mark Dilger

On Thu, Nov 07, 2019 at 06:25:54AM -0800, Mark Dilger wrote:
> Thanks for reviewing!

After a closer lookup, I have noticed that you missed a second code
path which is able to trigger the COPY failures as you use EXECUTE
with COPY in PL/pgSQL.  So I have added some tests for that, and
committed the patch.  Thanks.
--
Michael