Robert Haas <robertmhaas@gmail.com> writes:
> - Until I went back and found your earlier remarks on this thread, I
> was confused as to why you were replacing a JunkFilter with a
> ProjectionInfo. I think it would be good to try to add some more
> explicit comments about that design choice, perhaps as header comments
> for ExecGetUpdateNewTuple, or maybe there's a better place. I'm still
> not sure why we need to do the same thing for the insert case, which
> seems to just be about removing junk columns.
I wondered about that too; this patch allegedly isn't touching anything
interesting about INSERT cases, so why should we modify that? However,
when I tried to poke at that, I discovered that it seems to be dead code
anyway. A look at coverage.postgresql.org will show you that no
regression test reaches "junk_filter_needed = true" in
ExecInitModifyTable's inspection of INSERT tlists, and I've been unable to
create such a case manually either. I think the reason is that the parser
initially builds all INSERT ... SELECT cases with the SELECT as an
explicit subquery, giving rise to a SubqueryScan node just below the
ModifyTable, which will project exactly the desired columns and no more.
We'll optimize away the SubqueryScan if it's a no-op projection, but not
if it is getting rid of junk columns. There is room for more optimization
here: dropping the SubqueryScan in favor of making ModifyTable do the same
projection would win by removing one plan node's worth of overhead. But
I don't think we need to panic about whether the projection is done with
ExecProject or a junk filter --- we'd be strictly ahead of the current
situation either way.
Given that background, I agree with Amit's choice to change this,
just to reduce the difference between how INSERT and UPDATE cases work.
For now, there's no performance difference anyway, since neither the
ProjectionInfo nor the JunkFilter code can be reached. (Maybe a comment
about that would be useful.)
BTW, in the version of the patch that I'm working on (not ready to
post yet), I've thrown away everything that Amit did in setrefs.c
and tlist.c, so I don't recommend he spend time improving the comments
there ;-). I did not much like the idea of building a full TargetList
for each partition; that's per-partition cycles and storage space that
we won't be able to reclaim with the 0002 patch. And we don't really
need it, because there are only going to be three cases at runtime:
pull a column from the subplan result tuple, pull a column from the
old tuple, or insert a NULL for a dropped column. So I've replaced
the per-target-table tlists with integer lists of the attnums of the
UPDATE's target columns in that table. These are compact and they
don't require any further processing in setrefs.c, and the executor
can easily build a projection expression from that data.
regards, tom lane
