pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedMissing test of SPI copy functionality
4+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2019-11-09 1858b105b0 Add tests for COPY in PL/pgSQL
* Missing test of SPI copy functionality
@ 2019-11-07 00:16 Mark Dilger <hornschnorter@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Mark Dilger @ 2019-11-07 00:16 UTC (permalink / raw)
To: pgsql-hackers
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:
inline patch 1: src/backend/executor/spi.c (+0/-0 lines) download inline-1.patch
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
v1-0001-Testing-COPY-inside-PL-pgSQL.patch (text/x-patch, 4.3K) download
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
* Re: Missing test of SPI copy functionality
@ 2019-11-07 02:27 Michael Paquier <michael@paquier.xyz>
parent: Mark Dilger <hornschnorter@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Michael Paquier @ 2019-11-07 02:27 UTC (permalink / raw)
To: Mark Dilger <hornschnorter@gmail.com>; +Cc: pgsql-hackers
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Missing test of SPI copy functionality
@ 2019-11-07 14:25 Mark Dilger <hornschnorter@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 4+ messages in thread
From: Mark Dilger @ 2019-11-07 14:25 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Missing test of SPI copy functionality
@ 2019-11-09 05:59 Michael Paquier <michael@paquier.xyz>
parent: Mark Dilger <hornschnorter@gmail.com>
0 siblings, 0 replies; 4+ messages in thread
From: Michael Paquier @ 2019-11-09 05:59 UTC (permalink / raw)
To: Mark Dilger <hornschnorter@gmail.com>; +Cc: pgsql-hackers
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2019-11-09 05:59 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 00:16 Missing test of SPI copy functionality Mark Dilger <hornschnorter@gmail.com>
2019-11-07 02:27 ` Michael Paquier <michael@paquier.xyz>
2019-11-07 14:25 ` Mark Dilger <hornschnorter@gmail.com>
2019-11-09 05:59 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox