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
Attachments:
v1-0001-Testing-COPY-inside-PL-pgSQL.patchtext/x-patch; name=v1-0001-Testing-COPY-inside-PL-pgSQL.patchDownload
From 019a37280145ef437acd26b4cd2c0ce58aa7709a Mon Sep 17 00:00:00 2001
From: Mark Dilger <hornschnorter@gmail.com>
Date: Wed, 6 Nov 2019 15:37:50 -0800
Subject: [PATCH v1] Testing COPY inside PL/pgSQL
Adding a test for COPY being run inside PL/pgSQL code, as part of
verifying that COPY works correctly from inside SPI (which plpgsql uses
internally).
---
src/pl/plpgsql/src/Makefile | 2 +-
src/pl/plpgsql/src/data/copy1.data | 3 ++
src/pl/plpgsql/src/expected/.gitignore | 1 +
src/pl/plpgsql/src/input/plpgsql_copy.source | 41 +++++++++++++++
src/pl/plpgsql/src/output/plpgsql_copy.source | 50 +++++++++++++++++++
src/pl/plpgsql/src/sql/.gitignore | 1 +
6 files changed, 97 insertions(+), 1 deletion(-)
create mode 100644 src/pl/plpgsql/src/data/copy1.data
create mode 100644 src/pl/plpgsql/src/expected/.gitignore
create mode 100644 src/pl/plpgsql/src/input/plpgsql_copy.source
create mode 100644 src/pl/plpgsql/src/output/plpgsql_copy.source
create mode 100644 src/pl/plpgsql/src/sql/.gitignore
diff --git a/src/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefile
index 4d57bf5f1e..d5a5d6d449 100644
--- a/src/pl/plpgsql/src/Makefile
+++ b/src/pl/plpgsql/src/Makefile
@@ -32,7 +32,7 @@ DATA = plpgsql.control plpgsql--1.0.sql plpgsql--unpackaged--1.0.sql
REGRESS_OPTS = --dbname=$(PL_TESTDB)
-REGRESS = plpgsql_call plpgsql_control plpgsql_domain plpgsql_record \
+REGRESS = plpgsql_call plpgsql_control plpgsql_copy plpgsql_domain plpgsql_record \
plpgsql_cache plpgsql_transaction plpgsql_trap \
plpgsql_trigger plpgsql_varprops
diff --git a/src/pl/plpgsql/src/data/copy1.data b/src/pl/plpgsql/src/data/copy1.data
new file mode 100644
index 0000000000..5d8478facb
--- /dev/null
+++ b/src/pl/plpgsql/src/data/copy1.data
@@ -0,0 +1,3 @@
+1 1.1
+2 2.2
+3 3.3
diff --git a/src/pl/plpgsql/src/expected/.gitignore b/src/pl/plpgsql/src/expected/.gitignore
new file mode 100644
index 0000000000..13e5918721
--- /dev/null
+++ b/src/pl/plpgsql/src/expected/.gitignore
@@ -0,0 +1 @@
+/plpgsql_copy.out
diff --git a/src/pl/plpgsql/src/input/plpgsql_copy.source b/src/pl/plpgsql/src/input/plpgsql_copy.source
new file mode 100644
index 0000000000..c549243088
--- /dev/null
+++ b/src/pl/plpgsql/src/input/plpgsql_copy.source
@@ -0,0 +1,41 @@
+CREATE TABLE copy1 (a int, b float);
+
+-- error cases
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 TO stdout;
+END;
+$$;
+
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM stdin;
+END;
+$$;
+
+-- valid data copying
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM '@abs_srcdir@/data/copy1.data';
+END;
+$$;
+
+SELECT * FROM copy1;
+
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 TO '@abs_builddir@/results/copy1.data';
+END;
+$$;
+
+DELETE FROM copy1;
+
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM '@abs_builddir@/results/copy1.data';
+END;
+$$;
+
+SELECT * FROM copy1;
+
+DROP TABLE copy1;
diff --git a/src/pl/plpgsql/src/output/plpgsql_copy.source b/src/pl/plpgsql/src/output/plpgsql_copy.source
new file mode 100644
index 0000000000..ee9a3d793d
--- /dev/null
+++ b/src/pl/plpgsql/src/output/plpgsql_copy.source
@@ -0,0 +1,50 @@
+CREATE TABLE copy1 (a int, b float);
+-- error cases
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 TO stdout;
+END;
+$$;
+ERROR: cannot COPY to/from client in PL/pgSQL
+CONTEXT: PL/pgSQL function inline_code_block line 3 at SQL statement
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM stdin;
+END;
+$$;
+ERROR: cannot COPY to/from client in PL/pgSQL
+CONTEXT: PL/pgSQL function inline_code_block line 3 at SQL statement
+-- valid data copying
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM '@abs_srcdir@/data/copy1.data';
+END;
+$$;
+SELECT * FROM copy1;
+ a | b
+---+-----
+ 1 | 1.1
+ 2 | 2.2
+ 3 | 3.3
+(3 rows)
+
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 TO '@abs_builddir@/results/copy1.data';
+END;
+$$;
+DELETE FROM copy1;
+DO LANGUAGE plpgsql $$
+BEGIN
+ COPY copy1 FROM '@abs_builddir@/results/copy1.data';
+END;
+$$;
+SELECT * FROM copy1;
+ a | b
+---+-----
+ 1 | 1.1
+ 2 | 2.2
+ 3 | 3.3
+(3 rows)
+
+DROP TABLE copy1;
diff --git a/src/pl/plpgsql/src/sql/.gitignore b/src/pl/plpgsql/src/sql/.gitignore
new file mode 100644
index 0000000000..210bee188e
--- /dev/null
+++ b/src/pl/plpgsql/src/sql/.gitignore
@@ -0,0 +1 @@
+/plpgsql_copy.sql
base-commit: 6e3e6cc0e884a6091e1094dff29db430af08fb93
--
2.20.1
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