Обсуждение: "anyelement2" pseudotype
A few months ago at http://archives.postgresql.org/pgsql-general/2006-11/msg01770.php the notion of adding an "anyelement2" pseudotype was discussed. The context was a compatibility SQL function to support Oracle's DECODE function. Assuming this new pseudotype has not been added yet, I'm ready to look into doing this myself, and I'd like a bit of shove in the right direction.
"Matt Miller" <pgsql@mattmillersf.fastmail.fm> writes: > A few months ago at > http://archives.postgresql.org/pgsql-general/2006-11/msg01770.php the > notion of adding an "anyelement2" pseudotype was discussed. The context > was a compatibility SQL function to support Oracle's DECODE function. > Assuming this new pseudotype has not been added yet, I'm ready to look > into doing this myself, and I'd like a bit of shove in the right direction. The reason it's not in there already is we didn't seem to have quite enough use-case to justify it. Do you have more? As for actually adding it, grep for all references to ANYELEMENT and add code accordingly; shouldn't be that hard. Note you'd need to add an anyarray2 at the same time for things to keep working sanely. regards, tom lane
Tom Lane wrote: > As for actually adding it, grep for all references to ANYELEMENT and add > code accordingly; shouldn't be that hard. Note you'd need to add an > anyarray2 at the same time for things to keep working sanely. The enum patch [1] does exactly this with an ANYENUM pseudo-type. It should provide a pretty good overview of what will be required. Cheers Tom [1] http://archives.postgresql.org/pgsql-patches/2007-02/msg00239.php
Tom Dunstan <pgsql@tomd.cc> writes:
> The enum patch [1] does exactly this with an ANYENUM pseudo-type. It
> should provide a pretty good overview of what will be required.
ANYENUM? What's the use-case for that? These special cases in the
type system are enough of a pain-in-the-neck for the code that I'm
disinclined to add one without a very solid argument for it.
regards, tom lane
Tom Lane wrote: > Tom Dunstan <pgsql@tomd.cc> writes: > >> The enum patch [1] does exactly this with an ANYENUM pseudo-type. It >> should provide a pretty good overview of what will be required. >> > > ANYENUM? What's the use-case for that? These special cases in the > type system are enough of a pain-in-the-neck for the code that I'm > disinclined to add one without a very solid argument for it. > Well ... *somebody* suggested it here ... http://archives.postgresql.org/pgsql-hackers/2005-11/msg00457.php ;-) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > Tom Lane wrote: >> ANYENUM? What's the use-case for that? > Well ... *somebody* suggested it here ... > http://archives.postgresql.org/pgsql-hackers/2005-11/msg00457.php Well, in that usage (ie, for enum I/O functions) it's not actually necessary that the type system as a whole understand ANYENUM as "something that any enum type can be cast to", because you're going to hot-wire the pg_type entries during CREATE ENUM anyway. What I'm wondering is if there's a use-case for it during ordinary user operations with enums. regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> ANYENUM? What's the use-case for that? >>> > > >> Well ... *somebody* suggested it here ... >> http://archives.postgresql.org/pgsql-hackers/2005-11/msg00457.php >> > > Well, in that usage (ie, for enum I/O functions) it's not actually > necessary that the type system as a whole understand ANYENUM as > "something that any enum type can be cast to", because you're going to > hot-wire the pg_type entries during CREATE ENUM anyway. What I'm > wondering is if there's a use-case for it during ordinary user > operations with enums. > > > If you look further down in the thread you'll see that I suggested hiding it, because i didn't think there was much use case for it in user code, but you didn't seem to think much of that idea. cheers andrew
> > adding an "anyelement2" pseudotype ... The context was a > > compatibility SQL function to support Oracle's DECODE function. > > The reason it's not in there already is we didn't seem to have quite > enough use-case to justify it. Do you have more? No. Even this case, for me, is more an expedient than a necessity. I could just rewrite my Oracle code to use CASE, but I've a lot of code to convert, and the transformation is a bit error prone. I'm also looking at a scripted code edit to rewrite the Oracle stuff, and comparing this to the cost a PG compatibility function.
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Tom Lane wrote: >>> ANYENUM? What's the use-case for that? > >> Well ... *somebody* suggested it here ... >> http://archives.postgresql.org/pgsql-hackers/2005-11/msg00457.php > > Well, in that usage (ie, for enum I/O functions) it's not actually > necessary that the type system as a whole understand ANYENUM as > "something that any enum type can be cast to", because you're going to > hot-wire the pg_type entries during CREATE ENUM anyway. Well, it's not just I/O functions in pg_type, it's functions, operators, aggregates, index methods etc. There are 34 OIDs used up by the enum patch, and most of those catalog entries would have to be duplicated per enum type by CREATE TYPE in the absence of ANYENUM; since you'd given the hand-wavy suggestion anyway, it seemed better not to spam the catalogs. Regarding the type system understanding ANYENUM, most of the type system treats ANYENUM identically to ANYELEMENT, the only parts that really need to understand it are the bits that try to tie down concrete types. For those, non-enum types are rejected if the generic type is ANYENUM. That's it, basically. > What I'm > wondering is if there's a use-case for it during ordinary user > operations with enums. Not really. I allowed it to occur in plpgsql, mostly for completeness, but I didn't bother for the other P/Ls as there didn't seem to be much of a use case. Cheers Tom
Tom Dunstan <pgsql@tomd.cc> writes:
> Regarding the type system understanding ANYENUM, most of the type system
> treats ANYENUM identically to ANYELEMENT, the only parts that really
> need to understand it are the bits that try to tie down concrete types.
The reason I'm feeling annoyed with ANYfoo stuff today is that yesterday
I had to put a special hack for ANYARRAY into the ri_triggers code,
which you'd think would have no concern with it. But perhaps this is
just an indication that we need to refactor the code in parse_coerce.c.
(The problem in ri_triggers is that it uses find_coercion_pathway()
which does not concern itself with ANYfoo types.)
Anyway, objection withdrawn --- I just thought it seemed a good idea to
question whether we were adding a frammish we didn't really need.
regards, tom lane
I wrote:
> Tom Dunstan <pgsql@tomd.cc> writes:
>> Regarding the type system understanding ANYENUM, most of the type system
>> treats ANYENUM identically to ANYELEMENT, the only parts that really
>> need to understand it are the bits that try to tie down concrete types.
> The reason I'm feeling annoyed with ANYfoo stuff today is that yesterday
> I had to put a special hack for ANYARRAY into the ri_triggers code,
> which you'd think would have no concern with it.
Actually ... now that I re-read that remark, I think you may have done
the wrong things with ANYENUM. I think that ANYENUM may in fact be
closer to ANYARRAY than it is to ANYELEMENT, because ANYELEMENT pretty
nearly means "anything at all" whereas ANYARRAY identifies a subset of
types that share some properties, which is an accurate description of
ANYENUM as well. In particular, it is sensible to have b-tree index
opclasses that are declared to operate on ANYARRAY. If you've
got b-tree support for ANYENUM, as I hope you do, then you'll have to
patch that same spot in ri_triggers that now knows about ANYARRAY.
So you might want to take another pass through the code and see if you
shouldn't be modeling ANYENUM more closely on ANYARRAY than ANYELEMENT.
regards, tom lane
Tom Dunstan wrote: > Tom Lane wrote: >> As for actually adding it, grep for all references to ANYELEMENT and add >> code accordingly; shouldn't be that hard. Note you'd need to add an >> anyarray2 at the same time for things to keep working sanely. > > The enum patch [1] does exactly this with an ANYENUM pseudo-type. It > should provide a pretty good overview of what will be required. Whoops. I just had a look at the mail that Matt referenced at the top of this thread. An anyelement2 would require a bit more than what anyenum does, as the type-matching code that ensures that all generic args are of the same type would have to be changed, unlike anyenum. Hope I didn't lead you down the wrong path, Matt. OTOH, following the enum patch should land you in roughly the right areas, and you'd still need to add ANYELEMENT2 references in all the places that I had to add ANYENUM as well. Cheers Tom
Tom Lane wrote:
> Actually ... now that I re-read that remark, I think you may have done
> the wrong things with ANYENUM. I think that ANYENUM may in fact be
> closer to ANYARRAY than it is to ANYELEMENT, because ANYELEMENT pretty
> nearly means "anything at all" whereas ANYARRAY identifies a subset of
> types that share some properties, which is an accurate description of
> ANYENUM as well. In particular, it is sensible to have b-tree index
> opclasses that are declared to operate on ANYARRAY. If you've
> got b-tree support for ANYENUM, as I hope you do, then you'll have to
> patch that same spot in ri_triggers that now knows about ANYARRAY.
>
> So you might want to take another pass through the code and see if you
> shouldn't be modeling ANYENUM more closely on ANYARRAY than ANYELEMENT.
OK, thanks, I'll do that. Of course, they get used together all over the
place as well, lots ofif(typiod == ANYARRAY || typoid == ANYELEMENT) {
type of stuff in the code.
I do have b-tree (and hash) support for enums, so it sounds like I'll
have to hit the same spot. I've got what I thought was a reasonably
comprehensive test of all the enum features which passes make check, so
if there's a likely failure in that code then I'm missing a test
somewhere. Did you have a test case for the ri_triggers stuff that you
did? What's going to fail?
Thanks
Tom
Tom Dunstan <pgsql@tomd.cc> writes:
> I do have b-tree (and hash) support for enums, so it sounds like I'll
> have to hit the same spot. I've got what I thought was a reasonably
> comprehensive test of all the enum features which passes make check, so
> if there's a likely failure in that code then I'm missing a test
> somewhere. Did you have a test case for the ri_triggers stuff that you
> did? What's going to fail?
ri_HashCompareOp, which I think is mainly invoked in cases of UPDATEing
a PK or FK row (to see whether the relevant columns changed). If one
of the columns is an enum type, it's going to need to be able to realize
that coercing that to ANYENUM is a no-op.
regards, tom lane
Come to think of it, we really do need some refactoring in
parse_coerce.c. I just realized what CVS HEAD's RI code
does with array types:
regression=# create table aa (f1 int[] primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "aa_pkey" for table "aa"
CREATE TABLE
regression=# create table bb (b1 real[] references aa);
CREATE TABLE
regression=# insert into bb values('{1,1}');
ERROR: operator does not exist: integer[] pg_catalog.= real[]
It should have rejected the FK constraint right off the bat, but the
test in ATAddForeignKeyConstraint is effectively just "does real[]
coerce to anyarray" which is not good enough in this context. Your
patch will have the same misbehavior: it'll allow an FK reference to a
different enum type to be declared, but then fail at runtime.
So it seems neither can_coerce_type() nor find_coercion_pathway() are
really particularly well thought out in terms of what they test or don't
test. I'm not very sure what a good refactoring would look like,
but I am sure that I don't want all their call sites having to
individually account for ANYfoo types. Any thoughts?
regards, tom lane
Tom Lane wrote: > Come to think of it, we really do need some refactoring in > parse_coerce.c. [snip] > I'm not very sure what a good refactoring would look like, > but I am sure that I don't want all their call sites having to > individually account for ANYfoo types. Any thoughts? > > I was just thinking earlier that we need some sort of ANYany(oid) test. I guess a very simple minded approach would just macro expand it, to something like what's there now, or if we were more adventurous we could rearrange things so that a bitmask test would work. cheers andrew
Tom Lane wrote: > So it seems neither can_coerce_type() nor find_coercion_pathway() are > really particularly well thought out in terms of what they test or don't > test. I'm not very sure what a good refactoring would look like, > but I am sure that I don't want all their call sites having to > individually account for ANYfoo types. Any thoughts? Yeah, I remember thinking at the time that some of it was a bit backwards, but it's been almost 6 months since I did the original enum patch, so I'll need to refresh my memory. I'll have a look over the weekend and see if I can come up with something that'll work for these various cases. To begin with I'll need to do a survey of the call sites to see what they really need, since perhaps it isn't what the coerce functions are currently offering. :) I completely agree that anything requiring call sites to understand specifics about ANY* types is a bad idea, the most that we would want would be a generic IsGeneric(typoid) macro, but it would be nice to hide that inside a coerce function as well. We'll see. Cheers Tom
Tom Dunstan <pgsql@tomd.cc> writes:
> Tom Lane wrote:
>> So it seems neither can_coerce_type() nor find_coercion_pathway() are
>> really particularly well thought out in terms of what they test or don't
>> test. I'm not very sure what a good refactoring would look like,
>> but I am sure that I don't want all their call sites having to
>> individually account for ANYfoo types. Any thoughts?
> To begin with I'll need to do a survey of the call sites
> to see what they really need, since perhaps it isn't what the coerce
> functions are currently offering. :)
I realized that I can probably fix ATAddForeignKeyConstraint to do the
right thing by having it pass the two actual column types to
can_coerce_type, thus allowing check_generic_type_consistency to kick
in and detect the problem. I haven't got round to trying that (up to my
rear in planner bugs ATM :-() but I think the immediate problem can be
dealt with without refactoring. Still, if you have any ideas for making
this code cleaner, I'm all ears.
regards, tom lane
Tom Lane wrote: > I realized that I can probably fix ATAddForeignKeyConstraint to do the > right thing by having it pass the two actual column types to > can_coerce_type, thus allowing check_generic_type_consistency to kick > in and detect the problem. Yeah, I came to the same conclusion. No amount of refactoring in parse_coerce.c is going to get the original concrete types back to compare. That should fix the problem with arrays, enums and any potential future generic types without mentioning them explicitly in there a la the hack there currently, thankfully. Cheers Tom