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

On 7 March 2017 at 22:50, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> 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

Fixed.

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

Also, currently it does:

v->spl_ldatum = PointerGetDatum(datum_l);
v->spl_rdatum = PointerGetDatum(datum_r);

even though they're already Datum.

Downside of keeping them as SEG is we land up with:

seg_l = DatumGetPointer(DirectFunctionCall2(seg_union,
PointerGetDatum(datum_l),

PointerGetDatum(sort_items[i].data)));

but at least it's tied to the fmgr call.

>
> case RTOverlapStrategyNumber:
> - retval = (bool) seg_overlap(key, query);
> + retval =
> + !DatumGetBool(DirectFunctionCall2(seg_overlap, key, query));
> break;
>
> Accidentally flipped the logic?

Looks like it. I don't see a reason for it; there's no corresponding
change around seg_overlap and the other callsite isn't negated:

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

so I'd say copy-pasteo, given nearby negated bools.

Fixed. Didn't find any other cases.

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

Done. Makes for nicer formatting too.

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

If you're passing things around between other fmgr-using functions
it's often useful to just carry the Datum form around.

In this case it doesn't make much sense though. Done.

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

Removed. Though personally since the patch touches the file anyway it
doesn't seem to matter much either way.

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

Fixed.

> @@ -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?

Reverted. Though personally I'd like to be more forgiving of
unrelated-ish changes in docs, since they often need a tidy up when
you're touching the file anyway.

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

Doesn't make sense unless we then make it singlular, IMO, otherwise it
just reads weirdly. I'd rather keep the 'funcname'. If you're writing
C code it shouldn't be too much of an ask.

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

Done.

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

Applied.

Attached with the suggested amendments. I'll have a read-through, but
you seem to have done the fine-tooth comb treatment already.

Passes "make check" and recovery tests, check-world running now.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services