pgsql: Add foreign data wrapper error code values for SQL/MED.

Robert Haas <rhaas@postgresql.org> writes:

Add foreign data wrapper error code values for SQL/MED.
Extracted from a much larger patch by Shigeru Hanada.

This patch is quite incomplete. Any patch that adds to errcodes.h
*must* also touch
doc/src/sgml/errcodes.sgml
src/pl/plpgsql/src/plerrcodes.h

regards, tom lane

On Sat, Dec 25, 2010 at 3:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <rhaas@postgresql.org> writes:

Add foreign data wrapper error code values for SQL/MED.
Extracted from a much larger patch by Shigeru Hanada.

This patch is quite incomplete.  Any patch that adds to errcodes.h
*must* also touch
doc/src/sgml/errcodes.sgml
src/pl/plpgsql/src/plerrcodes.h

Drat. OK, will work on it tomorrow. I didn't realize those places
needed to be updated.

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

On 26/12/10 05:55, Robert Haas wrote:

On Sat, Dec 25, 2010 at 3:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <rhaas@postgresql.org> writes:

Add foreign data wrapper error code values for SQL/MED.
Extracted from a much larger patch by Shigeru Hanada.

This patch is quite incomplete. Any patch that adds to errcodes.h
*must* also touch
doc/src/sgml/errcodes.sgml
src/pl/plpgsql/src/plerrcodes.h

Drat. OK, will work on it tomorrow. I didn't realize those places
needed to be updated.

I noticed the other day that plerrcodes.h has a comment saying is should
be generated with some sed hackery, and as I needed to generate a Python
exception class for each error in errcodes.h I did the hackery.

See
https://github.com/wulczer/postgres/commit/44fc42b7708f23483156e4e0e1e321e68b2a7e2d#diff-0
for something that maybe could be used as a basis to autogenerate
errcodes.sgml and plerrcodes.h.

Cheers,
Jan

On Sun, Dec 26, 2010 at 8:03 AM, Jan Urbański <wulczer@wulczer.org> wrote:

See
https://github.com/wulczer/postgres/commit/44fc42b7708f23483156e4e0e1e321e68b2a7e2d#diff-0
for something that maybe could be used as a basis to autogenerate
errcodes.sgml and plerrcodes.h.

Interesting. It looks like this might be even easier with perl. For
now I've just written a couple of throwaway scripts to do what I
needed.

Proposed patch attached. This adds what I believe to be the correct
incantations to errcodes.sgml and plerrcodes.h, and also corrects a
typographical error in yesterday's commit which I failed to notice
while reviewing.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Dec 26, 2010 at 8:03 AM, Jan Urbański <wulczer@wulczer.org> wrote:

See
https://github.com/wulczer/postgres/commit/44fc42b7708f23483156e4e0e1e321e68b2a7e2d#diff-0
for something that maybe could be used as a basis to autogenerate
errcodes.sgml and plerrcodes.h.

Interesting. It looks like this might be even easier with perl.

The reason those files aren't autogenerated already is that at the time,
we had a policy of not requiring perl during a build. Now that that
restriction has gone down the drain, it might be worth thinking about.

Proposed patch attached. This adds what I believe to be the correct
incantations to errcodes.sgml and plerrcodes.h, and also corrects a
typographical error in yesterday's commit which I failed to notice
while reviewing.

Hmm. Even with the typo fixed, ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION
seems pretty darn opaque. Is it missing a word? Can we rephrase it so
people will have some idea what it means?

regards, tom lane

On 26/12/10 18:17, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Dec 26, 2010 at 8:03 AM, Jan Urbański <wulczer@wulczer.org> wrote:

See
https://github.com/wulczer/postgres/commit/44fc42b7708f23483156e4e0e1e321e68b2a7e2d#diff-0
for something that maybe could be used as a basis to autogenerate
errcodes.sgml and plerrcodes.h.

Interesting. It looks like this might be even easier with perl.

The reason those files aren't autogenerated already is that at the time,
we had a policy of not requiring perl during a build. Now that that
restriction has gone down the drain, it might be worth thinking about.

I took a crack at doing that for PL/Python and came up with this piece
of Perl:

https://github.com/wulczer/postgres/blob/616d08178519b61ab5446aa63277e9d7e4841d60/src/pl/plpython/generate-spiexceptions.pl

I then tried to do the same for plerrcodes.h, but found discrepancies.
For instance:

* errcodes.h defines ERRCODE_L_E_INVALID_SPECIFICATION and plerrcodes.h
has "invalid_locator_specification".

* ERRCODE_INVALID_ARGUMENT_FOR_LOG becomes "invalid_argument_for_logarithm"

* ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT is
"function_executed_no_return_statement"

* ... and quite some more like this

Additionally, there's one error missing from plerrcodes.h -
ERRCODE_NONSTANDARD_USE_OF_ESCAPE_CHARACTER does not appear there.

Should we a) forget about autogenerating plerrcodes.h and errcodes.sgml
or b) hardcode some exceptions in the Perl script? Changing condition
names would be a big backwards-compat no-no, I presume.

Cheers,
Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

I then tried to do the same for plerrcodes.h, but found discrepancies.

You shouldn't assume that errcodes.h necessarily includes all the info
needed to create the other two files.

The way I'd envision this working is that we have a master file
containing the five-letter SQLSTATE code, the ERRCODE macro name,
the string name to use in plerrcodes.h, and any other required items,
and then generate all three of the existing files from that.

regards, tom lane

On 26/12/10 20:33, Tom Lane wrote:

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

I then tried to do the same for plerrcodes.h, but found discrepancies.

You shouldn't assume that errcodes.h necessarily includes all the info
needed to create the other two files.

The way I'd envision this working is that we have a master file
containing the five-letter SQLSTATE code, the ERRCODE macro name,
the string name to use in plerrcodes.h, and any other required items,
and then generate all three of the existing files from that.

How about a format like this then:

# Comment
Section: Class 2F - SQL Routine Exception
macro_name sqlstate plpgsqlname is_error

That is: # and blank lines are comments, lines starting with "Section:"
are section names (needed for SGML output), the rest are whitespace
separated strings. is_error is 0 or 1, if it's 0 we don't generate a
plpgsql condition for it.

If we go for that format, we'll end up with one new plpgsql error
(nonstantard_use_of_escape_character) that was omitted from previous
releases.

Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

How about a format like this then:

# Comment
Section: Class 2F - SQL Routine Exception
macro_name sqlstate plpgsqlname is_error

That is: # and blank lines are comments, lines starting with "Section:"
are section names (needed for SGML output), the rest are whitespace
separated strings. is_error is 0 or 1, if it's 0 we don't generate a
plpgsql condition for it.

Or just leave out the plpgsqlname if we don't want a condition to be
generated?

Things might line up nicer if the sqlstate is the first column.

regards, tom lane

On 26/12/10 20:57, Tom Lane wrote:

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

How about a format like this then:

# Comment
Section: Class 2F - SQL Routine Exception
macro_name sqlstate plpgsqlname is_error

That is: # and blank lines are comments, lines starting with "Section:"
are section names (needed for SGML output), the rest are whitespace
separated strings. is_error is 0 or 1, if it's 0 we don't generate a
plpgsql condition for it.

Or just leave out the plpgsqlname if we don't want a condition to be
generated?

Makes sense. Wait, no, errcodes.sgml includes the entries for success
and warnings, but the plpgsql conditions list does not. So we need a
separate column to differentiate.

Things might line up nicer if the sqlstate is the first column.

I was thinking of separating the values with tabs anyway, but I'm fine
with putting sqlstate first.

Cheers,
Jan

On Dec 26, 2010, at 3:09 PM, Jan Urbański <wulczer@wulczer.org> wrote:

Things might line up nicer if the sqlstate is the first column.

I was thinking of separating the values with tabs anyway, but I'm fine
with putting sqlstate first.

+1 for putting SQLSTATE first.

...Robert

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

Makes sense. Wait, no, errcodes.sgml includes the entries for success
and warnings, but the plpgsql conditions list does not. So we need a
separate column to differentiate.

OK. But not 0/1 please. Maybe 'E', 'W', or 'S' ? And again, fixed
width columns first, so something like

sqlstate E/W/S errcode_macro_name plpgsql_condition_name

where I guess we could still make the plpgsql condition name optional.

regards, tom lane

On Sun, Dec 26, 2010 at 12:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Proposed patch attached.  This adds what I believe to be the correct
incantations to errcodes.sgml and plerrcodes.h, and also corrects a
typographical error in yesterday's commit which I failed to notice
while reviewing.

Hmm.  Even with the typo fixed, ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION
seems pretty darn opaque.  Is it missing a word?  Can we rephrase it so
people will have some idea what it means?

(looks at a draft copy of the spec)

Appears to be straight out of the spec. There's also some
incomprehensible (to me) language explaining what it's supposed to
mean. I think they're using the term "FDW execution" to mean
something along the lines of "an instance of grabbing data via an
FDW". Or something sort of vaguely like that. If I didn't know
better I'd think this had been deliberately obfuscated.

At any rate, I think rewording language straight out of the spec is
likely a bad plan.

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

On 26/12/10 21:17, Tom Lane wrote:

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

Makes sense. Wait, no, errcodes.sgml includes the entries for success
and warnings, but the plpgsql conditions list does not. So we need a
separate column to differentiate.

OK. But not 0/1 please. Maybe 'E', 'W', or 'S' ? And again, fixed
width columns first, so something like

sqlstate E/W/S errcode_macro_name plpgsql_condition_name

where I guess we could still make the plpgsql condition name optional.

All right, E/W/S sounds good. I'm actually faulty of a misnomer by
calling the field plpgsql_condition_name. It's more like spec_name, and
it will be used to generate plpgsql condition names for E entries and
rows in errcodes.sgml for all entries.

Remember that there will also be Section: lines there, because
errcodes.sgml needs to know where particular the error classes start and
end.

So:

sqlstate E/W/S errcode_macro_name spec_name

where spec_name is lowercase and underscore-separated.

errcodes.h would be generated by splitting sqlstate into letters and
emitting `#define errcode_macro_name MAKE_SQLSTATE('x','x','x','x','x')'

plerrcodes.h would be generated by emitting `{ "spec_name",
errcode_macro_name },' for each E line

errcodes.sgml would be generated by emitting a <row/> element with
sqlstate, spec_name in uppercase and with underscores stripped and just
spec_name. The Section: lines would generate table headers.

... and spiexceptions.h in plpython would be "spec_name" with
underscores converted to camel case and errcode_macro_name

How does that sound?

Jan

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Dec 26, 2010 at 12:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm. �Even with the typo fixed, ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION
seems pretty darn opaque. �Is it missing a word? �Can we rephrase it so
people will have some idea what it means?

Appears to be straight out of the spec.

Oh, okay.

At any rate, I think rewording language straight out of the spec is
likely a bad plan.

Agreed.

regards, tom lane

On Sun, Dec 26, 2010 at 4:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Dec 26, 2010 at 12:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm.  Even with the typo fixed, ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION
seems pretty darn opaque.  Is it missing a word?  Can we rephrase it so
people will have some idea what it means?

Appears to be straight out of the spec.

Oh, okay.

At any rate, I think rewording language straight out of the spec is
likely a bad plan.

Agreed.

OK, committed.

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

On 26/12/10 21:33, Jan Urbański wrote:

On 26/12/10 21:17, Tom Lane wrote:

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

Makes sense. Wait, no, errcodes.sgml includes the entries for success
and warnings, but the plpgsql conditions list does not. So we need a
separate column to differentiate.

OK. But not 0/1 please. Maybe 'E', 'W', or 'S' ? And again, fixed
width columns first, so something like

sqlstate E/W/S errcode_macro_name plpgsql_condition_name

where I guess we could still make the plpgsql condition name optional.

All right, E/W/S sounds good. I'm actually faulty of a misnomer by
calling the field plpgsql_condition_name. It's more like spec_name, and
it will be used to generate plpgsql condition names for E entries and
rows in errcodes.sgml for all entries.

Remember that there will also be Section: lines there, because
errcodes.sgml needs to know where particular the error classes start and
end.

Here's the basic errcodes.txt file and three scripts to generate
errcodes.h, plerrcodes.h and part of errcodes.sgml.

I tried wiring it into the build system, but failed, I can't figure out
which Makefiles should be updated in order to make errcodes.h and
plerrcodes.h generated headers. Could someone help with that?

This will actually remove a few entries from plerrcodes.h, that were
aliases of other entries (like array_element_error which was an alias of
array_subscript_error). Since they did not appear in errcodes.sgml, it
shouldn't be a problem.

It also adds a forgotten entry for nonstandard_use_of_escape_character
in plerrcodes.h.

Cheers,
Jan

On 28/12/10 12:25, Jan Urbański wrote:

Here's the basic errcodes.txt file and three scripts to generate
errcodes.h, plerrcodes.h and part of errcodes.sgml.

I tried wiring it into the build system, but failed, I can't figure out
which Makefiles should be updated in order to make errcodes.h and
plerrcodes.h generated headers. Could someone help with that?

Trying a bit harder to make src/include/utils/errcodes.h a generated
file I found that it's included so early it makes the task not trivial
at all. postgres.h includes elog.h, which includes errcodes.h (after
defining the MAKE_SQLSTATE macro). I'm not sure how to proceed:

1) just give up on it, and keep on maintaining 3 places where new error
codes have to go
2) do not include errcodes.h in elog.h, move the MAKE_SQLSTATE
definition to the top of errcodes.h, guarded by a #ifndef MAKE_SQLSTATE,
fix every place that included elog.h and assumed it has the sqlstate
values to explicitly include errcodes.h. This means that you can still
define your own MAKE_SQLSTATE macro and then include errcodes.h if you
want to redefine what it does.
3) try to wire generating errcodes.h somewhere very early in the makefiles

My preference is 2) followed by 1), because 3) seems too horrible. But I
can understand if people will want to keep things as they are and just
forget about generating errcodes.h

Cheers,
Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:

I tried wiring it into the build system, but failed, I can't figure out
which Makefiles should be updated in order to make errcodes.h and
plerrcodes.h generated headers. Could someone help with that?

Trying a bit harder to make src/include/utils/errcodes.h a generated
file I found that it's included so early it makes the task not trivial
at all. postgres.h includes elog.h, which includes errcodes.h (after
defining the MAKE_SQLSTATE macro). I'm not sure how to proceed:

Huh? Why in the world would the specific location of the #include have
anything to do with the problem?

regards, tom lane