So the more I look at this patch the fewer things I want to change in
it. I've several times thought I should make an improvement and then
realized I was really just making unnecessary tweaks that didn't
really make much difference.
It seems a shame that the node has to call the function and get back a
slot only to laboriously copy over all the attributes into a new slot.
Worse, it's actually storing all the original tuples in a tuplestore
without the ordinality and adding in the ordinality on output. This
works because the FunctionScan only supports rescan, not mark/restore
so it can easily recalculate them consistently if the tuplestore is
rescanned. I looked into what it would take to get the ordinality
added on the original scan and it would have to go so deep that it
doesn't really seem worthwhile.
I do find the logic and variable names a bit confusing so I'm tempted
to add some comments based on my initial confusion. And I'm tempted to
add an ordinalityAttNum field to the executor node so we don't need to
make these odd "scanslot means this unless we have ordinality in which
case it means that and funcslot means this" logic. That has the side
benefit that if the executor node ever wants to add more attributes it
won't have this assumption that the last column is the ordinality
baked in. I think it'll make the code a bit clearer.
Also, I really think the test cases are excessive. They're mostly
repeatedly testing the same functionality over and over in cases that
are superficially different but I'm fairly certain just invoke the
same codepaths. This will just be an annoyance if we make later
changes that require adjusting the output.
Notably the test that covers the view defintiion appears in pg_views
scares me. We bash around the formatting rules for view definition
outputs pretty regularly. On the other hand it is nice to have
regression tests that make sure these cases are covered. There's been
more than one bug in the past caused by omitting updating these
functions. I'm leaning towards leaving it in but in the long run we
probably need a better solution for this.
Oh, and I'm definitely leaning towards naming the column "ordinality".
Given we name columns things like "generate_series" and "sum" etc
there doesn't seem to be good reason to avoid doing that here.
All in all though I feel like I'm looking for trouble. It's not a very
complex patch and is definitely basically correct. Who should get
credit for it? David? Andrew? And who reviewed it? Dean? Anyone else?
