Re: Adding OLD/NEW support to RETURNING

On Wed, 8 Jan 2025 at 06:17, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
> two minor issues.
>
> if (qry->returningList == NIL)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("RETURNING must have at least one column"),
> parser_errposition(pstate,
>
> exprLocation(linitial(returningClause->exprs)))));
>
> we can reduce one level parenthesis

I don't think that's a good idea in this case. That's a pre-existing
error check in which all this patch does is replace "returningList"
with "returningClause->exprs". Removing the unneeded level of
parentheses would change a 1-line diff into a 5-line diff, making it
harder to see what the patch had actually changed.

In general, the code-base is littered with ereport()'s that have an
extra, now-unnecessary, level of parentheses. I don't think it should
be the job of every patch to try to fix those up, though I do think we
should try to avoid them when adding new ereport()'s.

> seems no tests for this error case.
> we can add one in case someone in future is wondering if this is ever reachable.
> like:
> create temp table s1();
> insert into s1 default values returning new.*;
> drop temp table s1;

Hmm, I was tempted to say that that's also just a pre-existing case,
but on reflection, I think it's probably worth confirming that "old.*,
new.*, *" can expand to zero columns. I had already added a set of
zero-column table tests in returning.sql, so I just added another test
case to that.

> transformReturningClause
> case RETURNING_OPTION_NEW: not tested,
> we can add one at src/test/regress/sql/returning.sql line 176:
>
> INSERT INTO foo DEFAULT VALUES RETURNING WITH (new AS n, old AS o, new as n) *;

OK, done.

I also noticed that I had failed to use quote_identifier() in
ruleutils.c for the new WITH aliases, so I've fixed that and adjusted
a couple of the test cases to test that.

It looks to me as though there might be pre-existing bugs in that
area, so I'll go investigate and start a new thread, if that's the
case.

Regards,
Dean