Horton

Thread

Commits

  1. Require callers of coerce_to_domain() to supply base type/typmod.

  2. Handle default NULL insertion a little better.

  1. preptlist.c can insert unprocessed expression trees

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-29T01:20:56Z

    I happened across a not-great behavior in expand_insert_targetlist.
If a column is omitted in an INSERT, and there's no column
default, the code generates a NULL Const to be inserted.
Furthermore, if the column is of a domain type, we wrap the
Const in CoerceToDomain, so as to throw a run-time error if the
domain has a NOT NULL constraint.  That's fine as far as
it goes, but there are two problems:

1. We're being sloppy about the type/typmod that the Const is
labeled with.  It really should have the domain's base type/typmod,
since it's the input to CoerceToDomain not the output.  This can
result in coerce_to_domain inserting a useless length-coercion
function (useless because it's being applied to a null).

2. We're not applying expression preprocessing (specifically,
eval_const_expressions) to the resulting expression tree.
The planner's primary expression-preprocessing pass already happened,
so that means the length coercion step and CoerceToDomain node miss
preprocessing altogether.

You can observe that there is a problem with this script:

-----
create domain d11 as varchar(11);
create table td11 (f1 int, f2 d11);
set debug_print_plan = 1;
insert into td11 values(0, null);
insert into td11 values(0);
-----

Comparing the plan tree dumps for the two INSERTs, the first just
shows a simple NULL Const of the domain type as the source for f2.
But the second shows a NULL Const of the domain type that is fed
to a varchar length-checking function and then to CoerceToDomain.
Of course they really ought to look the same.

When this code was last touched --- over twenty years ago, looks like
--- neither of these oversights meant anything more than a little
inefficiency.  However, as we've loaded more and more responsibility
onto eval_const_expressions, it's turned into what's probably a live
bug.  Specifically, if the length-coercion function call needed
default-argument insertion or named-argument reordering, things would
blow up pretty good.  That's not the case for any in-core data types,
but I wonder whether any extensions create such things.  The authors
wouldn't find out about the issue unless they tried making a domain
on top of their type, so it could have gone unreported.  So I think
this is something that needs to be fixed, and probably back-patched,
even though I don't have a test case that exhibits a crash.
(I actually found this while working on a patch that adds some
more work to eval_const_expressions, so we'll need to deal with
it going forward even if we opt not to back-patch.)

There are a few places in the rewriter that do the same sort of
thing (probably copied-and-pasted from preptlist at some point).
Those are before the planner so the results will get preprocessed
later, but it's still not great if they insert useless length-
coercion calls.  So I felt it was worth writing a utility function to
consolidate all those usages into one copy.  I'm not quite sure about
what to call it though.  In the attached 0001 patch I called it
coerce_null_to_domain and put it in parse_coerce.c.  Another idea
I considered was to consider it as a variant of makeConst and
put it in makefuncs.c.  But that would require makefuncs.c to call
parse_coerce.c which seems like a layering violation.  Anyone have
a better idea?

0001 does result in some cosmetic changes in postgres_fdw's
regression output.  That's because we're now careful to label
the null Const with the column's typmod, which we were not
before.

It seems to me that part of the problem here is that coerce_to_domain
is willing to look up the domain's base type/typmod if the caller
doesn't want to supply it.  With these changes, there's basically
noplace where the caller hasn't already looked that up, and I think
that that's probably required for correct usage.  (The caller has
to produce an input that's of the base type, after all.)  So it seems
like that's not a convenience so much as an encouragement to incorrect
coding.  I propose, for HEAD only, 0002 which removes that misfeature
and requires callers to supply the info.

			regards, tom lane

  2. Re: preptlist.c can insert unprocessed expression trees

    Sami Imseih <samimseih@gmail.com> — 2025-01-29T21:14:01Z

    > to a varchar length-checking function and then to CoerceToDomain.
> Of course they really ought to look the same.

+1. I confirmed this as well with the supplied test case.

> In the attached 0001 patch I called it
> coerce_null_to_domain and put it in parse_coerce.c.

I do think the name of the function is a bit misleading.
coerce_null_to_domain could optionally coerce to a domain if
typid != baseTypeId, but not necessarily.

From what I can tell, the coerce_null_to_domain function will be
called anytime an implicit NULL is supplied to an INSERT or
UPDATE and the target column does not have a default;
so maybe it should be called coerce_implicit_null ?

> (The caller has
> to produce an input that's of the base type, after all.)  So it seems
> like that's not a convenience so much as an encouragement to incorrect
> coding.  I propose, for HEAD only, 0002 which removes that misfeature
> and requires callers to supply the info.

This makes sense.

check-world passed with both patches applied.

Regards,

Sami