Re: Set Returning Functions (SRF) - request for patch review and comment

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> This seems unnecessary; couldn't we use the function name as the default
>> alias name? The reason we require an alias for a subselect is that
>> there's no obvious automatic choice for a subselect; but there is for a
>> function.

> Yeah, I was on the fence about this. The only problem I could see is
> when the function returns a base type, what do I use for the column
> alias? Is it OK to use the same alias for the relation and column?

Sure. foo.foo is valid for a column foo in a table foo, so I don't
see a problem with it for a function.

>> You may not want to hear this at this point ;-) but I'd be strongly
>> inclined to s/portal/function/ throughout the patch.

> I was already thinking the same thing. It will be a real PITA, but I do
> believe it is the right thing to do.

You could try doing the text substitution on the diff file and then
re-applying the diff to fresh sources. Might get a couple of merge
failures, but should be a lot less painful than doing the edit directly
on the full sources.

>> I've been toying with eliminating Iter nodes, which don't seem to do
>> anything especially worthwhile --- it'd make a lot more sense to add
>> a "returnsSet" boolean in Func nodes. Dunno if that simplifies life
>> for you. If you take the above advice you may find you don't really
>> care anymore whether there's an Iter node in the tree.

> Actually it gets in my way a bit, and I think I remember some
> discussions wrt removing it. But I wasn't sure how large the impact
> would be on the current API if I messed with it, so I thought I'd leave
> it for now. Do you think it's worth it to address this now?

Up to you; probably should wait to see if Iter is still in your way
after you do the other thing. I think removing it and instead inserting
returnsSet booleans in Oper and Func nodes would be a pretty
straightforward exercise, but it'll mean touching even more stuff.
Might be best to do that as a separate patch.

>> ExecPortalReScan does not look like it works yet (in fact, it looks like
>> it will dump core). This is important. It also brings up the question
>> of how you are handling parameters passed into the function. I think
>> there's a lot more to that than meets the eye.

> Yeah, I took a first shot at writing the rescan support, but have not
> yet begun to use/test it. I'd like to get the rest of the patch to an
> acceptable level first, then concentrate on get materialization and
> rescan working.

Fair enough. We should try to get the bulk of the patch applied soon
so that you don't have code drift problems. The rescan issues should
not involve touching nearly as much code.

regards, tom lane