|From:||Craig Ringer <craig(at)2ndquadrant(dot)com>|
|To:||Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>|
|Cc:||Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Time to drop old-style (V0) functions?|
|Views:||Raw Message | Whole Thread | Download mbox|
On 7 March 2017 at 22:50, Peter Eisentraut
> 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:
> 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.data, sizeof(SEG));
> + datum_l = PointerGetDatum(palloc(sizeof(SEG)));
> + memcpy(DatumGetPointer(datum_l), sort_items.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,
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));
> 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:
- 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.
> -seg_contains(SEG *a, SEG *b)
> - 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));
> -seg_contained(SEG *a, SEG *b)
> - 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.
> -seg_same(SEG *a, SEG *b)
> - 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.
> 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 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.
> extra blank line
> @@ -1655,8 +1652,8 @@ CREATE FUNCTION square_root(double precision) RETURNS double precision
> 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</></>
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)
> 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);
> result->radius = atof(coord);
> - 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.
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
|Next Message||David Rowley||2017-03-27 02:51:03||Re: Performance improvement for joins where outer side is unique|
|Previous Message||Haribabu Kommi||2017-03-27 02:27:07||Re: pg_stat_wal_write statistics view|