David Steele <da...@pgmasters.net> writes:
> Andrew, Tom, does the latest patch address your concerns?
[ reads patch quickly... ] I think the definition is fine now,
modulo possible bikeshedding on the GUC name. (I have no
great suggestion on that right now, but the current proposal
seems mighty verbose.)
The implementation feels weird though, mainly in that I don't like
Peter's choices for where to put the code. pquery.c is not where
I would have expected to find the support for this, and I do not
have any confidence that applying the format conversion while
filling portal->formats[] is enough to cover all cases. I'd have
thought that access/common/printtup.c or somewhere near there
would be where to do it.
Likewise, the code associated with caching the results of the type
OID lookups seems like it should be someplace where you'd be more
likely to find (a) type name lookup and (b) caching logic. I'm
not quite sure about the best place for that, but we could do
worse than put it in parse_type.c. (As I recall, the parser
already has some caching related to operator lookup, so doing
part (b) there isn't too much of a stretch.)
Also, if we need YA string-splitting function, please put it
beside the ones that already exist (SplitIdentifierString etc in
varlena.c). That way (a) it's available if some other code needs
it, and (b) when somebody gets around to refactoring all the
splitters, they won't have to dig into nooks and crannies to find
them.
Having said that, I wonder if we should define the parameter's
contents this way, i.e. as things that parseTypeString will
accept. At best that's overspecification, e.g. should people
expect that varchar(7) and varchar(9) are different things
(and, perhaps, that such entries *don't* match varchars of other
lengths?) I think a case could be made for requiring the entries
to be identifiers exactly matching pg_type.typname, possibly with
schema qualification. This'd allow tighter verification of the
GUC value's format in the GUC check hook.
Or we could drop all of that and go back to having it be a list
of type OIDs, which would remove a *whole lot* of the complexity,
and I'm not sure that it's materially less friendly. Applications
have had to deal with type OIDs in the protocol since forever.
BTW, I wonder whether we still need to restrict the GUC to not
be settable from postgresql.conf. The fact that the client has
to explicitly pass -1 seems to reduce any security issues quite
a bit.
regards, tom lane
