Horton

Thread

Commits

  1. Fix wrong assertion and poor error messages in "COPY (query) TO".

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

    The Post Office <noreply@postgresql.org> — 2024-10-20T19:00:01Z

    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.

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-21T12:52:34Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-21T17:30:26Z

    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

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-22T01:38:49Z

    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