Re: The flinfo->fn_extra question, from me this time.

From: Dent John <denty(at)QQdd(dot)eu>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Chapman Flack <chap(at)anastigmatix(dot)net>
Subject: Re: The flinfo->fn_extra question, from me this time.
Date: 2020-03-23 00:13:17
Message-ID: A770AD4C-1857-4D0C-8410-36CF1DF5443B@QQdd.eu
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 12 Mar 2020, at 18:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> […]
>
> I didn't want to spend any more effort on it than that, because I'm
> not really on board with this line of attack.

Appreciate that. It was about the approach that I was most keen to get feedback upon.

> This patch seems
> awfully invasive for what it's accomplishing, both at the code level
> and in terms of what users will see in EXPLAIN. No, I don't think
> that adding additional "SRF Scan" nodes below FunctionScan is an
> improvement, nor do I like your repurposing/abusing of Materialize.
> It might be okay if you were just using Materialize as-is, but if
> it's sort-of-materialize-but-not-always, I don't think that's going
> to make anyone less confused.

Okay. Makes sense.

I wonder whether you think it's valuable to retain in the EXPLAIN output the mode the SRF operated in?

That information is not available to end users, yet it is important to understand when trying to create a pipeline-able plan.

> More locally, this business with creating new "plan nodes" below the
> FunctionScan at executor startup is a real abuse of a whole lot of stuff,
> and I suspect that it's not unrelated to the assertion failures I'm
> seeing. Don't do that. If you want to build some data structures at
> executor start, fine, but they're not plans and shouldn't be mislabeled as
> that.

I felt that FunctionScan was duplicating a bunch of stuff that the Materialize node could be doing for it. But in the end, I agree. Actually making re-use of Materialize turned out quite invasive.

> On the other hand, if they do need to be plan nodes, they should be
> made by the planner (which in turn would require a lot of infrastructure
> you haven't built, eg copyfuncs/outfuncs/readfuncs/setrefs/...).
>
> The v3 patch seemed closer to the sort of thing I was expecting
> to get out of this (though I've not read it in any detail).

I did a bit more exploration down the route of pushing it into the planner. I figured perhaps some of the complexities would shake out by approaching it at the planner level, but I learned enough along the way to realise that it is a long journey.

I’ll dust off the v3 approach and resubmit. While I’m doing that, I'll pull it back from the CF.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message James Coleman 2020-03-23 00:33:46 Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Previous Message Kirill Bychik 2020-03-22 22:32:07 Re: WAL usage calculation patch