final light versions of Oracle compatibility (SQLSTATE, GREATEST, NEXT_DAY)

Pavel Stehule wrote:

1. SQLSTATE and SQLERRM exists only on exception's block, and allways
carry info about some exception.

Attached is a revised version of this patch. I'll apply it tonight or
tomorrow, barring any objections.

-Neil

Neil Conway <neilc@samurai.com> writes:

Attached is a revised version of this patch. I'll apply it tonight or
tomorrow, barring any objections.

I still find the grammar changes to be an ugly kluge --- it should be
possible to do this without introducing bogus nonterminals.

The ns push/pop operations don't appear to be correctly matched
(consider multiple WHEN clauses, a case the regression test does not
cover), nor do they surround the places where the variables are created.
It is likely that you don't need a push/pop at all; if it appears to
work now it's because the end of the block results in a pop and so
the variables disappear then anyway.

The patch is sloppy about whether free_var() is static or not.

I find the proposed change in plpgsql_ns_additem a distinct
disimprovement --- it's dubious even as a micro-optimization
and it certainly hurts legibility.

regards, tom lane

Also, do we want these features? Do they duplicate anything we already
have?

---------------------------------------------------------------------------

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

Attached is a revised version of this patch. I'll apply it tonight or
tomorrow, barring any objections.

I still find the grammar changes to be an ugly kluge --- it should be
possible to do this without introducing bogus nonterminals.

The ns push/pop operations don't appear to be correctly matched
(consider multiple WHEN clauses, a case the regression test does not
cover), nor do they surround the places where the variables are created.
It is likely that you don't need a push/pop at all; if it appears to
work now it's because the end of the block results in a pop and so
the variables disappear then anyway.

The patch is sloppy about whether free_var() is static or not.

I find the proposed change in plpgsql_ns_additem a distinct
disimprovement --- it's dubious even as a micro-optimization
and it certainly hurts legibility.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Also, do we want these features? Do they duplicate anything we already
have?

This patch only covers SQLSTATE/SQLERRM for plpgsql, which I think we're
now agreed on doing. The other stuff is still open for discussion ---
personally I'm OK with the concept of LEAST and GREATEST, not too
excited about the other things Pavel suggested. (Note I've not looked
at the proposed code for least/greatest yet, it may have issues.)

regards, tom lane

Tom Lane wrote:

I still find the grammar changes to be an ugly kluge --- it should be
possible to do this without introducing bogus nonterminals.

The scope-local variables need to be added to the namespace by the time
that we parse the WHEN clauses. I can see two ways to do that: adding a
bogus non-terminal, or using a mid-rule action. Mid-rule actions are
pretty ugly, though. Is there a better alternative?

The ns push/pop operations don't appear to be correctly matched

Sorry, asleep at the switch -- the ns_push/pop stuff isn't even needed,
it was from an early revision of the patch.

A revised patch is attached -- once the nonterminal stuff is sorted I'll
apply it.

-Neil

Neil Conway <neilc@samurai.com> writes:

Tom Lane wrote:

I still find the grammar changes to be an ugly kluge --- it should be
possible to do this without introducing bogus nonterminals.

The scope-local variables need to be added to the namespace by the time
that we parse the WHEN clauses. I can see two ways to do that: adding a
bogus non-terminal, or using a mid-rule action. Mid-rule actions are
pretty ugly, though. Is there a better alternative?

Right, mid-rule actions were what I had in mind. They're not uglier
than introducing empty nonterminals --- and in fact plpgsql's grammar
already relies on 'em. The cursor variable declaration production
(about line 359 in CVS tip) seems to me to offer a very direct parallel
to what we want to do here.

regards, tom lane

Tom Lane wrote:

Right, mid-rule actions were what I had in mind. They're not uglier
than introducing empty nonterminals

Well, IMHO they make the grammar rather hard to read when the action has
multiple lines (we would need at least 6 lines of code in the mid-rule
action, I believe). Unless we want two contiguous mid-rule actions
(which is even _less_ readable), we'll need to futz with adding another
member to %union to hold the two varnos the mid-rule action will
produce. Considering that the Bison manual suggests that it implements
mid-rule actions by introducing an implicit bogus non-terminal ([1]http://www.gnu.org/software/bison/manual/html_mono/bison.html#Mid_002dRule-Actions, toward the end of the section), I
don't think there is likely to be a difference in performance either
way, and I think mid-rule actions don't offer a notational improvement
in this case.

-Neil

[1]: http://www.gnu.org/software/bison/manual/html_mono/bison.html#Mid_002dRule-Actions, toward the end of the section
http://www.gnu.org/software/bison/manual/html_mono/bison.html#Mid_002dRule-Actions,
toward the end of the section

Neil Conway <neilc@samurai.com> writes:

Considering that the Bison manual suggests that it implements
mid-rule actions by introducing an implicit bogus non-terminal ([1]),

Indeed ... and the reason that they bothered to do that is that mid-rule
actions are more understandable ;-). A nonterminal that is not intended
to represent any real input, ever, is just plain weird.

Unless we want two contiguous mid-rule actions
(which is even _less_ readable), we'll need to futz with adding another
member to %union to hold the two varnos the mid-rule action will
produce.

Not at all. The right way to do this, I think, is for the mid-rule
action to palloc the PLpgSQL_exception_block, fill the variables into
that, and return the block as its semantic value. The end-of-rule
action then picks up the block and adds what it needs to.

One reason this is cleaner is that it scales to more SQLERRx variables
without further renumbering of the rule components.

regards, tom lane

Tom Lane wrote:

A nonterminal that is not intended to represent any real input, ever,
is just plain weird.

If you say so... PL/PgSQL already uses such a beast, though: the lno
nonterminal, for example.

Not at all. The right way to do this, I think, is for the mid-rule
action to palloc the PLpgSQL_exception_block, fill the variables into
that, and return the block as its semantic value. The end-of-rule
action then picks up the block and adds what it needs to.

Ah, I see -- that makes sense. Attached is a revised patch -- applied to
HEAD.

-Neil