Re: Review: UNNEST (and other functions) WITH ORDINALITY

Greg Stark said:
> 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's almost as though we actually thought about these things when
writing the patch...

> 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.

A design goal was that no SRF implementation should be affected by the
change, since there are dozens of C-language SRFs in contrib and third-
party modules.

The existence of materialize mode prevents us from changing the
structure of the tuplestore, since we're not the ones allocating it.
The only other approach that seemed possible was to have the tuplestore
code itself add an ordinality, which it would have to do unconditionally
since for materialize-mode SRFs it would have no way to know if one was
required. Doing it in FunctionScan when projecting out tuples seemed much,
much cleaner.

> 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.

I admit the (one single) use of checking func_slot for nullity rather
than checking the funcordinality flag is a micro-optimization
(choosing to fetch the value we know we will need rather than a value
which has no other purpose). I thought the comments there were
sufficient; perhaps you could indicate what isn't clear?

Having the ordinality be the last column is required by spec - I'm
sure we could think of pg-specific extensions that would change that,
but why complicate the code now?

> 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.

The majority of the tests are adding an "ordinality" version to
existing test cases that test a number of combinations of language,
SETOF, and base vs. composite type. These do exercise various different
code paths in expandRTE and thereabouts.

One thing I did do is dike out and replace the entire existing test
sequence that was commented as testing ExecReScanFunctionScan, because
many of the tests in it did not in fact invoke any rescans and
probably hadn't done since 7.4.

> 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.

There are a dozen of those kinds of tests scattered through the
regression tests (though many use pg_get_viewdef directly rather than
pg_views).

While it might be worth centralizing them somewhere, that's really a
separate issue from this patch, since it also affects aggregates.sql,
window.sql, and with.sql.

> 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.

I've already set out why I object to this.

> 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?

It was a joint project between David and myself. Credit to Dean for the
thorough review.

-- 
Andrew (irc:RhodiumToad)