Subject: Re: pg_get_domaindef
From: Neil Conway <neilc@samurai.com>
To: FAST PostgreSQL <fastpgs@fast.fujitsu.com.au>
Cc: pgsql-patches@postgresql.org
In-Reply-To: <12753.10901169180733.fast.fujitsu.com.au@MHS>
References: <12753.10901169180733.fast.fujitsu.com.au@MHS>
Content-Type: text/plain
Date: Fri, 19 Jan 2007 01:02:30 -0500
Message-Id: <1169186550.27197.10.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

On Sat, 2007-01-20 at 02:28 +1100, FAST PostgreSQL wrote:
> Attached is a small patch that implements the pg_get_domaindef(oid) function. 

A few minor comments:

- don't use C++-style comments

- why does this code append a "-" to the output when SPI_processed != 1,
rather than erroring out?

+       if (spirc != SPI_OK_SELECT)
+               elog(ERROR, "failed to get pg_type tuple for domain %u",
domainOid);
+       if (SPI_processed != 1)
+               appendStringInfo(&buf, "-");
+       else
+       {

- you probably want to elog(ERROR) if typeTuple is invalid:

+               /*
+                * Get the base type of the domain
+                */
+               typeTuple = SearchSysCache(TYPEOID,
+                                               ObjectIdGetDatum(basetypeOid),
+                                               0, 0, 0);
+ 
+               if (HeapTupleIsValid(typeTuple))
+               {
+                       typebasetype = pstrdup(NameStr(((Form_pg_type)
GETSTRUCT(typeTuple))->typname));
+                       appendStringInfo(&buf, "%s ",
quote_identifier(typebasetype));
+               }
+               ReleaseSysCache(typeTuple);

It's also debatable whether the function ought to be using a
*combination* of the syscaches and manual system catalog queries, since
these two views may be inconsistent. I guess this is a widespread
problem, though.

+               if (typnotnull || constraint != NULL)
+               {
+                       if ( ( (contype != NULL) && (strcmp(contype,
"c") != 0) ) || typnotnull )
+                       {
+                               appendStringInfo(&buf, "CONSTRAINT ");
+                       }
+                       if (typnotnull)
+                       {
+                               appendStringInfo(&buf, "NOT NULL ");
+                       }
+               }
+               if (constraint != NULL)
+               {
+                       appendStringInfo(&buf,
quote_identifier(constraint));
+               }

This logic seems pretty awkward. Perhaps simpler would be a check for
typnotnull (and then appending "CONSTRAINT NOT NULL"), and then handling
the non-typnotnull branch separately.

-Neil