Re: Adding OLD/NEW support to RETURNING

On Mon, 14 Oct 2024 at 07:41, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> typedef struct ReturningOption
> {
> NodeTag type;
> bool isNew;
> char *name;
> int location;
> } ReturningOption;
> location should be type ParseLoc?

Ah yes, nice catch! I missed that other commit.

> in process_sublinks_mutator
>
> else if (IsA(node, ReturningExpr))
> {
> if (((ReturningExpr *) node)->retlevelsup > 0)
> return node;
> }
> this part doesn't have a coverage test?

True, though that's just copy-and-paste code from the existing
examples (not all of which have code coverage in the tests either).

> the following is the minimum tests i come up with:
>
> create table s (a int, b int);
> create view sv as select * from s;
> explain insert into sv values(1,2) returning (select new from
> (values((select new))));
>
>
> explain insert into sv values(1,2) returning (select new from (((select new))));
> won't touch the changes we did.
> but these two explain output plans are the same.

Those plans are the same because the first example isn't actually
selecting from the innermost values subquery, which returns a column
called "column1", so "select new from (values..." isn't selecting that
value, it's selecting "new" from the outer query, making it the same
as the second example. If you rewrite it as

explain
insert into sv values(1,2) returning (select new from (values((select
new))) as v(new));

then it does pull from the innermost subquery and you get a different
plan, but that doesn't hit the new code, though I didn't look too
closely to see why.

AFAICS, all these examples are all producing the correct results, and
I've tweaked one of the existing tests to give coverage of that line,
because it was easy to do.

I did look at the coverage report, and IMO there is decent coverage of
all the new code. There are a few places that aren't covered, but
they're all trivial boilerplate code, more-or-less just copied from
nearby code, so I think it's OK.

Regards,
Dean