Обсуждение: update_pg_pwd
Hi,
actually the opr_sanity regression test complains about a
proc update_pg_pwd(). It has a return type of Opaque and is
declared as void return type in commands/user.c. It seems to
be nowhere directly called, nor does it appear to be a
trigger function.
I wonder if it is properly defined. Shouldn't it return at
least a valid type to be callable via SQL?
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#========================================= wieck@debis.com (Jan Wieck) #
wieck@debis.com (Jan Wieck) writes:
> actually the opr_sanity regression test complains about a
> proc update_pg_pwd(). It has a return type of Opaque and is
> declared as void return type in commands/user.c. It seems to
> be nowhere directly called, nor does it appear to be a
> trigger function.
It is a trigger function for pg_shadow updates, see PATCHES message
from a day or two back.
> I wonder if it is properly defined. Shouldn't it return at
> least a valid type to be callable via SQL?
opr_sanity is complaining because the declared return type is 0.
I am not very happy about taking out opr_sanity's check on return types;
perhaps I should lobby to have Opaque-valued trigger functions be
declared with an actually valid return-type OID. What do you think?
regards, tom lane
> It is a trigger function for pg_shadow updates, see PATCHES message
> from a day or two back.
>
> > I wonder if it is properly defined. Shouldn't it return at
> > least a valid type to be callable via SQL?
>
> opr_sanity is complaining because the declared return type is 0.
> I am not very happy about taking out opr_sanity's check on return types;
> perhaps I should lobby to have Opaque-valued trigger functions be
> declared with an actually valid return-type OID. What do you think?
Trigger functions should allways return at least a NULL
pointer of type HeapTuple, not be declared void. From this I
assume it's an AFTER ROW trigger,
There are already some exceptions coded into the test. These
are PL handlers. Since their real return value is HeapTuple,
you would have to make this defined special type not
selectable in another way. So why do you want?
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#========================================= wieck@debis.com (Jan Wieck) #
On Sun, 12 Dec 1999, Tom Lane wrote: > > I wonder if it is properly defined. Shouldn't it return at > > least a valid type to be callable via SQL? > > opr_sanity is complaining because the declared return type is 0. > I am not very happy about taking out opr_sanity's check on return types; > perhaps I should lobby to have Opaque-valued trigger functions be > declared with an actually valid return-type OID. What do you think? Please don't lose me here. Did I do something wrong? Isn't oid 0 used for opaque return types? What should an opaque function return in C? I don't see a good reason from a practical point of view to disallow opaque functions as triggers, for this very reason, achieving none-database side effects. At least the create trigger command should say something if it doesn't like it. If you have to tailor functionality around the regression tests, this is not the right direction. After all 0 is a valid oid in this context: it's opaque. -- Peter Eisentraut Sernanders vaeg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
On Mon, 13 Dec 1999, Jan Wieck wrote: > Trigger functions should allways return at least a NULL > pointer of type HeapTuple, not be declared void. From this I > assume it's an AFTER ROW trigger, Must be after row, because it has to wait until the change is actually written to pg_shadow. Better would be an AFTER STATEMENT is assume. > There are already some exceptions coded into the test. These > are PL handlers. Since their real return value is HeapTuple, > you would have to make this defined special type not > selectable in another way. So why do you want? I'm not sure I'm following you, but why would a function that doesn't have a useful return value return one? -- Peter Eisentraut Sernanders vaeg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
Peter Eisentraut wrote:
> On Mon, 13 Dec 1999, Jan Wieck wrote:
>
> I'm not sure I'm following you, but why would a function that doesn't have
> a useful return value return one?
AFTER ROW triggers indeed have no useful return value,
because it is ignored for now. But IMHO they still should
follow the trigger programming guidelines.
That means, the declaration should read
HeapTuple funcname(void);
Then they should contain
TriggerData *trigdata;
...
trigdata = CurrentTriggerData;
CurrentTriggerData = NULL;
and if they do not want to manipulate the actual action, just
to get informed that it happened, return
trigdata->tg_trigtuple;
I'll make these changes to update_pg_pwd(), now that I know
for sure what it is.
One last point though. The comment says it's using lower case
name now to be callable from SQL, what it isn't because of
it's Opaque return type in pg_proc.
pgsql=> select update_pg_pwd();
ERROR: typeidTypeRelid: Invalid type - oid = 0
Is that a wanted (needed) capability or should I better
change the comment to reflect it's real nature?
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#========================================= wieck@debis.com (Jan Wieck) #
Peter Eisentraut <e99re41@DoCS.UU.SE> writes:
> On Sun, 12 Dec 1999, Tom Lane wrote:
>> opr_sanity is complaining because the declared return type is 0.
>> I am not very happy about taking out opr_sanity's check on return types;
>> perhaps I should lobby to have Opaque-valued trigger functions be
>> declared with an actually valid return-type OID. What do you think?
> Please don't lose me here. Did I do something wrong?
No, I think you coded the function the way it's currently done. I'm
just muttering that the way it's currently done needs rethinking.
> If you have to tailor functionality around the regression tests, this is
> not the right direction. After all 0 is a valid oid in this context: it's
> opaque.
The thing I'm unhappy about is that "0" is being overloaded way too far
as a function argument/result type in pg_proc. Currently it could mean:* unused position in proargtype array;*
erroneousdefinition;* "C string" parameter to a type input function (but, for who knows what reason, C string outputs
fromtype-output functions are represented differently);* user proc returning some kind of tuple;* user proc returning
nothingin particular;
and who knows what else. This is bogus. I've complained before that
there ought to be a specific OID value associated with "C string" and
that type input/output functions ought to be declared to take or return
that type ID, even though it wouldn't be a "real" type in the sense of
ever appearing as a column type. The parser already has a similar
concept in its "UNKNOWN" type, which it uses for string constants that
are of as-yet-undetermined type. UNKNOWN is real to the extent of
having a specific OID.
I'm thinking maybe we ought to invent another type OID (or two?) that
can be used for user functions that are declared OPAQUE. Triggers in
particular. That would allow more error checking, and it'd let me take
out the kluges that presently exist in the opr_sanity regress test to
keep it from spitting up on things that are practically
indistinguishable from genuine errors.
regards, tom lane
I wrote:
> The thing I'm unhappy about is that "0" is being overloaded way too far
> as a function argument/result type in pg_proc. Currently it could mean:
> * unused position in proargtype array;
> * erroneous definition;
> * "C string" parameter to a type input function (but, for who
> knows what reason, C string outputs from type-output functions
> are represented differently);
> * user proc returning some kind of tuple;
> * user proc returning nothing in particular;
> and who knows what else.
Almost forgot:* function accepting any data type whatever
(I think COUNT() is the only one at present).
regards, tom lane
wieck@debis.com (Jan Wieck) writes:
> One last point though. The comment says it's using lower case
> name now to be callable from SQL, what it isn't because of
> it's Opaque return type in pg_proc.
> pgsql=> select update_pg_pwd();
> ERROR: typeidTypeRelid: Invalid type - oid = 0
> Is that a wanted (needed) capability or should I better
> change the comment to reflect it's real nature?
What would you expect the SELECT to produce here? I think the error
message is pretty poor, but I can't really see OPAQUE functions being
allowed in expression contexts...
I don't really like the description of these functions as returning
something "OPAQUE", anyway, particularly when that is already being
(mis) used for user-defined type input/output functions. I wish
they were declared as returning something like "TUPLE".
regards, tom lane
Tom Lane wrote:
> wieck@debis.com (Jan Wieck) writes:
> > One last point though. The comment says it's using lower case
> > name now to be callable from SQL, what it isn't because of
> > it's Opaque return type in pg_proc.
>
> > pgsql=> select update_pg_pwd();
> > ERROR: typeidTypeRelid: Invalid type - oid = 0
>
> > Is that a wanted (needed) capability or should I better
> > change the comment to reflect it's real nature?
>
> What would you expect the SELECT to produce here? I think the error
> message is pretty poor, but I can't really see OPAQUE functions being
> allowed in expression contexts...
Exactly that error message, you know as I know why it should
happen. What I wanted to know is, if there could be a reason
to make it callable via such a SELECT, to force it to do it's
action, or if that's just an old comment that's wrong now.
> I don't really like the description of these functions as returning
> something "OPAQUE", anyway, particularly when that is already being
> (mis) used for user-defined type input/output functions. I wish
> they were declared as returning something like "TUPLE".
Yes, that would clearly separate trigger proc's from
functions. And for unused arguments I would suggest VOID.
But I expect (hope), you want to do this all during the fmgr
redesign, not right now, no?
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#========================================= wieck@debis.com (Jan Wieck) #
wieck@debis.com (Jan Wieck) writes:
>> I don't really like the description of these functions as returning
>> something "OPAQUE", anyway, particularly when that is already being
>> (mis) used for user-defined type input/output functions. I wish
>> they were declared as returning something like "TUPLE".
> Yes, that would clearly separate trigger proc's from
> functions. And for unused arguments I would suggest VOID.
> But I expect (hope), you want to do this all during the fmgr
> redesign, not right now, no?
Yes, this ought to go along with fmgr changes, probably. But I'm still
unhappy about the idea of doing all these updates for long values to
varlena datatypes without doing the fmgr update at the same time.
I have been thinking some more about the schedule issue, and I still
think it's foolhardy to try to do the long-values change by Feb 1.
If you recall, that date was set on the assumption that we were only
going to clean up what we had before making the release, not insert
major new features.
If people are really excited about getting this done for the next
release, I propose that we forget all about Feb 1 and just say
"we'll release when this set of changes are done". We ought to deal
with all of these issues together:* support long values for varlena datatypes;* eliminate memory leaks for pass-by-ref
datatypes(both varlena and fixed-length);* rewrite fmgr interface to fix NULL and portability bugs.
If we don't do it that way, then not only will we ourselves be
having to visit each datatype module multiple times, but we will be
breaking user-added functions in two successive releases. Users
will not be happy about that. We should change these coding rules
that affect user datatypes *once*, and get it right the first time.
I'd personally prefer to see us put off all these issues till after
a Feb-1-beta release, but I fear I am fighting a losing battle there.
Let's at least be sane enough to recognize that we don't know quite
how long this will take.
regards, tom lane
On 1999-12-13, Jan Wieck mentioned: > I'll make these changes to update_pg_pwd(), now that I know > for sure what it is. > > One last point though. The comment says it's using lower case > name now to be callable from SQL, what it isn't because of > it's Opaque return type in pg_proc. > > pgsql=> select update_pg_pwd(); > ERROR: typeidTypeRelid: Invalid type - oid = 0 > > Is that a wanted (needed) capability or should I better > change the comment to reflect it's real nature? Do as you seem fit. I just copied that together from other places. What's important though, is that this function is also called other places, so if you make it "trigger fit", then you ought to make it a wrapper around the real one. -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
> wieck@debis.com (Jan Wieck) writes: > >> I don't really like the description of these functions as returning > >> something "OPAQUE", anyway, particularly when that is already being > >> (mis) used for user-defined type input/output functions. I wish > >> they were declared as returning something like "TUPLE". > > > Yes, that would clearly separate trigger proc's from > > functions. And for unused arguments I would suggest VOID. > > > But I expect (hope), you want to do this all during the fmgr > > redesign, not right now, no? > > Yes, this ought to go along with fmgr changes, probably. But I'm still > unhappy about the idea of doing all these updates for long values to > varlena datatypes without doing the fmgr update at the same time. > > I have been thinking some more about the schedule issue, and I still > think it's foolhardy to try to do the long-values change by Feb 1. > If you recall, that date was set on the assumption that we were only > going to clean up what we had before making the release, not insert > major new features. The scheme followed in previous releases was to put in features just before beta with little testing, because you can fix bugs in beta, but not add new features. I know I did that trick a few times. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026