Re: Time to drop old-style (V0) functions? - Mailing list pgsql-hackers

On 7 March 2017 at 22:50, Peter Eisentraut
<peter.eisentraut@2ndquadrant.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