Re: WIP: Faster Expression Processing v4

Andres Freund <andres(at)anarazel(dot)de> writes:
> [ latest patches ]

I looked through 0001 (the composite-type-dependencies one). Although
I agree that it'd be good to tighten things up in that area, I do not
think we want this specific patch: it tightens things too much. Consider
this variant of the existing test case in create_view.sql:

create table tt (f1 int, f2 int, f3 int);
create function ttf() returns setof tt as 'select * from tt' language sql;
create view vv as select f3 from ttf();
alter table tt drop column f3;

Current code allows the above (and causes the view to return nulls
afterwards). The 0001 patch would forbid the DROP, which is fine,
but it would also forbid dropping either of the other two table
columns, which I think is insupportable.

Also, given the above table and function, consider

create view vvv as select ttf();

This view returns a 3-column composite type. Now do

alter table tt add column f4 int;

Now the view returns a 4-column composite type. But at this point the
patch will let you drop the f4 column, but not any of the earlier three.
That's just weird.

So I'm unhappy with the specific decisions made in 0001. I think what we
really want there, probably, is for find_expr_references_walker to do more
than nothing with Vars referencing non-RELATION RTEs.

Having said all that, I think that 0001 is contributing very little to the
goals of this patch set. Andres stated that he wanted it so as to drop
some of the one-time checks that execQual.c currently does for Vars, but
I'm not really convinced that we could do that safely even with these
additional dependencies in place. Moreover, I really doubt that there's
a horrible performance cost from including something like

if (unlikely(op->first_execution))
out_of_line_checking_subroutine(...);

in the execution code for Vars. And that certainly isn't adding any
complexity for JIT compilation that we don't face anyway for other
execution step types.

So my recommendation is to drop 0001 and include the same one-time
checks that execQual.c currently has as out-of-line one-time checks
in the new code. We can revisit that later, but time grows short for
v10. I would much rather have a solid version of 0004 and not 0001,
than not have anything for v10 because we spent too much time dealing
with adding new dependencies.

regards, tom lane