BUG #14350: VIEW with INSTEAD OF INSERT TRIGGER and COPY. Missing feature or working as designed.
The following bug has been logged on the website:
Bug reference: 14350
Logged by: Maksym Boguk
Email address: maxim.boguk@gmail.com
PostgreSQL version: 9.5.4
Operating system: Linux
Description:
During developing the database structure migration with maximum
compatibility for an outside application code (a lot of views with instead
of triggers to transparently restructure underlying data), I found that one
critical (for me) feature missing.
I expected that I should be possible to COPY directly into a VIEW with
INSTEAD OF INSERT trigger on it.
But reality bite me again.
Test case:
create table ttt(id serial, name text);
create view ttt_v AS select ''::text AS str;
CREATE FUNCTION tf_ttt() RETURNS trigger AS $tf_ttt$
BEGIN
INSERT INTO ttt (name) VALUES (NEW.str);
RETURN NULL;
END;
$tf_ttt$ LANGUAGE plpgsql;
CREATE TRIGGER t_ttt_v INSTEAD OF INSERT ON ttt_v FOR EACH ROW EXECUTE
PROCEDURE tf_ttt();
COPY ttt_v FROM stdin;
Some string
Another string
\.
^C
ERROR: cannot copy to view "ttt_v"
Unfortunately application use COPY to batch load in lot places.
Is this a bug? Missing feature? Work as designed?
PS: if it had been already discussed - sorry, I tried to search mail list
archive but found nothing relevant.
Maksym
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, Oct 3, 2016 at 10:53 PM, <maxim.boguk@gmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 14350
Logged by: Maksym Boguk
Email address: maxim.boguk@gmail.com
PostgreSQL version: 9.5.4
Operating system: Linux
Description:During developing the database structure migration with maximum
compatibility for an outside application code (a lot of views with instead
of triggers to transparently restructure underlying data), I found that one
critical (for me) feature missing.I expected that I should be possible to COPY directly into a VIEW with
INSTEAD OF INSERT trigger on it.
But reality bite me again.Test case:
create table ttt(id serial, name text);
create view ttt_v AS select ''::text AS str;
CREATE FUNCTION tf_ttt() RETURNS trigger AS $tf_ttt$
BEGIN
INSERT INTO ttt (name) VALUES (NEW.str);
RETURN NULL;
END;
$tf_ttt$ LANGUAGE plpgsql;
CREATE TRIGGER t_ttt_v INSTEAD OF INSERT ON ttt_v FOR EACH ROW EXECUTE
PROCEDURE tf_ttt();
COPY ttt_v FROM stdin;
Some string
Another string
\.
^CERROR: cannot copy to view "ttt_v"
Unfortunately application use COPY to batch load in lot places.
Is this a bug? Missing feature? Work as designed?PS: if it had been already discussed - sorry, I tried to search mail list
archive but found nothing relevant.
I think currently there is no handling of INSTEAD of triggers in the copy
functionality.
It didn't seem difficult to the support the same, until unless there are any
problems for complext queries, so after adding the INSTEAD of triggers
check and calling the ExecIRInsertTriggers function, the Copy is also
working for the view.
Attached is a POC patch of the same. I didn't checked all the possible
scenarios.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
copy_to_view_poc.patchapplication/octet-stream; name=copy_to_view_poc.patchDownload
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 457c9bb..59a2e1c 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2293,7 +2293,9 @@ CopyFrom(CopyState cstate)
Assert(cstate->rel);
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION
+ && (!cstate->rel->trigdesc ||
+ !cstate->rel->trigdesc->trig_insert_instead_row))
{
if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
ereport(ERROR,
@@ -2520,52 +2522,61 @@ CopyFrom(CopyState cstate)
if (!skip_tuple)
{
- /* Check the constraints of the tuple */
- if (cstate->rel->rd_att->constr)
- ExecConstraints(resultRelInfo, slot, estate);
-
- if (useHeapMultiInsert)
+ /* INSTEAD ROW INSERT Triggers */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row)
{
- /* Add this tuple to the tuple buffer */
- if (nBufferedTuples == 0)
- firstBufferedLineNo = cstate->cur_lineno;
- bufferedTuples[nBufferedTuples++] = tuple;
- bufferedTuplesSize += tuple->t_len;
-
- /*
- * If the buffer filled up, flush it. Also flush if the total
- * size of all the tuples in the buffer becomes large, to
- * avoid using large amounts of memory for the buffers when
- * the tuples are exceptionally wide.
- */
- if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
- bufferedTuplesSize > 65535)
- {
- CopyFromInsertBatch(cstate, estate, mycid, hi_options,
- resultRelInfo, myslot, bistate,
- nBufferedTuples, bufferedTuples,
- firstBufferedLineNo);
- nBufferedTuples = 0;
- bufferedTuplesSize = 0;
- }
+ ExecIRInsertTriggers(estate, resultRelInfo, slot);
}
else
{
- List *recheckIndexes = NIL;
+ /* Check the constraints of the tuple */
+ if (cstate->rel->rd_att->constr)
+ ExecConstraints(resultRelInfo, slot, estate);
- /* OK, store the tuple and create index entries for it */
- heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
+ if (useHeapMultiInsert)
+ {
+ /* Add this tuple to the tuple buffer */
+ if (nBufferedTuples == 0)
+ firstBufferedLineNo = cstate->cur_lineno;
+ bufferedTuples[nBufferedTuples++] = tuple;
+ bufferedTuplesSize += tuple->t_len;
- if (resultRelInfo->ri_NumIndices > 0)
- recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
- estate, false, NULL,
- NIL);
+ /*
+ * If the buffer filled up, flush it. Also flush if the total
+ * size of all the tuples in the buffer becomes large, to
+ * avoid using large amounts of memory for the buffers when
+ * the tuples are exceptionally wide.
+ */
+ if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
+ bufferedTuplesSize > 65535)
+ {
+ CopyFromInsertBatch(cstate, estate, mycid, hi_options,
+ resultRelInfo, myslot, bistate,
+ nBufferedTuples, bufferedTuples,
+ firstBufferedLineNo);
+ nBufferedTuples = 0;
+ bufferedTuplesSize = 0;
+ }
+ }
+ else
+ {
+ List *recheckIndexes = NIL;
+
+ /* OK, store the tuple and create index entries for it */
+ heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
- /* AFTER ROW INSERT Triggers */
- ExecARInsertTriggers(estate, resultRelInfo, tuple,
- recheckIndexes);
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
+ estate, false, NULL,
+ NIL);
- list_free(recheckIndexes);
+ /* AFTER ROW INSERT Triggers */
+ ExecARInsertTriggers(estate, resultRelInfo, tuple,
+ recheckIndexes);
+
+ list_free(recheckIndexes);
+ }
}
/*
On Tue, Oct 4, 2016 at 1:07 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
I think currently there is no handling of INSTEAD of triggers in the copy
functionality.It didn't seem difficult to the support the same, until unless there are any
problems for complext queries, so after adding the INSTEAD of triggers
check and calling the ExecIRInsertTriggers function, the Copy is also
working for the view.Attached is a POC patch of the same. I didn't checked all the possible
scenarios.
We support insert into view in below 2 cases..
1. INSTEAD OF INSERT trigger
2. or an unconditional ON INSERT DO INSTEAD rule
In your patch we are supporting first one in COPY command, Will it not
be good to support second one also in COPY command ?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Tue, Nov 1, 2016 at 3:54 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Tue, Oct 4, 2016 at 1:07 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:I think currently there is no handling of INSTEAD of triggers in the copy
functionality.It didn't seem difficult to the support the same, until unless there are
any
problems for complext queries, so after adding the INSTEAD of triggers
check and calling the ExecIRInsertTriggers function, the Copy is also
working for the view.Attached is a POC patch of the same. I didn't checked all the possible
scenarios.We support insert into view in below 2 cases..
1. INSTEAD OF INSERT trigger
2. or an unconditional ON INSERT DO INSTEAD rule
Yes, I agree that the above are the two cases where the insert is possible
on a view other than updatable views.
In your patch we are supporting first one in COPY command, Will it not
be good to support second one also in COPY command ?
COPY command is treated as an UTILITY command, During the query
processing, the rules are not applied on the COPY command, and in the
execution of COPY command, it just inserts the data into the heap and
indexes with direct calls.
I feel supporting the COPY command for the case-2 is little bit complex
and may not be that much worth.
Attached is the updated patch with doc changes.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
copy_to_view_1.patchapplication/octet-stream; name=copy_to_view_1.patchDownload
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..4d13478 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -399,6 +399,11 @@ COPY <replaceable class="parameter">count</replaceable>
with views. However, you can write <literal>COPY (SELECT * FROM
<replaceable class="parameter">viewname</replaceable>) TO ...</literal>.
</para>
+
+ <para>
+ <command>COPY FROM</command> can only be used with plain tables and views
+ with INSTEAD of INSERT triggers.
+ </para>
<para>
<command>COPY</command> only deals with the specific table named;
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index b4140eb..c4f36a2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2269,7 +2269,15 @@ CopyFrom(CopyState cstate)
Assert(cstate->rel);
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ /*
+ * Check whether the target RELKIND is not a RELATION and raise an error
+ * if the relation doesn't contains any INSTEAD of triggers. Creation of
+ * INSTEAD triggers are possible only on VIEWs, check CreateTrigger
+ * function in trigger.c file.
+ */
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION
+ && (!cstate->rel->trigdesc ||
+ !cstate->rel->trigdesc->trig_insert_instead_row))
{
if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
ereport(ERROR,
@@ -2496,52 +2504,61 @@ CopyFrom(CopyState cstate)
if (!skip_tuple)
{
- /* Check the constraints of the tuple */
- if (cstate->rel->rd_att->constr)
- ExecConstraints(resultRelInfo, slot, estate);
-
- if (useHeapMultiInsert)
+ /* INSTEAD ROW INSERT Triggers */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row)
{
- /* Add this tuple to the tuple buffer */
- if (nBufferedTuples == 0)
- firstBufferedLineNo = cstate->cur_lineno;
- bufferedTuples[nBufferedTuples++] = tuple;
- bufferedTuplesSize += tuple->t_len;
-
- /*
- * If the buffer filled up, flush it. Also flush if the total
- * size of all the tuples in the buffer becomes large, to
- * avoid using large amounts of memory for the buffers when
- * the tuples are exceptionally wide.
- */
- if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
- bufferedTuplesSize > 65535)
- {
- CopyFromInsertBatch(cstate, estate, mycid, hi_options,
- resultRelInfo, myslot, bistate,
- nBufferedTuples, bufferedTuples,
- firstBufferedLineNo);
- nBufferedTuples = 0;
- bufferedTuplesSize = 0;
- }
+ ExecIRInsertTriggers(estate, resultRelInfo, slot);
}
else
{
- List *recheckIndexes = NIL;
+ /* Check the constraints of the tuple */
+ if (cstate->rel->rd_att->constr)
+ ExecConstraints(resultRelInfo, slot, estate);
- /* OK, store the tuple and create index entries for it */
- heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
+ if (useHeapMultiInsert)
+ {
+ /* Add this tuple to the tuple buffer */
+ if (nBufferedTuples == 0)
+ firstBufferedLineNo = cstate->cur_lineno;
+ bufferedTuples[nBufferedTuples++] = tuple;
+ bufferedTuplesSize += tuple->t_len;
- if (resultRelInfo->ri_NumIndices > 0)
- recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
+ /*
+ * If the buffer filled up, flush it. Also flush if the
+ * total size of all the tuples in the buffer becomes
+ * large, to avoid using large amounts of memory for the
+ * buffers when the tuples are exceptionally wide.
+ */
+ if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
+ bufferedTuplesSize > 65535)
+ {
+ CopyFromInsertBatch(cstate, estate, mycid, hi_options,
+ resultRelInfo, myslot, bistate,
+ nBufferedTuples, bufferedTuples,
+ firstBufferedLineNo);
+ nBufferedTuples = 0;
+ bufferedTuplesSize = 0;
+ }
+ }
+ else
+ {
+ List *recheckIndexes = NIL;
+
+ /* OK, store the tuple and create index entries for it */
+ heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
+
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
estate, false, NULL,
- NIL);
+ NIL);
- /* AFTER ROW INSERT Triggers */
- ExecARInsertTriggers(estate, resultRelInfo, tuple,
- recheckIndexes);
+ /* AFTER ROW INSERT Triggers */
+ ExecARInsertTriggers(estate, resultRelInfo, tuple,
+ recheckIndexes);
- list_free(recheckIndexes);
+ list_free(recheckIndexes);
+ }
}
/*
On Tue, Nov 1, 2016 at 12:40 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
COPY command is treated as an UTILITY command, During the query
processing, the rules are not applied on the COPY command, and in the
execution of COPY command, it just inserts the data into the heap and
indexes with direct calls.I feel supporting the COPY command for the case-2 is little bit complex
and may not be that much worth.
I agree it will be complex..
Attached is the updated patch with doc changes.
Now since we are adding support for INSTEAD OF TRIGGER in COPY FROM
command, It will be good that we provide a HINT to user if INSTEAD of
trigger does not exist, like we do in case of insert ?
INSERT case:
postgres=# insert into ttt_v values(7);
2016-11-01 14:31:39.845 IST [72343] ERROR: cannot insert into view "ttt_v"
2016-11-01 14:31:39.845 IST [72343] DETAIL: Views that do not select
from a single table or view are not automatically updatable.
2016-11-01 14:31:39.845 IST [72343] HINT: To enable inserting into
the view, provide an INSTEAD OF INSERT trigger or an unconditional ON
INSERT DO INSTEAD rule.
COPY case:
postgres=# COPY ttt_v FROM stdin;
2016-11-01 14:31:52.235 IST [72343] ERROR: cannot copy to view "ttt_v"
2016-11-01 14:31:52.235 IST [72343] STATEMENT: COPY ttt_v FROM stdin;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 1, 2016 at 8:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Tue, Nov 1, 2016 at 12:40 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:COPY command is treated as an UTILITY command, During the query
processing, the rules are not applied on the COPY command, and in the
execution of COPY command, it just inserts the data into the heap and
indexes with direct calls.I feel supporting the COPY command for the case-2 is little bit complex
and may not be that much worth.I agree it will be complex..
Attached is the updated patch with doc changes.
Now since we are adding support for INSTEAD OF TRIGGER in COPY FROM
command, It will be good that we provide a HINT to user if INSTEAD of
trigger does not exist, like we do in case of insert ?INSERT case:
postgres=# insert into ttt_v values(7);
2016-11-01 14:31:39.845 IST [72343] ERROR: cannot insert into view "ttt_v"
2016-11-01 14:31:39.845 IST [72343] DETAIL: Views that do not select
from a single table or view are not automatically updatable.
2016-11-01 14:31:39.845 IST [72343] HINT: To enable inserting into
the view, provide an INSTEAD OF INSERT trigger or an unconditional ON
INSERT DO INSTEAD rule.COPY case:
postgres=# COPY ttt_v FROM stdin;
2016-11-01 14:31:52.235 IST [72343] ERROR: cannot copy to view "ttt_v"
2016-11-01 14:31:52.235 IST [72343] STATEMENT: COPY ttt_v FROM stdin;
Thanks for your suggestion. Yes, I agree that adding a hint is good.
Updated patch is attached with addition of hint message.
2016-11-03 14:56:28.685 AEDT [7822] ERROR: cannot copy to view "ttt_v"
2016-11-03 14:56:28.685 AEDT [7822] HINT: To enable copy to view, provide
an INSTEAD OF INSERT trigger
2016-11-03 14:56:28.685 AEDT [7822] STATEMENT: COPY ttt_v FROM stdin;
Regards,
Hari Babu
Fujitsu Australia
Attachments:
copy_to_view_2.patchapplication/octet-stream; name=copy_to_view_2.patchDownload
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..4d13478 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -399,6 +399,11 @@ COPY <replaceable class="parameter">count</replaceable>
with views. However, you can write <literal>COPY (SELECT * FROM
<replaceable class="parameter">viewname</replaceable>) TO ...</literal>.
</para>
+
+ <para>
+ <command>COPY FROM</command> can only be used with plain tables and views
+ with INSTEAD of INSERT triggers.
+ </para>
<para>
<command>COPY</command> only deals with the specific table named;
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index b4140eb..df4b169 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2269,13 +2269,23 @@ CopyFrom(CopyState cstate)
Assert(cstate->rel);
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ /*
+ * Check whether the target RELKIND is not a RELATION and raise an error
+ * if the relation doesn't contains any INSTEAD of triggers. Creation of
+ * INSTEAD triggers are possible only on VIEWs, check CreateTrigger
+ * function in trigger.c file.
+ */
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION
+ && (!cstate->rel->trigdesc ||
+ !cstate->rel->trigdesc->trig_insert_instead_row))
{
if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot copy to view \"%s\"",
- RelationGetRelationName(cstate->rel))));
+ RelationGetRelationName(cstate->rel)),
+ errhint("To enable copy to view, provide"
+ " an INSTEAD OF INSERT trigger")));
else if (cstate->rel->rd_rel->relkind == RELKIND_MATVIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
@@ -2496,52 +2506,61 @@ CopyFrom(CopyState cstate)
if (!skip_tuple)
{
- /* Check the constraints of the tuple */
- if (cstate->rel->rd_att->constr)
- ExecConstraints(resultRelInfo, slot, estate);
-
- if (useHeapMultiInsert)
+ /* INSTEAD ROW INSERT Triggers */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row)
{
- /* Add this tuple to the tuple buffer */
- if (nBufferedTuples == 0)
- firstBufferedLineNo = cstate->cur_lineno;
- bufferedTuples[nBufferedTuples++] = tuple;
- bufferedTuplesSize += tuple->t_len;
-
- /*
- * If the buffer filled up, flush it. Also flush if the total
- * size of all the tuples in the buffer becomes large, to
- * avoid using large amounts of memory for the buffers when
- * the tuples are exceptionally wide.
- */
- if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
- bufferedTuplesSize > 65535)
- {
- CopyFromInsertBatch(cstate, estate, mycid, hi_options,
- resultRelInfo, myslot, bistate,
- nBufferedTuples, bufferedTuples,
- firstBufferedLineNo);
- nBufferedTuples = 0;
- bufferedTuplesSize = 0;
- }
+ ExecIRInsertTriggers(estate, resultRelInfo, slot);
}
else
{
- List *recheckIndexes = NIL;
+ /* Check the constraints of the tuple */
+ if (cstate->rel->rd_att->constr)
+ ExecConstraints(resultRelInfo, slot, estate);
+
+ if (useHeapMultiInsert)
+ {
+ /* Add this tuple to the tuple buffer */
+ if (nBufferedTuples == 0)
+ firstBufferedLineNo = cstate->cur_lineno;
+ bufferedTuples[nBufferedTuples++] = tuple;
+ bufferedTuplesSize += tuple->t_len;
+
+ /*
+ * If the buffer filled up, flush it. Also flush if the
+ * total size of all the tuples in the buffer becomes
+ * large, to avoid using large amounts of memory for the
+ * buffers when the tuples are exceptionally wide.
+ */
+ if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
+ bufferedTuplesSize > 65535)
+ {
+ CopyFromInsertBatch(cstate, estate, mycid, hi_options,
+ resultRelInfo, myslot, bistate,
+ nBufferedTuples, bufferedTuples,
+ firstBufferedLineNo);
+ nBufferedTuples = 0;
+ bufferedTuplesSize = 0;
+ }
+ }
+ else
+ {
+ List *recheckIndexes = NIL;
- /* OK, store the tuple and create index entries for it */
- heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
+ /* OK, store the tuple and create index entries for it */
+ heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
- if (resultRelInfo->ri_NumIndices > 0)
- recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
estate, false, NULL,
- NIL);
+ NIL);
- /* AFTER ROW INSERT Triggers */
- ExecARInsertTriggers(estate, resultRelInfo, tuple,
- recheckIndexes);
+ /* AFTER ROW INSERT Triggers */
+ ExecARInsertTriggers(estate, resultRelInfo, tuple,
+ recheckIndexes);
- list_free(recheckIndexes);
+ list_free(recheckIndexes);
+ }
}
/*
On Thu, Nov 3, 2016 at 9:57 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
Thanks for your suggestion. Yes, I agree that adding a hint is good.
Updated patch is attached with addition of hint message.2016-11-03 14:56:28.685 AEDT [7822] ERROR: cannot copy to view "ttt_v"
2016-11-03 14:56:28.685 AEDT [7822] HINT: To enable copy to view, provide
an INSTEAD OF INSERT trigger
2016-11-03 14:56:28.685 AEDT [7822] STATEMENT: COPY ttt_v FROM stdin;
Okay, Patch in general looks fine to me. One cosmetic comments, IMHO
in PG we follow operator at end of the line, so move '&&' to end of
the previous line.
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION
+ && (!cstate->rel->trigdesc ||
+ !cstate->rel->trigdesc->trig_insert_instead_row))Meanwhile I will test it and give the feedback.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 3, 2016 at 5:23 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Nov 3, 2016 at 9:57 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:Thanks for your suggestion. Yes, I agree that adding a hint is good.
Updated patch is attached with addition of hint message.2016-11-03 14:56:28.685 AEDT [7822] ERROR: cannot copy to view "ttt_v"
2016-11-03 14:56:28.685 AEDT [7822] HINT: To enable copy to view,provide
an INSTEAD OF INSERT trigger
2016-11-03 14:56:28.685 AEDT [7822] STATEMENT: COPY ttt_v FROM stdin;Okay, Patch in general looks fine to me. One cosmetic comments, IMHO
in PG we follow operator at end of the line, so move '&&' to end of
the previous line.+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION + && (!cstate->rel->trigdesc || + !cstate->rel->trigdesc->trig_insert_instead_row))
changed.
Meanwhile I will test it and give the feedback.
Thanks.
Updated patch is attached with added regression tests.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
copy_to_view_3.patchapplication/octet-stream; name=copy_to_view_3.patchDownload
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..4d13478 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -399,6 +399,11 @@ COPY <replaceable class="parameter">count</replaceable>
with views. However, you can write <literal>COPY (SELECT * FROM
<replaceable class="parameter">viewname</replaceable>) TO ...</literal>.
</para>
+
+ <para>
+ <command>COPY FROM</command> can only be used with plain tables and views
+ with INSTEAD of INSERT triggers.
+ </para>
<para>
<command>COPY</command> only deals with the specific table named;
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index b4140eb..9c9c8c0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2269,13 +2269,23 @@ CopyFrom(CopyState cstate)
Assert(cstate->rel);
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ /*
+ * Check whether the target RELKIND is not a RELATION and raise an error
+ * if the relation doesn't contains any INSTEAD of triggers. Creation of
+ * INSTEAD triggers are possible only on VIEWs, check CreateTrigger
+ * function in trigger.c file.
+ */
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION &&
+ (!cstate->rel->trigdesc ||
+ !cstate->rel->trigdesc->trig_insert_instead_row))
{
if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot copy to view \"%s\"",
- RelationGetRelationName(cstate->rel))));
+ RelationGetRelationName(cstate->rel)),
+ errhint("To enable copy to view, provide"
+ " an INSTEAD OF INSERT trigger")));
else if (cstate->rel->rd_rel->relkind == RELKIND_MATVIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
@@ -2496,52 +2506,61 @@ CopyFrom(CopyState cstate)
if (!skip_tuple)
{
- /* Check the constraints of the tuple */
- if (cstate->rel->rd_att->constr)
- ExecConstraints(resultRelInfo, slot, estate);
-
- if (useHeapMultiInsert)
+ /* INSTEAD ROW INSERT Triggers */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row)
{
- /* Add this tuple to the tuple buffer */
- if (nBufferedTuples == 0)
- firstBufferedLineNo = cstate->cur_lineno;
- bufferedTuples[nBufferedTuples++] = tuple;
- bufferedTuplesSize += tuple->t_len;
-
- /*
- * If the buffer filled up, flush it. Also flush if the total
- * size of all the tuples in the buffer becomes large, to
- * avoid using large amounts of memory for the buffers when
- * the tuples are exceptionally wide.
- */
- if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
- bufferedTuplesSize > 65535)
- {
- CopyFromInsertBatch(cstate, estate, mycid, hi_options,
- resultRelInfo, myslot, bistate,
- nBufferedTuples, bufferedTuples,
- firstBufferedLineNo);
- nBufferedTuples = 0;
- bufferedTuplesSize = 0;
- }
+ ExecIRInsertTriggers(estate, resultRelInfo, slot);
}
else
{
- List *recheckIndexes = NIL;
+ /* Check the constraints of the tuple */
+ if (cstate->rel->rd_att->constr)
+ ExecConstraints(resultRelInfo, slot, estate);
+
+ if (useHeapMultiInsert)
+ {
+ /* Add this tuple to the tuple buffer */
+ if (nBufferedTuples == 0)
+ firstBufferedLineNo = cstate->cur_lineno;
+ bufferedTuples[nBufferedTuples++] = tuple;
+ bufferedTuplesSize += tuple->t_len;
+
+ /*
+ * If the buffer filled up, flush it. Also flush if the
+ * total size of all the tuples in the buffer becomes
+ * large, to avoid using large amounts of memory for the
+ * buffers when the tuples are exceptionally wide.
+ */
+ if (nBufferedTuples == MAX_BUFFERED_TUPLES ||
+ bufferedTuplesSize > 65535)
+ {
+ CopyFromInsertBatch(cstate, estate, mycid, hi_options,
+ resultRelInfo, myslot, bistate,
+ nBufferedTuples, bufferedTuples,
+ firstBufferedLineNo);
+ nBufferedTuples = 0;
+ bufferedTuplesSize = 0;
+ }
+ }
+ else
+ {
+ List *recheckIndexes = NIL;
- /* OK, store the tuple and create index entries for it */
- heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
+ /* OK, store the tuple and create index entries for it */
+ heap_insert(cstate->rel, tuple, mycid, hi_options, bistate);
- if (resultRelInfo->ri_NumIndices > 0)
- recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(slot, &(tuple->t_self),
estate, false, NULL,
- NIL);
+ NIL);
- /* AFTER ROW INSERT Triggers */
- ExecARInsertTriggers(estate, resultRelInfo, tuple,
- recheckIndexes);
+ /* AFTER ROW INSERT Triggers */
+ ExecARInsertTriggers(estate, resultRelInfo, tuple,
+ recheckIndexes);
- list_free(recheckIndexes);
+ list_free(recheckIndexes);
+ }
}
/*
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 9a8922d..d0b6029 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -535,6 +535,30 @@ COPY rls_t1 (a, b) TO stdout;
2 3
4 1
RESET SESSION AUTHORIZATION;
+-- test with INSTEAD OF INSERT trigger on a view
+CREATE TABLE instead_of_insert_tbl(id serial, name text);
+CREATE VIEW instead_of_insert_tbl_view AS SELECT ''::text AS str;
+CREATE FUNCTION fun_instead_of_insert_tbl() RETURNS trigger AS $fun_instead_of_insert_tbl$
+BEGIN
+INSERT INTO instead_of_insert_tbl (name) VALUES (NEW.str);
+RETURN NULL;
+END;
+$fun_instead_of_insert_tbl$ LANGUAGE plpgsql;
+COPY instead_of_insert_tbl_view FROM stdin; -- failure case
+ERROR: cannot copy to view "instead_of_insert_tbl_view"
+HINT: To enable copy to view, provide an INSTEAD OF INSERT trigger
+CREATE TRIGGER trig_instead_of_insert_tbl_view INSTEAD OF INSERT ON instead_of_insert_tbl_view FOR EACH ROW EXECUTE
+PROCEDURE fun_instead_of_insert_tbl();
+COPY instead_of_insert_tbl_view FROM stdin;
+SELECT * from instead_of_insert_tbl;
+ id | name
+----+-------
+ 1 | test1
+(1 row)
+
+DROP FUNCTION fun_instead_of_insert_tbl() CASCADE;
+NOTICE: drop cascades to trigger trig_instead_of_insert_tbl_view on view instead_of_insert_tbl_view
+DROP TABLE instead_of_insert_tbl CASCADE;
DROP TABLE forcetest;
DROP TABLE vistest;
DROP FUNCTION truncate_in_subxact();
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index 89d0a39..f8aa7e4 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -387,6 +387,31 @@ COPY rls_t1 (a, b) TO stdout;
RESET SESSION AUTHORIZATION;
+-- test with INSTEAD OF INSERT trigger on a view
+CREATE TABLE instead_of_insert_tbl(id serial, name text);
+CREATE VIEW instead_of_insert_tbl_view AS SELECT ''::text AS str;
+CREATE FUNCTION fun_instead_of_insert_tbl() RETURNS trigger AS $fun_instead_of_insert_tbl$
+BEGIN
+INSERT INTO instead_of_insert_tbl (name) VALUES (NEW.str);
+RETURN NULL;
+END;
+$fun_instead_of_insert_tbl$ LANGUAGE plpgsql;
+
+COPY instead_of_insert_tbl_view FROM stdin; -- failure case
+test1
+\.
+
+CREATE TRIGGER trig_instead_of_insert_tbl_view INSTEAD OF INSERT ON instead_of_insert_tbl_view FOR EACH ROW EXECUTE
+PROCEDURE fun_instead_of_insert_tbl();
+COPY instead_of_insert_tbl_view FROM stdin;
+test1
+\.
+
+SELECT * from instead_of_insert_tbl;
+
+DROP FUNCTION fun_instead_of_insert_tbl() CASCADE;
+DROP TABLE instead_of_insert_tbl CASCADE;
+
DROP TABLE forcetest;
DROP TABLE vistest;
DROP FUNCTION truncate_in_subxact();
On Fri, Nov 4, 2016 at 7:14 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION + && (!cstate->rel->trigdesc || + !cstate->rel->trigdesc->trig_insert_instead_row))changed.
Meanwhile I will test it and give the feedback.
Thanks.
Updated patch is attached with added regression tests.
I have done with review and test, patch looks fine to me.
moved to "Ready for Committer"
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Haribabu Kommi <kommi.haribabu@gmail.com> writes:
[ copy_to_view_3.patch ]
Pushed with cosmetic adjustments.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 11, 2016 at 6:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Haribabu Kommi <kommi.haribabu@gmail.com> writes:
[ copy_to_view_3.patch ]
Pushed with cosmetic adjustments.
Thank you.
Regards,
Hari Babu
Fujitsu Australia