BUG #18664: Assert in BeginCopyTo() fails when source DML query rewritten with notifying rule

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

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

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