Enums patch v1

From: Tom Dunstan <pgsql(at)tomd(dot)cc>
To: pgsql-patches(at)postgresql(dot)org
Subject: Enums patch v1
Date: 2006-08-31 16:09:34
Message-ID: 44F709BE.50700@tomd.cc
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

Hi folks

Here's a first cut of the enums patch for feedback when people have
time. It follows an anyenum pseudo-type approach as foreseen by
Nostradamus in one of the original threads.
(http://archives.postgresql.org/pgsql-hackers/2005-11/msg00457.php).
That made the patch a little more intrusive than I had been hoping for,
but hopefully the bits that touch core files like parse_coerce.c are
easy to follow.

The patch is largely feature complete as far as what I wanted to
achieve, although it's currently missing documentation patches. I'll add
those at some point soonish. It's currently only been tested on my x86
FC5 box.

A few notes:

- In much of the code, anyenum is treated identically to anyelement,
except when trying to tie down generic parameters: at this point types
which aren't enums will be rejected if the declared type is anyenum.
Thus there are quite a few places in the patch where ANYENUMOID has just
kinda been added in next to ANYELEMENTOID.

- The error messages in enum.c need some love. Apparently there's a
document that I need to read in the sgml docco which explains error
codes to me; currently I've got FIXME TOM comments in there.

- One difficulty that I had was trying to write a cast-text-to-enum
function. While input functions pass their type oid as a second
parameter, all that cast functions can get is the typmod. I did a bit of
an ugly hack to pass the enum oid in as an int in that parameter if it's
an enum type; it might be cleaner to allow cast functions that take an
oid there rather than an int and pass the correct parameter depending on
that type. I wasn't comfortable making a change like that in this patch
without discussion, though.

- It appeared that to be cached in a syscache properly, the enum names
had to be a Name/NameData thing rather than a text or whatever, so
there's a limit of 64 characters per enum value. 64 characters should be
enough for anybody :). Hmm, as I write this I realize that I should
probably add some length checking to the create type function. And
something prohibiting commas, since they're the array delimiter.
Consider that on the TODO as well. :) Do we want to allow a customizable
delimiter? IMO someone putting commas inside an enum value is doing
something sick and twisted anyway, but maybe there's an argument to be
made. They could always hack the delimiter on the array type, although
that wouldn't survive a dump.

- You can't create generic anyenum functions in the PL's except for
plpgsql. You can create functions taking/returning the concrete type,
but not anyenum. I don't consider that a huge loss; we couldn't really
think of a use case. I haven't added a regression test involving enums
to e.g. plperl, although I have tested it with perlpython/tcl. Is that
worth doing, or is it being paranoid?

- This patch requires an initdb, but I haven't bumped the catalog
version number in the patch since I don't know when (if ever) it will be
applied

Now a few questions from a first-time contributor:

- Do I need to call CatalogUpdateIndexes() after I delete rows when
dropping the type? Following the code for creating dropping standard
user types, it's called on creation but not deletion.

- I'm a little unclear about when pfree() should be called. My
understanding is that it will deallocate memory from the current memory
context, but that context will be chucked once the current statement or
whatever is dead. Is it just nice to do when we know that we're done
with a particular chunk and to stop the context from growing, or are
there other occasions? Or have I misunderstood what it does?

- Is pg_indent intended to be run by us mortals? I tried following the
instructions in the README to make sure my indentation was kosher, but
ran into issues with pgindent complaining about something called detab
being missing. It's not on my Fedora box, and some quick Googling didn't
unearth an obvious candidate, although a few dodgy looking scripts
popped up. Is it a BSD tool?

Anyway, all comments / criticisms welcome. Have a look at the regression
test to see what should be working.

Cheers

Tom

Attachment Content-Type Size
enums-v1.patch.gz application/x-gzip 20.3 KB

Browse pgsql-patches by date

  From Date Subject
Next Message Guillaume Smet 2006-08-31 17:33:14 Re: [HACKERS] [PATCHES] log_statement output for protocol
Previous Message Tom Lane 2006-08-31 15:10:47 Re: [HACKERS] Updatable views