Thread: update_pg_pwd

update_pg_pwd

From
wieck@debis.com (Jan Wieck)
Date:
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) #

Re: [HACKERS] update_pg_pwd

From
Tom Lane
Date:
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


Re: [HACKERS] update_pg_pwd

From
wieck@debis.com (Jan Wieck)
Date:
> 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) #

Re: [HACKERS] update_pg_pwd

From
Peter Eisentraut
Date:
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



Re: [HACKERS] update_pg_pwd

From
Peter Eisentraut
Date:
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



Re: [HACKERS] update_pg_pwd

From
wieck@debis.com (Jan Wieck)
Date:
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) #

Re: [HACKERS] update_pg_pwd

From
Tom Lane
Date:
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


Re: [HACKERS] update_pg_pwd

From
Tom Lane
Date:
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


Re: [HACKERS] update_pg_pwd

From
Tom Lane
Date:
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


Re: [HACKERS] update_pg_pwd

From
wieck@debis.com (Jan Wieck)
Date:
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) #

Re: [HACKERS] update_pg_pwd

From
Tom Lane
Date:
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


Re: [HACKERS] update_pg_pwd

From
Peter Eisentraut
Date:
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




Re: [HACKERS] update_pg_pwd

From
Bruce Momjian
Date:
> 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