Re: pg_get_domaindef

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