Обсуждение: tightening up on use of oid 0

I am currently cleaning up a few places where OID 0 could get used as a
parameter type (causing the backend to try to infer a type). For
example, this occurs when setObject() is passed an object that it can't
classify (not any of Integer, Boolean, PGobject, etc), or when calling
setObject() with a non-PGobject and Types.OTHER.

I think this is a bad idea: we do not have any mechanism in place to
check that the type actually matches the expected type. It also seems
quite fragile to rely on this behaviour when everything else in JDBC is
fairly strongly typed.

Another place I am less sure about is setNull(x, Types.OTHER); this
passes the NULL with type OID 0. Without this, there is no other way to
set a nonstandard-typed parameter to null. I think this is still
problematic. Consider the case where you have two functions:

   foo(line)
   foo(box)

Executing "SELECT foo(?)" via PreparedStatement will work fine if you
pass a non-null PGline or PGbox argument to setObject, but if you try to
setNull() then you will get ambiguity between the two functions at
execution time.

I can't see a way to fix this without a postgresql extension of some
sort. Options I can think of are:

1) PGStatement.setNull(int parameterIndex,String databaseType): sets
parameter to null with the given type.

2) PGStatement.setTypeHint(int parameterIndex,String databaseType): sets
parameter's type, only, to the given type; this is then used by later
calls to setNull().

3) PGConnection.addDataType(String type, Class klass, int sqlType):
register a new java.sql.Types value along with the type handler.

(3) seems like the least intrusive approach, but it requires that
extension types somehow decide on a unique Types value. I'm not sure if
this is practical.

-O

On Fri, 8 Oct 2004, Oliver Jowett wrote:
>
> Executing "SELECT foo(?)" via PreparedStatement will work fine if you
> pass a non-null PGline or PGbox argument to setObject, but if you try to
> setNull() then you will get ambiguity between the two functions at
> execution time.
>
> I can't see a way to fix this without a postgresql extension of some
> sort. Options I can think of are:

Can't the existing PGobject interface handle this.  You can create a
PGobject with the correct datatype and a null value and that should work.

Kris Jurka

Kris Jurka wrote:
>
> On Fri, 8 Oct 2004, Oliver Jowett wrote:
>
>>Executing "SELECT foo(?)" via PreparedStatement will work fine if you
>>pass a non-null PGline or PGbox argument to setObject, but if you try to
>>setNull() then you will get ambiguity between the two functions at
>>execution time.
>>
>>I can't see a way to fix this without a postgresql extension of some
>>sort. Options I can think of are:
>
>
> Can't the existing PGobject interface handle this.  You can create a
> PGobject with the correct datatype and a null value and that should work.

OK, so getValue() and toString() return null?

This would still mean that setNull() would not work for Types.OTHER, though.

-O

On Sat, 9 Oct 2004, Oliver Jowett wrote:

> > Can't the existing PGobject interface handle this.  You can create a
> > PGobject with the correct datatype and a null value and that should work.
>
> OK, so getValue() and toString() return null?

Right.

> This would still mean that setNull() would not work for Types.OTHER, though.
>

I don't think this is a very big deal.  The only place this looks
important is for an overloaded function call.  The previous driver didn't
have any typing at all for NULLs, and I can only recall one complaint
about.  The other API additions you suggested didn't really appeal to me
and we have an "out" with PGobject, so I think we're OK here.

Kris Jurka

Oliver Jowett wrote:
> I am currently cleaning up a few places where OID 0 could get used as a
> parameter type (causing the backend to try to infer a type).

Here is a patch to do this, including the PGobject changes to handle SQL
NULLs.

The PGobject / geometric type changes are a bit ugly in places, mostly
because those types are mutable (unnecessarily, IMO).

-O
Index: org/postgresql/errors.properties
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/errors.properties,v
retrieving revision 1.38
diff -u -c -r1.38 errors.properties
*** org/postgresql/errors.properties    9 Oct 2004 06:02:58 -0000    1.38
--- org/postgresql/errors.properties    9 Oct 2004 10:13:41 -0000
***************
*** 73,78 ****
--- 73,80 ----
  postgresql.prep.type:Unknown Types value.
  postgresql.prep.typenotfound:Unknown type {0}.
  postgresql.prep.zeroinstring:Zero bytes may not occur in string parameters.
+ postgresql.prep.untypedsetnull:setNull(i,Types.OTHER) and setObject(i,null,Types.OTHER) are not supported. Instead,
usesetObject(i,object,Types.OTHER) where 'object' is an appropriate PGobject subclass instance representing a NULL. 
+ postgresql.prep.untypedsetobject:setObject(i,null) is not supported. Instead, use setNull(i,type) or
setObject(i,null,type).
  postgresql.res.badbigdec:Bad BigDecimal {0}
  postgresql.res.badbyte:Bad Byte {0}
  postgresql.res.baddate:Bad Date Format {0}
Index: org/postgresql/geometric/PGbox.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGbox.java,v
retrieving revision 1.8
diff -u -c -r1.8 PGbox.java
*** org/postgresql/geometric/PGbox.java    29 Jun 2004 06:43:26 -0000    1.8
--- org/postgresql/geometric/PGbox.java    9 Oct 2004 10:13:41 -0000
***************
*** 23,31 ****
  public class PGbox extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points.
       */
!     public PGpoint point[] = new PGpoint[2];

      /**
       * @param x1 first x coordinate
--- 23,31 ----
  public class PGbox extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points, or null for a SQL NULL.
       */
!     public PGpoint point[];

      /**
       * @param x1 first x coordinate
***************
*** 35,43 ****
       */
      public PGbox(double x1, double y1, double x2, double y2)
      {
!         this();
!         this.point[0] = new PGpoint(x1, y1);
!         this.point[1] = new PGpoint(x2, y2);
      }

      /**
--- 35,41 ----
       */
      public PGbox(double x1, double y1, double x2, double y2)
      {
!         this(new PGpoint(x1, y1), new PGpoint(x2, y2));
      }

      /**
***************
*** 47,54 ****
      public PGbox(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point[0] = p1;
!         this.point[1] = p2;
      }

      /**
--- 45,51 ----
      public PGbox(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point = new PGpoint[] { p1, p2 };
      }

      /**
***************
*** 82,89 ****
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.box", PSQLState.DATA_TYPE_MISMATCH, value);

!         point[0] = new PGpoint(t.getToken(0));
!         point[1] = new PGpoint(t.getToken(1));
      }

      /**
--- 79,88 ----
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.box", PSQLState.DATA_TYPE_MISMATCH, value);

!         point = new PGpoint[] {
!             new PGpoint(t.getToken(0)),
!             new PGpoint(t.getToken(1))
!         };
      }

      /**
***************
*** 96,101 ****
--- 95,106 ----
          {
              PGbox p = (PGbox)obj;

+             if (point == p.point)
+                 return true;
+
+             if (point == null || p.point == null)
+                 return false;
+
              // Same points.
              if (p.point[0].equals(point[0]) && p.point[1].equals(point[1]))
                  return true;
***************
*** 126,136 ****
--- 131,148 ----
          // its X and Y components; we end up with an exclusive-OR of the two X and
          // two Y components, which is equal whenever equals() would return true
          // since xor is commutative.
+
+         if (point == null)
+             return 0;
+
          return point[0].hashCode() ^ point[1].hashCode();
      }

      public Object clone()
      {
+         if (point == null)
+             return new PGbox();
+
          return new PGbox((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
      }

***************
*** 139,144 ****
--- 151,161 ----
       */
      public String getValue()
      {
+         if (point == null)
+             return null;
+
          return point[0].toString() + "," + point[1].toString();
      }
+
+     public final static PGbox NULL = new PGbox();
  }
Index: org/postgresql/geometric/PGcircle.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGcircle.java,v
retrieving revision 1.10
diff -u -c -r1.10 PGcircle.java
*** org/postgresql/geometric/PGcircle.java    29 Jun 2004 06:43:26 -0000    1.10
--- org/postgresql/geometric/PGcircle.java    9 Oct 2004 10:13:41 -0000
***************
*** 24,30 ****
  public class PGcircle extends PGobject implements Serializable, Cloneable
  {
      /**
!      * This is the center point
       */
      public PGpoint center;

--- 24,30 ----
  public class PGcircle extends PGobject implements Serializable, Cloneable
  {
      /**
!      * This is the center point, or null for a NULL value
       */
      public PGpoint center;

***************
*** 102,107 ****
--- 102,114 ----
          if (obj instanceof PGcircle)
          {
              PGcircle p = (PGcircle)obj;
+
+             if (center == null && p.center == null)
+                 return true;
+
+             if (center == null || p.center == null)
+                 return false;
+
              return p.center.equals(center) && p.radius == radius;
          }
          return false;
***************
*** 109,120 ****
--- 116,133 ----

      public int hashCode()
      {
+         if (center == null)
+             return 0;
+
          long v = Double.doubleToLongBits(radius);
          return (int) (center.hashCode() ^ v ^ (v>>>32));
      }

      public Object clone()
      {
+         if (center == null)
+             return new PGcircle();
+
          return new PGcircle((PGpoint)center.clone(), radius);
      }

***************
*** 123,128 ****
--- 136,146 ----
       */
      public String getValue()
      {
+         if (center == null)
+             return null;
+
          return "<" + center + "," + radius + ">";
      }
+
+     public final static PGcircle NULL = new PGcircle();
  }
Index: org/postgresql/geometric/PGline.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGline.java,v
retrieving revision 1.8
diff -u -c -r1.8 PGline.java
*** org/postgresql/geometric/PGline.java    29 Jun 2004 06:43:26 -0000    1.8
--- org/postgresql/geometric/PGline.java    9 Oct 2004 10:13:41 -0000
***************
*** 26,34 ****
  public class PGline extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points.
       */
!     public PGpoint point[] = new PGpoint[2];

      /**
       * @param x1 coordinate for first point
--- 26,34 ----
  public class PGline extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points, or null for a SQL NULL.
       */
!     public PGpoint point[];

      /**
       * @param x1 coordinate for first point
***************
*** 48,55 ****
      public PGline(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point[0] = p1;
!         this.point[1] = p2;
      }

      /**
--- 48,54 ----
      public PGline(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point = new PGpoint[] { p1, p2 };
      }

      /**
***************
*** 80,87 ****
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.line", PSQLState.DATA_TYPE_MISMATCH, s);

!         point[0] = new PGpoint(t.getToken(0));
!         point[1] = new PGpoint(t.getToken(1));
      }

      /**
--- 79,88 ----
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.line", PSQLState.DATA_TYPE_MISMATCH, s);

!         point = new PGpoint[] {
!             new PGpoint(t.getToken(0)),
!             new PGpoint(t.getToken(1))
!         };
      }

      /**
***************
*** 93,98 ****
--- 94,106 ----
          if (obj instanceof PGline)
          {
              PGline p = (PGline)obj;
+
+             if (point == p.point)
+                 return true;
+
+             if (point == null || p.point == null)
+                 return false;
+
              return (p.point[0].equals(point[0]) && p.point[1].equals(point[1])) ||
                     (p.point[0].equals(point[1]) && p.point[1].equals(point[0]));
          }
***************
*** 100,110 ****
--- 108,124 ----
      }

      public int hashCode() {
+         if (point == null)
+             return 0;
+
          return point[0].hashCode() ^ point[1].hashCode();
      }

      public Object clone()
      {
+         if (point == null)
+             return new PGline();
+
          return new PGline((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
      }

***************
*** 113,118 ****
--- 127,137 ----
       */
      public String getValue()
      {
+         if (point == null)
+             return null;
+
          return "[" + point[0] + "," + point[1] + "]";
      }
+
+     public static final PGline NULL = new PGline();
  }
Index: org/postgresql/geometric/PGlseg.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGlseg.java,v
retrieving revision 1.8
diff -u -c -r1.8 PGlseg.java
*** org/postgresql/geometric/PGlseg.java    29 Jun 2004 06:43:26 -0000    1.8
--- org/postgresql/geometric/PGlseg.java    9 Oct 2004 10:13:41 -0000
***************
*** 23,31 ****
  public class PGlseg extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points.
       */
!     public PGpoint point[] = new PGpoint[2];

      /**
       * @param x1 coordinate for first point
--- 23,31 ----
  public class PGlseg extends PGobject implements Serializable, Cloneable
  {
      /**
!      * These are the two points, or null for a SQL NULL.
       */
!     public PGpoint point[];

      /**
       * @param x1 coordinate for first point
***************
*** 45,52 ****
      public PGlseg(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point[0] = p1;
!         this.point[1] = p2;
      }

      /**
--- 45,51 ----
      public PGlseg(PGpoint p1, PGpoint p2)
      {
          this();
!         this.point = new PGpoint[] { p1, p2 };
      }

      /**
***************
*** 77,84 ****
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.lseg", PSQLState.DATA_TYPE_MISMATCH);

!         point[0] = new PGpoint(t.getToken(0));
!         point[1] = new PGpoint(t.getToken(1));
      }

      /**
--- 76,85 ----
          if (t.getSize() != 2)
              throw new PSQLException("postgresql.geo.lseg", PSQLState.DATA_TYPE_MISMATCH);

!         point = new PGpoint[] {
!             new PGpoint(t.getToken(0)),
!             new PGpoint(t.getToken(1))
!         };
      }

      /**
***************
*** 90,95 ****
--- 91,103 ----
          if (obj instanceof PGlseg)
          {
              PGlseg p = (PGlseg)obj;
+
+             if (point == p.point)
+                 return true;
+
+             if (point == null || p.point == null)
+                 return false;
+
              return (p.point[0].equals(point[0]) && p.point[1].equals(point[1])) ||
                     (p.point[0].equals(point[1]) && p.point[1].equals(point[0]));
          }
***************
*** 98,108 ****
--- 106,122 ----

      public int hashCode()
      {
+         if (point == null)
+             return 0;
+
          return point[0].hashCode() ^ point[1].hashCode();
      }

      public Object clone()
      {
+         if (point == null)
+             return new PGlseg();
+
          return new PGlseg((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
      }

***************
*** 111,116 ****
--- 125,135 ----
       */
      public String getValue()
      {
+         if (point == null)
+             return null;
+
          return "[" + point[0] + "," + point[1] + "]";
      }
+
+     public final static PGlseg NULL = new PGlseg();
  }
Index: org/postgresql/geometric/PGpath.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGpath.java,v
retrieving revision 1.9
diff -u -c -r1.9 PGpath.java
*** org/postgresql/geometric/PGpath.java    29 Jun 2004 06:43:26 -0000    1.9
--- org/postgresql/geometric/PGpath.java    9 Oct 2004 10:13:41 -0000
***************
*** 28,34 ****
      public boolean open;

      /**
!      * The points defining this path
       */
      public PGpoint points[];

--- 28,34 ----
      public boolean open;

      /**
!      * The points defining this path, or null for a SQL NULL.
       */
      public PGpoint points[];

***************
*** 98,103 ****
--- 98,109 ----
          {
              PGpath p = (PGpath)obj;

+             if (points == null && p.points == null)
+                 return true;
+
+             if (points == null || p.points == null)
+                 return false;
+
              if (p.points.length != points.length)
                  return false;

***************
*** 115,120 ****
--- 121,130 ----

      public int hashCode() {
          // XXX not very good..
+
+         if (points == null)
+             return 0;
+
          int hash = 0;
          for (int i = 0; i < points.length && i < 5; ++i) {
              hash = hash ^ points[i].hashCode();
***************
*** 124,129 ****
--- 134,142 ----

      public Object clone()
      {
+         if (points == null)
+             return new PGpath();
+
          PGpoint ary[] = new PGpoint[points.length];
          for (int i = 0;i < points.length;i++)
              ary[i] = (PGpoint)points[i].clone();
***************
*** 135,140 ****
--- 148,156 ----
       */
      public String getValue()
      {
+         if (points == null)
+             return null;
+
          StringBuffer b = new StringBuffer(open ? "[" : "(");

          for (int p = 0;p < points.length;p++)
***************
*** 167,170 ****
--- 183,188 ----
      {
          open = true;
      }
+
+     public final static PGpath NULL = new PGpath();
  }
Index: org/postgresql/geometric/PGpoint.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGpoint.java,v
retrieving revision 1.9
diff -u -c -r1.9 PGpoint.java
*** org/postgresql/geometric/PGpoint.java    29 Jun 2004 06:43:26 -0000    1.9
--- org/postgresql/geometric/PGpoint.java    9 Oct 2004 10:13:41 -0000
***************
*** 94,99 ****
--- 94,106 ----
          if (obj instanceof PGpoint)
          {
              PGpoint p = (PGpoint)obj;
+
+             if (this == NULL && p == NULL)
+                 return true;
+
+             if (this == NULL || p == NULL)
+                 return false;
+
              return x == p.x && y == p.y;
          }
          return false;
***************
*** 101,106 ****
--- 108,116 ----

      public int hashCode()
      {
+         if (this == NULL)
+             return 0;
+
          long v1 = Double.doubleToLongBits(x);
          long v2 = Double.doubleToLongBits(y);
          return (int) (v1 ^ v2 ^ (v1>>>32) ^ (v2>>>32));
***************
*** 108,113 ****
--- 118,126 ----

      public Object clone()
      {
+         if (this == NULL)
+             return NULL;
+
          return new PGpoint(x, y);
      }

***************
*** 116,121 ****
--- 129,137 ----
       */
      public String getValue()
      {
+         if (this == NULL)
+             return null;
+
          return "(" + x + "," + y + ")";
      }

***************
*** 184,187 ****
--- 200,207 ----
          setLocation(p.x, p.y);
      }

+     // Ugly hack -- this unique object, by object identity, is a SQL NULL.
+     // Cloning NULL gives you NULL. You can mutate NULL, but it doesn't
+     // affect its NULL-ness.
+     public final static PGpoint NULL = new PGpoint();
  }
Index: org/postgresql/geometric/PGpolygon.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/geometric/PGpolygon.java,v
retrieving revision 1.8
diff -u -c -r1.8 PGpolygon.java
*** org/postgresql/geometric/PGpolygon.java    29 Jun 2004 06:43:26 -0000    1.8
--- org/postgresql/geometric/PGpolygon.java    9 Oct 2004 10:13:41 -0000
***************
*** 20,26 ****
  public class PGpolygon extends PGobject implements Serializable, Cloneable
  {
      /**
!      * The points defining the polygon
       */
      public PGpoint points[];

--- 20,26 ----
  public class PGpolygon extends PGobject implements Serializable, Cloneable
  {
      /**
!      * The points defining the polygon, or null if this is a SQL NULL.
       */
      public PGpoint points[];

***************
*** 76,81 ****
--- 76,87 ----
          {
              PGpolygon p = (PGpolygon)obj;

+             if (points == p.points)
+                 return true;
+
+             if (points == null || p.points == null)
+                 return false;
+
              if (p.points.length != points.length)
                  return false;

***************
*** 89,94 ****
--- 95,103 ----
      }

      public int hashCode() {
+         if (points == null)
+             return 0;
+
          // XXX not very good..
          int hash = 0;
          for (int i = 0; i < points.length && i < 5; ++i) {
***************
*** 99,104 ****
--- 108,116 ----

      public Object clone()
      {
+         if (points == null)
+             return new PGpolygon();
+
          PGpoint ary[] = new PGpoint[points.length];
          for (int i = 0;i < points.length;i++)
              ary[i] = (PGpoint)points[i].clone();
***************
*** 110,115 ****
--- 122,130 ----
       */
      public String getValue()
      {
+         if (points == null)
+             return null;
+
          StringBuffer b = new StringBuffer();
          b.append("(");
          for (int p = 0;p < points.length;p++)
***************
*** 121,124 ****
--- 136,141 ----
          b.append(")");
          return b.toString();
      }
+
+     public final static PGpolygon NULL = new PGpolygon();
  }
Index: org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java,v
retrieving revision 1.48
diff -u -c -r1.48 AbstractJdbc2ResultSet.java
*** org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    30 Sep 2004 18:16:52 -0000    1.48
--- org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    9 Oct 2004 10:13:42 -0000
***************
*** 31,36 ****
--- 31,37 ----
  import org.postgresql.Driver;
  import org.postgresql.core.*;
  import org.postgresql.largeobject.*;
+ import org.postgresql.util.PGobject;
  import org.postgresql.util.PGbytea;
  import org.postgresql.util.PGtokenizer;
  import org.postgresql.util.PSQLException;
***************
*** 766,775 ****
              {
                  String key = (String) keys.nextElement();
                  Object o = updateValues.get(key);
!                 if (o instanceof NullObject)
!                     insertStatement.setNull(i,java.sql.Types.NULL);
!                 else
!                     insertStatement.setObject(i, o);
              }

              insertStatement.executeUpdate();
--- 767,773 ----
              {
                  String key = (String) keys.nextElement();
                  Object o = updateValues.get(key);
!                 insertStatement.setObject(i, o);
              }

              insertStatement.executeUpdate();
***************
*** 1084,1090 ****
      public synchronized void updateNull(int columnIndex)
      throws SQLException
      {
!         updateValue(columnIndex, new NullObject());
      }


--- 1082,1089 ----
      public synchronized void updateNull(int columnIndex)
      throws SQLException
      {
!         String columnTypeName = connection.getPGType(fields[columnIndex - 1].getOID());
!         updateValue(columnIndex, new NullObject(columnTypeName));
      }


***************
*** 1245,1254 ****
                  for (; iterator.hasNext(); i++)
                  {
                      Object o = iterator.next();
!                     if (o instanceof NullObject)
!                         updateStatement.setNull(i+1,java.sql.Types.NULL);
!                     else
!                         updateStatement.setObject( i + 1, o );

                  }
                  for ( int j = 0; j < numKeys; j++, i++)
--- 1244,1250 ----
                  for (; iterator.hasNext(); i++)
                  {
                      Object o = iterator.next();
!                     updateStatement.setObject( i + 1, o );

                  }
                  for ( int j = 0; j < numKeys; j++, i++)
***************
*** 2613,2620 ****
          }
      };

!     class NullObject {
!     };

  }

--- 2609,2628 ----
          }
      };

!     //
!     // We need to specify the type of NULL when updating a column to NULL, so
!     // NullObject is a simple extension of PGobject that always returns null
!     // values but retains column type info.
!     //

+     class NullObject extends PGobject {
+         NullObject(String type) {
+             setType(type);
+         }
+
+         public String getValue() {
+             return null;
+         }
+     };
  }

Index: org/postgresql/jdbc2/AbstractJdbc2Statement.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Statement.java,v
retrieving revision 1.32
diff -u -c -r1.32 AbstractJdbc2Statement.java
*** org/postgresql/jdbc2/AbstractJdbc2Statement.java    9 Oct 2004 06:02:58 -0000    1.32
--- org/postgresql/jdbc2/AbstractJdbc2Statement.java    9 Oct 2004 10:13:42 -0000
***************
*** 816,824 ****
                  oid = Oid.BYTEA;
                  break;
              case Types.OTHER:
              default:
!                 oid = 0;
!                 break;
          }

          preparedParameters.setNull(adjustParamIndex(parameterIndex), oid);
--- 816,826 ----
                  oid = Oid.BYTEA;
                  break;
              case Types.OTHER:
+                 // We cannot determine an appropriate OID in this case.
+                 throw new PSQLException("postgresql.prep.untypednull", PSQLState.INVALID_PARAMETER_TYPE);
              default:
!                 // Bad Types value.
!                 throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
          }

          preparedParameters.setNull(adjustParamIndex(parameterIndex), oid);
***************
*** 1372,1377 ****
--- 1374,1389 ----
              return new BigDecimal(x.toString()).toString();
      }

+     // Helper method for setting parameters to PGobject subclasses.
+     private void setPGobject(int parameterIndex, PGobject x) throws SQLException {
+         String typename = x.getType();
+         int oid = connection.getPGType(typename);
+         if (oid == Oid.INVALID)
+             throw new PSQLException("postgresql.prep.typenotfound", PSQLState.INVALID_PARAMETER_TYPE, typename);
+
+         setString(parameterIndex, x.getValue(), oid);
+     }
+
      /*
       * Set the value of a parameter using an object; use the java.lang
       * equivalent objects for integral values.
***************
*** 1478,1487 ****
                  setObject(parameterIndex, x);
                  break;
              case Types.OTHER:
!                 if (x instanceof PGobject)
!                     setString(parameterIndex, ((PGobject)x).getValue(), connection.getPGType( ((PGobject)x).getType()
));
!                 else
                      throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
                  break;
              default:
                  throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
--- 1490,1501 ----
                  setObject(parameterIndex, x);
                  break;
              case Types.OTHER:
!                 if (x instanceof PGobject) {
!                     setPGobject(parameterIndex, (PGobject)x);
!                 } else {
!                     // Nope. Go away!
                      throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
+                 }
                  break;
              default:
                  throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
***************
*** 1499,1509 ****
      public void setObject(int parameterIndex, Object x) throws SQLException
      {
          checkClosed();
!         if (x == null)
!         {
!             setNull(parameterIndex, Types.OTHER);
!             return;
          }
          if (x instanceof String)
              setString(parameterIndex, (String)x);
          else if (x instanceof BigDecimal)
--- 1513,1523 ----
      public void setObject(int parameterIndex, Object x) throws SQLException
      {
          checkClosed();
!         if (x == null) {
!             // We cannot determine an appropriate OID in this case.
!             throw new PSQLException("postgresql.prep.untypedsetobject", PSQLState.INVALID_PARAMETER_TYPE);
          }
+
          if (x instanceof String)
              setString(parameterIndex, (String)x);
          else if (x instanceof BigDecimal)
***************
*** 1529,1538 ****
          else if (x instanceof Boolean)
              setBoolean(parameterIndex, ((Boolean)x).booleanValue());
          else if (x instanceof PGobject)
!             setString(parameterIndex, ((PGobject)x).getValue(), connection.getPGType(((PGobject)x).getType()));
!         else
!             // Try to store as a string in database
!             setString(parameterIndex, x.toString(), 0);
      }

      /*
--- 1543,1553 ----
          else if (x instanceof Boolean)
              setBoolean(parameterIndex, ((Boolean)x).booleanValue());
          else if (x instanceof PGobject)
!             setPGobject(parameterIndex, (PGobject)x);
!         else {
!             // Nope. Go away!
!             throw new PSQLException("postgresql.prep.type", PSQLState.INVALID_PARAMETER_TYPE);
!         }
      }

      /*
Index: org/postgresql/test/jdbc2/GeometricTest.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/jdbc2/GeometricTest.java,v
retrieving revision 1.1
diff -u -c -r1.1 GeometricTest.java
*** org/postgresql/test/jdbc2/GeometricTest.java    29 Jun 2004 06:43:28 -0000    1.1
--- org/postgresql/test/jdbc2/GeometricTest.java    9 Oct 2004 10:13:42 -0000
***************
*** 44,50 ****
          Statement stmt = con.createStatement();
          ResultSet rs = stmt.executeQuery("SELECT " + column + " FROM testgeometric");
          assertTrue(rs.next());
!         assertEquals(obj, rs.getObject(1));
          rs.close();

          stmt.executeUpdate("DELETE FROM testgeometric");
--- 44,56 ----
          Statement stmt = con.createStatement();
          ResultSet rs = stmt.executeQuery("SELECT " + column + " FROM testgeometric");
          assertTrue(rs.next());
!         Object check = rs.getObject(1);
!         if (obj.getValue() == null) {
!             assertNull(check);
!             assertTrue(rs.wasNull());
!         } else {
!             assertEquals(obj, check);
!         }
          rs.close();

          stmt.executeUpdate("DELETE FROM testgeometric");
***************
*** 57,68 ****
--- 63,76 ----
          checkReadWrite(new PGbox(1.0, -2.0, 3.0, 4.0),  "boxval");
          checkReadWrite(new PGbox(1.0, 2.0, -3.0, 4.0),  "boxval");
          checkReadWrite(new PGbox(1.0, 2.0, 3.0, -4.0),  "boxval");
+         checkReadWrite(PGbox.NULL,  "boxval");
      }

      public void testPGcircle() throws Exception {
          checkReadWrite(new PGcircle(1.0, 2.0, 3.0),    "circleval");
          checkReadWrite(new PGcircle(-1.0, 2.0, 3.0),    "circleval");
          checkReadWrite(new PGcircle(1.0, -2.0, 3.0),    "circleval");
+         checkReadWrite(PGcircle.NULL, "circleval");
      }

      public void testPGlseg() throws Exception {
***************
*** 71,76 ****
--- 79,85 ----
          checkReadWrite(new PGlseg(1.0, -2.0, 3.0, 4.0), "lsegval");
          checkReadWrite(new PGlseg(1.0, 2.0, -3.0, 4.0), "lsegval");
          checkReadWrite(new PGlseg(1.0, 2.0, 3.0, -4.0), "lsegval");
+         checkReadWrite(PGlseg.NULL, "lsegval");
      }

      public void testPGpath() throws Exception {
***************
*** 85,90 ****
--- 94,100 ----

          checkReadWrite(new PGpath(points, true),  "pathval");
          checkReadWrite(new PGpath(points, false), "pathval");
+         checkReadWrite(PGpath.NULL, "pathval");
      }

      public void testPGpolygon() throws Exception {
***************
*** 98,106 ****
--- 108,118 ----
          };

          checkReadWrite(new PGpolygon(points), "polygonval");
+         checkReadWrite(PGpolygon.NULL, "polygonval");
      }

      public void testPGpoint() throws Exception {
          checkReadWrite(new PGpoint(1.0, 2.0), "pointval");
+         checkReadWrite(PGpoint.NULL, "pointval");
      }
  }
Index: org/postgresql/util/PGInterval.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/util/PGInterval.java,v
retrieving revision 1.2
diff -u -c -r1.2 PGInterval.java
*** org/postgresql/util/PGInterval.java    20 Sep 2004 08:36:51 -0000    1.2
--- org/postgresql/util/PGInterval.java    9 Oct 2004 10:13:42 -0000
***************
*** 4,23 ****

  public class PGInterval extends PGobject implements Serializable, Cloneable
  {
!   public PGInterval()
!   {
!     setType("interval");
!   }
!   public PGInterval(String value )
!   {
!     this.value = value;
!   }

!   /*
!    * This must be overidden to allow the object to be cloned
!    */
!   public Object clone()
!   {
!     return new PGInterval( value );
!   }
  }
--- 4,26 ----

  public class PGInterval extends PGobject implements Serializable, Cloneable
  {
!     public PGInterval()
!     {
!         setType("interval");
!     }

!     public PGInterval(String value)
!     {
!         this.value = value;
!     }
!
!     /*
!      * This must be overidden to allow the object to be cloned
!      */
!     public Object clone()
!     {
!         return new PGInterval( value );
!     }
!
!     public final static PGInterval NULL = new PGInterval();
  }
Index: org/postgresql/util/PGmoney.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/util/PGmoney.java,v
retrieving revision 1.7
diff -u -c -r1.7 PGmoney.java
*** org/postgresql/util/PGmoney.java    29 Nov 2003 19:52:11 -0000    1.7
--- org/postgresql/util/PGmoney.java    9 Oct 2004 10:13:42 -0000
***************
*** 81,86 ****
--- 81,93 ----
          if (obj instanceof PGmoney)
          {
              PGmoney p = (PGmoney)obj;
+
+             if (this == p)
+                 return true;
+
+             if (this == NULL || p == NULL)
+                 return false;
+
              return val == p.val;
          }
          return false;
***************
*** 91,101 ****
--- 98,114 ----
       */
      public Object clone()
      {
+         if (this == NULL)
+             return NULL;
+
          return new PGmoney(val);
      }

      public String getValue()
      {
+         if (this == NULL)
+             return null;
+
          if (val < 0)
          {
              return "-$" + ( -val);
***************
*** 105,108 ****
--- 118,127 ----
              return "$" + val;
          }
      }
+
+
+     // Ugly hack -- this unique object, by object identity, is a SQL NULL.
+     // Cloning NULL gives you NULL. You can mutate NULL, but it doesn't
+     // affect its NULL-ness.
+     public final static PGmoney NULL = new PGmoney();
  }
Index: org/postgresql/util/PGobject.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/util/PGobject.java,v
retrieving revision 1.6
diff -u -c -r1.6 PGobject.java
*** org/postgresql/util/PGobject.java    7 Jun 2004 21:52:46 -0000    1.6
--- org/postgresql/util/PGobject.java    9 Oct 2004 10:13:42 -0000
***************
*** 62,68 ****

      /**
       * This must be overidden, to return the value of the object, in the
!      * form required by org.postgresql.
       * @return the value of this object
       */
      public String getValue()
--- 62,70 ----

      /**
       * This must be overidden, to return the value of the object, in the
!      * form required by org.postgresql. If this returns null, the object
!      * represents an appropriately-typed SQL NULL.
!      *
       * @return the value of this object
       */
      public String getValue()
***************
*** 99,104 ****
       */
      public String toString()
      {
!         return getValue();
      }
  }
--- 101,109 ----
       */
      public String toString()
      {
!         String value = getValue();
!         if (value == null)
!             return "NULL";
!         return value;
      }
  }

On Sat, 9 Oct 2004, Oliver Jowett wrote:

> Oliver Jowett wrote:
> > I am currently cleaning up a few places where OID 0 could get used as a
> > parameter type (causing the backend to try to infer a type).
>
> Here is a patch to do this, including the PGobject changes to handle SQL
> NULLs.
>

What I was suggesting before was a means to allow users to specify a pg
type for the Types.OTHER case, but not to require it.  I don't see the
danger in allowing OID 0 in this case.  I know you are after complete
strong typing, but I don't see the benefit while I do see the drawback.
Comments?

Kris Jurka

Kris Jurka wrote:
>
> On Sat, 9 Oct 2004, Oliver Jowett wrote:
>
>
>>Oliver Jowett wrote:
>>
>>>I am currently cleaning up a few places where OID 0 could get used as a
>>>parameter type (causing the backend to try to infer a type).
>>
>>Here is a patch to do this, including the PGobject changes to handle SQL
>>NULLs.
>>
>
>
> What I was suggesting before was a means to allow users to specify a pg
> type for the Types.OTHER case, but not to require it.  I don't see the
> danger in allowing OID 0 in this case.

Ah, I thought you were OK with disallowing setNull(Types.OTHER)
entirely, I must have misunderstood what you said earlier.

I described one problem with allowing it in an earlier email:

>> Consider the case where you have two functions:
>>
>>   foo(line)
>>   foo(box)
>>
>> Executing "SELECT foo(?)" via PreparedStatement will work fine if you pass a non-null PGline or PGbox argument to
setObject,but if you try to setNull() then you will get ambiguity between the two functions at execution time.  

That's quite unpredictable behaviour.

> I know you are after complete
> strong typing, but I don't see the benefit while I do see the drawback.

What is the drawback? The only case that will change is the case that is
currently ambiguous. And there is a fairly simple mechanism for
disambiguating it via PGobject.

-O

On Wed, 13 Oct 2004, Oliver Jowett wrote:

> >> Consider the case where you have two functions:
> >>
> >>   foo(line)
> >>   foo(box)
> >>
> >> Executing "SELECT foo(?)" via PreparedStatement will work fine if you
> >> pass a non-null PGline or PGbox argument to setObject, but if you try
> >> to setNull() then you will get ambiguity between the two functions at
> >> execution time.
>

I was expecting to see this "ERROR:  function f("unknown") is not unique"
in all ambiguous situations.  Your approach has the benefit of being
fail-fast as adding a new function to the database suddently can't
produce an ambiguity for Java code.  I don't think it's a common
situation to have overloaded functions that get called with non-typed
nulls, so I wanted to allow it to work as usual for non-ambiguous
cases.

I was testing this out a little and this doesn't produce the error I
expected:

CREATE FUNCTION g(int) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
CREATE FUNCTION g(float) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
SELECT g(NULL);

Instead it returns 2 indicating the float version was called.  I don't
know if this is a bug and/or oddity of the type system, but if it's the
expected behavior then I definitely agree with you.

> What is the drawback? The only case that will change is the case that is
> currently ambiguous. And there is a fairly simple mechanism for
> disambiguating it via PGobject.

The case that will change is that all the non-ambiguous cases can
no longer be called with untyped nulls:
 - non ambiguous functions
 - INSERT/UPDATE statements that use nulls

Kris Jurka

Kris Jurka wrote:

> I was testing this out a little and this doesn't produce the error I
> expected:
>
> CREATE FUNCTION g(int) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
> CREATE FUNCTION g(float) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
> SELECT g(NULL);

I think implicit casting is interfering here somehow.

With types that don't have implicit casts, you get ambiguity:

>> test=> CREATE FUNCTION h(line) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
>> CREATE FUNCTION
>> test=> CREATE FUNCTION h(point) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
>> CREATE FUNCTION
>> test=> SELECT h(NULL);
>> ERROR:  function h("unknown") is not unique
>> HINT:  Could not choose a best candidate function. You may need to add explicit type casts.

-O

Kris Jurka wrote:

> Your approach has the benefit of being
> fail-fast as adding a new function to the database suddently can't
> produce an ambiguity for Java code.

Right. The driver cannot guarantee that setNull(i,Types.OTHER) will
always be able to infer the right type. It seems safer to me to
completely disallow it than have it sometimes succeed and sometimes fail
depending on the exact state of the database. It may break older apps,
but new apps would be more robust.

General question for the list: how much code is out there that performs
one of these (equivalent) calls?

   PreparedStatement.setObject(i, null);
   PreparedStatement.setObject(i, null, Types.OTHER);
   PreparedStatement.setNull(i, Types.OTHER);

> I don't think it's a common
> situation to have overloaded functions that get called with non-typed
> nulls, so I wanted to allow it to work as usual for non-ambiguous
> cases.

After some experimentation, it's not just overloaded functions that are
affected.

- "? IS NULL" breaks if the parameter is an untyped NULL (this was the
original issue -- in an off-list email --that made me look at this area)

- Functions taking 'anyarray' or 'anyelement' don't like untyped NULLs,
even if they are STRICT (ERROR:  could not determine anyarray/anyelement
type because input has type "unknown")

I'd like to catch those errors earlier than query execution if possible.
If nothing else, it'll encourage application developers to provide
correct type info to the driver in all cases..

-O

Hi,

For my part, I've never used any of those calls.

FWIW, I always use reference objects such as "Integer" as opposed to "int"
so I've never done it that way. All calls to setObject or set<Anything>
would always use a variable which is typed (and may be null) my assumption
is that I avoid all such ambiguities that way.

regards
iain

> Kris Jurka wrote:

> General question for the list: how much code is out there that performs
> one of these (equivalent) calls?
>
>   PreparedStatement.setObject(i, null);
>   PreparedStatement.setObject(i, null, Types.OTHER);
>   PreparedStatement.setNull(i, Types.OTHER);
>

Oliver Jowett <oliver@opencloud.com> writes:
> - "? IS NULL" breaks if the parameter is an untyped NULL (this was the
> original issue -- in an off-list email --that made me look at this area)

Hmm.  The system doesn't complain if you do "select 'z' IS NULL".  It
knows that it doesn't have a hard idea about the datatype of 'z', but it
also knows that it doesn't matter much.  The reason that you are seeing
a failure is that exec_parse_message() explicitly fails if any parameter
datatypes remain UNKNOWN after parsing.  I made it do that because I
expected that client code would be unhappy to get UNKNOWN back as a
"resolved" parameter datatype.  Would you rather get that result or
a failure?

> - Functions taking 'anyarray' or 'anyelement' don't like untyped NULLs,
> even if they are STRICT (ERROR:  could not determine anyarray/anyelement
> type because input has type "unknown")

This you're just stuck with.  There has to be some way to determine the
actual datatype imputed to the function result, and if you supply an
untyped parameter then there isn't.  It hasn't got anything to do with
whether the parameter is NULL or not.

            regards, tom lane

On Wed, 13 Oct 2004, Oliver Jowett wrote:

> I'd like to catch those errors earlier than query execution if possible.
> If nothing else, it'll encourage application developers to provide
> correct type info to the driver in all cases..

I don't see the real benefit of catching this one statement earlier.  It's
still a runtime failure.

Kris Jurka

Iain wrote:
> Hi,
>
> For my part, I've never used any of those calls.
>
> FWIW, I always use reference objects such as "Integer" as opposed to
> "int" so I've never done it that way. All calls to setObject or
> set<Anything> would always use a variable which is typed (and may be
> null) my assumption is that I avoid all such ambiguities that way.

Just to clarify.. these calls are typed:

   setInt(i, 42);
   setObject(i, new Integer(42));
   setObject(i, new Integer(42), Types.INTEGER);
   setObject(i, null, Types.INTEGER);
   setNull(i, Types.INTEGER);
   setObject(i, new PGline(...), Types.OTHER);

These calls are not (sufficiently) typed:

   setObject(i, null);
   setObject(i, (Integer)null);     // (*)
   setObject(i, null, Types.OTHER);
   setNull(i, Types.OTHER);

Types.OTHER on its own is not specific enough to identify a particular
backend type, and Java nulls have no inherent type ('instanceof' will
always return false).

 From your description it sounds like you may use the case marked (*) ?

-O

Kris Jurka wrote:
>
> On Wed, 13 Oct 2004, Oliver Jowett wrote:
>
>
>>I'd like to catch those errors earlier than query execution if possible.
>>If nothing else, it'll encourage application developers to provide
>>correct type info to the driver in all cases..
>
>
> I don't see the real benefit of catching this one statement earlier.  It's
> still a runtime failure.

My point was that it is useful to generate an error on *all* uses of
untyped nulls, so that the developer sees the error in all cases
regardless of the database state or statement context.

Consider, for example, framework code that calls user-specified or
user-provided functions. If it uses untyped nulls, and the driver uses
Oid.UNKNOWN, you will only see the problem when the framework tries to
pass a NULL to an overloaded function. If the driver rejects the call
with an error in all cases where a NULL is used, then the framework
developer sees the error as soon as they exercise the NULL path,
regardless of whether their test environment has overloaded functions or
not.

====

The spec has this to say about the setObject case:

>> If setObject is called without a type parameter, the Java Object is
>> implicitly mapped using the default mapping for that object type.

There is no default mapping (in appendix B table B-4) for Java nulls.

Of course the spec then goes on to say:

>> If a Java null is passed to any of the setter methods that take a Java
>> object, the parameter will be set to JDBC NULL.

I don't know which takes precedence. The way that setNull() is organized
implies that JDBC requires NULLs to be typed, but there's nothing
explicit about it.

-O

Hi Oliver,

Just out of interest, is the case you marked,

>   setObject(i, (Integer)null);     // (*)

equivalent to

    Integer someInteger = null;
    setObject(i, someInteger);

?

From what I remember of my code I'd be surprised if I was doing either as
this case would use setInt instead of setObject. I don't think I use
setObject anywhere.

I would ask the question then, is there any situation where there is no
alternative to the insufficiantly typed calls you listed? From my limited
view of the situation, my feeling is that there isn't, so I would say that
such calls should produce errors rather than some kind of default behavour.

Cheers
Iain

Iain wrote:
> Hi Oliver,
>
> Just out of interest, is the case you marked,
>
>>   setObject(i, (Integer)null);     // (*)
>
> equivalent to
>
>    Integer someInteger = null;
>    setObject(i, someInteger);
>
> ?

Yes.

> I would ask the question then, is there any situation where there is no
> alternative to the insufficiantly typed calls you listed?

I think there is always an alternative.

For standard types you can use setNull or setObject with a type code:

   setNull(i, Types.INTEGER);
   setObject(i, null, Types.INTEGER);

For extension types (classed as Types.OTHER) you can use the singleton
NULL objects I introduced in my patch:

   setObject(i, PGline.NULL);
   setObject(i, PGline.NULL, Types.OTHER);

-O

Kris Jurka wrote:

> I was looking at the assorted changes to the PGobject extensions and I'm
> unclear on exactly how NULL is handled.  Consider PGmoney has tests for
> NULL in equals, clone, and getValue, but PGbox does not.  Is this simply
> an oversight or is there something more profound going on here.

I ended up with two approaches for this.

For those types where there was already a field I could hijack to
represent NULL -- e.g. PGbox's points array -- I used that to represent
null values. The singleton NULL is just a normal object that happens to
have a null value. You can have several different objects that all
represent null if you like, and you can mutate an object representing a
null value just like any other object of the type. This is consistent
with the way other instances of the type operate, but it's slightly
dangerous as it's possible to modify the NULL singleton so it no longer
has a null value (pity we don't have 'const'..)

For the other types that didn't have a suitable field, I'd have needed
to add a field to every instance of the type to indicate whether the
object was a null or not. Instead, I used the identity of the NULL
singleton to decide when an object is null. In that case, there is only
ever one object that represents a null, and the actual value it holds is
irrelevant -- getValue() etc. check the identity of 'this' before
looking at the actual value.

It's hardly ideal but it kept the changes to a minimum. If you don't
mind a more invasive set of changes, I can probably come up with
something better.

-O

On Thu, 14 Oct 2004, Oliver Jowett wrote:

> Kris Jurka wrote:
>
> > I was looking at the assorted changes to the PGobject extensions and I'm
> > unclear on exactly how NULL is handled.  Consider PGmoney has tests for
> > NULL in equals, clone, and getValue, but PGbox does not.  Is this simply
> > an oversight or is there something more profound going on here.
>
> I ended up with two approaches for this.

I don't like the lack of consistency here, "new PGbox()" is NULL, but "new
PGmoney()" is zero instead.  I also don't like the ability to mutate away
NULLness.  This means another application can break mine by modifying the
shared PGbox.NULL object.

> It's hardly ideal but it kept the changes to a minimum. If you don't
> mind a more invasive set of changes, I can probably come up with
> something better.

Yes, let's think about this a little more.  I unfortunately don't have any
brilliant ideas, perhaps just adding a boolean everywhere is simplest.

Kris Jurka

Here's a merged version of the patch, if it helps:

http://www.ejurka.com/pgsql/patches/

Hi,

Interesting, thanks for your feedback.

regards
Iain
----- Original Message -----
From: "Oliver Jowett" <oliver@opencloud.com>
To: "Iain" <iain@mst.co.jp>
Cc: <pgsql-jdbc@postgresql.org>
Sent: Thursday, October 14, 2004 11:40 AM
Subject: Re: [JDBC] tightening up on use of oid 0


> Iain wrote:
>> Hi Oliver,
>>
>> Just out of interest, is the case you marked,
>>
>>>   setObject(i, (Integer)null);     // (*)
>>
>> equivalent to
>>
>>    Integer someInteger = null;
>>    setObject(i, someInteger);
>>
>> ?
>
> Yes.
>
>> I would ask the question then, is there any situation where there is no
>> alternative to the insufficiantly typed calls you listed?
>
> I think there is always an alternative.
>
> For standard types you can use setNull or setObject with a type code:
>
>   setNull(i, Types.INTEGER);
>   setObject(i, null, Types.INTEGER);
>
> For extension types (classed as Types.OTHER) you can use the singleton
> NULL objects I introduced in my patch:
>
>   setObject(i, PGline.NULL);
>   setObject(i, PGline.NULL, Types.OTHER);
>
> -O
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>      joining column's datatypes do not match

Kris Jurka wrote:
>
> On Thu, 14 Oct 2004, Oliver Jowett wrote:
>   [... PGobject and NULL ...]
>>It's hardly ideal but it kept the changes to a minimum. If you don't
>>mind a more invasive set of changes, I can probably come up with
>>something better.
>
> Yes, let's think about this a little more.  I unfortunately don't have any
> brilliant ideas, perhaps just adding a boolean everywhere is simplest.

I've applied the non-PGobject bits of this patch.

For PGobject it turned into a bit of a general overhaul. Currently I have:

PGobject layer:

- PGobject becomes an interface
- Implementations of PGobject should provide a ctor taking a single
String; this is called by the driver to construct non-null objects.
- PGobject.setType() and PGobject.setValue() go away entirely
- A new helper class, PGunknown, provides an immutable type+value
implementation of PGobject (i.e. 'new PGunknown("mytype","myvalue")').
This gives the functionality currently available via PGobject.setType()
/ setValue, and is used by the driver when it receives an unhandled type
in a resultset.

PGobject subclasses:

Mutability:
- Generally, classes become immutable where it's easy to do (PGmoney,
PGinterval, PGpoint, PGline, PGlseg, PGcircle, PGbox)
- PGpoint.translate() returns a new PGpoint rather than modifying the
existing point (I'm not sure why this method even exists really..)
- Constant-sized PGpoint[] arrays in PGline, PGlseg, PGbox become
separate fields; this makes immutability much easier.
- PGpolygon and PGpath remain mutable as it's hard to make them
immutable without incurring lots of array copies.

NULL-handling:
- No-arg ctors construct NULL objects.
- PGmoney and PGpoint get boolean isNull fields, the other types reuse
an existing reference field.
- equals(), hashCode(), getValue() take account of NULL-ness.
- Add per-class static NULL singleton fields for convenience; for
mutable types, I'm not sure whether to just omit the singletons
(slightly less convenient/readable), or to use PGunknown to get an
immutable NULL (works, but 'instanceof' on NULL becomes misleading).

Much of the mutability changes here are just personal preference,
they're not necessary to support NULLs but I thought I'd clean things up
while I was in the area.

When we come to do binary-format types, I'd expect we would have a
subinterface (PGbinaryobject?) that adds whatever methods are needed for
binary parameter formatting. Objects that implement PGbinaryobject
become candidates for binary transfer.

Any thoughts on this general approach?

-O

Oliver Jowett wrote:

> For PGobject it turned into a bit of a general overhaul. Currently I have: [...]

I've put my current changes up at:
http://visible.randomly.org/pgjdbc/pgjdbc-pgobject-changes.txt

-O

Hi, Oliver,

On Thu, 28 Oct 2004 15:18:13 +1300
Oliver Jowett <oliver@opencloud.com> wrote:

> - Implementations of PGobject should provide a ctor taking a single
> String; this is called by the driver to construct non-null objects.

Is it possible to use a static factory function instead?

This would make it possible to produce different subclasses depending on
the String, which would be useful e. G. for PostGIS, als all the
geometry classes share the same postgres type "geometry".

> When we come to do binary-format types, I'd expect we would have a
> subinterface (PGbinaryobject?) that adds whatever methods are needed for
> binary parameter formatting. Objects that implement PGbinaryobject
> become candidates for binary transfer.

What do you think about the factory / handler object approach that AFAIR
was discussed here some days ago?

So the driver gets registered one PGfactory for every postgres type, and
this factory then has methods to transform objects to text and binary
representation and vice-versa.

This would allow us to read and write instances of third-party defined
classes that don't implement any postgres specific interface.

We still could have a default factory implementation that gets used
whenever any legacy application uses PGObject subclasses.

Markus

--
markus schaber | dipl. informatiker
logi-track ag | rennweg 14-16 | ch 8001 zürich
phone +41-43-888 62 52 | fax +41-43-888 62 53
mailto:schabios@logi-track.com | www.logi-track.com

On Thu, 28 Oct 2004, Oliver Jowett wrote:

> For PGobject it turned into a bit of a general overhaul. Currently I have:

These changes are way too drastic for something as minor as preventing a
user from accidentally mutating a NULL PGobject.  These API changes suck
for both developers and users.  There's no way to make a PGobject
implementation compile against both 7.4 and 8.0 drivers.  Altering the
PGline API means user code can't compile against both 7.4 and 8.0 drivers.

If we were providing exciting new features, then maybe, but for now we've
got to find a way to make this work without huge API changes or we should
abandon the whole idea and go back to your original patch.  What
immediately comes to mind is making the PGobject interface an abstract
class with all abstract methods so that a developer can implement a type
that can work with both driver versions.


Kris Jurka

Kris Jurka wrote:
>
> On Thu, 28 Oct 2004, Oliver Jowett wrote:
>
>
>>For PGobject it turned into a bit of a general overhaul. Currently I have:
>
>
> These changes are way too drastic for something as minor as preventing a
> user from accidentally mutating a NULL PGobject.

That wasn't really my motivation. It was a general cleanup of PGobject
and subclasses. The current implementations leave something to be desired.

> These API changes suck
> for both developers and users.  There's no way to make a PGobject
> implementation compile against both 7.4 and 8.0 drivers.  Altering the
> PGline API means user code can't compile against both 7.4 and 8.0 drivers.

The feedback I got earlier was that changing the PGobject API wasn't a
big deal and that external users of the API (which seems to boil down to
PostGIS & co only) would just track the changes. Is this not true?

> If we were providing exciting new features, then maybe, but for now we've
> got to find a way to make this work without huge API changes or we should
> abandon the whole idea and go back to your original patch.  What
> immediately comes to mind is making the PGobject interface an abstract
> class with all abstract methods so that a developer can implement a type
> that can work with both driver versions.

That's possible. I'd almost prefer doing a new interface and having
PGobject be a completely-abstract implementation of it. Having the
driver-required bit as an interface makes it much easier to integrate
into whatever representation of the data is convenient to the application.

This might be moot if we look at a mapping mechanism that is external to
the data objects themselves, as suggested by Markus.

Either way, I can't really justify spending much more time on this: we
don't use extension types in our code at all, and there is still an
(admittedly ugly) way to set NULL values for extension types in the
existing driver.

-O