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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Dent John <denty(at)QQdd(dot)eu>
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-12 18:51:09
Message-ID: 15840.1584039069@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

The cfbot is still not happy with this, because you're ignoring the
project style rule against C99-like mixing of code and declarations.
I went to fix that, and soon found that the code doesn't compile,
much less pass regression tests, with --enable-cassert. That's
really a serious error on your part: basically, nobody should ever
do backend code development in non-cassert builds, because there is
too much useful error checking you forego that way. (Performance
testing is a different matter ... but you need to make the code
work before you worry about speed.)

Anyway, attached is a marginal update that gets this to the point
where it should compile in the cfbot, but it'll still fail regression
tests there. (At least on the Linux side. I guess the cfbot's
Windows builds are sans cassert, which seems like an odd choice.)

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

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

regards, tom lane

Attachment Content-Type Size
0001-pipeline-functionscan-v6.patch text/x-diff 86.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2020-03-12 19:16:26 Re: Additional size of hash table is alway zero for hash aggregates
Previous Message Julien Rouhaud 2020-03-12 18:36:33 Re: Planning counters in pg_stat_statements (using pgss_store)