Re: Time to drop old-style (V0) functions?

I think we have consensus to go ahead with this, and the patches are
mostly mechanical, so I only have a few comments on style and one
possible bug below:

0001-Move-contrib-seg-to-only-use-V1-calling-conventions.patch

static int restore(char *s, float val, int n);

-
/*****************************************************************************
* Input/Output functions
*****************************************************************************/

+

doesn't seem like the right whitespace change

@@ -359,13 +360,14 @@ gseg_picksplit(GistEntryVector *entryvec,
/*
* Emit segments to the left output page, and compute its bounding box.
*/
- datum_l = (SEG *) palloc(sizeof(SEG));
- memcpy(datum_l, sort_items[0].data, sizeof(SEG));
+ datum_l = PointerGetDatum(palloc(sizeof(SEG)));
+ memcpy(DatumGetPointer(datum_l), sort_items[0].data, sizeof(SEG));

There would be a little bit less back-and-forth here if you kept
datum_l and datum_r of type SEG *.

case RTOverlapStrategyNumber:
- retval = (bool) seg_overlap(key, query);
+ retval =
+ !DatumGetBool(DirectFunctionCall2(seg_overlap, key, query));
break;

Accidentally flipped the logic?

-bool
-seg_contains(SEG *a, SEG *b)
+Datum
+seg_contains(PG_FUNCTION_ARGS)
{
- return ((a->lower <= b->lower) && (a->upper >= b->upper));
+ SEG *a = (SEG *) PG_GETARG_POINTER(0);
+ SEG *b = (SEG *) PG_GETARG_POINTER(1);
+
+ PG_RETURN_BOOL((a->lower <= b->lower) && (a->upper >= b->upper));
}

-bool
-seg_contained(SEG *a, SEG *b)
+Datum
+seg_contained(PG_FUNCTION_ARGS)
{
- return (seg_contains(b, a));
+ PG_RETURN_DATUM(
+ DirectFunctionCall2(seg_contains,
+ PG_GETARG_DATUM(1),
+ PG_GETARG_DATUM(0)));
}

Maybe in seg_contained also assign the arguments to a and b, so it's
easier to see the relationship between contains and contained.

-bool
-seg_same(SEG *a, SEG *b)
+Datum
+seg_same(PG_FUNCTION_ARGS)
{
- return seg_cmp(a, b) == 0;
+ Datum cmp =
+ DirectFunctionCall2(seg_cmp, PG_GETARG_DATUM(0), PG_GETARG_DATUM(1));
+
+ PG_RETURN_BOOL(DatumGetInt32(cmp) == 0);
}

I would write this as

int32 cmp = DatumGetInt32(DirectFunctionCall2(seg_cmp, PG_GETARG_DATUM(0), PG_GETARG_DATUM(1));

Having a variable of type Datum is a bit meaningless.

Oh, I see you did it that way later in seg_lt() etc., so same here.

0002-Remove-support-for-version-0-calling-conventions.patch

diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 255bfddad7..cd41b89136 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -675,7 +675,7 @@ CREATE FUNCTION mleast(VARIADIC arr numeric[]) RETURNS numeric AS $$
$$ LANGUAGE SQL;

SELECT mleast(10, -1, 5, 4.4);
- mleast
+ mleast
--------
-1
(1 row)

These changes are neither right nor wrong, but we have had previous
discussions about this and settled on leaving the whitespace as psql
outputs it. In any case it seems unrelated here.

<para>
- Two different calling conventions are currently used for C functions.
- The newer <quote>version 1</quote> calling convention is indicated by writing
- a <literal>PG_FUNCTION_INFO_V1()</literal> macro call for the function,
- as illustrated below. Lack of such a macro indicates an old-style
- (<quote>version 0</quote>) function. The language name specified in <command>CREATE FUNCTION</command>
- is <literal>C</literal> in either case. Old-style functions are now deprecated
- because of portability problems and lack of functionality, but they
- are still supported for compatibility reasons.
+
+ Currently only one calling convention is used for C functions
+ (<quote>version 1</quote>). Support for that calling convention is
+ indicated by writing a <literal>PG_FUNCTION_INFO_V1()</literal> macro
+ call for the function, as illustrated below.
</para>

extra blank line

@@ -1655,8 +1652,8 @@ CREATE FUNCTION square_root(double precision) RETURNS double precision
<para>
If the name starts with the string <literal>$libdir</literal>,
that part is replaced by the <productname>PostgreSQL</> package
- library directory
- name, which is determined at build time.<indexterm><primary>$libdir</></>
+ library directory name, which is determined at build time.
+ <indexterm><primary>$libdir</></>
</para>
</listitem>

unrelated?

@@ -455,9 +394,12 @@ fetch_finfo_record(void *filehandle, char *funcname)
infofuncname);
if (infofunc == NULL)
{
- /* Not found --- assume version 0 */
- pfree(infofuncname);
- return &default_inforec;
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("could not find function information for function \"%s\"",
+ funcname),
+ errhint("SQL-callable functions need an accompanying PG_FUNCTION_INFO_V1(funcname).")));
+ return NULL; /* silence compiler */
}

/* Found, so call it */

Perhaps plug in the actual C function name into the error message, like

errhint("SQL-callable functions need an accompanying PG_FUNCTION_INFO_V1(%s).", funcname)

@@ -270,14 +269,16 @@ widget_in(char *str)
result->center.y = atof(coord[1]);
result->radius = atof(coord[2]);

- return result;
+ PG_RETURN_DATUM(PointerGetDatum(result));
}

Could be PG_RETURN_POINTER().

Attached is a patch for src/backend/utils/fmgr/README that edits out the
transitional comments and just keeps the parts still relevant.

Tests pass for me. So this is mostly ready to go AFAICS.

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