Обсуждение: Make CREATE AGGREGATE check validity of initcond value?

Поиск
Список
Период
Сортировка

Make CREATE AGGREGATE check validity of initcond value?

От
Tom Lane
Дата:
In http://archives.postgresql.org/pgsql-general/2012-10/msg00138.php
we see an example where a user tried to create an aggregate whose
"initcond" (initial transition value) wasn't valid for the transition
data type.  CREATE AGGREGATE didn't complain because it just stores the
initial condition as a text string.  It seems to me that it'd be a lot
more user-friendly if it did check the value, as per the attached
proposed patch.

Does anyone have an objection to this?  I can imagine cases where the
check would reject values that would get accepted at runtime, if the
type's input function was sensitive to the phase of the moon or
something.  But it doesn't seem very probable, whereas checking the
value seems like an eminently useful thing to do.  Or maybe I'm just
overreacting to the report --- I can't recall any previous complaints
like this, so maybe entering a bogus initcond is a corner case too.

            regards, tom lane

diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
index c99c07c..b9f8711 100644
*** a/src/backend/commands/aggregatecmds.c
--- b/src/backend/commands/aggregatecmds.c
*************** DefineAggregate(List *name, List *args,
*** 61,66 ****
--- 61,67 ----
      Oid           *aggArgTypes;
      int            numArgs;
      Oid            transTypeId;
+     char        transTypeType;
      ListCell   *pl;

      /* Convert list of names to a name and namespace */
*************** DefineAggregate(List *name, List *args,
*** 181,187 ****
       * aggregate.
       */
      transTypeId = typenameTypeId(NULL, transType);
!     if (get_typtype(transTypeId) == TYPTYPE_PSEUDO &&
          !IsPolymorphicType(transTypeId))
      {
          if (transTypeId == INTERNALOID && superuser())
--- 182,189 ----
       * aggregate.
       */
      transTypeId = typenameTypeId(NULL, transType);
!     transTypeType = get_typtype(transTypeId);
!     if (transTypeType == TYPTYPE_PSEUDO &&
          !IsPolymorphicType(transTypeId))
      {
          if (transTypeId == INTERNALOID && superuser())
*************** DefineAggregate(List *name, List *args,
*** 194,199 ****
--- 196,219 ----
      }

      /*
+      * If we have an initval, and it's not for a pseudotype (particularly a
+      * polymorphic type), make sure it's acceptable to the type's input
+      * function.  We will store the initval as text, because the input
+      * function isn't necessarily immutable (consider "now" for timestamp),
+      * and we want to use the runtime not creation-time interpretation of the
+      * value.  However, if it's an incorrect value it seems much more
+      * user-friendly to complain at CREATE AGGREGATE time.
+      */
+     if (initval && transTypeType != TYPTYPE_PSEUDO)
+     {
+         Oid            typinput,
+                     typioparam;
+
+         getTypeInputInfo(transTypeId, &typinput, &typioparam);
+         (void) OidInputFunctionCall(typinput, initval, typioparam, -1);
+     }
+
+     /*
       * Most of the argument-checking is done inside of AggregateCreate
       */
      AggregateCreate(aggName,    /* aggregate name */

Re: Make CREATE AGGREGATE check validity of initcond value?

От
Jaime Casanova
Дата:
<p><br /> El 03/10/2012 21:38, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>> escribió:<br
/>><br /> > Does anyone have an objection to this?  I can imagine cases where the<br /> > check would reject
valuesthat would get accepted at runtime, if the<br /> > type's input function was sensitive to the phase of the
moonor<br /> > something.  But it doesn't seem very probable, whereas checking the<br /> > value seems like an
eminentlyuseful thing to do.  Or maybe I'm just<br /> > overreacting to the report --- I can't recall any previous
complaints<br/> > like this, so maybe entering a bogus initcond is a corner case too.<p>I guess a wrong initcond
value,probably is a pilot error. <br /> So, my first reaction is +1 to make it an error.<br /><br /> But if you feel
therea corner cases maybe at least a warning<p>--<br /> Jaime Casanova<br /> 2ndQuadrant: Your PostgreSQL partner <br
/>