pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule
4+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2024-10-21 68ad9816c1 6a57a457cf beab395a42 3685ad6188 6c3b2d204f c80a1e0483 5e94f616c8 Fix wrong assertion and poor error messages in "COPY (query) TO".
* BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule
@ 2024-10-20 19:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: PG Bug reporting form @ 2024-10-20 19:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18664
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17.0
Operating system: Ubuntu 22.04
Description:
The following script:
CREATE TABLE t(i int);
CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTIFY c;
COPY (INSERT INTO t VALUES (1)) TO stdout;
triggers an Assert in BeginCopyTo():
TRAP: failed Assert("query->utilityStmt == NULL"), File: "copyto.c", Line:
495, PID: 1689825
ExceptionalCondition at assert.c:52:13
BeginCopyTo at copyto.c:501:12
DoCopy at copy.c:313:12
standard_ProcessUtility at utility.c:738:8
ProcessUtility at utility.c:523:3
PortalRunUtility at pquery.c:1158:2
PortalRunMulti at pquery.c:1315:5
PortalRun at pquery.c:795:5
...
Reproduced starting from 92e38182d.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule
@ 2024-10-21 12:52 Tender Wang <tndrwang@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: Tender Wang @ 2024-10-21 12:52 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> 于2024年10月21日周一 19:32写道：
> The following bug has been logged on the website:
>
> Bug reference: 18664
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 17.0
> Operating system: Ubuntu 22.04
> Description:
>
> The following script:
> CREATE TABLE t(i int);
> CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTIFY c;
> COPY (INSERT INTO t VALUES (1)) TO stdout;
>
> triggers an Assert in BeginCopyTo():
> TRAP: failed Assert("query->utilityStmt == NULL"), File: "copyto.c", Line:
> 495, PID: 1689825
> ExceptionalCondition at assert.c:52:13
> BeginCopyTo at copyto.c:501:12
> DoCopy at copy.c:313:12
> standard_ProcessUtility at utility.c:738:8
> ProcessUtility at utility.c:523:3
> PortalRunUtility at pquery.c:1158:2
> PortalRunMulti at pquery.c:1315:5
> PortalRun at pquery.c:795:5
> ...
>
> Reproduced starting from 92e38182d.
>
>
Thanks for reporting. I can reproduce on HEAD.
I take a quick look. It seems 92e38182d ignored this case.
I gave a fix that remove the Assert and add an Assert in another if blocks.
Any thoughts?
--
Thanks,
Tender Wang
Attachments:
0001-Remove-a-correct-Assert.patch (application/octet-stream, 2.4K) download
From c8e5992fa7564fca9bf3aaf9840dd3f0eb24ee2b Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Mon, 21 Oct 2024 20:47:20 +0800
Subject: [PATCH] Remove a correct Assert.
---
src/backend/commands/copyto.c | 5 ++---
src/test/regress/expected/copy2.out | 5 +++++
src/test/regress/sql/copy2.sql | 5 +++++
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 463083e645..24c4bbcccb 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -492,8 +492,6 @@ BeginCopyTo(ParseState *pstate,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY (SELECT INTO) is not supported")));
- Assert(query->utilityStmt == NULL);
-
/*
* Similarly the grammar doesn't enforce the presence of a RETURNING
* clause, but this is required here.
@@ -504,7 +502,8 @@ BeginCopyTo(ParseState *pstate,
Assert(query->commandType == CMD_INSERT ||
query->commandType == CMD_UPDATE ||
query->commandType == CMD_DELETE ||
- query->commandType == CMD_MERGE);
+ query->commandType == CMD_MERGE ||
+ query->commandType == CMD_UTILITY);
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 64ea33aeae..86c2a7eea4 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -929,3 +929,8 @@ truncate copy_default;
-- DEFAULT cannot be used in COPY TO
copy (select 1 as test) TO stdout with (default '\D');
ERROR: COPY DEFAULT cannot be used with COPY TO
+-- NOTIFY cannnot be used in COPY TO
+CREATE TABLE r_notify_t(i int);
+CREATE RULE r_notify AS ON INSERT TO r_notify_t DO INSTEAD NOTIFY r_c;
+COPY (INSERT INTO r_notify_t VALUES (1)) TO stdout;
+ERROR: COPY query must have a RETURNING clause
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index 45273557ce..aa34d7efb7 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -707,3 +707,8 @@ truncate copy_default;
-- DEFAULT cannot be used in COPY TO
copy (select 1 as test) TO stdout with (default '\D');
+
+-- NOTIFY cannnot be used in COPY TO
+CREATE TABLE r_notify_t(i int);
+CREATE RULE r_notify AS ON INSERT TO r_notify_t DO INSTEAD NOTIFY r_c;
+COPY (INSERT INTO r_notify_t VALUES (1)) TO stdout;
--
2.25.1
* Re: BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule
@ 2024-10-21 17:30 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2024-10-21 17:30 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> writes:
> PG Bug reporting form <noreply@postgresql.org> 于2024年10月21日周一 19:32写道：
>> The following script:
>> CREATE TABLE t(i int);
>> CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTIFY c;
>> COPY (INSERT INTO t VALUES (1)) TO stdout;
>>
>> triggers an Assert in BeginCopyTo():
> I take a quick look. It seems 92e38182d ignored this case.
> I gave a fix that remove the Assert and add an Assert in another if blocks.
> Any thoughts?
I don't like this fix, because the error message is just completely
misleading for this case. "COPY query must have a RETURNING clause"
makes it look like your mistake was to not write
COPY (INSERT INTO t VALUES (1) RETURNING *) TO stdout;
Of course doing that will fix nothing, and you'd still get the exact
same error message, leaving the user quite confused. So I think
it's worth issuing a more specific message.
I also noted somebody's faulty grammar in the nearby message
"DO ALSO rules are not supported for the COPY".
So I think we want the attached. I went back and forth about
whether to make the new message be specifically "COPY query must
not be NOTIFY", but decided that we'd surely forget to update it
if we ever allow any other sort of utility command in rules.
So better to word it generically.
regards, tom lane
Attachments:
v2-fix-bad-assert-in-COPY.patch (text/x-diff, 3.9K) download
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 463083e645..f55e6d9675 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -475,7 +475,7 @@ BeginCopyTo(ParseState *pstate,
if (q->querySource == QSRC_NON_INSTEAD_RULE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("DO ALSO rules are not supported for the COPY")));
+ errmsg("DO ALSO rules are not supported for COPY")));
}
ereport(ERROR,
@@ -492,7 +492,11 @@ BeginCopyTo(ParseState *pstate,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY (SELECT INTO) is not supported")));
- Assert(query->utilityStmt == NULL);
+ /* The only other utility command we could see is NOTIFY */
+ if (query->utilityStmt != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY query must not be a utility command")));
/*
* Similarly the grammar doesn't enforce the presence of a RETURNING
diff --git a/src/test/regress/expected/copydml.out b/src/test/regress/expected/copydml.out
index b5a225628f..e91e83260a 100644
--- a/src/test/regress/expected/copydml.out
+++ b/src/test/regress/expected/copydml.out
@@ -38,7 +38,7 @@ ERROR: DO INSTEAD NOTHING rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on insert to copydml_test do also delete from copydml_test;
copy (insert into copydml_test default values) to stdout;
-ERROR: DO ALSO rules are not supported for the COPY
+ERROR: DO ALSO rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on insert to copydml_test do instead (delete from copydml_test; delete from copydml_test);
copy (insert into copydml_test default values) to stdout;
@@ -54,7 +54,7 @@ ERROR: DO INSTEAD NOTHING rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on update to copydml_test do also delete from copydml_test;
copy (update copydml_test set t = 'f') to stdout;
-ERROR: DO ALSO rules are not supported for the COPY
+ERROR: DO ALSO rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on update to copydml_test do instead (delete from copydml_test; delete from copydml_test);
copy (update copydml_test set t = 'f') to stdout;
@@ -70,7 +70,7 @@ ERROR: DO INSTEAD NOTHING rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on delete to copydml_test do also insert into copydml_test default values;
copy (delete from copydml_test) to stdout;
-ERROR: DO ALSO rules are not supported for the COPY
+ERROR: DO ALSO rules are not supported for COPY
drop rule qqq on copydml_test;
create rule qqq as on delete to copydml_test do instead (insert into copydml_test default values; insert into copydml_test default values);
copy (delete from copydml_test) to stdout;
@@ -80,6 +80,10 @@ create rule qqq as on delete to copydml_test where old.t <> 'f' do instead inser
copy (delete from copydml_test) to stdout;
ERROR: conditional DO INSTEAD rules are not supported for COPY
drop rule qqq on copydml_test;
+create rule qqq as on insert to copydml_test do instead notify copydml_test;
+copy (insert into copydml_test default values) to stdout;
+ERROR: COPY query must not be a utility command
+drop rule qqq on copydml_test;
-- triggers
create function qqq_trig() returns trigger as $$
begin
diff --git a/src/test/regress/sql/copydml.sql b/src/test/regress/sql/copydml.sql
index 4578342253..b7eeb0eed8 100644
--- a/src/test/regress/sql/copydml.sql
+++ b/src/test/regress/sql/copydml.sql
@@ -66,6 +66,10 @@ create rule qqq as on delete to copydml_test where old.t <> 'f' do instead inser
copy (delete from copydml_test) to stdout;
drop rule qqq on copydml_test;
+create rule qqq as on insert to copydml_test do instead notify copydml_test;
+copy (insert into copydml_test default values) to stdout;
+drop rule qqq on copydml_test;
+
-- triggers
create function qqq_trig() returns trigger as $$
begin
* Re: BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule
@ 2024-10-22 01:38 Tender Wang <tndrwang@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 4+ messages in thread
From: Tender Wang @ 2024-10-22 01:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tom Lane <tgl@sss.pgh.pa.us> 于2024年10月22日周二 01:30写道：
> Tender Wang <tndrwang@gmail.com> writes:
> > PG Bug reporting form <noreply@postgresql.org> 于2024年10月21日周一 19:32写道：
> >> The following script:
> >> CREATE TABLE t(i int);
> >> CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTIFY c;
> >> COPY (INSERT INTO t VALUES (1)) TO stdout;
> >>
> >> triggers an Assert in BeginCopyTo():
>
> > I take a quick look. It seems 92e38182d ignored this case.
> > I gave a fix that remove the Assert and add an Assert in another if
> blocks.
> > Any thoughts?
>
> I don't like this fix, because the error message is just completely
> misleading for this case. "COPY query must have a RETURNING clause"
> makes it look like your mistake was to not write
>
> COPY (INSERT INTO t VALUES (1) RETURNING *) TO stdout;
>
> Of course doing that will fix nothing, and you'd still get the exact
> same error message, leaving the user quite confused. So I think
> it's worth issuing a more specific message.
>
Agree.
>
> I also noted somebody's faulty grammar in the nearby message
> "DO ALSO rules are not supported for the COPY".
>
> So I think we want the attached. I went back and forth about
> whether to make the new message be specifically "COPY query must
> not be NOTIFY", but decided that we'd surely forget to update it
> if we ever allow any other sort of utility command in rules.
> So better to word it generically.
>
LGTM
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-10-22 01:38 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-10-20 19:00 BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule PG Bug reporting form <noreply@postgresql.org>
2024-10-21 12:52 ` Tender Wang <tndrwang@gmail.com>
2024-10-21 17:30 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-22 01:38 ` Tender Wang <tndrwang@gmail.com>
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