Re: Fixing contrib/isn for float8-pass-by-value

The problem reported by Rushabh Lathia boils down to the fact that
contrib/isn intends to define a datatype that is represented "just like
int8", but it supposes that int8 must be pass by reference. There is
not anything wrong with the C code; the problem is the CREATE TYPE
command in isn.sql. To fix this we need some way of passing the state
of FLOAT8PASSBYVAL into that SQL script.

It seems reasonably likely that isn.sql isn't going to be the only
place with such a problem, either.

What I propose doing about this is to extend pgxs.mk's rule

%.sql: %.sql.in
sed 's,MODULE_PATHNAME,$$libdir/$*,g' $< >$@
endif

to also be prepared to replace "FLOAT8PASSBYVAL" with either
"PASSEDBYVALUE," (note the comma) or empty as appropriate.
Likewise for FLOAT4PASSBYVAL. This will allow construction of
CREATE TYPE commands that properly reflect the attributes of the
various float and int64 types.

This is kinda ugly but I don't really see anything better.
Comments?

regards, tom lane

I wrote:
> The problem reported by Rushabh Lathia boils down to the fact that
> contrib/isn intends to define a datatype that is represented "just like
> int8", but it supposes that int8 must be pass by reference. There is
> not anything wrong with the C code; the problem is the CREATE TYPE
> command in isn.sql. To fix this we need some way of passing the state
> of FLOAT8PASSBYVAL into that SQL script.
> ...
> This is kinda ugly but I don't really see anything better.

I had a better but more invasive idea: invent a new attribute for
CREATE TYPE

LIKE = typename

which means "copy any unspecified representational details from the
given pre-existing type". This would allow the .sql file to know
less instead of more about what's happening, so it seems more
future-proof. It's a bit more work than the makefile hack I was
thinking about, but seems like a good investment. (I wouldn't
propose this if I thought contrib/isn was the only user --- I expect
there are other third-party modules with similar needs.)

Comments?

regards, tom lane

I was going to say something like that but couldn't come up with a
nice syntax. The syntax you gave seems obvious in retrospect.

I wonder if this should be tied in some way with creating binary-
compatible casts or anything. Probably not since the data type can
just make them itself.

greg

On 28 Nov 2008, at 08:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I wrote:
>> The problem reported by Rushabh Lathia boils down to the fact that
>> contrib/isn intends to define a datatype that is represented "just
>> like
>> int8", but it supposes that int8 must be pass by reference. There is
>> not anything wrong with the C code; the problem is the CREATE TYPE
>> command in isn.sql. To fix this we need some way of passing the
>> state
>> of FLOAT8PASSBYVAL into that SQL script.
>> ...
>> This is kinda ugly but I don't really see anything better.
>
> I had a better but more invasive idea: invent a new attribute for
> CREATE TYPE
>
> LIKE = typename
>
> which means "copy any unspecified representational details from the
> given pre-existing type". This would allow the .sql file to know
> less instead of more about what's happening, so it seems more
> future-proof. It's a bit more work than the makefile hack I was
> thinking about, but seems like a good investment. (I wouldn't
> propose this if I thought contrib/isn was the only user --- I expect
> there are other third-party modules with similar needs.)
>
> Comments?
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Greg Stark <greg(dot)stark(at)enterprisedb(dot)com> writes:
> I was going to say something like that but couldn't come up with a
> nice syntax. The syntax you gave seems obvious in retrospect.

Here's a draft patch for this. It actually works out pretty well --
the bulk of the diff is just to allow processing the CREATE TYPE
parameters in an order that's independent of the syntax.

> I wonder if this should be tied in some way with creating binary-
> compatible casts or anything. Probably not since the data type can
> just make them itself.

Yeah. I thought for awhile about copying *all* the create-type
parameters from the LIKE type, but soon decided it was a bad idea.
We should keep the inheritance to a minimal level, just the basic
type-storage parameters.

regards, tom lane

On Friday 28 November 2008 22:02:52 Tom Lane wrote:
> I had a better but more invasive idea: invent a new attribute for
> CREATE TYPE
>
> LIKE = typename
>
> which means "copy any unspecified representational details from the
> given pre-existing type". This would allow the .sql file to know
> less instead of more about what's happening, so it seems more
> future-proof. It's a bit more work than the makefile hack I was
> thinking about, but seems like a good investment. (I wouldn't
> propose this if I thought contrib/isn was the only user --- I expect
> there are other third-party modules with similar needs.)

Well, this is basically what I had planned with distinct types, at least in a
future phase. You will notice that the existing distinct type patch already
pretty much has the effect of copying another types represenation. From
there the difference is essentially only the casting behavior: distinct types
per SQL have fewer casts, domains have more casts. I was also planning to
devise some in-between cases for various use cases, such as creating
compatibility types.

I'm going to try to write up a wiki page collecting various use cases and
requirements so the bigger picture becomes clearer.