From a4db7fe0f71463eb7256d3c42b7a582d3943d604 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Mon, 19 Nov 2018 11:16:28 -0300 Subject: [PATCH] Disallow COPY FREEZE on partitioned tables This didn't actually work: COPY would fail to flush the right files, and instead would try to flush a non-existing file, causing the whole transaction to fail. Cope by raising an error as soon as the command is sent instead, to avoid a nasty later surprise. Of course, it would be much better to make it work, but we don't have a patch for that yet, and we don't know if we'll want to backpatch one when we do. Reported-by: Tomas Vondra Author: David Rowley Reviewed-by: Amit Langote, Steve Singer, Tomas Vondra --- doc/src/sgml/perform.sgml | 4 ++-- doc/src/sgml/ref/copy.sgml | 4 +++- src/backend/commands/copy.c | 29 +++++++++++++++++++++++++++-- src/test/regress/input/copy.source | 29 +++++++++++++++++++++++++++++ src/test/regress/output/copy.source | 23 +++++++++++++++++++++++ 5 files changed, 84 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml index b50357c3b4..d4232bf11b 100644 --- a/doc/src/sgml/perform.sgml +++ b/doc/src/sgml/perform.sgml @@ -1535,8 +1535,8 @@ SELECT * FROM x, y, a, b, c WHERE something AND somethingelse; needs to be written, because in case of an error, the files containing the newly loaded data will be removed anyway. However, this consideration only applies when - is minimal as all commands - must write WAL otherwise. + is minimal for + non-partitioned tables as all commands must write WAL otherwise. diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 13a8b68d95..9f3c85bf7f 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -224,7 +224,9 @@ COPY { table_name [ ( COPY FREEZE on + a partitioned table. Note that all other sessions will immediately be able to see the data diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 3a66cb5025..7fa29531cc 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2397,11 +2397,20 @@ CopyFrom(CopyState cstate) * go into pages containing tuples from any other transactions --- but this * must be the case if we have a new table or new relfilenode, so we need * no additional work to enforce that. + * + * We currently don't support this optimization if the COPY target is a + * partitioned table as we currently only lazily initialize partition + * information when routing the first tuple to the partition. We cannot + * know at this stage if we can perform this optimization. It should be + * possible to improve on this, but it does mean maintaining heap insert + * option flags per partition and setting them when we first open the + * partition. *---------- */ /* createSubid is creation check, newRelfilenodeSubid is truncation check */ - if (cstate->rel->rd_createSubid != InvalidSubTransactionId || - cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId) + if (cstate->rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && + (cstate->rel->rd_createSubid != InvalidSubTransactionId || + cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId)) { hi_options |= HEAP_INSERT_SKIP_FSM; if (!XLogIsNeeded()) @@ -2421,6 +2430,22 @@ CopyFrom(CopyState cstate) */ if (cstate->freeze) { + /* + * We currently disallow COPY FREEZE on partitioned tables. The + * reason for this is that we've simply not yet opened the partitions + * to determine if the optimization can be applied to them. We could + * go and open them all here, but doing so may be quite a costly + * overhead for small copies. In any case, we may just end up routing + * tuples to a small number of partitions. It seems better just to + * raise an ERROR for partitioned tables. + */ + if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot perform FREEZE on a partitioned table"))); + } + /* * Tolerate one registration for the benefit of FirstXactSnapshot. * Scan-bearing queries generally create at least two registrations, diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index cb13606d14..ce285a8f31 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -133,3 +133,32 @@ this is just a line full of junk that would error out if parsed \. copy copytest3 to stdout csv header; + +-- test copy from with a partitioned table +create table parted_copytest ( + a int, + b int, + c text +) partition by list (b); + +create table parted_copytest_a1 (c text, b int, a int); +create table parted_copytest_a2 (a int, c text, b int); + +alter table parted_copytest attach partition parted_copytest_a1 for values in(1); +alter table parted_copytest attach partition parted_copytest_a2 for values in(2); + +-- We must insert enough rows to trigger multi-inserts. These are only +-- enabled adaptively when there are few enough partition changes. +insert into parted_copytest select x,1,'One' from generate_series(1,1000) x; +insert into parted_copytest select x,2,'Two' from generate_series(1001,1010) x; +insert into parted_copytest select x,1,'One' from generate_series(1011,1020) x; + +copy (select * from parted_copytest order by a) to '@abs_builddir@/results/parted_copytest.csv'; + +-- Ensure COPY FREEZE errors for partitioned tables. +begin; +truncate parted_copytest; +copy parted_copytest from '@abs_builddir@/results/parted_copytest.csv' (freeze); +rollback; + +drop table parted_copytest; diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index b7e372d61b..cd65f13b46 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -95,3 +95,26 @@ copy copytest3 to stdout csv header; c1,"col with , comma","col with "" quote" 1,a,1 2,b,2 +-- test copy from with a partitioned table +create table parted_copytest ( + a int, + b int, + c text +) partition by list (b); +create table parted_copytest_a1 (c text, b int, a int); +create table parted_copytest_a2 (a int, c text, b int); +alter table parted_copytest attach partition parted_copytest_a1 for values in(1); +alter table parted_copytest attach partition parted_copytest_a2 for values in(2); +-- We must insert enough rows to trigger multi-inserts. These are only +-- enabled adaptively when there are few enough partition changes. +insert into parted_copytest select x,1,'One' from generate_series(1,1000) x; +insert into parted_copytest select x,2,'Two' from generate_series(1001,1010) x; +insert into parted_copytest select x,1,'One' from generate_series(1011,1020) x; +copy (select * from parted_copytest order by a) to '@abs_builddir@/results/parted_copytest.csv'; +-- Ensure COPY FREEZE errors for partitioned tables. +begin; +truncate parted_copytest; +copy parted_copytest from '@abs_builddir@/results/parted_copytest.csv' (freeze); +ERROR: cannot perform FREEZE on a partitioned table +rollback; +drop table parted_copytest;