Thread: domain constraints and UNKNOWN params

David Wheeler reported the following bug: when a protocol-level prepared
statement with a parameter of UNKNOWN type is used, any domain
constraints that are associated with the inferred type of the parameter
are not checked when the statement is executed. Attached is a script
David sent me to reproduce the problem: when pg_server_prepare is
enabled (the default), DBD::Pg uses protocol-level prepared statements,
and the script is able to insert a tuple that violates the domain's
check constraint. Disabling pg_server_prepare results in a constraint
failure, as it should.

I've also attached a patch that should fix the issue -- coerce_type()
neglected to apply coerce_to_domain() to the type inferred for an
UNKNOWN Param. Barring any objections, I intend to apply the patch to
HEAD and release branches as far back as the problem exists (likely 8.0
and 8.1, and possibly 7.4 -- I haven't checked yet).

It would be nice to add regression tests for this issue, but AFAICS
there's no way to specify parameters of UNKNOWN type to a prepared
statement created via SQL -- which might be worth implementing anyway...

-Neil

Neil Conway <neilc@samurai.com> writes:
> I've also attached a patch that should fix the issue -- coerce_type()
> neglected to apply coerce_to_domain() to the type inferred for an
> UNKNOWN Param.

This is a good catch, but the patch's added check on targetTyptype is a
waste of code and cycles.  coerce_to_domain is perfectly capable of
doing nothing when nothing is called for.  (The reason the other paths
in the routine make such checks is that they can do so more or less for
free, thereby saving one catalog lookup in coerce_to_domain.  If it's
going to cost a catalog lookup anyway to find out if the type is a
domain, you might as well let coerce_to_domain do it.)

IOW, all you need to add is a coerce_to_domain call.  Compare a somewhat
related recent patch here:
http://archives.postgresql.org/pgsql-committers/2006-01/msg00125.php

> Barring any objections, I intend to apply the patch to
> HEAD and release branches as far back as the problem exists (likely 8.0
> and 8.1, and possibly 7.4 -- I haven't checked yet).

I think it's probably in 7.4 too.

I wonder whether there is any reasonably simple way to audit the whole
backend for missing domain processing...

            regards, tom lane

On Wed, 2006-01-11 at 23:31 -0500, Tom Lane wrote:
> This is a good catch, but the patch's added check on targetTyptype is a
> waste of code and cycles.  coerce_to_domain is perfectly capable of
> doing nothing when nothing is called for.

Ah, right. Attached is a corrected patch.

> I wonder whether there is any reasonably simple way to audit the whole
> backend for missing domain processing...

Yeah, I've been thinking along the same lines -- the recent spate of
issues doesn't give me a lot of confidence in the correctness or
completeness of the current implementation. I don't really see a way to
check the code that doesn't require a fair amount of manual auditing,
though...

-Neil

Neil Conway <neilc@samurai.com> writes:
> On Wed, 2006-01-11 at 23:31 -0500, Tom Lane wrote:
>> I wonder whether there is any reasonably simple way to audit the whole
>> backend for missing domain processing...

> I don't really see a way to
> check the code that doesn't require a fair amount of manual auditing,

Me either.  I have a nagging feeling that the problem is really a
question of mis-factoring, ie we should be doing the domain checks
at a different level than now, but I don't yet see how to translate
that intuition into code.

            regards, tom lane

On Thu, 2006-01-12 at 00:28 -0500, Neil Conway wrote:
> Ah, right. Attached is a corrected patch.

Applied to HEAD, REL8_1_STABLE, REL8_0_STABLE, and REL7_4_STABLE.

-Neil

On Jan 11, 2006, at 9:28 PM, Neil Conway wrote:

> Ah, right. Attached is a corrected patch.

I can confirm that this patch does indeed fix the issue for me on 8.1.2.

Thanks!

David