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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
