Re: Last call for comments: fmgr rewrite [LONG]

I attach the current draft of my document explaining the upcoming
function manager interface changes. This has been modified from the
previously circulated version on the basis of comments from Jan and
others. There is also a preliminary fmgr.h showing actual code for
the proposed interface macros.

Current implementation status: the core fmgr routines have been
rewritten and tested, also the executor's function-call code and the
function handler routines for the three PL languages. I haven't yet
changed over any pg_proc entries to new-style except the PL handlers.
However, passing nulls into and out of SQL, plpgsql, and plperl
functions works properly now, and it would work in pltcl if pltcl had a
convention for distinguishing nulls from empty strings (Jan, do you want
to do something about that?). I still need to change trigger handling
so we can get rid of the CurrentTriggerData global variable, and then
can start working on updating individual function routines to the new
conventions. I will probably not start on that work until after we make
the 7.1 branch and I can commit what I have.

regards, tom lane

Tom Lane wrote:

> typedef struct
> {
> FmgrInfo *flinfo; /* ptr to lookup info used for this call */
> Node *context; /* pass info about context of call */
> Node *resultinfo; /* pass or return extra info about result */
> bool isnull; /* function must set true if result is NULL */
> short nargs; /* # arguments actually passed */
> Datum arg[FUNC_MAX_ARGS]; /* Arguments passed to function */
> bool argnull[FUNC_MAX_ARGS]; /* T if arg[i] is actually NULL */
> } FunctionCallInfoData;

Just wondering what the implications of FUNC_MAX_ARGS is, and whether
something like...

struct FuncArg
{
Datum arg;
bool argnull;
};

typedef struct
{
FmgrInfo *flinfo; /* ptr to lookup info used for this call
*/
Node *context; /* pass info about context of call */
Node *resultinfo; /* pass or return extra info about
result */
bool isnull; /* function must set true if result is
NULL */
short nargs; /* # arguments actually passed */
struct FuncArg args[];
} FunctionCallInfoData;

might remove an arbitrary argument limit?

> int32
> int4pl(int32 arg1, int32 arg2)
> {
> return arg1 + arg2;
> }
> to new-style
> Datum
> int4pl(FunctionCallInfo fcinfo)
> {
> /* we assume the function is marked "strict", so we can ignore
> * NULL-value handling */
>
> return Int32GetDatum(DatumGetInt32(fcinfo->arg[0]) +
> DatumGetInt32(fcinfo->arg[1]));
> }

Wondering if some stub code generator might be appropriate so that
functions can can continue to look as readable as before?

> I attach the current draft of my document explaining the upcoming
> function manager interface changes. This has been modified from the
> previously circulated version on the basis of comments from Jan and
> others. There is also a preliminary fmgr.h showing actual code for
> the proposed interface macros.

Frankly, everything is very quiet. I have no problem branching the CVS
tree and getting started soon, if people want that.

--
Bruce Momjian | http://www.op.net/~candle
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> Just wondering what the implications of FUNC_MAX_ARGS is, and whether
> something like...

> struct FuncArg
> {
> Datum arg;
> bool argnull;
> };

I did consider that but it's probably not worth near-doubling the size
of the struct (think about how that will pack, especially if Datum
becomes 8 bytes). The average callee will probably not be looking at
the argnull array at all, so it won't have a dependency on the offset to
argnull in the first place. Furthermore FUNC_MAX_ARGS is not going to
vanish in the foreseeable future; we have fixed-size arrays in places
like pg_proc and there's just not enough reason to go to the pain of
making those variable-size. So the only possible win would be to make
dynamically loaded functions binary-compatible across installations with
varying FUNC_MAX_ARGS values ... and since that'd matter only if they
looked at argnull *and* not at any other structure that depends on
FUNC_MAX_ARGS, it's probably not worth it.

> Wondering if some stub code generator might be appropriate so that
> functions can can continue to look as readable as before?

Er, did you read to the end of the proposal?

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Frankly, everything is very quiet. I have no problem branching the CVS
> tree and getting started soon, if people want that.

Yeah, it seems like we could do a 7.0.1 and make the 7.1 CVS branch
sooner than the end of the month. Maybe sometime this week?

regards, tom lane

Tom Lane wrote:
>
> Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> > Just wondering what the implications of FUNC_MAX_ARGS is, and whether
> > something like...
>
> > struct FuncArg
> > {
> > Datum arg;
> > bool argnull;
> > };
>
> I did consider that but it's probably not worth near-doubling the size
> of the struct (think about how that will pack, especially if Datum
> becomes 8 bytes).

But FUNC_MAX_ARGS is currently 16. 98% of functions are probably 1 or 2
arguments. So your way you always use 144 bytes. With my proposal most
will use 16 or 32 bytes because of the variable struct size and you
won't have an arbitrary limit of 16 args.

> Furthermore FUNC_MAX_ARGS is not going to
> vanish in the foreseeable future; we have fixed-size arrays in places
> like pg_proc and there's just not enough reason to go to the pain of
> making those variable-size.

Well if anybody ever wanted to do it, not having to re-write every
function in the system would be a nice win. Maybe there are other wins
we don't see yet in not having a fixed limit?

> So the only possible win would be to make
> dynamically loaded functions binary-compatible across installations with
> varying FUNC_MAX_ARGS values ... and since that'd matter only if they
> looked at argnull *and* not at any other structure that depends on
> FUNC_MAX_ARGS, it's probably not worth it.

Hmm. Looks like a possible future win to me. Anybody who has a library
of functions might not have to recompile.

> > Wondering if some stub code generator might be appropriate so that
> > functions can can continue to look as readable as before?
>
> Er, did you read to the end of the proposal?

Yep. Did I miss your point?

Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> Tom Lane wrote:
>> I did consider that but it's probably not worth near-doubling the size
>> of the struct (think about how that will pack, especially if Datum
>> becomes 8 bytes).

> But FUNC_MAX_ARGS is currently 16. 98% of functions are probably 1 or 2
> arguments. So your way you always use 144 bytes. With my proposal most
> will use 16 or 32 bytes because of the variable struct size and you
> won't have an arbitrary limit of 16 args.

No, because we aren't ever going to be dynamically allocating these
things; they'll be local variables in the calling function. Typical
code looks like this:

static Datum
ExecMakeFunctionResult(Node *node, List *arguments, ExprContext *econtext,
bool *isNull, bool *isDone)
{
FunctionCallInfoData fcinfo;
Datum result;

MemSet(&fcinfo, 0, sizeof(fcinfo));

/* ... fill non-defaulted fields of fcinfo here ... */

result = FunctionCallInvoke(&fcinfo);
*isNull = fcinfo.isnull;
return result;
}

To take advantage of a variable-length struct we'd need to do a palloc,
which is pointless and slow. The only reason I care about the size of
the struct at all is that I don't want that MemSet() to take longer
than it has to. (While I don't absolutely have to zero the whole
struct, it's simple and clean to do that, and it ensures that unused
fields will have a predictable value.)

Bottom line is that there *will* be a FUNC_MAX_ARGS limit. The only
question is whether there's any point in making the binary-level API
for called functions be independent of the exact value of FUNC_MAX_ARGS.
I kinda doubt it. There are a lot of other things that are more likely
to vary across installations than FUNC_MAX_ARGS; I don't see this as
being the limiting factor for portability.

> Well if anybody ever wanted to do it, not having to re-write every
> function in the system would be a nice win.

We already did the legwork of not having to rewrite anything. It's
only a config.h twiddle and recompile. I think that's plenty close
enough...

>>>> Wondering if some stub code generator might be appropriate so that
>>>> functions can can continue to look as readable as before?
>>
>> Er, did you read to the end of the proposal?

> Yep. Did I miss your point?

Possibly, or else I'm missing yours. What would a stub code generator
do for us that the proposed GETARG and RETURN macros won't do?

regards, tom lane

Tom Lane wrote:

> No, because we aren't ever going to be dynamically allocating these
> things; they'll be local variables in the calling function.

Fair enough then. Although that being the case, I don't see the big deal
about using a few more bytes of stack space which costs absolutely
nothing, even though the binary compatibility is a small but still real
advantage.

> >>>> Wondering if some stub code generator might be appropriate so that
> >>>> functions can can continue to look as readable as before?
> >>
> >> Er, did you read to the end of the proposal?
>
> > Yep. Did I miss your point?
>
> Possibly, or else I'm missing yours. What would a stub code generator
> do for us that the proposed GETARG and RETURN macros won't do?

Only that it might be slightly cleaner code, but you're probably right.
I just have experience doing this sort of thing and know that manually
grabbing each argument can be painful with hundreds of functions.

Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
>> Possibly, or else I'm missing yours. What would a stub code generator
>> do for us that the proposed GETARG and RETURN macros won't do?

> Only that it might be slightly cleaner code, but you're probably right.
> I just have experience doing this sort of thing and know that manually
> grabbing each argument can be painful with hundreds of functions.

The conversion is going to be a major pain in the rear, no doubt about
that :-(. I suspect it may take us more than one release cycle to get
rid of all the old-style functions in the distribution, and we perhaps
will never be able to drop support for old-style dynamically loaded
functions.

OTOH, I also have experience with code preprocessors and they're no fun
either in an open-source environment. You gotta port the preprocessor
to everywhere you intend to run, make it robust against a variety of
coding styles, etc etc. Don't really want to go there.

On the third hand, you've got the germ of an idea: maybe a really
quick-and-dirty script would be worth writing to do some of the basic
conversion editing. It wouldn't have to be bulletproof because we
would go over the results by hand anyway, but it could help...

regards, tom lane

> Tom Lane wrote:
>
> > No, because we aren't ever going to be dynamically allocating these
> > things; they'll be local variables in the calling function.
>
> Fair enough then. Although that being the case, I don't see the big deal
> about using a few more bytes of stack space which costs absolutely
> nothing, even though the binary compatibility is a small but still real
> advantage.

I like Tom's clean design better. Flexibility for little payback
usually just messes up clarity of the code.

--
Bruce Momjian | http://www.op.net/~candle
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Tom Lane wrote:

> OTOH, I also have experience with code preprocessors and they're no fun
> either in an open-source environment. You gotta port the preprocessor
> to everywhere you intend to run, make it robust against a variety of
> coding styles, etc etc. Don't really want to go there.

I was thinking of something more along the lines of a Corba idl code
generator, only simpler. Maybe as simple as a file like:

int4plus: INT4, INT4
int4minus: INT4, INT4
etc...

that gets generated into some stubs that call the real code...

Datum
int4pl_stub(PG_FUNCTION_ARGS)
{
int32 arg1 = PG_GETARG_INT32(0);
int32 arg2 = PG_GETARG_INT32(1);

return PG_RETURN_INT32(int4pl(arg1, arg2));
}

Bruce Momjian wrote:
>
> > Tom Lane wrote:
> >
> > > No, because we aren't ever going to be dynamically allocating these
> > > things; they'll be local variables in the calling function.
> >
> > Fair enough then. Although that being the case, I don't see the big deal
> > about using a few more bytes of stack space which costs absolutely
> > nothing, even though the binary compatibility is a small but still real
> > advantage.
>
> I like Tom's clean design better. Flexibility for little payback
> usually just messes up clarity of the code.

I tend to think grouping data that belongs together as by definition
"clean". Whenever I'm tempted to have concurrent arrays like this I
always pull back because it seems to lead to major pain later. For
example, I can see situations where I'd like to pass an argument around
together with it's is-null information...

struct FuncArg
{
Datum arg;
bool argnull;
};

typedef struct
{
struct FuncArg args[];
} FunctionCallInfoData;

Datum someFunc(FunctionCallInfo fcinfo)
{
return INT32(foo(fcinfo.args[0]) +
bar(fcinfo.args[1], fcinfo.args[2]));
}

int foo(FuncArg a) {
if (a.argnull && INT32(a.arg) > 0 ||
(!a.argnull && INT32(a.arg <= 0)
return 3;
else
return 4;
}

int bar(FuncArg a, FuncArg b) {
if (a.argnull || !b.argnull)
return 0
else
return INT32(a.arg) ~ INT32(b.arg);
}

Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> I was thinking of something more along the lines of a Corba idl code
> generator, only simpler. Maybe as simple as a file like:

> int4plus: INT4, INT4
> int4minus: INT4, INT4
> etc...

> that gets generated into some stubs that call the real code...

> Datum
> int4pl_stub(PG_FUNCTION_ARGS)
> {
> int32 arg1 = PG_GETARG_INT32(0);
> int32 arg2 = PG_GETARG_INT32(1);

> return PG_RETURN_INT32(int4pl(arg1, arg2));
> }

OK ... but I don't think we want to leave a useless extra level of
function call in the code forever. What I'm starting to visualize
is a simple editing script that adds the above decoration to an existing
function definition, and then you go back and do any necessary cleanup
by hand. There is a lot of cruft that we should be able to rip out of
the existing code (checks for NULL arguments that are no longer needed
if the function is declared strict, manipulation of pass-by-ref args
and results for float4/float8/int8 datatypes, etc etc) so a hand
editing pass will surely be needed. But maybe we could mechanize
creation of the basic GETARG/RETURN decorations...

regards, tom lane

Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> Whenever I'm tempted to have concurrent arrays like this I always pull
> back because it seems to lead to major pain later. For example, I can
> see situations where I'd like to pass an argument around together with
> it's is-null information...

That's not an unreasonable point ... although most of the existing code
that needs to do that seems to need additional values as well (the
datum's type OID, length, pass-by-ref flag are commonly needed).
Something close to the Const node type is what you tend to end up with.
The fmgr interface is (and should be, IMHO) optimized for the case where
the called code knows exactly what it's supposed to get and doesn't need
the overhead info. In particular, the vast majority of C-coded
functions in the backend should be marked 'strict' in pg_proc, and will
then not need to bother with argnull at all...

regards, tom lane

Tom Lane wrote:
>
> ------------------------------------------------------------------------------
> Proposal for function-manager redesign 21-May-2000
> --------------------------------------
>
>
> Note that neither the old function manager nor the redesign are intended
> to handle functions that accept or return sets. Those sorts of functions
> need to be handled by special querytree structures.

Does the redesign allow functions that accept/return tuples ?

On my first reading at least I did not notice it.

-----------
Hannu

Tom Lane wrote:
>
> Chris Bitmead <chrisb(at)nimrod(dot)itg(dot)telstra(dot)com(dot)au> writes:
> > Whenever I'm tempted to have concurrent arrays like this I always pull
> > back because it seems to lead to major pain later. For example, I can
> > see situations where I'd like to pass an argument around together with
> > it's is-null information...
>
> That's not an unreasonable point ... although most of the existing code
> that needs to do that seems to need additional values as well (the
> datum's type OID, length, pass-by-ref flag are commonly needed).
> Something close to the Const node type is what you tend to end up with.
> The fmgr interface is (and should be, IMHO) optimized for the case where
> the called code knows exactly what it's supposed to get and doesn't need
> the overhead info.

It may be true for C functions, but functions in higher level languages
often like to be able to operate on several types of arguments (or at least
to operate on both NULL and NOT NULL args)

> In particular, the vast majority of C-coded functions in the backend
> should be marked 'strict' in pg_proc, and will then not need to bother
> with argnull at all...

But the main aim of fmgr redesign is imho _not_ to make existing functions
work better but to enable a clean way for designing new functions/languages.

I'm probably wrong, but to me it seems that the current proposal solves only
the problem with NULLs, and leaves untouched the other problem of arbitrary
restrictions on number of arguments (unless argcount > MAX is meant to be
passed using VARIABLE i.e. -1)

------------------------
Hannu

I just got my hands on the real SQL99 stuff, dated September 1999, and it
contains a function creation syntax that is strikingly similar to ours,
which would make it a shame not to at least try to play along. Below is a
heavily reduced BNF which should give you some idea -- note in particular
the NULL call conventions. Download your copy at
<ftp://jerry.ece.umassd.edu/isowg3/x3h2/Standards/>.

<schema function> ::=
CREATE FUNCTION <schema qualified name>
<SQL parameter declaration list>
RETURNS <data type>
[ <routine characteristics>... ]
[ <dispatch clause> ]
<routine body>

<dispatch clause> ::= STATIC DISPATCH /* no idea */

<SQL parameter declaration list> ::=
<left paren>
[ <SQL parameter declaration> [ { <comma> <SQL parameter declaration> }... ] ]
<right paren>

<SQL parameter declaration> ::=
[ <parameter mode> ] [ <SQL parameter name> ]
<parameter type>
[ RESULT ]

<parameter mode> ::= IN | OUT | INOUT
/* default is IN */

<routine body> ::=
<SQL routine body>
| <external body reference>

<SQL routine body> ::= <SQL procedure statement>
/* which means a particular subset of SQL statements */

<external body reference> ::=
EXTERNAL [ NAME <external routine name> ]
[ <parameter style clause> ]
[ <external security clause> ]

<routine characteristic> ::=
LANGUAGE { ADA | C | COBOL | FORTRAN | MUMPS | PASCAL | PLI | SQL }
| PARAMETER STYLE { SQL | GENERAL }
| SPECIFIC <specific name> /* apparently to disambiguate overloaded functions */
| { DETERMINISTIC | NOT DETERMINISTIC }
| { NO SQL | CONTAINS SQL | READS SQL DATA | MODIFIES SQL DATA }
| { RETURNS NULL ON NULL INPUT | CALLED ON NULL INPUT }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| <transform group specification>
| <dynamic result sets characteristic>

--
Peter Eisentraut Sernanders väg 10:115
peter_e(at)gmx(dot)net 75262 Uppsala
http://yi.org/peter-e/ Sweden