Thread: proposal: plpgsql - Assert statement

On 2014-09-05 08:16, Pavel Stehule wrote:
> Assert is usually implemented as custom functions and used via PERFORM
> statement now
>
> -- usual current solution
> PERFORM Assert(some expression)
>
> I would to implement Assert as plpgsql internal statement due bigger
> possibilities to design syntax and internal implementation now and in
> future. More - as plpgsql statement should be compatible with any current
> code - because there should not be collision between SQL and PLpgSQL space.
> So this design doesn't break any current code.

It does require making ASSERT an unreserved keyword, no?  That would 
break code where someone used "assert" as a variable name, for example.

> I propose following syntax with following ecosystem:
>
> ASSERT [ NOTICE, WARNING, >>EXCEPTION<< ]
>                [ string expression or literal - explicit message ]
>                [ USING clause - same as RAISE stmt (possible in future ) ]
>    ( ROW_COUNT ( = | <> ) ( 1 | 0 ) |
>    ( QUERY some query should not be empty ) |
>    ( CHECK some expression should be true )
>    ( IS NOT NULL expression should not be null )

> UPDATE tab SET c = 1 WHERE pk = somevar;
> ASSERT ROW_COUNT = 1; -- knows what is previous DML or Dynamic DML
> ASSERT CHECK a < 100;
> ASSERT IS NOT NULL pk;
> ASSERT QUERY SELECT id FROM tab WHERE x = 1;
> ASSERT CHECK 2 = (SELECT count(*) FROM tab WHERE x = 1);

I don't see the need for specialized syntax.  If the syntax was just 
ASSERT (<expr>), these could be written as:

ASSERT (row_count = 1); -- assuming we provide a special variable 
instead of having to do GET DIAGNOSTICS
ASSERT (a < 100);  -- or perhaps ASSERT((a < 100) IS TRUE); depending on 
how NULLs are handled
ASSERT (pk IS NOT NULL);
ASSERT (EXISTS(SELECT id FROM tab WHERE x = 1));
ASSERT (2 = (SELECT count(*) FROM tab WHERE x = 1));

the idea being that it gets turned into  SELECT <expr>;  and then evaluated.

> ASSERT WARNING "data are there" QUERY SELECT ...

I think this could still be parsed correctly, though I'm not 100% sure 
on that:

ASSERT WARNING (EXISTS(SELECT ..)), 'data are there';

For extra points the error detail could work similarly to 
print_strict_params.  e.g.  ASSERT(row_count = 1);  would display the 
value of row_count in the DETAIL line, since row_count was a parameter 
to the expression.



.marko

On 9/5/14 10:09 AM, Pavel Stehule wrote:
>> I think this could still be parsed correctly, though I'm not 100% sure on
>> that:
>>
>> ASSERT WARNING (EXISTS(SELECT ..)), 'data are there';
>>
>
> PLpgSQL uses a ';' or some plpgsql keyword as SQL statement delimiter. It
> reason why RETURN QUERY ... ';' So in this case can practical to place SQL
> statement on the end of plpgsql statement.

*shrug*  There are lots of cases where a comma is used as well, e.g. 
RAISE NOTICE '..', <expr>, <expr>;

> parenthesis are not practical, because it is hard to identify bug ..

I don't see why.  The PL/PgSQL SQL parser goes to great lengths to 
identify unmatched parenthesis.  But the parens probably aren't 
necessary in the first place; you could just omit them and keep parsing 
until the next comma AFAICT.  So the syntax would be:

RAISE [ NOTICE | WARNING | EXCEPTION/ASSERT/WHATEVER ]
boolean_expr [, error_message [, error_message_param [, ... ] ] ];

> A simplicity of integration SQL and PLpgSQL is in using "smart" keywords -
> It is more verbose, and it allow to well diagnostics

I disagree.  The new keywords provide nothing of value here.  They even 
encourage the use of quirky syntax in *exchange* for verbosity ("IS NOT 
NULL pk"? really?).


.marko

On 9/5/14 10:40 AM, Pavel Stehule wrote:
> 2014-09-05 10:33 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
>> I don't see why.  The PL/PgSQL SQL parser goes to great lengths to
>> identify unmatched parenthesis.  But the parens probably aren't necessary
>> in the first place; you could just omit them and keep parsing until the
>> next comma AFAICT.  So the syntax would be:
>>
>> RAISE [ NOTICE | WARNING | EXCEPTION/ASSERT/WHATEVER ]
>> boolean_expr [, error_message [, error_message_param [, ... ] ] ];
>>
>
> extension RAISE about ASSERT level has minimal new value

Oops.  I meant to type ASSERT there, instead of RAISE.  Does that make 
more sense?

>> I disagree.  The new keywords provide nothing of value here.  They even
>> encourage the use of quirky syntax in *exchange* for verbosity ("IS NOT
>> NULL pk"? really?).
>>
>
> It is about semantic and conformity of proposed tools. Sure,  all can
> reduced to ASSERT(expr) .. but where is some benefit against function call

I see several benefits:
  1) Standard syntax, available anywhere  2) Since the RAISE EXCEPTION happens at the caller's site, we can 
provide information not available to an ordinary function, such as the 
values of the parameters passed to it  3) We can make the exception uncatchable  4) ASSERTs can be easily disabled (if
wechoose to allow that), even 
 
per-function

> I am able to do without any change of language as plpgsql extension - there
> is no necessary to do any change for too thin proposal

What *exactly* about my proposal is "too thin"?  What does your thing do 
that mine doesn't?  If you're saying your suggestion allows us to give a 
better error message, I disagree:

  ( ROW_COUNT ( = | <> ) ( 1 | 0 ) |

I've already addressed this: we can print the parameters and their 
values automatically, so printing the row count here doesn't give any 
additional value.
  ( QUERY some query should not be empty ) |

With this definition, absolutely zero value over ASSERT EXISTS(..);
  ( CHECK some expression should be true )

No additional value; it's either NULL, FALSE or TRUE and both syntaxes 
can display what the expression evaluated to.
  ( IS NOT NULL expression should not be null )

It's either NULL or it isn't.  No additional value.



.marko

On 9/5/14 11:08 AM, Pavel Stehule wrote:
> 2014-09-05 10:57 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
>> Oops.  I meant to type ASSERT there, instead of RAISE.  Does that make
>> more sense?
>
> for me a basic limit is boolean_expr

I don't understand what you mean by this.

>>> It is about semantic and conformity of proposed tools. Sure,  all can
>>> reduced to ASSERT(expr) .. but where is some benefit against function call
>>>
>>
>> I see several benefits:
>>
>>    1) Standard syntax, available anywhere
>>    2) Since the RAISE EXCEPTION happens at the caller's site, we can
>> provide information not available to an ordinary function, such as the
>> values of the parameters passed to it
>>    3) We can make the exception uncatchable
>>    4) ASSERTs can be easily disabled (if we choose to allow that), even
>> per-function
>>
>
> All points can be solved  by extension on PGXN ..

#3 probably.  Being able to satisfy #1 through PGXN is ridiculous.  #2 
and #4 would require at least hooking into PL/PgSQL, which might be 
possible, but it's intrusive and honestly feels fragile.

But perhaps more to the point, why would that criticism apply to my 
suggestion, but not yours?  Why don't we just reject any ASSERT syntax?

> and your proposed syntax
> can be better processed on Postgres level than PLpgSQL level.

*shrug*  Doing it in SQL would probably break more stuff.  I'm trying to 
contain the damage.  And arguably, this is mostly only useful in PL/PgSQL.

>>   I am able to do without any change of language as plpgsql extension -
>>> there
>>> is no necessary to do any change for too thin proposal
>>>
>>
>> What *exactly* about my proposal is "too thin"?  What does your thing do
>> that mine doesn't?  If you're saying your suggestion allows us to give a
>> better error message, I disagree:
>>
>
> "Too thin" it means so there is not possibility to extensibility,
> possibility to define dafaults, possibility to use it for static analyse.

Again, why does this criticism only apply to my suggestion and not yours?

>>    ( ROW_COUNT ( = | <> ) ( 1 | 0 ) |
>>
>> I've already addressed this: we can print the parameters and their values
>> automatically, so printing the row count here doesn't give any additional
>> value.
>>
>
> In this case, I can use a plpgsql parser for static analyse - I can do
> warning, if related query has not filter PK and similar.

You can do that anyway, you just need to work a bit harder.  I don't see 
the problem, really.


.marko

On 09/05/2014 04:40 AM, Pavel Stehule wrote:
>
>
>
> 2014-09-05 10:33 GMT+02:00 Marko Tiikkaja <marko@joh.to
> <mailto:marko@joh.to>>:
>
>     On 9/5/14 10:09 AM, Pavel Stehule wrote:
>
>             I think this could still be parsed correctly, though I'm not
>             100% sure on
>             that:
>
>             ASSERT WARNING (EXISTS(SELECT ..)), 'data are there';
>
>
>         PLpgSQL uses a ';' or some plpgsql keyword as SQL statement
>         delimiter. It
>         reason why RETURN QUERY ... ';' So in this case can practical to
>         place SQL
>         statement on the end of plpgsql statement.
>
>
>     *shrug*  There are lots of cases where a comma is used as well, e.g.
>     RAISE NOTICE '..', <expr>, <expr>;
>
>         parenthesis are not practical, because it is hard to identify bug ..
>
>
>     I don't see why.  The PL/PgSQL SQL parser goes to great lengths to
>     identify unmatched parenthesis.  But the parens probably aren't
>     necessary in the first place; you could just omit them and keep
>     parsing until the next comma AFAICT.  So the syntax would be:
>
>     RAISE [ NOTICE | WARNING | EXCEPTION/ASSERT/WHATEVER ]
>     boolean_expr [, error_message [, error_message_param [, ... ] ] ];
>
>
> extension RAISE about ASSERT level has minimal new value

Adding a WHEN clause to RAISE would have the benefit of not needing any 
new keywords at all.

RAISE EXCEPTION 'format' [, expr ...] WHEN row_count <> 1;


Regards,
Jan

>
>
>         A simplicity of integration SQL and PLpgSQL is in using "smart"
>         keywords -
>         It is more verbose, and it allow to well diagnostics
>
>
>     I disagree.  The new keywords provide nothing of value here.  They
>     even encourage the use of quirky syntax in *exchange* for verbosity
>     ("IS NOT NULL pk"? really?).
>
>
> It is about semantic and conformity of proposed tools. Sure,  all can
> reduced to ASSERT(expr) .. but where is some benefit against function call
>
> I am able to do without any change of language as plpgsql extension -
> there is no necessary to do any change for too thin proposal
>
>
>
>     .marko
>
>


-- 
Jan Wieck
Senior Software Engineer
http://slony.info

On 05/09/14 14:35, Jan Wieck wrote:
> On 09/05/2014 04:40 AM, Pavel Stehule wrote:
>
> Adding a WHEN clause to RAISE would have the benefit of not needing any
> new keywords at all.
>
> RAISE EXCEPTION 'format' [, expr ...] WHEN row_count <> 1;
>

+1

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 2014-09-06 7:27 AM, Pavel Stehule wrote:
> 2014-09-05 14:35 GMT+02:00 Jan Wieck <jan@wi3ck.info>:
>> Adding a WHEN clause to RAISE would have the benefit of not needing any
>> new keywords at all.
>>
>> RAISE EXCEPTION 'format' [, expr ...] WHEN row_count <> 1;
>>
>
> It was one my older proposal.
>
> Can we find a agreement there?

I find:
  1) The syntax less readable than  IF row_count <> 1 THEN RAISE 
EXCEPTION ..; END IF;  2) It needless to require the user to specify an error message for 
every assertion.  3) Allowing these to be disabled would be weird (though I might be 
the only one who wants that feature at this point).  4) It would also be weird to display the parameters passed to the

WHEN clause like I suggested here: 
http://www.postgresql.org/message-id/54096BA4.5030600@joh.to .  I think 
that's a crucial part of the feature.

So at least the vote isn't unanimous: -1 from me.


.marko

On 2014-09-06 7:49 PM, Pavel Stehule wrote:
> this doesn't to supply assertions, it is just shorter form

The original proposal very clearly seems to be "why don't we do this 
*instead* of assertions?"  And in that case all of my points apply, and 
I'm very much against this syntax.  If this is supposed to be in the 
language *in addition to* assertions, let's please be clear about that.  (In that case I wouldn't object, though I
probablywouldn't use this 
 
feature either.)


.marko

On Fri, Sep 5, 2014 at 2:16 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Assert is usually implemented as custom functions and used via PERFORM
> statement now
>
> -- usual current solution
> PERFORM Assert(some expression)
>
> I would to implement Assert as plpgsql internal statement due bigger
> possibilities to design syntax and internal implementation now and in
> future. More - as plpgsql statement should be compatible with any current
> code - because there should not be collision between SQL and PLpgSQL space.
> So this design doesn't break any current code.
>
> I propose following syntax with following ecosystem:
>
> ASSERT [ NOTICE, WARNING, >>EXCEPTION<< ]
>               [ string expression or literal - explicit message ]
>               [ USING clause - same as RAISE stmt (possible in future ) ]
>   ( ROW_COUNT ( = | <> ) ( 1 | 0 ) |
>   ( QUERY some query should not be empty ) |
>   ( CHECK some expression should be true )
>   ( IS NOT NULL expression should not be null )
>
> Every variant (ROW_COUNT, QUERY, CHECK, IS NOT NULL) has own default message

That's probably not the ugliest syntax I've *ever* seen, but it's
definitely the ugliest syntax I've seen today.

I previously proposed RAISE ASSERT ... WHERE, which seems like a nice
variant of what we've already got, but perhaps this whole discussion
merely illustrates that it's hard to get more than 1 vote for any
proposal in this area.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/05/2014 05:21 PM, Pavel Stehule wrote:
> 
> *shrug*  Doing it in SQL would probably break more stuff.  I'm trying to
> contain the damage.  And arguably, this is mostly only useful in PL/PgSQL.

I've wanted assertions in SQL enough that I often write trivial wrappers
around `raise` in PL/PgSQL for use in `CASE` statements etc.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 09/09/2014 05:20 AM, Robert Haas wrote:
> 
> I previously proposed RAISE ASSERT ... WHERE, which seems like a nice
> variant of what we've already got, but perhaps this whole discussion
> merely illustrates that it's hard to get more than 1 vote for any
> proposal in this area.

Well, you have Petr's for RAISE EXCEPTION ... WHEN, and I'd also like
that or RAISE ASSERT ... WHEN.

Much (much) saner than the other proposals on this thread IMO.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 2014-09-09 07:54, Craig Ringer wrote:
> On 09/05/2014 05:21 PM, Pavel Stehule wrote:
>>
>> *shrug*  Doing it in SQL would probably break more stuff.  I'm trying to
>> contain the damage.  And arguably, this is mostly only useful in PL/PgSQL.
>
> I've wanted assertions in SQL enough that I often write trivial wrappers
> around `raise` in PL/PgSQL for use in `CASE` statements etc.

Yeah, as have I.  I've also advocated that there should be a 
raise_exception(text, anyelement) anyelement  function shipped with 
postgres.

But this is something different; this is just a single statement which 
asserts that some expression evaluates to true.  Even if we allowed it 
to be used as a scalar expression, there's still the problem anyelement 
is commonly used to work around.


.marko

On 09/14/2014 02:25 PM, Pavel Stehule wrote:
> a) Isn't possible handle a assertion exception anywhere .. it enforce
> ROLLBACK in 100%
>
> b) Assertions should be disabled globally .. I am not sure, it it is a
> good idea, but I can understand so some tests based on queries to data
> can be performance issue.
>
> Important question is a relation assertations and exceptions. Is it only
> shortcut for exception or some different?

I think that most data integrity issues can be handled by a well 
designed database schema that uses UNIQUE, NOT NULL, REFERENCES and 
CHECK constraints. Assertions are usually found inside of complex code 
constructs to check values of local variables. I don't think it is even 
a good idea to implement assertions that can query arbitrary data.


Jan

-- 
Jan Wieck
Senior Software Engineer
http://slony.info

Jan Wieck wrote:
> On 09/14/2014 02:25 PM, Pavel Stehule wrote:
> >a) Isn't possible handle a assertion exception anywhere .. it enforce
> >ROLLBACK in 100%
> >
> >b) Assertions should be disabled globally .. I am not sure, it it is a
> >good idea, but I can understand so some tests based on queries to data
> >can be performance issue.
> >
> >Important question is a relation assertations and exceptions. Is it only
> >shortcut for exception or some different?
> 
> I think that most data integrity issues can be handled by a well
> designed database schema that uses UNIQUE, NOT NULL, REFERENCES and
> CHECK constraints. Assertions are usually found inside of complex
> code constructs to check values of local variables. I don't think it
> is even a good idea to implement assertions that can query arbitrary
> data.

Actually Peter Eisentraut posted a patch for SQL assertions:
http://www.postgresql.org/message-id/1384486216.5008.17.camel@vanquo.pezone.net

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9/16/14 12:01 AM, Alvaro Herrera wrote:
> Jan Wieck wrote:
>> I think that most data integrity issues can be handled by a well
>> designed database schema that uses UNIQUE, NOT NULL, REFERENCES and
>> CHECK constraints. Assertions are usually found inside of complex
>> code constructs to check values of local variables. I don't think it
>> is even a good idea to implement assertions that can query arbitrary
>> data.
> 
> Actually Peter Eisentraut posted a patch for SQL assertions:
> http://www.postgresql.org/message-id/1384486216.5008.17.camel@vanquo.pezone.net

SQL assertions are just a kind of CHECK constraint, so fully
Jan-compliant. ;-)

I don't mind PL/pgSQL having an "assert" statement like many programming
languages, but I find a lot of the proposed details dubious.

On 9/14/14 2:49 PM, Jan Wieck wrote:
> I don't think it is even a good idea to implement assertions that can
> query arbitrary data.

In a normal programming language, an assertion is usually a static fault
in your program.  If the assertion ever fails, you fix your program and
then it hopefully never happens again.

Assertion that query the state of the database or result row counts are
pushing that concept quite a bit.  Those are not assertions, those are
just plain old error handling.

On 9/17/14, 9:00 PM, Peter Eisentraut wrote:
> On 9/14/14 2:49 PM, Jan Wieck wrote:
>> I don't think it is even a good idea to implement assertions that can
>> query arbitrary data.
>
> In a normal programming language, an assertion is usually a static fault
> in your program.  If the assertion ever fails, you fix your program and
> then it hopefully never happens again.
>
> Assertion that query the state of the database or result row counts are
> pushing that concept quite a bit.  Those are not assertions, those are
> just plain old error handling.

*shrug*  I don't see them as error handling if they're just checking 
conditions which should never happen.

That said, in PL/PgSQL these expressions would likely have to be SQL 
expressions, and then you'd have to go out of your way to implement 
assertions which *can't* query arbitrary data.  And that just seems silly.


.marko

On 9/17/14 3:04 PM, Pavel Stehule wrote:
> What is difference between content of variable or content of database?
> You can test any prerequisite, but when this prerequisite is not solved,
> than exception is very very hard without possible handling.

If the assertion tests arbitrary Boolean expressions, then we can't stop
the user from abusing them.

But it's another thing if we design specific syntax that encourages such
abuse, as proposed earlier.

On 09/17/2014 03:36 PM, Peter Eisentraut wrote:
> On 9/17/14 3:04 PM, Pavel Stehule wrote:
>> What is difference between content of variable or content of database?
>> You can test any prerequisite, but when this prerequisite is not solved,
>> than exception is very very hard without possible handling.
>
> If the assertion tests arbitrary Boolean expressions, then we can't stop
> the user from abusing them.

Exactly. Doing something like
    ASSERT (select count(*) from foo        where fk not in (select pk from bar)) = 0;

is a perfectly fine, arbitrary boolean expression. It will probably work 
well in a development environment too. And I am very sure that it will 
not scale well once that code gets deployed. And I know how DBAs react 
to the guaranteed following performance problem. They will disable ALL 
assert ... or was there some sort of assert class system proposed that I 
missed?

>
> But it's another thing if we design specific syntax that encourages such
> abuse, as proposed earlier.

The design should explicitly discourage that sort of nonsense.


Jan


-- 
Jan Wieck
Senior Software Engineer
http://slony.info

On 9/17/14, 7:40 PM, Jan Wieck wrote:
> Exactly. Doing something like
>
>      ASSERT (select count(*) from foo
>          where fk not in (select pk from bar)) = 0;
>
> is a perfectly fine, arbitrary boolean expression. It will probably work well in a development environment too. And I
amvery sure that it will not scale well once that code gets deployed. And I know how DBAs react to the guaranteed
followingperformance problem. They will disable ALL assert ... or was there some sort of assert class system proposed
thatI missed?
 

Keep in mind that nothing we come up with will be immune to abuse, and trying to solve what is fundamentally an
educationproblem through technology rarely turns out well.
 

We're also putting too much weight on the term "assert" here. C-style asserts are generally not nearly as useful in a
databaseas general sanity-checking or error handling, especially if you're trying to use the database to enforce data
sanity.

My wish-list for "asserts" is:

- Works at a SQL level
- Unique/clear way to identify asserts (so you're not guessing where the assert came from)
- Allows for over-riding individual asserts (so if you need to do something you're "not supposed to do" you still have
theprotection of all other asserts)
 
- Less verbose than IF THEN RAISE END IF
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

On 09/09/14 17:37, Pavel Stehule wrote:
> Ada is language with strong character, and PLpgSQL is little bit strange
> fork - so it isn't easy to find some form, how to solve all requirements.
>
> My requests:
>
> * be consistent with current PLpgSQL syntax and logic
> * allow some future extensibility
> * allow a static analyses without hard expression processing
>
> But I am thinking so there are some points where can be some agreement -
> although it is not ASSERT implementation.
>
> enhancing RAISE WHEN - please, see attached patch -
>
> I prefer RAISE WHEN again RAISE WHERE due consistency with EXIT and
> CONTINUE [ WHEN ];
>

Short review of the patch. Note that this has nothing to do with actual 
assertions, at the moment it's just enhancement of RAISE statement to 
make it optionally conditional. As I was one of the people who voted for 
it I do think we want this and I like the syntax.

Code applies cleanly, seems formatted according to project standards - 
there is unnecessary whitespace added in variable declaration part of 
exec_stmt_raise which should be removed.

Passes make check, I would prefer to have little more complex expression 
than just "true" in the new regression test added for this feature.

Did some manual testing, seems to work as advertised.

There are no docs at the moment which is the only show-stopper that I 
can see so far.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 16/10/14 13:29, Pavel Stehule wrote:
> Hi
>
> 2014-10-14 22:57 GMT+02:00 Petr Jelinek <petr@2ndquadrant.com
>
>     Short review of the patch. Note that this has nothing to do with
>     actual assertions, at the moment it's just enhancement of RAISE
>     statement to make it optionally conditional. As I was one of the
>     people who voted for it I do think we want this and I like the syntax.
>
>     Code applies cleanly, seems formatted according to project standards
>     - there is unnecessary whitespace added in variable declaration part
>     of exec_stmt_raise which should be removed.
>
>
> fixed
>
>
>     Passes make check, I would prefer to have little more complex
>     expression than just "true" in the new regression test added for
>     this feature.
>
>
> fixed
>
>
>     Did some manual testing, seems to work as advertised.
>
>     There are no docs at the moment which is the only show-stopper that
>     I can see so far.
>
>
> fixed
>

Great, looks good to me, marking as ready for committer.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

>> Great, looks good to me, marking as ready for committer.

What is wrong with using IF ?

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 11/17/14, 4:58 PM, Simon Riggs wrote:
>>> Great, looks good to me, marking as ready for committer.
>
> What is wrong with using IF ?

It's a hell of a lot wordier. I've previously created a more sophisticated "assert" framework to allow more control
overthings, but ended up also using it just for simple sanity checking because it was much nicer than typeing IF THEN
RAISEERROR END IF.
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 18 November 2014 01:00, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 11/17/14, 4:58 PM, Simon Riggs wrote:
>>>>
>>>> Great, looks good to me, marking as ready for committer.
>>
>>
>> What is wrong with using IF ?
>
>
> It's a hell of a lot wordier. I've previously created a more sophisticated
> "assert" framework to allow more control over things, but ended up also
> using it just for simple sanity checking because it was much nicer than
> typeing IF THEN RAISE ERROR END IF.

Why is that not a requirement for a less wordier form of IF?

IF (something) THEN action

Why is this problem specific to RAISE?


-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 11/18/2014 04:23 AM, Simon Riggs wrote:
> On 18 November 2014 01:00, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>> On 11/17/14, 4:58 PM, Simon Riggs wrote:
>>>>> Great, looks good to me, marking as ready for committer.
>>>
>>> What is wrong with using IF ?
>>
>> It's a hell of a lot wordier. I've previously created a more sophisticated
>> "assert" framework to allow more control over things, but ended up also
>> using it just for simple sanity checking because it was much nicer than
>> typeing IF THEN RAISE ERROR END IF.
> Why is that not a requirement for a less wordier form of IF?
>
> IF (something) THEN action
>
> Why is this problem specific to RAISE?
>
>


Please, no. The use of closed form rather than open form IF statements 
is one of the things Ada (and by inheritance PLPGSQL) got right.

Frankly, I find this whole proposal, and all the suggested alternatives, 
somewhat ill-conceived. PLPGSQL is a wordy language. If you want 
something more terse, use something else. Adding these sorts of 
syntactic sugar warts onto the language doesn't seem like a terribly 
good way to proceed.

cheers

andrew

On 18 November 2014 12:29, Pavel Stehule <pavel.stehule@gmail.com> wrote:

>> Why is that not a requirement for a less wordier form of IF?
>>
>> IF (something) THEN action
>
>
> statement IF is a control statement - and syntax, pattern for control
> statements in plpgsql is consistent. I don't want to break it (more,
> probably it is hardly implemented due problems in bison). PL/pgSQL, PL/SQL,
> Ada are well designed (in my opinion). Conditional statement has precedent
> in PL/pgSQL now. We support EXIT and CONTINUE WHEN, so we don't propose a
> new pattern, only reuse some existing.

I commend your wish to improve PL/pgSQL, I'm sorry to say that I just
don't see how this moves us forwards.

What this does is introduce a fairly restricted new feature that
removes backwards compatibility and takes us further away from Oracle
compatibility.

If I want to write an Assert style test that fits on a single line, just write  PEFORM raise_error_when(boolean
expression);

which requires a very short function like this
CREATE OR REPLACE FUNCTION raise_error_when(test boolean) returns void
language plpgsql
AS $$ DECLARE BEGIN    IF (test) THEN         RAISE EXCEPTION 'assertion failure';    END IF; END;
$$;

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 11/18/14, 9:31 AM, Andrew Dunstan wrote:
>
> Frankly, I find this whole proposal, and all the suggested alternatives, somewhat ill-conceived. PLPGSQL is a wordy
language.If you want something more terse, use something else. Adding these sorts of syntactic sugar warts onto the
languagedoesn't seem like a terribly good way to proceed.
 

Such as?

The enormous advantage of plpgsql is how easy it is to run SQL. Every other PL I've looked at makes that WAY harder.
Andthat's assuming you're in an environment where you can install another PL.
 

And honestly, I've never really found plpgsql to be terribly wordy except in a few cases ("assert" being one of them).
Mygeneral experience has been that when I'm doing an IF (other than assert), I'm doing multiple things in the IF block,
soit's really not that big a deal.
 

As for why not do this in a separate function; yes, you can do that. But then you've needlessly added to your context
stack,it's going to be a lot slower, and you can only really replace RAISE's functionality if you're in a version that
hasformat().
 

If someone has another brain-flash on how to make this better I'm all ears. But I don't think arguments like "use
anotherPL" or "it's just syntax sugar" improve things for our users.
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 11/18/2014 02:53 PM, Jim Nasby wrote:
> On 11/18/14, 9:31 AM, Andrew Dunstan wrote:
>>
>> Frankly, I find this whole proposal, and all the suggested 
>> alternatives, somewhat ill-conceived. PLPGSQL is a wordy language. If 
>> you want something more terse, use something else. Adding these sorts 
>> of syntactic sugar warts onto the language doesn't seem like a 
>> terribly good way to proceed.
>
> Such as?
>
> The enormous advantage of plpgsql is how easy it is to run SQL. Every 
> other PL I've looked at makes that WAY harder. And that's assuming 
> you're in an environment where you can install another PL.
>
> And honestly, I've never really found plpgsql to be terribly wordy 
> except in a few cases ("assert" being one of them). My general 
> experience has been that when I'm doing an IF (other than assert), I'm 
> doing multiple things in the IF block, so it's really not that big a 
> deal.
>


I frequently write one-statement bodies of IF statements. To me that's 
not a big deal either :-)

cheers

andrew

On 18/11/14 22:11, Pavel Stehule wrote:
>
> 2014-11-18 21:27 GMT+01:00 Andrew Dunstan <andrew@dunslane.net
> <mailto:andrew@dunslane.net>>:
>
>
>     I frequently write one-statement bodies of IF statements. To me
>     that's not a big deal either :-)
>
>
> anybody did it, but it doesn't need so it is perfect :) I understand
> well to Jim' feeling.
>
> I am looking to Ada 2005 language ... a design of RAISE WITH shows so
> RAISE statement is extensible in Ada too. Sure - we can live without it,
> but I don't think so we do some wrong with introduction RAISE WHEN and I
> am sure, so a live with this feature can be more fun for someone, who
> intensive use this pattern.
>

Personally, I see this as natural extension of the conditional block 
control which we already have for loops with CONTINUE WHEN and EXIT 
WHEN. This basically extends it to any block and it seems quite natural 
to have it for me...

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 11/18/2014 04:11 PM, Pavel Stehule wrote:
>
>
> 2014-11-18 21:27 GMT+01:00 Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>>:
>
>
>     On 11/18/2014 02:53 PM, Jim Nasby wrote:
>
>         On 11/18/14, 9:31 AM, Andrew Dunstan wrote:
>
>
>             Frankly, I find this whole proposal, and all the suggested
>             alternatives, somewhat ill-conceived. PLPGSQL is a wordy
>             language. If you want something more terse, use something
>             else. Adding these sorts of syntactic sugar warts onto the
>             language doesn't seem like a terribly good way to proceed.
>
>
>         Such as?
>
>         The enormous advantage of plpgsql is how easy it is to run
>         SQL. Every other PL I've looked at makes that WAY harder. And
>         that's assuming you're in an environment where you can install
>         another PL.
>
>         And honestly, I've never really found plpgsql to be terribly
>         wordy except in a few cases ("assert" being one of them). My
>         general experience has been that when I'm doing an IF (other
>         than assert), I'm doing multiple things in the IF block, so
>         it's really not that big a deal.
>
>
>
>     I frequently write one-statement bodies of IF statements. To me
>     that's not a big deal either :-)
>
>
> anybody did it, but it doesn't need so it is perfect :) I understand 
> well to Jim' feeling.
>
> I am looking to Ada 2005 language ... a design of RAISE WITH shows so 
> RAISE statement is extensible in Ada too. Sure - we can live without 
> it, but I don't think so we do some wrong with introduction RAISE WHEN 
> and I am sure, so a live with this feature can be more fun for 
> someone, who intensive use this pattern.
>
>


(drags out recently purchased copy of Barnes "Ada 2012")

Ada's
    RAISE exception_name WITH "string";

is more or less the equivalent of our
    RAISE level 'format_string';

So I don't think there's much analogy there.


I'm not going to die in a ditch over this, but it does seem to me very 
largely unnecessary.

cheers

andrew

On 18 November 2014 21:19, Petr Jelinek <petr@2ndquadrant.com> wrote:

> Personally, I see this as natural extension of the conditional block control
> which we already have for loops with CONTINUE WHEN and EXIT WHEN. This
> basically extends it to any block and it seems quite natural to have it for
> me...

That's a reasonable argument to include it.

I seem to share the same opinion with Andrew: its not going to hurt to
include this, but its not gonna cause dancing in the streets either. I
would characterize that as 2 very neutral and unimpressed people, plus
3 in favour. Which seems enough to commit.

Perhaps I misunderstand, Andrew?

Any objectors, say so now or I will commit tomorrow.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 11/19/2014 06:35 AM, Simon Riggs wrote:
> On 18 November 2014 21:19, Petr Jelinek <petr@2ndquadrant.com> wrote:
>
>> Personally, I see this as natural extension of the conditional block control
>> which we already have for loops with CONTINUE WHEN and EXIT WHEN. This
>> basically extends it to any block and it seems quite natural to have it for
>> me...
> That's a reasonable argument to include it.
>
> I seem to share the same opinion with Andrew: its not going to hurt to
> include this, but its not gonna cause dancing in the streets either. I
> would characterize that as 2 very neutral and unimpressed people, plus
> 3 in favour. Which seems enough to commit.
>
> Perhaps I misunderstand, Andrew?
>


That's about right, although I would put it a bit stronger than that. 
But if we're the only people unimpressed I'm not going to object further.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/19/2014 06:35 AM, Simon Riggs wrote:
>> I seem to share the same opinion with Andrew: its not going to hurt to
>> include this, but its not gonna cause dancing in the streets either. I
>> would characterize that as 2 very neutral and unimpressed people, plus
>> 3 in favour. Which seems enough to commit.

> That's about right, although I would put it a bit stronger than that. 
> But if we're the only people unimpressed I'm not going to object further.

FWIW, I would vote against it also.  I do not find this to be a natural
extension of RAISE; it adds all sorts of semantic issues.  (In particular,
what is the evaluation order of the WHEN versus the other subexpressions
of the RAISE?)
        regards, tom lane

On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/19/2014 06:35 AM, Simon Riggs wrote:
>>> I seem to share the same opinion with Andrew: its not going to hurt to
>>> include this, but its not gonna cause dancing in the streets either. I
>>> would characterize that as 2 very neutral and unimpressed people, plus
>>> 3 in favour. Which seems enough to commit.
>
>> That's about right, although I would put it a bit stronger than that.
>> But if we're the only people unimpressed I'm not going to object further.
>
> FWIW, I would vote against it also.  I do not find this to be a natural
> extension of RAISE; it adds all sorts of semantic issues.  (In particular,
> what is the evaluation order of the WHEN versus the other subexpressions
> of the RAISE?)

What I liked about this syntax was that we could eventually have:

RAISE ASSERT WHEN stuff;

...and if assertions are disabled, we can skip evaluating the
condition.  If you just write an IF .. THEN block you can't do that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> FWIW, I would vote against it also.  I do not find this to be a natural
>> extension of RAISE; it adds all sorts of semantic issues.  (In particular,
>> what is the evaluation order of the WHEN versus the other subexpressions
>> of the RAISE?)

> What I liked about this syntax was that we could eventually have:
> RAISE ASSERT WHEN stuff;
> ...and if assertions are disabled, we can skip evaluating the
> condition.  If you just write an IF .. THEN block you can't do that.

Well, if that's what you want, let's just invent

ASSERT condition

and not tangle RAISE into it.  The analogy to EXIT WHEN is a lot
cleaner in this form: no order-of-evaluation issues, no questions
of whether a sub-clause results in totally changing the meaning
of the command.  And if your argument is partially based on
how much you have to type, doesn't this way dominate all others?
        regards, tom lane

On Wed, Nov 19, 2014 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> FWIW, I would vote against it also.  I do not find this to be a natural
>>> extension of RAISE; it adds all sorts of semantic issues.  (In particular,
>>> what is the evaluation order of the WHEN versus the other subexpressions
>>> of the RAISE?)
>
>> What I liked about this syntax was that we could eventually have:
>> RAISE ASSERT WHEN stuff;
>> ...and if assertions are disabled, we can skip evaluating the
>> condition.  If you just write an IF .. THEN block you can't do that.
>
> Well, if that's what you want, let's just invent
>
> ASSERT condition
>
> and not tangle RAISE into it.  The analogy to EXIT WHEN is a lot
> cleaner in this form: no order-of-evaluation issues, no questions
> of whether a sub-clause results in totally changing the meaning
> of the command.  And if your argument is partially based on
> how much you have to type, doesn't this way dominate all others?

That doesn't bother me any.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-11-19 23:18, Pavel Stehule wrote:
> 2014-11-19 18:01 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> FWIW, I would vote against it also.  I do not find this to be a natural
>>>> extension of RAISE; it adds all sorts of semantic issues.  (In
>> particular,
>>>> what is the evaluation order of the WHEN versus the other subexpressions
>>>> of the RAISE?)
>>
>>> What I liked about this syntax was that we could eventually have:
>>> RAISE ASSERT WHEN stuff;
>>> ...and if assertions are disabled, we can skip evaluating the
>>> condition.  If you just write an IF .. THEN block you can't do that.
>>
>> Well, if that's what you want, let's just invent
>>
>> ASSERT condition
>>
>>
> there was this proposal .. ASSERT statement .. related discuss was
> finished, because it needs a reserved keyword "ASSERT".

Finished?  Really?

This was Heikki's take on the discussion that took place a good while 
ago: http://www.postgresql.org/message-id/5405FF73.1010206@vmware.com. 
And in the same thread you also said you like it: 
http://www.postgresql.org/message-id/CAFj8pRAC-ZWDrbU-uj=xQOWQtbAqR5oXsM1xYOyhZmyeuvZvQA@mail.gmail.co.  But perhaps
you'vechanged your mind since then (which is fine.)  Or 
 
maybe that was only in the case where we'd have a special mode where you 
could opt-in if you're willing to accept the backwards compatibility issue?

I also went back to the original thread, and I think Heikki's summary 
dismissed e.g. Robert's criticism quite lightly: 
http://www.postgresql.org/message-id/CA+TgmobWoSSRNcV_iJK3xhsytXb7Dv0AWGvWkMEurNTOVEZYyw@mail.gmail.com


.marko

Marko Tiikkaja <marko@joh.to> writes:
> I also went back to the original thread, and I think Heikki's summary 
> dismissed e.g. Robert's criticism quite lightly: 
> http://www.postgresql.org/message-id/CA+TgmobWoSSRNcV_iJK3xhsytXb7Dv0AWGvWkMEurNTOVEZYyw@mail.gmail.com

The core of that complaint is that we'd have to make ASSERT a plpgsql
reserved word, which is true enough as things stand today.  However,
why is it that plpgsql statement-introducing keywords need to be
reserved?  The only reason for that AFAICS is to allow us to distinguish
the statements from assignments.  But it seems like that could possibly
be gotten around with some work.  The first thing that comes to mind is
for the lexer to do one-token lookahead and assume that the second
token of an assignment must be ":=" (aka "="), ".", or "[".  (Have
I missed any cases?)  Then, any statement for which the second token
can't be one of those, which is surely most if not all of them, could
have an unreserved leading keyword.  This would at a stroke dereserve
about half of plpgsql's existing reserved words, as well as remove a
roadblock for ASSERT and other new statements.
        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2014-11-19 23:54 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> The core of that complaint is that we'd have to make ASSERT a plpgsql
>> reserved word, which is true enough as things stand today.  However,
>> why is it that plpgsql statement-introducing keywords need to be
>> reserved?

> Doesn't it close a doors to implement a procedures call without explicit
> CALL statement (like PL/SQL) ?

So, in order to leave the door open for implicit CALL (which nobody's
ever proposed or tried to implement AFAIR), you're willing to see every
other proposal for new plpgsql statements go down the drain due to
objections to new reserved words?  I think your priorities are skewed.

(If we did want to allow implicit CALL, I suspect that things could be
hacked so that it worked for any function name that wasn't already an
unreserved keyword, anyway.  So you'd be no worse off.)

> Personally I doesn't feel to introduce lot of new keywords (statements) to
> plpgsql. Probably only two - ASSERT (assertions), PRAGMA (some cooperation
> with plpgsql extensions).

I can't say that either of those excite me particularly, so the idea
that those two are the only new statements we'd ever want to add seems
improbable.
        regards, tom lane

On 19 November 2014 23:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2014-11-19 23:54 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>> The core of that complaint is that we'd have to make ASSERT a plpgsql
>>> reserved word, which is true enough as things stand today.  However,
>>> why is it that plpgsql statement-introducing keywords need to be
>>> reserved?
>
>> Doesn't it close a doors to implement a procedures call without explicit
>> CALL statement (like PL/SQL) ?
>
> So, in order to leave the door open for implicit CALL (which nobody's
> ever proposed or tried to implement AFAIR), you're willing to see every
> other proposal for new plpgsql statements go down the drain due to
> objections to new reserved words?  I think your priorities are skewed.
>
> (If we did want to allow implicit CALL, I suspect that things could be
> hacked so that it worked for any function name that wasn't already an
> unreserved keyword, anyway.  So you'd be no worse off.)

Implictly CALLed procedures/function-that-return-void would be a great
feature for 9.5

Great proposal.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

I wrote:
> The core of that complaint is that we'd have to make ASSERT a plpgsql
> reserved word, which is true enough as things stand today.  However,
> why is it that plpgsql statement-introducing keywords need to be
> reserved?  The only reason for that AFAICS is to allow us to distinguish
> the statements from assignments.  But it seems like that could possibly
> be gotten around with some work.

Attached is a draft patch that de-reserves 17 of plpgsql's existing
reserved words, leaving 24 still reserved (of which only 9 are not also
reserved in the core grammar).  This would make it painless to invent an
ASSERT statement, as well as any other new statement type that's not
associated with looping or block structure.  (The limiting factor on those
is that if a statement could have an opt_block_label, its keyword still
has to be reserved, unless we want to complicate matters a bunch more.)

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 82378c7..4af28ea 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** unreserved_keyword    :
*** 2315,2346 ****
--- 2315,2360 ----
                  | K_ALIAS
                  | K_ARRAY
                  | K_BACKWARD
+                 | K_CLOSE
+                 | K_COLLATE
                  | K_COLUMN
                  | K_COLUMN_NAME
                  | K_CONSTANT
                  | K_CONSTRAINT
                  | K_CONSTRAINT_NAME
+                 | K_CONTINUE
                  | K_CURRENT
                  | K_CURSOR
                  | K_DATATYPE
                  | K_DEBUG
+                 | K_DEFAULT
                  | K_DETAIL
+                 | K_DIAGNOSTICS
                  | K_DUMP
+                 | K_ELSIF
                  | K_ERRCODE
                  | K_ERROR
+                 | K_EXCEPTION
+                 | K_EXIT
+                 | K_FETCH
                  | K_FIRST
                  | K_FORWARD
+                 | K_GET
                  | K_HINT
                  | K_INFO
+                 | K_INSERT
                  | K_IS
                  | K_LAST
                  | K_LOG
                  | K_MESSAGE
                  | K_MESSAGE_TEXT
+                 | K_MOVE
                  | K_NEXT
                  | K_NO
                  | K_NOTICE
+                 | K_OPEN
                  | K_OPTION
+                 | K_PERFORM
                  | K_PG_CONTEXT
                  | K_PG_DATATYPE_NAME
                  | K_PG_EXCEPTION_CONTEXT
*************** unreserved_keyword    :
*** 2349,2356 ****
--- 2363,2372 ----
                  | K_PRINT_STRICT_PARAMS
                  | K_PRIOR
                  | K_QUERY
+                 | K_RAISE
                  | K_RELATIVE
                  | K_RESULT_OID
+                 | K_RETURN
                  | K_RETURNED_SQLSTATE
                  | K_REVERSE
                  | K_ROW_COUNT
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
index 6a5a04b..db01ba5 100644
*** a/src/pl/plpgsql/src/pl_scanner.c
--- b/src/pl/plpgsql/src/pl_scanner.c
*************** IdentifierLookup plpgsql_IdentifierLooku
*** 31,51 ****
   *
   * We keep reserved and unreserved keywords in separate arrays.  The
   * reserved keywords are passed to the core scanner, so they will be
!  * recognized before (and instead of) any variable name.  Unreserved
!  * words are checked for separately, after determining that the identifier
   * isn't a known variable name.  If plpgsql_IdentifierLookup is DECLARE then
   * no variable names will be recognized, so the unreserved words always work.
   * (Note in particular that this helps us avoid reserving keywords that are
   * only needed in DECLARE sections.)
   *
   * In certain contexts it is desirable to prefer recognizing an unreserved
!  * keyword over recognizing a variable name.  Those cases are handled in
!  * pl_gram.y using tok_is_keyword().
   *
!  * For the most part, the reserved keywords are those that start a PL/pgSQL
!  * statement (and so would conflict with an assignment to a variable of the
!  * same name).  We also don't sweat it much about reserving keywords that
!  * are reserved in the core grammar.  Try to avoid reserving other words.
   */

  /*
--- 31,58 ----
   *
   * We keep reserved and unreserved keywords in separate arrays.  The
   * reserved keywords are passed to the core scanner, so they will be
!  * recognized before (and instead of) any variable name.  Unreserved words
!  * are checked for separately, usually after determining that the identifier
   * isn't a known variable name.  If plpgsql_IdentifierLookup is DECLARE then
   * no variable names will be recognized, so the unreserved words always work.
   * (Note in particular that this helps us avoid reserving keywords that are
   * only needed in DECLARE sections.)
   *
   * In certain contexts it is desirable to prefer recognizing an unreserved
!  * keyword over recognizing a variable name.  In particular, at the start
!  * of a statement we should prefer unreserved keywords unless the statement
!  * looks like an assignment (i.e., first token is followed by ':=' or '[').
!  * This rule allows most statement-introducing keywords to be kept unreserved.
!  * (We still have to reserve initial keywords that might follow a block
!  * label, unfortunately, since the method used to determine if we are at
!  * start of statement doesn't recognize such cases.  We'd also have to
!  * reserve any keyword that could legitimately be followed by ':=' or '['.)
!  * Some additional cases are handled in pl_gram.y using tok_is_keyword().
   *
!  * We try to avoid reserving more keywords than we have to; but there's
!  * little point in not reserving a word if it's reserved in the core grammar.
!  * Currently, the following words are reserved here but not in the core:
!  * BEGIN BY DECLARE EXECUTE FOREACH IF LOOP STRICT WHILE
   */

  /*
*************** static const ScanKeyword reserved_keywor
*** 63,99 ****
      PG_KEYWORD("begin", K_BEGIN, RESERVED_KEYWORD)
      PG_KEYWORD("by", K_BY, RESERVED_KEYWORD)
      PG_KEYWORD("case", K_CASE, RESERVED_KEYWORD)
-     PG_KEYWORD("close", K_CLOSE, RESERVED_KEYWORD)
-     PG_KEYWORD("collate", K_COLLATE, RESERVED_KEYWORD)
-     PG_KEYWORD("continue", K_CONTINUE, RESERVED_KEYWORD)
      PG_KEYWORD("declare", K_DECLARE, RESERVED_KEYWORD)
-     PG_KEYWORD("default", K_DEFAULT, RESERVED_KEYWORD)
-     PG_KEYWORD("diagnostics", K_DIAGNOSTICS, RESERVED_KEYWORD)
      PG_KEYWORD("else", K_ELSE, RESERVED_KEYWORD)
-     PG_KEYWORD("elseif", K_ELSIF, RESERVED_KEYWORD)
-     PG_KEYWORD("elsif", K_ELSIF, RESERVED_KEYWORD)
      PG_KEYWORD("end", K_END, RESERVED_KEYWORD)
-     PG_KEYWORD("exception", K_EXCEPTION, RESERVED_KEYWORD)
      PG_KEYWORD("execute", K_EXECUTE, RESERVED_KEYWORD)
-     PG_KEYWORD("exit", K_EXIT, RESERVED_KEYWORD)
-     PG_KEYWORD("fetch", K_FETCH, RESERVED_KEYWORD)
      PG_KEYWORD("for", K_FOR, RESERVED_KEYWORD)
      PG_KEYWORD("foreach", K_FOREACH, RESERVED_KEYWORD)
      PG_KEYWORD("from", K_FROM, RESERVED_KEYWORD)
-     PG_KEYWORD("get", K_GET, RESERVED_KEYWORD)
      PG_KEYWORD("if", K_IF, RESERVED_KEYWORD)
      PG_KEYWORD("in", K_IN, RESERVED_KEYWORD)
-     PG_KEYWORD("insert", K_INSERT, RESERVED_KEYWORD)
      PG_KEYWORD("into", K_INTO, RESERVED_KEYWORD)
      PG_KEYWORD("loop", K_LOOP, RESERVED_KEYWORD)
-     PG_KEYWORD("move", K_MOVE, RESERVED_KEYWORD)
      PG_KEYWORD("not", K_NOT, RESERVED_KEYWORD)
      PG_KEYWORD("null", K_NULL, RESERVED_KEYWORD)
-     PG_KEYWORD("open", K_OPEN, RESERVED_KEYWORD)
      PG_KEYWORD("or", K_OR, RESERVED_KEYWORD)
-     PG_KEYWORD("perform", K_PERFORM, RESERVED_KEYWORD)
-     PG_KEYWORD("raise", K_RAISE, RESERVED_KEYWORD)
-     PG_KEYWORD("return", K_RETURN, RESERVED_KEYWORD)
      PG_KEYWORD("strict", K_STRICT, RESERVED_KEYWORD)
      PG_KEYWORD("then", K_THEN, RESERVED_KEYWORD)
      PG_KEYWORD("to", K_TO, RESERVED_KEYWORD)
--- 70,89 ----
*************** static const ScanKeyword unreserved_keyw
*** 109,140 ****
--- 99,145 ----
      PG_KEYWORD("alias", K_ALIAS, UNRESERVED_KEYWORD)
      PG_KEYWORD("array", K_ARRAY, UNRESERVED_KEYWORD)
      PG_KEYWORD("backward", K_BACKWARD, UNRESERVED_KEYWORD)
+     PG_KEYWORD("close", K_CLOSE, UNRESERVED_KEYWORD)
+     PG_KEYWORD("collate", K_COLLATE, UNRESERVED_KEYWORD)
      PG_KEYWORD("column", K_COLUMN, UNRESERVED_KEYWORD)
      PG_KEYWORD("column_name", K_COLUMN_NAME, UNRESERVED_KEYWORD)
      PG_KEYWORD("constant", K_CONSTANT, UNRESERVED_KEYWORD)
      PG_KEYWORD("constraint", K_CONSTRAINT, UNRESERVED_KEYWORD)
      PG_KEYWORD("constraint_name", K_CONSTRAINT_NAME, UNRESERVED_KEYWORD)
+     PG_KEYWORD("continue", K_CONTINUE, UNRESERVED_KEYWORD)
      PG_KEYWORD("current", K_CURRENT, UNRESERVED_KEYWORD)
      PG_KEYWORD("cursor", K_CURSOR, UNRESERVED_KEYWORD)
      PG_KEYWORD("datatype", K_DATATYPE, UNRESERVED_KEYWORD)
      PG_KEYWORD("debug", K_DEBUG, UNRESERVED_KEYWORD)
+     PG_KEYWORD("default", K_DEFAULT, UNRESERVED_KEYWORD)
      PG_KEYWORD("detail", K_DETAIL, UNRESERVED_KEYWORD)
+     PG_KEYWORD("diagnostics", K_DIAGNOSTICS, UNRESERVED_KEYWORD)
      PG_KEYWORD("dump", K_DUMP, UNRESERVED_KEYWORD)
+     PG_KEYWORD("elseif", K_ELSIF, UNRESERVED_KEYWORD)
+     PG_KEYWORD("elsif", K_ELSIF, UNRESERVED_KEYWORD)
      PG_KEYWORD("errcode", K_ERRCODE, UNRESERVED_KEYWORD)
      PG_KEYWORD("error", K_ERROR, UNRESERVED_KEYWORD)
+     PG_KEYWORD("exception", K_EXCEPTION, UNRESERVED_KEYWORD)
+     PG_KEYWORD("exit", K_EXIT, UNRESERVED_KEYWORD)
+     PG_KEYWORD("fetch", K_FETCH, UNRESERVED_KEYWORD)
      PG_KEYWORD("first", K_FIRST, UNRESERVED_KEYWORD)
      PG_KEYWORD("forward", K_FORWARD, UNRESERVED_KEYWORD)
+     PG_KEYWORD("get", K_GET, UNRESERVED_KEYWORD)
      PG_KEYWORD("hint", K_HINT, UNRESERVED_KEYWORD)
      PG_KEYWORD("info", K_INFO, UNRESERVED_KEYWORD)
+     PG_KEYWORD("insert", K_INSERT, UNRESERVED_KEYWORD)
      PG_KEYWORD("is", K_IS, UNRESERVED_KEYWORD)
      PG_KEYWORD("last", K_LAST, UNRESERVED_KEYWORD)
      PG_KEYWORD("log", K_LOG, UNRESERVED_KEYWORD)
      PG_KEYWORD("message", K_MESSAGE, UNRESERVED_KEYWORD)
      PG_KEYWORD("message_text", K_MESSAGE_TEXT, UNRESERVED_KEYWORD)
+     PG_KEYWORD("move", K_MOVE, UNRESERVED_KEYWORD)
      PG_KEYWORD("next", K_NEXT, UNRESERVED_KEYWORD)
      PG_KEYWORD("no", K_NO, UNRESERVED_KEYWORD)
      PG_KEYWORD("notice", K_NOTICE, UNRESERVED_KEYWORD)
+     PG_KEYWORD("open", K_OPEN, UNRESERVED_KEYWORD)
      PG_KEYWORD("option", K_OPTION, UNRESERVED_KEYWORD)
+     PG_KEYWORD("perform", K_PERFORM, UNRESERVED_KEYWORD)
      PG_KEYWORD("pg_context", K_PG_CONTEXT, UNRESERVED_KEYWORD)
      PG_KEYWORD("pg_datatype_name", K_PG_DATATYPE_NAME, UNRESERVED_KEYWORD)
      PG_KEYWORD("pg_exception_context", K_PG_EXCEPTION_CONTEXT, UNRESERVED_KEYWORD)
*************** static const ScanKeyword unreserved_keyw
*** 143,150 ****
--- 148,157 ----
      PG_KEYWORD("print_strict_params", K_PRINT_STRICT_PARAMS, UNRESERVED_KEYWORD)
      PG_KEYWORD("prior", K_PRIOR, UNRESERVED_KEYWORD)
      PG_KEYWORD("query", K_QUERY, UNRESERVED_KEYWORD)
+     PG_KEYWORD("raise", K_RAISE, UNRESERVED_KEYWORD)
      PG_KEYWORD("relative", K_RELATIVE, UNRESERVED_KEYWORD)
      PG_KEYWORD("result_oid", K_RESULT_OID, UNRESERVED_KEYWORD)
+     PG_KEYWORD("return", K_RETURN, UNRESERVED_KEYWORD)
      PG_KEYWORD("returned_sqlstate", K_RETURNED_SQLSTATE, UNRESERVED_KEYWORD)
      PG_KEYWORD("reverse", K_REVERSE, UNRESERVED_KEYWORD)
      PG_KEYWORD("row_count", K_ROW_COUNT, UNRESERVED_KEYWORD)
*************** static const ScanKeyword unreserved_keyw
*** 166,171 ****
--- 173,191 ----

  static const int num_unreserved_keywords = lengthof(unreserved_keywords);

+ /*
+  * This macro must recognize all tokens that can immediately precede a
+  * PL/pgSQL executable statement (that is, proc_sect or proc_stmt in the
+  * grammar).  Fortunately, there are not very many, so hard-coding in this
+  * fashion seems sufficient.
+  */
+ #define AT_STMT_START(prev_token) \
+     ((prev_token) == ';' || \
+      (prev_token) == K_BEGIN || \
+      (prev_token) == K_THEN || \
+      (prev_token) == K_ELSE || \
+      (prev_token) == K_LOOP)
+

  /* Auxiliary data about a token (other than the token type) */
  typedef struct
*************** static const char *scanorig;
*** 192,197 ****
--- 212,220 ----
  /* Current token's length (corresponds to plpgsql_yylval and plpgsql_yylloc) */
  static int    plpgsql_yyleng;

+ /* Current token's code (corresponds to plpgsql_yylval and plpgsql_yylloc) */
+ static int    plpgsql_yytoken;
+
  /* Token pushback stack */
  #define MAX_PUSHBACKS 4

*************** plpgsql_yylex(void)
*** 315,345 ****
          {
              /* not A.B, so just process A */
              push_back_token(tok2, &aux2);
!             if (plpgsql_parse_word(aux1.lval.str,
!                                    core_yy.scanbuf + aux1.lloc,
!                                    &aux1.lval.wdatum,
!                                    &aux1.lval.word))
!                 tok1 = T_DATUM;
!             else if (!aux1.lval.word.quoted &&
!                      (kw = ScanKeywordLookup(aux1.lval.word.ident,
!                                              unreserved_keywords,
!                                              num_unreserved_keywords)))
              {
!                 aux1.lval.keyword = kw->name;
!                 tok1 = kw->value;
              }
              else
!                 tok1 = T_WORD;
          }
      }
      else
      {
!         /* Not a potential plpgsql variable name, just return the data */
      }

      plpgsql_yylval = aux1.lval;
      plpgsql_yylloc = aux1.lloc;
      plpgsql_yyleng = aux1.leng;
      return tok1;
  }

--- 338,412 ----
          {
              /* not A.B, so just process A */
              push_back_token(tok2, &aux2);
!
!             /*
!              * If we are at start of statement, prefer unreserved keywords
!              * over variable names, unless the next token is assignment or
!              * '[', in which case prefer variable names.  (Note we need not
!              * consider '.' as the next token; that case was handled above,
!              * and we always prefer variable names in that case.)  If we are
!              * not at start of statement, always prefer variable names over
!              * unreserved keywords.
!              */
!             if (AT_STMT_START(plpgsql_yytoken) &&
!                 !(tok2 == '=' || tok2 == COLON_EQUALS || tok2 == '['))
              {
!                 /* try for unreserved keyword, then for variable name */
!                 if (core_yy.scanbuf[aux1.lloc] != '"' &&
!                     (kw = ScanKeywordLookup(aux1.lval.str,
!                                             unreserved_keywords,
!                                             num_unreserved_keywords)))
!                 {
!                     aux1.lval.keyword = kw->name;
!                     tok1 = kw->value;
!                 }
!                 else if (plpgsql_parse_word(aux1.lval.str,
!                                             core_yy.scanbuf + aux1.lloc,
!                                             &aux1.lval.wdatum,
!                                             &aux1.lval.word))
!                     tok1 = T_DATUM;
!                 else
!                     tok1 = T_WORD;
              }
              else
!             {
!                 /* try for variable name, then for unreserved keyword */
!                 if (plpgsql_parse_word(aux1.lval.str,
!                                        core_yy.scanbuf + aux1.lloc,
!                                        &aux1.lval.wdatum,
!                                        &aux1.lval.word))
!                     tok1 = T_DATUM;
!                 else if (!aux1.lval.word.quoted &&
!                          (kw = ScanKeywordLookup(aux1.lval.word.ident,
!                                                  unreserved_keywords,
!                                                  num_unreserved_keywords)))
!                 {
!                     aux1.lval.keyword = kw->name;
!                     tok1 = kw->value;
!                 }
!                 else
!                     tok1 = T_WORD;
!             }
          }
      }
      else
      {
!         /*
!          * Not a potential plpgsql variable name, just return the data.
!          *
!          * Note that we also come through here if the grammar pushed back a
!          * T_DATUM, T_CWORD, T_WORD, or unreserved-keyword token returned by a
!          * previous lookup cycle; thus, pushbacks do not incur extra lookup
!          * work, since we'll never do the above code twice for the same token.
!          * This property also makes it safe to rely on the old value of
!          * plpgsql_yytoken in the is-this-start-of-statement test above.
!          */
      }

      plpgsql_yylval = aux1.lval;
      plpgsql_yylloc = aux1.lloc;
      plpgsql_yyleng = aux1.leng;
+     plpgsql_yytoken = tok1;
      return tok1;
  }

*************** plpgsql_scanner_init(const char *str)
*** 645,650 ****
--- 712,718 ----

      /* Other setup */
      plpgsql_IdentifierLookup = IDENTIFIER_LOOKUP_NORMAL;
+     plpgsql_yytoken = 0;

      num_pushbacks = 0;

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 983f1b8..daf3447 100644
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** select unreserved_test();
*** 4906,4911 ****
--- 4906,4925 ----
                42
  (1 row)

+ create or replace function unreserved_test() returns int as $$
+ declare
+   return int := 42;
+ begin
+   return := return + 1;
+   return return;
+ end
+ $$ language plpgsql;
+ select unreserved_test();
+  unreserved_test
+ -----------------
+               43
+ (1 row)
+
  drop function unreserved_test();
  --
  -- Test FOREACH over arrays
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 2abcbc8..a0840c9 100644
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
*************** $$ language plpgsql;
*** 3940,3945 ****
--- 3940,3956 ----

  select unreserved_test();

+ create or replace function unreserved_test() returns int as $$
+ declare
+   return int := 42;
+ begin
+   return := return + 1;
+   return return;
+ end
+ $$ language plpgsql;
+
+ select unreserved_test();
+
  drop function unreserved_test();

  --

On Sun, Nov 23, 2014 at 4:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> The core of that complaint is that we'd have to make ASSERT a plpgsql
>> reserved word, which is true enough as things stand today.  However,
>> why is it that plpgsql statement-introducing keywords need to be
>> reserved?  The only reason for that AFAICS is to allow us to distinguish
>> the statements from assignments.  But it seems like that could possibly
>> be gotten around with some work.
>
> Attached is a draft patch that de-reserves 17 of plpgsql's existing
> reserved words, leaving 24 still reserved (of which only 9 are not also
> reserved in the core grammar).  This would make it painless to invent an
> ASSERT statement, as well as any other new statement type that's not
> associated with looping or block structure.  (The limiting factor on those
> is that if a statement could have an opt_block_label, its keyword still
> has to be reserved, unless we want to complicate matters a bunch more.)

I like the idea of making these keywords less-reserved, but I'm
wondering how future-proof it is.  It seems to rely heavily on the
fact that the syntax for lvalues is extremely restricted.  Allowing
foo(bar) as an lvalue, for example, would pretty much require
completely reverting this, AFAICS, as would any other type of lvalue
that needs more than one token of lookahead to identify.  How sure are
we that we're never going to want to do something like that?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Nov 23, 2014 at 4:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached is a draft patch that de-reserves 17 of plpgsql's existing
>> reserved words, leaving 24 still reserved (of which only 9 are not also
>> reserved in the core grammar).  This would make it painless to invent an
>> ASSERT statement, as well as any other new statement type that's not
>> associated with looping or block structure.  (The limiting factor on those
>> is that if a statement could have an opt_block_label, its keyword still
>> has to be reserved, unless we want to complicate matters a bunch more.)

> I like the idea of making these keywords less-reserved, but I'm
> wondering how future-proof it is.  It seems to rely heavily on the
> fact that the syntax for lvalues is extremely restricted.  Allowing
> foo(bar) as an lvalue, for example, would pretty much require
> completely reverting this, AFAICS, as would any other type of lvalue
> that needs more than one token of lookahead to identify.  How sure are
> we that we're never going to want to do something like that?

Two comments on that:

1. What is the likely use-case for such a thing (considering SQL is not
exactly friendly to pointers or reference types), and is it more
interesting than new statements that we are going to reject on the grounds
that we don't want to reserve any more plpgsql keywords?

2. The actual behavior would be the same as it would be for the case of
implicit-CALL that we discussed upthread.  Namely, that it'd work just
fine as long as "foo" isn't any unreserved keyword.  So the net effect
would be that plpgsql keywords would be sort of semi-reserved, much like
col_name_keywords in the core grammar: you can use 'em as variable names
but not necessarily as function names.  With the current behavior where
they're fully reserved, you can't use them as either, not even where the
context is completely unambiguous.  I have not heard anyone complaining
that col_name_keyword is a dumb idea and we should make all keywords fully
reserved.
        regards, tom lane

On Mon, Nov 24, 2014 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Two comments on that:
>
> 1. What is the likely use-case for such a thing (considering SQL is not
> exactly friendly to pointers or reference types),

I happen to know that Oracle supports more possible LHS syntaxes in
PL/SQL than we do in PL/pgsql, including things like foo(1) := 3.
There is more than one problem with supporting that syntax in
PL/pgSQL, and I haven't heard anyone complaining about its absence.
But it doesn't have to be that thing particularly: anything that even
vaguely resembles a general expression syntax on the LHS is going to
run into this.

> and is it more
> interesting than new statements that we are going to reject on the grounds
> that we don't want to reserve any more plpgsql keywords?

Probably not, but my crystal ball isn't working too well today.

> 2. The actual behavior would be the same as it would be for the case of
> implicit-CALL that we discussed upthread.  Namely, that it'd work just
> fine as long as "foo" isn't any unreserved keyword.  So the net effect
> would be that plpgsql keywords would be sort of semi-reserved, much like
> col_name_keywords in the core grammar: you can use 'em as variable names
> but not necessarily as function names.  With the current behavior where
> they're fully reserved, you can't use them as either, not even where the
> context is completely unambiguous.  I have not heard anyone complaining
> that col_name_keyword is a dumb idea and we should make all keywords fully
> reserved.

I see.  Well, that sounds fine, then.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/26/14 8:55 AM, Pavel Stehule wrote:
> * should be assertions globally enabled/disabled? - I have no personal
> preference in this question.

I think so.  The way I would use this function is to put expensive 
checks into strategic locations which would only run when developing 
locally (and additionally perhaps in one of the test environments.)  And 
in that case I'd like to globally disable them for the live environment.

> * can be ASSERT exception handled ? - I prefer this be unhandled exception
> - like query_canceled because It should not be masked by plpgsql EXCEPTION
> WHEN OTHERS ...

I don't care much either way, as long as we get good information about 
what went wrong.  A stack trace and hopefully something like 
print_strict_params for parameters to the "expr".


.marko

Pavel Stehule wrote:
> Hi
> 
> any comments to last proposal and patch?

WTH is that pstrdup("hint is null") thing?  Debugging leftover?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/21/15 3:10 PM, Pavel Stehule wrote:
>
> is there some agreement on this behave of ASSERT statement?
>
> I would to assign last patch to next commitfest.
>
> Possible changes:
>
> * I would to simplify a behave of evaluating of message expression - probably I disallow NULL there.

Well, the only thing I could see you doing there is throwing a different error if the hint is null. I don't see that as
animprovement. I'd just leave it as-is.
 

> * GUC enable_asserts will be supported

That would be good. Would that allow for enabling/disabling on a per-function basis too?

> * a assert exception should not be handled by PLpgSQL handler -- like CANCEL exception

+1
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 1/22/15 2:01 PM, Pavel Stehule wrote:
>
>         * I would to simplify a behave of evaluating of message expression - probably I disallow NULL there.
>
>
>     Well, the only thing I could see you doing there is throwing a different error if the hint is null. I don't see
thatas an improvement. I'd just leave it as-is.
 
>
>
> I enabled a NULL - but enforced a WARNING before.

I don't see the separate warning as being helpful. I'd just do something like

+                 (err_hint != NULL) ? errhint("%s", err_hint) : errhint("Message attached to failed assertion is
null")));
 

There should also be a test case for a NULL message.

>         * GUC enable_asserts will be supported
>
>
>     That would be good. Would that allow for enabling/disabling on a per-function basis too?
>
>
> sure - there is only question if we develop a #option enable|disable_asserts. I have no string idea.

The option would be nice, but I don't think it's strictly necessary. The big thing is being able to control this on a
per-functionbasis (which I think you can do with ALTER FUNCTION SET?)
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 1/27/15 1:30 PM, Pavel Stehule wrote:
>     I don't see the separate warning as being helpful. I'd just do something like
>
>     +                                (err_hint != NULL) ? errhint("%s", err_hint) : errhint("Message attached to
failedassertion is null") ));
 
>
>
> done
>
>
>     There should also be a test case for a NULL message.
>
>
> is there, if I understand well

I see it now. Looks good.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Note that pgcrypto is failing 3 tests, same as in master.

The new status of this patch is: Ready for Committer

Pavel Stehule <pavel.stehule@gmail.com> writes:
> updated version with Jim Nasby's doc and rebase against last changes in
> plpgsql.

I started looking at this patch.  ISTM there are some pretty questionable
design decisions in it:

1. Why create a core GUC to control a behavior that's plpgsql-only?
I think it'd make more sense for this to be a plgsql custom GUC
(ie, "plpgsql.enable_asserts" or some such name).

2. I find the use of errdetail to report whether the assertion condition
evaluated to FALSE or to NULL to be pretty useless.  (BTW, is considering
NULL to be a failure the right thing?  SQL CHECK conditions consider NULL
to be allowed ...)  I also don't care at all for reporting the internal
text of the assertion expression in the errdetail: that will expose
implementation details (ie, exactly what does plpgsql convert the user
expression to, does it remove comments, etc) which we will then be
constrained from changing for fear of breaking client code that expects a
particular spelling of the condition.  I think we should just drop that
whole business.  The user can report the condition in her message, if she
feels the need to.

3. If we drop the errdetail as per #2, then reporting the optional
user-supplied string as a HINT would be just plain bizarre; not that it
wasn't bizarre already, because there's no good reason to suppose that
whatever the programmer has to say about the assertion is merely a hint.
I also find the behavior for string-evaluates-to-NULL bizarre; it'd be
saner just to leave out the message field, same as if the argument weren't
there.  I would suggest that we adopt one of these two definitions for the
optional string:

3a. If string is present and not null, use it as the primary message text
(otherwise use "assertion failed").

3b. If string is present and not null, use it as errdetail, with the
primary message text always being "assertion failed".

I mildly prefer #3a, but could be talked into #3b.

Comments?
        regards, tom lane

On 3/25/15 1:21 AM, Pavel Stehule wrote:
> 2015-03-25 0:17 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>>:
>
>     Pavel Stehule <pavel.stehule@gmail.com
>     <mailto:pavel.stehule@gmail.com>> writes:
>     > updated version with Jim Nasby's doc and rebase against last changes in
>     > plpgsql.
>
>     I started looking at this patch.  ISTM there are some pretty
>     questionable
>     design decisions in it:
>
>     1. Why create a core GUC to control a behavior that's plpgsql-only?
>     I think it'd make more sense for this to be a plgsql custom GUC
>     (ie, "plpgsql.enable_asserts" or some such name).
>
>
> This type of assertations can be implemented in any PL language - so I
> prefer global setting. But I have not strong option in this case - this
> is question about granularity - and more ways are valid.

+1

>     2. I find the use of errdetail to report whether the assertion condition
>     evaluated to FALSE or to NULL to be pretty useless.  (BTW, is
>     considering
>     NULL to be a failure the right thing?  SQL CHECK conditions consider
>     NULL
>     to be allowed ...)
>
>
> This is a question - I am happy with SQL CHECK for data, but I am not
> sure if same behave is safe for plpgsql (procedural) assert. More
> stricter behave is safer  - and some bugs in procedures are based on
> unhandled NULLs in variables. So in this topic I prefer implemented
> behave. It is some like:

+1. I think POLA here is that an assert must be true and only true to be 
valid. If someone was unhappy with that they could always coalesce(..., 
true).
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 3/25/15 1:21 AM, Pavel Stehule wrote:
>> 2015-03-25 0:17 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us
>> <mailto:tgl@sss.pgh.pa.us>>:
>>> (BTW, is considering
>>> NULL to be a failure the right thing?  SQL CHECK conditions consider
>>> NULL to be allowed ...)

>> This is a question - I am happy with SQL CHECK for data, but I am not
>> sure if same behave is safe for plpgsql (procedural) assert. More
>> stricter behave is safer  - and some bugs in procedures are based on
>> unhandled NULLs in variables. So in this topic I prefer implemented
>> behave. It is some like:

> +1. I think POLA here is that an assert must be true and only true to be 
> valid. If someone was unhappy with that they could always coalesce(..., 
> true).

Fair enough.  Committed with the other changes.
        regards, tom lane