Re: Domains versus arrays versus typmods - Mailing list pgsql-hackers

On Tue, Oct 19, 2010 at 6:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In bug #5717, Richard Huxton complains that a domain declared like
>        CREATE DOMAIN mynums numeric(4,2)[1];
> doesn't work properly, ie, the typmod isn't enforced in places where
> it reasonably ought to be.  I dug into this a bit, and found that there
> are more worms in this can than I first expected.
>
> In the first place, plpgsql seems a few bricks shy of a load, in that
> its code to handle assignment to an array element doesn't appear to
> be considering arrays with typmod at all: it just passes typmod -1 to
> exec_simple_cast_value() in the PLPGSQL_DTYPE_ARRAYELEM case in
> exec_assign_value().  Nonetheless, if you try a case like
>
>        declare
>          x numeric(4,2)[1];
>        begin
>          x[1] := $1;
>
> you'll find the typmod *is* enforced.  It turns out that the reason that
> it works is that after constructing the modified array value,
> exec_assign_value() calls itself recursively to do the actual assignment
> to the array variable --- and that call sees that the array variable has
> a typmod, so it applies the cast *at the array level*, ie it
> disassembles the array, re-coerces each element, and builds a new array.
> So it's horridly inefficient but it works.
>
> That could and should be improved, but it seems to be just a matter
> of local changes in pl_exec.c, and it's only an efficiency hack anyway.
> The big problem is that none of this happens when the variable is
> declared as a domain, and I believe that that is a significantly more
> wide-ranging issue.
>
> The real issue as I see it is that it's possible to subscript an array
> without realizing that the array's type is really a domain that
> incorporates a typmod specification.  This happens because of a hack
> we did to simplify implementation of domains over arrays: we expose the
> array element type as typelem of the domain as well.  For example, given
> Richard's declaration we have
>
> regression=# select typname,oid,typelem,typbasetype,typtypmod from pg_type where typname = 'mynums';
>  typname |  oid  | typelem | typbasetype | typtypmod
> ---------+-------+---------+-------------+-----------
>  mynums  | 92960 |    1700 |        1231 |    262150
> (1 row)
>
> If we were to wrap another domain around that, we'd have
>
> regression=# create domain mynums2 as mynums;
> CREATE DOMAIN
> regression=# select typname,oid,typelem,typbasetype,typtypmod from pg_type where typname = 'mynums2';
>  typname |  oid  | typelem | typbasetype | typtypmod
> ---------+-------+---------+-------------+-----------
>  mynums2 | 92976 |    1700 |       92960 |        -1
> (1 row)
>
> and at this point it's completely unobvious from inspection of the
> pg_type entry that any typmod needs to be applied when subscripting.
>
> So I'm suspicious that there are a boatload of bugs related to this kind
> of domain declaration, not just the one case in plpgsql.
>
> I think that what we ought to do about it is to stop exposing typelem
> in domains' pg_type rows.  If you want to subscript a domain value, you
> should have to drill down to its base type with getBaseTypeAndTypmod,
> which would also give you the correct typmod to apply.  If we set
> typelem to zero in domain pg_type rows, it shouldn't take too long to
> find any places that are missing this consideration --- the breakage
> will be obvious rather than subtle.

I fear that this is going to degrade the performance of common cases
as a way of debugging rare cases.

> This catalog definitional change obviously shouldn't be back-patched,
> but possibly the ensuing code changes could be, since the typelem change
> is just to expose where things are wrong and wouldn't be a prerequisite
> for corrected code to behave correctly.  Or we could decide that this is
> a corner case we're not going to try to fix in back branches.  It's been
> wrong since day 0, so there's certainly an argument that it's not
> important enough to risk back-patching.
>
> Comments?

It might be reasonable to back-patch whatever we decide on into 9.0,
because it is so new, but I would be reluctant to go back further
unless we have some evidence that it's bothering people.  It seems to
me that this can could have a lot of worms in it, and I fear that
there could be several rounds of fixes, which I would rather not
inflict on users of supposedly-stable branches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company