Time to drop old-style (V0) functions?

Andres, all,

* Andres Freund (andres@anarazel.de) wrote:

I'm wondering if it's not time for $subject:
- V0 causes confusion / weird crashes when PG_FUNCTION_INFO_V1 was
forgotten
- They have us keep weird hacks around just for the sake of testing V0
- they actually cost performance, because we have to zero initialize Datums, even if
the corresponding isnull marker is set.
- they allow to call arbitrary functions pretty easily

I don't see any reason to keep them around. If seriously doubt anybody
is using them seriously in anything but error.

+100

Thanks!

Stephen

Andres Freund <andres@anarazel.de> writes:

I'm wondering if it's not time for $subject:
- V0 causes confusion / weird crashes when PG_FUNCTION_INFO_V1 was
forgotten
- They have us keep weird hacks around just for the sake of testing V0
- they actually cost performance, because we have to zero initialize Datums, even if
the corresponding isnull marker is set.
- they allow to call arbitrary functions pretty easily

If by the first point you mean "assume V1 when no info function is found",
I object to that. If you mean you want to require an info function, that
might be OK.

The habit of zero-initializing Datums has got exactly nothing to do with
V0 functions; it's about ensuring consistent results and avoiding
heisenbugs from use of uninitialized memory. I do not think we should
drop it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-08 17:38:38 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I'm wondering if it's not time for $subject:
- V0 causes confusion / weird crashes when PG_FUNCTION_INFO_V1 was
forgotten
- They have us keep weird hacks around just for the sake of testing V0
- they actually cost performance, because we have to zero initialize Datums, even if
the corresponding isnull marker is set.
- they allow to call arbitrary functions pretty easily

If by the first point you mean "assume V1 when no info function is found",
I object to that. If you mean you want to require an info function, that
might be OK.

I mean throwing an error. Silently assuming V1 seems like a horrible
idea to me. It doesn't seem unlikely that we want to introduce a new
call interface at some point given the runtime cost of the current one,
and that'd just bring back the current problem.

The habit of zero-initializing Datums has got exactly nothing to do with
V0 functions; it's about ensuring consistent results and avoiding
heisenbugs from use of uninitialized memory. I do not think we should
drop it.

Well, V0 functions don't have a real way to get information about NULL,
and we allow non-strict V0 functions, so?

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-08 14:53:58 -0800, Andres Freund wrote:

On 2016-12-08 17:38:38 -0500, Tom Lane wrote:

The habit of zero-initializing Datums has got exactly nothing to do with
V0 functions; it's about ensuring consistent results and avoiding
heisenbugs from use of uninitialized memory. I do not think we should
drop it.

Well, V0 functions don't have a real way to get information about NULL,
and we allow non-strict V0 functions, so?

Oh, and the regression tests fail in V0 functions if you drop that.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@anarazel.de> writes:

On 2016-12-08 17:38:38 -0500, Tom Lane wrote:

The habit of zero-initializing Datums has got exactly nothing to do with
V0 functions; it's about ensuring consistent results and avoiding
heisenbugs from use of uninitialized memory. I do not think we should
drop it.

Well, V0 functions don't have a real way to get information about NULL,
and we allow non-strict V0 functions, so?

Non-strict V0 functions are pretty fundamentally broken, although IIRC
there was some hack whereby they could see the isnull marker for their
first argument, which is why we didn't just disallow the case. There was
never any expectation that checking for value == 0 was an appropriate
coding method for detecting nulls, because it couldn't work for
pass-by-value data types.

Again, the point of initializing those values is not to support broken
tests for nullness. It's to ensure consistent behavior in case of
buggy attempts to use null values. It's much like the fact that makeNode
zero-fills new node structs: that's mostly wasted work, if you want to
look at it in a certain way, but it's good for reproducibility.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-08 18:03:04 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-12-08 17:38:38 -0500, Tom Lane wrote:

The habit of zero-initializing Datums has got exactly nothing to do with
V0 functions; it's about ensuring consistent results and avoiding
heisenbugs from use of uninitialized memory. I do not think we should
drop it.

Well, V0 functions don't have a real way to get information about NULL,
and we allow non-strict V0 functions, so?

Non-strict V0 functions are pretty fundamentally broken, although IIRC
there was some hack whereby they could see the isnull marker for their
first argument, which is why we didn't just disallow the case. There was
never any expectation that checking for value == 0 was an appropriate
coding method for detecting nulls, because it couldn't work for
pass-by-value data types.

Well, we have a bunch in our regression tests ;). And I'm not saying
it's *good* that they rely on that, I think it's a reason to drop the
whole V0 interface.

(I also suspect there's a bunch in brin related to this)

Again, the point of initializing those values is not to support broken
tests for nullness. It's to ensure consistent behavior in case of
buggy attempts to use null values.

Well, it also makes such attempts undetectable. I'm not really convinced
that that's such an improvement.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Dec 8, 2016 at 5:53 PM, Andres Freund <andres@anarazel.de> wrote:

I mean throwing an error. Silently assuming V1 seems like a horrible
idea to me. It doesn't seem unlikely that we want to introduce a new
call interface at some point given the runtime cost of the current one,
and that'd just bring back the current problem.

I don't have a problem with getting rid of V0; I think it's an
annoyance. But I think a much more interesting question is how to
redesign this interface. I think V0 actually had the right idea in
passing arguments as C arguments rather than marshaling them in some
other data structure. If the function doesn't need flinfo or context
or collation and takes a fixed number of argument each of which is
either strict or a pass-by-reference data type (where you can use
DatumGetPointer(NULL) to represent a NULL!) and either never returns
NULL or returns a pass-by-reference data type, then you don't need any
of this complicated fcinfo stuff. You can just pass the arguments and
get back the result. That's less work for both the caller (who
doesn't have to stick the arguments into an fcinfo) and the callee
(who doesn't have to fish them back out). In the sortsupport
machinery, we've gone to some lengths to make it possible - at least
in the context of sorts - to get back to something closer to that kind
of interface. But that interface goes to considerable trouble to
achieve that result, making it practical only for bulk operations.
It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct. Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

There obviously has to be some provision for functions that actually
need all the stuff we pass in the V1 calling convention, but there are
a huge number of functions that don't need any of it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12/9/16 7:52 AM, Robert Haas wrote:

It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct. Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

If this is a problem (it might be), then we can just make those calls,
er, direct C function calls to different function. For example,

result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));

could just be

result = oidin_internal(pro_name_or_oid);

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Dec 19, 2016 at 3:13 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 12/9/16 7:52 AM, Robert Haas wrote:

It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct. Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

If this is a problem (it might be), then we can just make those calls,
er, direct C function calls to different function. For example,

result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));

could just be

result = oidin_internal(pro_name_or_oid);

Yeah, true -- that works for simple cases, and might be beneficial
where you're doing lots and lots of calls in a tight loop.

For the more general problem, I wonder if we should try to figure out
something where we have one calling convention for "simple" functions
(those that little or none of the information in fcinfo and can pretty
much just take their SQL args as C args) and another for "complex"
functions (where we do the full push-ups).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-19 15:25:42 -0500, Robert Haas wrote:

On Mon, Dec 19, 2016 at 3:13 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 12/9/16 7:52 AM, Robert Haas wrote:

It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct. Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

If this is a problem (it might be), then we can just make those calls,
er, direct C function calls to different function. For example,

result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));

could just be

result = oidin_internal(pro_name_or_oid);

Yeah, true -- that works for simple cases, and might be beneficial
where you're doing lots and lots of calls in a tight loop.

For the more general problem, I wonder if we should try to figure out
something where we have one calling convention for "simple" functions
(those that little or none of the information in fcinfo and can pretty
much just take their SQL args as C args) and another for "complex"
functions (where we do the full push-ups).

Unfortunately I think so would likely also increase overhead in a bunch
of places, because suddenly we need branches determining which callling
convention is in use. There's a fair amount of places, some of them
performance sensitive, that deal with functions that can get different
number of arguments. Prominently nodeAgg.c's transition functions for
example.

When JITing expressions that doesn't matter, because we avoid doing so
repetitively. But that's not really sufficient imo, non JITed
performance matters a lot too.

There's imo primarily two parts that make our calling convention
expensive:
a) Additional instructions required to push to/from fcinfo->arg[null],
including stack spilling required to have space for the arguments.
b) Increased number of cachelines touched, even for even trivial
functions. We need to read/write to at least five:
1) fcinfo->isnull to reset it
2) fcinfo->arg[0...] to set the argument
3) fcinfo->argnull[0...] to set argnull (separate cacheline)
4) fcinfo->flinfo->fn_addr to get the actual address to call
5) instruction cache miss for the function's contents

We can doctor around this some. A retively easy way to reduce the
overhead of a similar function call interface would be to restructure
things so we have something like:

typedef struct FunctionArg2
{
Datum arg;
bool argnull;
} FunctionArg2;

typedef struct FunctionCallInfoData2
{
/* cached function address from ->flinfo */
PGFunction fn_addr;
/* ptr to lookup info used for this call */
FmgrInfo *flinfo;
/* function must set true if result is NULL */
bool isnull;
/* # arguments actually passed */
short nargs;
/* collation for function to use */
Oid fncollation; /* collation for function to use */
/* pass or return extra info about result */
fmNodePtr resultinfo;
/* array big enough to fit flinfo->fn_nargs */
FunctionArg2 args[FLEXIBLE_ARRAY_MEMBER];
} FunctionCallInfoData2;

That'd mean some code changes because accessing arguments can't be done
quite as now, but it'd be fairly minor. We'd end up with a lot fewer
cache misses for common cases, because there's no need to fetch fn_addr,
and because the first two arg/argnull pairs still fit within the first
cacheline (which they never can in the current interface!).

But we'd still be passing arguments via memory, instead of using
registers.

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

with additional arguments passed via fcinfo as currently done. Since
most of the performance critical function calls only have two arguments
that should allow to keep usage of fcinfo-> to the cases where we need
the extra information. On 64bit linuxes the above will be entirely in
registers, on 64bit windows isnull would be placed on the stack.

I don't quite see how we could avoid stack usage at all for windows, any
of the above arguments seems important.

There'd be a need for some trickery to make PG_GETARG_DATUM() work
efficiently - afaics the argument numbers passed to PG_GETARG_*() are
always constants, so that shouldn't be too bad.

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-12-20 9:11 GMT+01:00 Andres Freund <andres@anarazel.de>:

On 2016-12-19 15:25:42 -0500, Robert Haas wrote:

On Mon, Dec 19, 2016 at 3:13 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 12/9/16 7:52 AM, Robert Haas wrote:

It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct. Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

If this is a problem (it might be), then we can just make those calls,
er, direct C function calls to different function. For example,

result = DatumGetObjectId(DirectFunctionCall1(oidin,
CStringGetDatum(pro_name_or_oid)));

could just be

result = oidin_internal(pro_name_or_oid);

Yeah, true -- that works for simple cases, and might be beneficial
where you're doing lots and lots of calls in a tight loop.

For the more general problem, I wonder if we should try to figure out
something where we have one calling convention for "simple" functions
(those that little or none of the information in fcinfo and can pretty
much just take their SQL args as C args) and another for "complex"
functions (where we do the full push-ups).

Unfortunately I think so would likely also increase overhead in a bunch
of places, because suddenly we need branches determining which callling
convention is in use. There's a fair amount of places, some of them
performance sensitive, that deal with functions that can get different
number of arguments. Prominently nodeAgg.c's transition functions for
example.

When JITing expressions that doesn't matter, because we avoid doing so
repetitively. But that's not really sufficient imo, non JITed
performance matters a lot too.

There's imo primarily two parts that make our calling convention
expensive:
a) Additional instructions required to push to/from fcinfo->arg[null],
including stack spilling required to have space for the arguments.
b) Increased number of cachelines touched, even for even trivial
functions. We need to read/write to at least five:
1) fcinfo->isnull to reset it
2) fcinfo->arg[0...] to set the argument
3) fcinfo->argnull[0...] to set argnull (separate cacheline)
4) fcinfo->flinfo->fn_addr to get the actual address to call
5) instruction cache miss for the function's contents

We can doctor around this some. A retively easy way to reduce the
overhead of a similar function call interface would be to restructure
things so we have something like:

typedef struct FunctionArg2
{
Datum arg;
bool argnull;
} FunctionArg2;

typedef struct FunctionCallInfoData2
{
/* cached function address from ->flinfo */
PGFunction fn_addr;
/* ptr to lookup info used for this call */
FmgrInfo *flinfo;
/* function must set true if result is NULL */
bool isnull;
/* # arguments actually passed */
short nargs;
/* collation for function to use */
Oid fncollation; /* collation for function to use */
/* pass or return extra info about result */
fmNodePtr resultinfo;
/* array big enough to fit flinfo->fn_nargs */
FunctionArg2 args[FLEXIBLE_ARRAY_MEMBER];
} FunctionCallInfoData2;

That'd mean some code changes because accessing arguments can't be done
quite as now, but it'd be fairly minor. We'd end up with a lot fewer
cache misses for common cases, because there's no need to fetch fn_addr,
and because the first two arg/argnull pairs still fit within the first
cacheline (which they never can in the current interface!).

But we'd still be passing arguments via memory, instead of using
registers.

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

with additional arguments passed via fcinfo as currently done. Since
most of the performance critical function calls only have two arguments
that should allow to keep usage of fcinfo-> to the cases where we need
the extra information. On 64bit linuxes the above will be entirely in
registers, on 64bit windows isnull would be placed on the stack.

I don't quite see how we could avoid stack usage at all for windows, any
of the above arguments seems important.

There'd be a need for some trickery to make PG_GETARG_DATUM() work
efficiently - afaics the argument numbers passed to PG_GETARG_*() are
always constants, so that shouldn't be too bad.

In this case some benchmark can be very important (and interesting). I am
not sure if faster function execution has significant benefit on Vulcano
like executor.

Regards

Pavel

On 2016-12-20 09:59:43 +0100, Pavel Stehule wrote:

In this case some benchmark can be very important (and interesting). I am
not sure if faster function execution has significant benefit on Vulcano
like executor.

It's fairly to see function calls as significant overhead. In fact, I
moved things *away* from a pure Vulcano style executor, and the benefits
weren't huge, primarily due to expression evaluation overhead (of which
function call overhead is one part). After JITing of expressions, it
becomes even more noticeable, because the overhead of the expression
evaluation is reduced.

I don't quite see why a Vulcano style executor should make function call
overhead meaningless?

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-20 01:14:10 -0800, Andres Freund wrote:

On 2016-12-20 09:59:43 +0100, Pavel Stehule wrote:

In this case some benchmark can be very important (and interesting). I am
not sure if faster function execution has significant benefit on Vulcano
like executor.

It's fairly to see function calls as significant overhead. In fact, I
moved things *away* from a pure Vulcano style executor, and the benefits
weren't huge, primarily due to expression evaluation overhead (of which
function call overhead is one part). After JITing of expressions, it
becomes even more noticeable, because the overhead of the expression
evaluation is reduced.

As an example, here's a JITed TPCH Q1 profile:
+   15.48%  postgres  postgres                          [.] slot_deform_tuple
+    8.42%  postgres  perf-27760.map                    [.] evalexpr90
+    5.98%  postgres  postgres                          [.] float8_accum
+    4.63%  postgres  postgres                          [.] slot_getattr
+    3.69%  postgres  postgres                          [.] bpchareq
+    3.39%  postgres  postgres                          [.] heap_getnext
+    3.22%  postgres  postgres                          [.] float8pl
+    2.86%  postgres  postgres                          [.] TupleHashTableMatch.isra.7
+    2.77%  postgres  postgres                          [.] hashbpchar
+    2.77%  postgres  postgres                          [.] float8mul
+    2.73%  postgres  postgres                          [.] ExecAgg
+    2.40%  postgres  postgres                          [.] hash_any
+    2.34%  postgres  postgres                          [.] MemoryContextReset
+    1.98%  postgres  postgres                          [.] pg_detoast_datum_packed

evalexpr90 is the expression that does the aggregate transition
function. float8_accum, bpchareq, float8pl , float8mul, ... are all
function calls, and a good percentage of the overhead in evalexpr90 is
pushing arguments onto fcinfo->arg[nulls].

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-12-20 10:28 GMT+01:00 Andres Freund <andres@anarazel.de>:

On 2016-12-20 01:14:10 -0800, Andres Freund wrote:

On 2016-12-20 09:59:43 +0100, Pavel Stehule wrote:

In this case some benchmark can be very important (and interesting). I

am

not sure if faster function execution has significant benefit on

Vulcano

like executor.

It's fairly to see function calls as significant overhead. In fact, I
moved things *away* from a pure Vulcano style executor, and the benefits
weren't huge, primarily due to expression evaluation overhead (of which
function call overhead is one part). After JITing of expressions, it
becomes even more noticeable, because the overhead of the expression
evaluation is reduced.

For me a Vulcano style is row by row processing. Using JIT or not using has
not significant impact.

Interesting change can be block level processing.

As an example, here's a JITed TPCH Q1 profile:
+   15.48%  postgres  postgres                          [.]
slot_deform_tuple
+    8.42%  postgres  perf-27760.map                    [.] evalexpr90
+    5.98%  postgres  postgres                          [.] float8_accum
+    4.63%  postgres  postgres                          [.] slot_getattr
+    3.69%  postgres  postgres                          [.] bpchareq
+    3.39%  postgres  postgres                          [.] heap_getnext
+    3.22%  postgres  postgres                          [.] float8pl
+    2.86%  postgres  postgres                          [.]
TupleHashTableMatch.isra.7
+    2.77%  postgres  postgres                          [.] hashbpchar
+    2.77%  postgres  postgres                          [.] float8mul
+    2.73%  postgres  postgres                          [.] ExecAgg
+    2.40%  postgres  postgres                          [.] hash_any
+    2.34%  postgres  postgres                          [.]
MemoryContextReset
+    1.98%  postgres  postgres                          [.]
pg_detoast_datum_packed

Our bottle neck is row format and row related processing.

On 2016-12-20 10:44:35 +0100, Pavel Stehule wrote:

2016-12-20 10:28 GMT+01:00 Andres Freund <andres@anarazel.de>:

On 2016-12-20 01:14:10 -0800, Andres Freund wrote:

On 2016-12-20 09:59:43 +0100, Pavel Stehule wrote:

In this case some benchmark can be very important (and interesting). I

am

not sure if faster function execution has significant benefit on

Vulcano

like executor.

It's fairly to see function calls as significant overhead. In fact, I
moved things *away* from a pure Vulcano style executor, and the benefits
weren't huge, primarily due to expression evaluation overhead (of which
function call overhead is one part). After JITing of expressions, it
becomes even more noticeable, because the overhead of the expression
evaluation is reduced.

For me a Vulcano style is row by row processing. Using JIT or not using has
not significant impact.

Interesting change can be block level processing.

I don't think that's true. The largest bottlenecks atm have relatively
little to do with block level processing. I know, because I went
there. We have so many other bottlenecks that row-by-row processing
vanishes behind them. Without changing the tuple flow, the performance
with either applied or posted patches for TPCH-Q1 already went up by
more than a factor of 2.5.

Anyway, this seems like a side-track discussion, better done in another
thread.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Dec 20, 2016 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

Yeah, that's kind of nice. I like the uint64 for nulls, although
FUNC_MAX_ARGS > 64 by default and certainly can be configured that
way. It wouldn't be a problem for any common cases, of course.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-20 08:15:07 -0500, Robert Haas wrote:

On Tue, Dec 20, 2016 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

Yeah, that's kind of nice. I like the uint64 for nulls, although
FUNC_MAX_ARGS > 64 by default and certainly can be configured that
way. It wouldn't be a problem for any common cases, of course.

Well, I suspect we'd have to change that then. Is anybody seriously
interested in supporting FUNC_MAX_ARGS > 64? We don't have to make our
life hard by supporting useless features... I'd even question using
64bit for this on 32bit platforms.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Dec 20, 2016 at 8:21 AM, Andres Freund <andres@anarazel.de> wrote:

On 2016-12-20 08:15:07 -0500, Robert Haas wrote:

On Tue, Dec 20, 2016 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

Yeah, that's kind of nice. I like the uint64 for nulls, although
FUNC_MAX_ARGS > 64 by default and certainly can be configured that
way. It wouldn't be a problem for any common cases, of course.

Well, I suspect we'd have to change that then. Is anybody seriously
interested in supporting FUNC_MAX_ARGS > 64? We don't have to make our
life hard by supporting useless features... I'd even question using
64bit for this on 32bit platforms.

Advanced Server uses 256, and we had a customer complain recently that
256 wasn't high enough. So apparently the use case for functions with
ridiculous numbers of arguments isn't exactly 0. For most people it's
not a big problem in practice, but actually I think that it's pretty
lame that we have a limit at all. I mean, there's no reason that we
can't use dynamic allocation here. If we put the first, say, 8
arguments in the FunctionCallInfoData itself and then separately
allocated space any extras, the structure could be a lot smaller
without needing an arbitrary limit on the argument count.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-12-20 08:35:05 -0500, Robert Haas wrote:

On Tue, Dec 20, 2016 at 8:21 AM, Andres Freund <andres@anarazel.de> wrote:

On 2016-12-20 08:15:07 -0500, Robert Haas wrote:

On Tue, Dec 20, 2016 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:

I think a more efficient variant would make the function signature look
something like:

Datum /* directly returned argument */
pgfunc(
/* extra information about function call */
FunctionCallInfo *fcinfo,
/* bitmap of NULL arguments */
uint64_t nulls,
/* first argument */
Datum arg0,
/* second argument */
Datum arg1,
/* returned NULL */
bool *isnull
);

Yeah, that's kind of nice. I like the uint64 for nulls, although
FUNC_MAX_ARGS > 64 by default and certainly can be configured that
way. It wouldn't be a problem for any common cases, of course.

Well, I suspect we'd have to change that then. Is anybody seriously
interested in supporting FUNC_MAX_ARGS > 64? We don't have to make our
life hard by supporting useless features... I'd even question using
64bit for this on 32bit platforms.

Advanced Server uses 256, and we had a customer complain recently that
256 wasn't high enough. So apparently the use case for functions with
ridiculous numbers of arguments isn't exactly 0.

Well, there's a cost/benefit analysis involved here, as in a lot of
other places. There's a lot of things one can conceive a use-case for,
doesn't mean it's worth catering for all of them.

I mean, there's no reason that we can't use dynamic allocation here.
If we put the first, say, 8 arguments in the FunctionCallInfoData
itself and then separately allocated space any extras, the structure
could be a lot smaller without needing an arbitrary limit on the
argument count.

Except that that'll mean additional overhead during evaluation of
function arguments, an overhead that everyone will have to pay. Suddenly
you need two loops that fill in arguments, depending on their
argument-number (i.e. additional branches in a thight spot). And not
being able to pass the null bitmap via an register argument also costs
everyone.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers