Обсуждение: dunction issue

Hi,

i have a problem solving my function trouble.
this function should return an email address stored in a table (tmp_newsletterreg) based on a number (sessions ID).
if the session id is not find it should return a string corresponding to and error.
if the email in found but already exists into another table (users), it should also return a string value relative the this error.

here is my stored procedure.

CREATE OR REPLACE FUNCTION cust_portal.sp_u_002(id_session character varying)
  RETURNS character varying AS
$BODY$

DECLARE

    ret_email CHARACTER VARYING(512) :='';
    usr_exists INTEGER := 0;
    usr_exists_2 INTEGER := 0;

BEGIN
    set search_path = cust_portal;

    SELECT count(*) INTO usr_exists FROM tmp_newsletterreg WHERE tmp_usr_id = id_session;
    IF (usr_exists = 1) THEN
        SELECT email INTO ret_email FROM tmp_newsletterreg WHERE tmp_usr_id = id_session;
        IF (ret_email IS NULL || ret_email='') THEN
            RETURN ('-3');
        ELSE
            SELECT count(*) INTO usr_exists_2 FROM users WHERE users.email = ret_email;
            IF (usr_exists_2 = 0) THEN -- first try of user to get registered
                RETURN (ret_email);
            ELSE     -- user already exists into users tables (several tries to register)
               RETURN ('-2');
            END IF;
        END IF;
    ELSE
        RETURN('-1');
    END IF;
END;

if the session id is wrong, it works correctly.
however if the session id is correct it does not return me the email address (even if it really exist into table tmp_newsletterreg / but not in table users.)
so i think my eyes are tired, because i do not see an error...

thanks.
--
Alain
------------------------------------
Windows XP SP2
PostgreSQL 8.2.4 / MS SQL server 2005
Apache 2.2.4
PHP 5.2.4
C# 2005-2008

 -------------- Original message ----------------------
From: "Alain Roger" <raf.news@gmail.com>
> Hi,
>
> i have a problem solving my function trouble.
> this function should return an email address stored in a table
> (tmp_newsletterreg) based on a number (sessions ID).
> if the session id is not find it should return a string corresponding to and
> error.
> if the email in found but already exists into another table (users), it
> should also return a string value relative the this error.
>
> here is my stored procedure.
>
> > CREATE OR REPLACE FUNCTION cust_portal.sp_u_002(id_session character
> > varying)
> >   RETURNS character varying AS
> > $BODY$
> >
> > DECLARE
> >
> >     ret_email CHARACTER VARYING(512) :='';
> >     usr_exists INTEGER := 0;
> >     usr_exists_2 INTEGER := 0;
> >
> > BEGIN
> >     set search_path = cust_portal;
> >
> >     SELECT count(*) INTO usr_exists FROM tmp_newsletterreg WHERE
> > tmp_usr_id = id_session;
> >     IF (usr_exists = 1) THEN
> >         SELECT email INTO ret_email FROM tmp_newsletterreg WHERE
> > tmp_usr_id = id_session;
> >         IF (ret_email IS NULL || ret_email='') THEN

See if I can do better this time.  I believe your problem is here:
IF (ret_email IS NULL || ret_email='') THEN
|| is the string concatenation operator. If you are trying to test both cases then you need to do something along lines
of
IF (ret_email IS NULL ) THEN
        RETURN ('-3')
ELSIF (ret_email='')
        RETURN ('-3')

> >             RETURN ('-3');
> >         ELSE
> >             SELECT count(*) INTO usr_exists_2 FROM users WHERE users.email=
> ret_email;
> >             IF (usr_exists_2 = 0) THEN -- first try of user to get
> > registered
> >                 RETURN (ret_email);
> >             ELSE     -- user already exists into users tables (several
> > tries to register)
> >                RETURN ('-2');
> >             END IF;
> >         END IF;
> >     ELSE
> >         RETURN('-1');
> >     END IF;
> > END;
> >
>
> if the session id is wrong, it works correctly.
> however if the session id is correct it does not return me the email address
> (even if it really exist into table tmp_newsletterreg / but not in table
> users.)
> so i think my eyes are tired, because i do not see an error...
>
> thanks.
> --
> Alain
> ------------------------------------
> Windows XP SP2
> PostgreSQL 8.2.4 / MS SQL server 2005
> Apache 2.2.4
> PHP 5.2.4
> C# 2005-2008

--
Adrian Klaver
aklaver@comcast.net

On Thu, Mar 27, 2008 at 08:43:46PM +0100, Alain Roger wrote:
> i have a problem solving my function trouble.
> this function should return an email address stored in a table
> (tmp_newsletterreg) based on a number (sessions ID).
> if the session id is not find it should return a string corresponding to and
> error.
> if the email in found but already exists into another table (users), it
> should also return a string value relative the this error.

This is fun isn't it!

> here is my stored procedure.

And here it is in a single, unreadable, SQL statement:

  SELECT CASE WHEN s.email = u.email THEN 'email already exists'
              ELSE COALESCE(s.email, 'no such session') END AS msg
  FROM (VALUES (1)) x(one)
    LEFT JOIN (
      SELECT email FROM tmp_newsletterreg
      WHERE sessionid = $1) s ON TRUE
    LEFT JOIN (SELECT email FROM users) u ON s.email = u.email;

Why not put a foreign key on the "email" column to the users table---one
less error to handle that way?


  Sam

On Thursday 27 March 2008 3:17 pm, Sam Mason wrote:
> On Thu, Mar 27, 2008 at 08:43:46PM +0100, Alain Roger wrote:
> > i have a problem solving my function trouble.
> > this function should return an email address stored in a table
> > (tmp_newsletterreg) based on a number (sessions ID).
> > if the session id is not find it should return a string corresponding to
> > and error.
> > if the email in found but already exists into another table (users), it
> > should also return a string value relative the this error.
>
> This is fun isn't it!
>
> > here is my stored procedure.
>
> And here it is in a single, unreadable, SQL statement:
>
>   SELECT CASE WHEN s.email = u.email THEN 'email already exists'
>               ELSE COALESCE(s.email, 'no such session') END AS msg
>   FROM (VALUES (1)) x(one)
>     LEFT JOIN (
>       SELECT email FROM tmp_newsletterreg
>       WHERE sessionid = $1) s ON TRUE
>     LEFT JOIN (SELECT email FROM users) u ON s.email = u.email;
>
> Why not put a foreign key on the "email" column to the users table---one
> less error to handle that way?
>
>
>   Sam

Or a simpler way to do handle my previous suggestion:

IF (ret_email IS NULL ) OR (ret_email='')  THEN
        RETURN ('-3')

--
Adrian Klaver
aklaver@comcast.net

On Thu, Mar 27, 2008 at 03:34:49PM -0700, Adrian Klaver wrote:
> Or a simpler way to do handle my previous suggestion:
>
> IF (ret_email IS NULL ) OR (ret_email='')  THEN
>     RETURN ('-3')

That would be the sane fix, yes.

Based on the previous emails from the OP, he seems to be missing a
lot of the tools that databases' give you.  Transactions and unique
constraints being two significant ones.  Writing stored procedures to do
their work is just going to introduce unnecessary bugs and complication.


  Sam

I do not agree with you Sam.

Stored procedure are safe from hacking (from external access).
From my point of view transitions should be used only as internal purpose or via intrAnet and not thru intErnet.

at list this is how under MS SQL they use to teach.

regarding unique constraint, i already setup it. :-)
unique violation will not help me in this case, or only to know if the email is stored several time.... which i do not test...directly :-)

Al.




--
Alain
------------------------------------
Windows XP SP2
PostgreSQL 8.2.4 / MS SQL server 2005
Apache 2.2.4
PHP 5.2.4
C# 2005-2008

Alain Roger wrote:
> I do not agree with you Sam.
>
> Stored procedure are safe from hacking (from external access).

In that a stored procedure encapsulates a series of data operations,
meaning that the client doesn't have to know the details or even have
privileges to run the individual operations ? Yes, that can be really
useful, but it's hardly the full story.

Proper use of things like foreign keys, unique constraints, CHECK
constraints, etc adds another level of protection. I'd use those tools
before I restored to using a stored procedure. Like stored procedures,
users with appropriately limited priveleges are unable to bypass, drop,
or modify constraints.

That's true in any database with any sort of user privileges model.

For example, if you want to enforce the rule that a certain field must
have unique values in a table, do you think it's better to do it with a
stored procedure, or by adding a UNIQUE constraint to the field?

I'd say the UNIQUE constraint is better in every way. It's faster. It's
simple and unlike the stored procedure isn't at risk of being bypassed
by coding errors in the stored procedure. It's also self documenting, in
that the schema clearly shows the unque constraint rather than hiding it
in code. It's if anything more secure than a stored procedure because
it's simpler and can be easily protected against user modification.

The same goes for NOT NULL, CHECK constraints, foreign keys, etc.


You're also doing a lot of things in your stored procedure code the long
way. For example, instead of selecting a count() aggregate into a
variable and testing it, why not just use `EXISTS', or select the
information you really wanted and then use `IF FOUND' ?

For example:

-- It seems like not having an email address on record is an error.
-- Ensure that the problem is detected at INSERT/UPDATE time.
ALTER TABLE tmp_newsletterreg ALTER COLUMN email SET NOT NULL;

-- Really basic valiation of email addresses. It's not worth doing much
-- more than this sort of thing IMO because of performance issues and
-- transcient errors (MX lookup fail etc) when doing proper email
-- validation. At least now you don't have to revalidate in every
-- procedure.
ALTER TABLE tmp_newsletterreg ADD CONSTRAINT simplistic_email_check
CHECK lower(trim(both ' ' from email)) LIKE '%_@_%';

-- Add the same check on the user table. I imagine a NOT NULL
-- constraint there would also make sense.
ALTER TABLE users ADD CONSTRAINT simplistic_email_check
CHECK lower(trim(both ' ' from email)) LIKE '%_@_%';

-- Now, using those rules, redefine the stored procedure

CREATE OR REPLACE FUNCTION cust_portal.sp_u_002
  (id_session character varying)
  RETURNS character varying AS $BODY$
DECLARE
     ret_email CHARACTER VARYING(512) :='';
 BEGIN
     set search_path = cust_portal;

     -- Find the customer's email address, or NULL (and set NOT FOUND)
     -- if no such customer exists.
     SELECT email INTO ret_email FROM tmp_newsletterreg
       WHERE tmp_usr_id = id_session;
     IF FOUND THEN
         IF NOT EXISTS (SELECT 1 FROM users
           WHERE users.email= ret_email)
         THEN
             RETURN (ret_email);
         ELSE
            RETURN ('-2');
         END IF;
     ELSE
         RETURN('-1');
     END IF;
 END;
$BODY$ LANGUAGE 'plpgsql';


Personally I think the use of text string error codes gets ugly fast.
I'd either rewrite the function to at least return an integer error code
as an OUT parameter:


CREATE OR REPLACE FUNCTION cust_portal.sp_u_002
  (IN id_session character varying,
   OUT ret_email character varying,
   OUT err_code integer)
  RETURNS record AS $BODY$
DECLARE
 BEGIN
     set search_path = cust_portal;

     -- If we don't find the session, return -1 .
     ret_email := NULL;
     err_code := -1;

     -- Find the customer's email address, or NULL (and set NOT FOUND)
     -- if no such customer exists.
     SELECT email INTO ret_email FROM tmp_newsletterreg
       WHERE tmp_usr_id = id_session;
     IF FOUND THEN
         IF EXISTS (SELECT 1 FROM users
           WHERE users.email= ret_email)
         THEN
            -- User already registered
            ret_email := NULL;
            err_code := '-2';
         END IF;
     END IF;
 RETURN;
 END;
$BODY$ LANGUAGE 'plpgsql';


[note: the above code hasn't actually been tested]

... or preferably throw informative exceptions. However, I do find it
frustrating that I can't attach a value or list of values to a
PostgreSQL exception in a way that is easy for the client app to extract
- I have to resort to text parsing (mega-ugly and unsafe) if I need to
do it.

Especially in an internationalised environment that's not nice.

Being able to obtain the exact exception name (as opposed to the full
error message), the full error string from an exception (without its
context) and also obtain the individual parameters substituted into an
exception string would be AMAZINGLY handy for use with JDBC etc.

> From my point of view transitions should be used only as internal purpose or
> via intrAnet and not thru intErnet.
>
> at list this is how under MS SQL they use to teach.

Do you mean transactions?

Transactions really aren't a security feature/issue. They're a tool
useful for preserving data integrity and consistency by bundling up
related operations into a single change that the database guarantees
will all be applied at once, or not applied at all.

They're critically important in non-trivial database use.

Because you're wrapping most of your operations in stored procedures
they're happening transactionally anyway, but it's still an important
thing to think about.

--
Craig Ringer

On Fri, Mar 28, 2008 at 06:43:00PM +0900, Craig Ringer wrote:
> Alain Roger wrote:
> > I do not agree with you Sam.
> >
> > Stored procedure are safe from hacking (from external access).
>
> In that a stored procedure encapsulates a series of data operations,
> meaning that the client doesn't have to know the details or even have
> privileges to run the individual operations ? Yes, that can be really
> useful, but it's hardly the full story.

Indeed.  And in my experience, it's the program's own developers you've
got to be most cautious about.  "Hackers" would have very little trouble
breaking most software these days---almost everything is far too big
and complicated, ignoring rules like keeping it simple, respecting the
principle of least authority and other time tested rules.  Attackers
also tend to go around the barriers you put in their way, not through
them, the most general attack would be the physical one, i.e. paying a
cleaner to remove something important.  Another way of looking at it is
to witness the types of bugs being fixed in software, almost all of them
have no security implications and are straight human fallibility.

> Proper use of things like foreign keys, unique constraints, CHECK
> constraints, etc adds another level of protection. I'd use those tools
> before I restored to using a stored procedure. Like stored procedures,
> users with appropriately limited priveleges are unable to bypass, drop,
> or modify constraints.

Indeed, use the simplest possible tool to get the job done.  And if
possible reuse an existing one (i.e. all the work that has gone into
getting the constraint handling working correctly in all the known
cases).

> -- Really basic valiation of email addresses. It's not worth doing much
> -- more than this sort of thing IMO because of performance issues and
> -- transcient errors (MX lookup fail etc) when doing proper email
> -- validation. At least now you don't have to revalidate in every
> -- procedure.
> ALTER TABLE tmp_newsletterreg ADD CONSTRAINT simplistic_email_check
> CHECK lower(trim(both ' ' from email)) LIKE '%_@_%';

Just out of interest, what's the lower() function call doing?

I'd almost be tempted to do something like:

  CREATE DOMAIN emailaddr AS text
    CHECK (VALUE ~ '^[^ ]+@[^ ]+$');

and then use this instead of text/varchar types.

> ... or preferably throw informative exceptions.

This would be my preference.  It'll probably do the "right" thing if the
code is called from other stored procedures then.

> However, I do find it
> frustrating that I can't attach a value or list of values to a
> PostgreSQL exception in a way that is easy for the client app to extract
> - I have to resort to text parsing (mega-ugly and unsafe) if I need to
> do it.

Yup, why is this so often ignored when people write database drivers.  I
used the "pyPgSQL" python module (I think) for a bit, before realising
that it even went as far as to "helpfully" automatically start a new
transaction when the last one aborted.  The resulting semantics meant my
code did the most horrible things.


And I'd agree with the remainder of your comments!


  Sam

Sam Mason wrote:
>> ALTER TABLE tmp_newsletterreg ADD CONSTRAINT simplistic_email_check
>> CHECK lower(trim(both ' ' from email)) LIKE '%_@_%';
>>
>
> Just out of interest, what's the lower() function call doing?
>
>
Absolutely nothing. That's what I get for reading my mail at
stupid-o-clock in the morning (Australia) instead of something sensible
like sleeping.

It's there because I was thinking about case insensitive domain
comparison, but I couldn't begin to guess how it made its way into the
constraint expression.
> I'd almost be tempted to do something like:
>
>   CREATE DOMAIN emailaddr AS text
>     CHECK (VALUE ~ '^[^ ]+@[^ ]+$');
>
> and then use this instead of text/varchar types.
>
I was thinking about something like that, but my own storage of email
addresses actually splits them into user part and domain part (so it can
handle the case sensitivity differently - user parts are may be case
sensitive depending on the mail system so you can't assume they're the
same if they only differ in case; domain parts are never case sensitive)
and that would've unnecessarily complicated the example. I didn't think
to go for the half way point.
>> ... or preferably throw informative exceptions.
>>
>
> This would be my preference.  It'll probably do the "right" thing if the
> code is called from other stored procedures then.
>
Yep, it's what I'll do in almost all cases. I often land up writing
client / UI data validation code to perform the same checks and catch
the issue before submitting anything to the DB, but I don't consider
this unreasonable. The DB's checks are protecting data integrity and
consistency; the UI's checks are improving the user/app interaction by
providing early (and usually more friendly) notification of data issues.
They're really quite different jobs.

Occasionally, though, I do have something where the DB-using app must
just submit a request to the DB and see if it works. Either the UI
doesn't have the privileges to run the same checks its self, or they're
just too expensive to do from the client (or to do twice). In those
cases I start to find Pg's error reporting frustrating, and I either
resort to a "return value" sort of approach or embed a unique error code
and some parseable values in the exception string. Eg:

   Some kind of human-readable error description goes here
[ERR114:ID=12200;CONFLICTING-ID=1111]

It's not pretty, but it works.
> Yup, why is this so often ignored when people write database drivers.  I
> used the "pyPgSQL" python module (I think) for a bit, before realising
> that it even went as far as to "helpfully" automatically start a new
> transaction when the last one aborted.  The resulting semantics meant my
> code did the most horrible things.
>
That is indeed horrible, and I'd be running from a DB interface like
that as fast as I could.

Much of what I've done with PostgreSQL has been with Python (I do a lot
of C++ too, but not with databases) and I've thankfully not run into
anything like that. psycopg (the alternative PostgreSQL interface for
Python) handles exceptions about as well as is possible with
PostgreSQL's purely text based exception reporting, and I've found it
very useful.  I understand that it's also a LOT faster than PyPgSQL,
though I don't have any direct experience there as I've never used
PyPgSQL. It sounds like I unwittingly dodged a bullet there.

As far as I'm concerned any DB interface that's ignoring errors behind
your back needs to die. Especially in an exception-capable language like
Python, where throwing and letting the upper layers handle it is the
obviously sane thing to do.

--
Craig Ringer

On Sat, Mar 29, 2008 at 04:05:15AM +0900, Craig Ringer wrote:
> Sam Mason wrote:
> >>ALTER TABLE tmp_newsletterreg ADD CONSTRAINT simplistic_email_check
> >>CHECK lower(trim(both ' ' from email)) LIKE '%_@_%';
> >
> >Just out of interest, what's the lower() function call doing?
>
> Absolutely nothing. That's what I get for reading my mail at
> stupid-o-clock in the morning (Australia) instead of something sensible
> like sleeping.

OK, good to know I wasn't missing something :)

> >I'd almost be tempted to do something like:
> >
> >  CREATE DOMAIN emailaddr AS text
> >    CHECK (VALUE ~ '^[^ ]+@[^ ]+$');
> >
> >and then use this instead of text/varchar types.
> >
> I was thinking about something like that, but my own storage of email
> addresses actually splits them into user part and domain part (so it can
> handle the case sensitivity differently - user parts are may be case
> sensitive depending on the mail system so you can't assume they're the
> same if they only differ in case; domain parts are never case sensitive)
> and that would've unnecessarily complicated the example. I didn't think
> to go for the half way point.

I'd never bothered to go that way before.  My reasoning being that
emails get forwarded, aliases exist and other such fun and games.  I
only want the unique constraint there to keep my code working.

> The DB's checks are protecting data integrity and
> consistency; the UI's checks are improving the user/app interaction by
> providing early (and usually more friendly) notification of data issues.
> They're really quite different jobs.

Humm, I'm getting the feeling we both learned programming at the same
school!

> Occasionally, though, I do have something where the DB-using app must
> just submit a request to the DB and see if it works. Either the UI
> doesn't have the privileges to run the same checks its self, or they're
> just too expensive to do from the client (or to do twice). In those
> cases I start to find Pg's error reporting frustrating, and I either
> resort to a "return value" sort of approach or embed a unique error code
> and some parseable values in the exception string. Eg:
>
>   Some kind of human-readable error description goes here
> [ERR114:ID=12200;CONFLICTING-ID=1111]
>
> It's not pretty, but it works.

sounds sensible.  Do any other databaes/other tools work better that you
know of?  I keep looking for projects, but this could end up touching
quite a lot of code.

I'm lucky in that I've got a small userbase and they seem to be OK
(after a few initial frights) with the raw error messages from the
database.

> >Yup, why is this so often ignored when people write database drivers.  I
> >used the "pyPgSQL" python module (I think) for a bit, before realising
> >that it even went as far as to "helpfully" automatically start a new
> >transaction when the last one aborted.  The resulting semantics meant my
> >code did the most horrible things.
>
> That is indeed horrible, and I'd be running from a DB interface like
> that as fast as I could.

Yes, luckily I found out reasonably early.  I don't do much with python
and wanted to see how well it worked.  It was a bit of an off putting
experience.

> Much of what I've done with PostgreSQL has been with Python (I do a lot
> of C++ too, but not with databases) and I've thankfully not run into
> anything like that.

Most of the stuff I do with PG at work is through VB.  At least I've
managed get away from access at the back end!

My little hobby programming things tend to be in much more formally
specified things like Haskell, or lower level in C.

> psycopg (the alternative PostgreSQL interface for
> Python) handles exceptions about as well as is possible with
> PostgreSQL's purely text based exception reporting, and I've found it
> very useful.  I understand that it's also a LOT faster than PyPgSQL,
> though I don't have any direct experience there as I've never used
> PyPgSQL. It sounds like I unwittingly dodged a bullet there.

If I have reason to go back to Python I'll try and remember, thanks!

> As far as I'm concerned any DB interface that's ignoring errors behind
> your back needs to die. Especially in an exception-capable language like
> Python, where throwing and letting the upper layers handle it is the
> obviously sane thing to do.

I think the author was honestly trying to he helpful.  It's just
that (s)he hadn't quite realised the consequences of this automatic
transaction handling.  Looking in its readme it's got comments about
"To achieve the DB-API 2.0 mandated behaviour"..."a new transaction is
created on the next call to execute()".  I think maybe something got
changed so that it also did an automatic rollback on an exception (I'm
pretty sure I'd turned off autocommit pretty early, but it was a while
ago).


  Sam

Sam Mason wrote:

>> Occasionally, though, I do have something where the DB-using app must
>> just submit a request to the DB and see if it works. Either the UI
>> doesn't have the privileges to run the same checks its self, or they're
>> just too expensive to do from the client (or to do twice). In those
>> cases I start to find Pg's error reporting frustrating, and I either
>> resort to a "return value" sort of approach or embed a unique error code
>> and some parseable values in the exception string. Eg:
>>
>>   Some kind of human-readable error description goes here
>> [ERR114:ID=12200;CONFLICTING-ID=1111]
>>
>> It's not pretty, but it works.
>>
>
> sounds sensible.  Do any other databaes/other tools work better that you
> know of?  I keep looking for projects, but this could end up touching
> quite a lot of code.
>

Unfortunately I don't know of anything that already does this sort of
more detailed exception reporting.

Being able to access:

- Individual exception substitution values with their original types or
as text;
- The original exception text with substituted-in values but no
context/location information; and
- A user-specified exception  "code" or subtype for PL/PgSQL-thrown
exceptions

would be *so* nice. Even just the last one would make a big difference I
think.

Anybody have any idea how tricky that might be to implement while
maintaining 3.0 protocol compatibility for existing DB interfaces? It
might be a pretty cool GSoC project, though I suspect it might involve a
bit too much of Pg's internals.

Am I the only one who'd want something like that, or does it sound like
something that might be a worthwhile wishlist/distant-todo item?

--
Craig Ringer