Thread: Re: [COMMITTERS] pgsql: Add foreign data wrapper error code values for SQL/MED.

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

Jan Urbański <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:
> Jan Urbański <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

Jan Urbański <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:
> Jan Urbański <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

Jan Urbański <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:
> Jan Urbański <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 go2) 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

Jan Urbański <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

On 11/01/11 17:11, Tom Lane wrote:
> Jan Urbański <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?

I'v having a hard time convincing make to generate errcodes.h before
compiling any .c file that includes postgres.h. The only way I found was
to make src/include/errcodes.h a dependancy of the all target.

For instance, I tried to copy the way we generate fmgroids.h and it
turned out that it doesn't work (it tries to compile things in src/port
before entering src/include, and things in src/port include postgres.h).

Jan Urbański <wulczer@wulczer.org> writes:
> On 11/01/11 17:11, Tom Lane wrote:
>> Huh?  Why in the world would the specific location of the #include have
>> anything to do with the problem?

> I'v having a hard time convincing make to generate errcodes.h before
> compiling any .c file that includes postgres.h. The only way I found was
> to make src/include/errcodes.h a dependancy of the all target.

Peter would probably be a better person than me to answer that, but I
imagine that what you want is similar to what src/backend/Makefile does
for parser/gram.h, only applied at the src/ level or maybe even the
root.
        regards, tom lane

On 11/01/11 18:59, Tom Lane wrote:
> Jan Urbański <wulczer@wulczer.org> writes:
>> On 11/01/11 17:11, Tom Lane wrote:
>>> Huh?  Why in the world would the specific location of the #include have
>>> anything to do with the problem?
> 
>> I'v having a hard time convincing make to generate errcodes.h before
>> compiling any .c file that includes postgres.h. The only way I found was
>> to make src/include/errcodes.h a dependancy of the all target.
> 
> Peter would probably be a better person than me to answer that, but I
> imagine that what you want is similar to what src/backend/Makefile does
> for parser/gram.h, only applied at the src/ level or maybe even the
> root.

OK, that was a nudge in the right direction. Basing the rules from
src/backend/Makefile I changed src/Makefile to rebuild
src/include/errcodes.h before building the subdirectories and... it
failed miserably. There's some trickery beyond my understanding here.
There's a rule like this in src/backend/Makefile:

$(SUBDIRS:%=%-recursive): $(top_builddir)/src/include/parser/gram.h

that triggers checking whether gram.h needs to be rebuilt before
recursing into each SUBDIR.

A similar trick in src/Makefile doesn't work, because it's
src/backend/common.mk that is responsible for the SUBDIR-recursive
calls, and src/Makefile does not include it.

From what I gathered by reading Makefile.global, the $(recurse) call in
enters each SUBDIR and builds a target called <target>-<subdir>-recurse.
And actually, if I change my rule to read:

$(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h

it works. Now whether that's acceptable or not is another thing entirely...

Cheers,
Jan

On 11/01/11 21:21, Jan Urbański wrote:
> On 11/01/11 18:59, Tom Lane wrote:
>> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
>>> On 11/01/11 17:11, Tom Lane wrote:
>> Peter would probably be a better person than me to answer that, but I
>> imagine that what you want is similar to what src/backend/Makefile does
>> for parser/gram.h, only applied at the src/ level or maybe even the
>> root.

> And actually, if I change my rule to read:
>
> $(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h
>
> it works. Now whether that's acceptable or not is another thing entirely...

And so I came up with three patches to make errcodes.h, plerrcodes.h and
errcodes.sgml (respectively) generated files.

The autogenerated files are almost identical with the originals (except
for formatting, sometimes, and comments) except:

 * in errcodes.sgml the Meaning field for INVALID ARGUMENT FOR NTH_VALUE
FUNCTION changed to INVALID ARGUMENT FOR NTH VALUE FUNCTION. Not ideal,
but I wouldn't shed any tears. The Constant field stays the same, and
that's what's important for a PL/pgSQL programmer

 * in errcodes.h there are a few changes like that:

 #define ERRCODE_DATETIME_FIELD_OVERFLOW        MAKE_SQLSTATE('2','2',
'0','0','8')
-#define ERRCODE_DATETIME_VALUE_OUT_OF_RANGE ERRCODE_DATETIME_FIELD_OVERFLOW
+#define ERRCODE_DATETIME_VALUE_OUT_OF_RANGE
MAKE_SQLSTATE('2','2','0','0','8')
 #define ERRCODE_DIVISION_BY_ZERO           MAKE_SQLSTATE('2','2',
'0','1','2')

unless your MAKE_SQLSTATE macro has side-effects, it should not be a problem

 * in plerrcodes.h a few entries disappeard, and that's because they had
duplicated SQLSTATE values. As they were not documented in
errcodes.sgml, no one should even know they existed, and as they catch
the same SQLSTATE, again it shouldn't be an issue.

As a bonus, a documented but forgotten exception got added. Try this in
HEAD:

do $$ begin begin exception when nonstandard_use_of_escape_character
then null; end; end$$;

It will fail.

Cheers,
Jan

On 12/01/11 19:57, Jan Urbański wrote:
> On 11/01/11 21:21, Jan Urbański wrote:
>> On 11/01/11 18:59, Tom Lane wrote:
>>> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
>>>> On 11/01/11 17:11, Tom Lane wrote:
>>> Peter would probably be a better person than me to answer that, but I
>>> imagine that what you want is similar to what src/backend/Makefile does
>>> for parser/gram.h, only applied at the src/ level or maybe even the
>>> root.
>
>> And actually, if I change my rule to read:
>>
>> $(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h
>>
>> it works. Now whether that's acceptable or not is another thing entirely...
>
> And so I came up with three patches to make errcodes.h, plerrcodes.h and
> errcodes.sgml (respectively) generated files.

Darn, forgot about MSVC again. See
http://archives.postgresql.org/message-id/4D2DF996.9000100@wulczer.org
for details on how this works.

Attached are the previous 3 patches and a fourth one that adds MSVC support.

Cheers,
Jan

On Sun, Jan 30, 2011 at 5:23 PM, Jan Urbański <wulczer@wulczer.org> wrote:
> On 30/01/11 23:08, Robert Haas wrote:
>> On Wed, Jan 12, 2011 at 5:10 PM, Jan Urbański <wulczer@wulczer.org> wrote:
>>> On 12/01/11 19:57, Jan Urbański wrote:
>>>> On 11/01/11 21:21, Jan Urbański wrote:
>>>>> On 11/01/11 18:59, Tom Lane wrote:
>>>>>> Jan Urbański <wulczer@wulczer.org> writes:
>>>>>>> On 11/01/11 17:11, Tom Lane wrote:
>>>>>> Peter would probably be a better person than me to answer that, but I
>>>>>> imagine that what you want is similar to what src/backend/Makefile does
>>>>>> for parser/gram.h, only applied at the src/ level or maybe even the
>>>>>> root.
>>>>
>>>>> And actually, if I change my rule to read:
>>>>>
>>>>> $(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h
>>>>>
>>>>> it works. Now whether that's acceptable or not is another thing entirely...
>>>>
>>>> And so I came up with three patches to make errcodes.h, plerrcodes.h and
>>>> errcodes.sgml (respectively) generated files.
>>>
>>> Darn, forgot about MSVC again. See
>>> http://archives.postgresql.org/message-id/4D2DF996.9000100@wulczer.org
>>> for details on how this works.
>>>
>>> Attached are the previous 3 patches and a fourth one that adds MSVC support.
>>
>> I think these look good.  I'm not sure there's any value in stripping
>> the duplicates out of plerrcodes.h, though, even if they were
>> undocumented:
>
> I think that if you don't strip them out, they will get documented (as
> the SGML is generated).
>
> For PL/pgSQL nothing horrible will happen, because if you write
> EXCEPTION WHEN foo, it will look up the "foo" label and then compare the
> exception's SQLSTATE to decide if it should be handled by that block.
> But for PL/Python, the process is reverse: the exception object is
> looked up by looking at the SQLSTATE and then injected into Python

OK.  If there's a reason for doing it this way, I'm OK with it.

I'll mark this one Ready for Committer.

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

On Wed, Jan 12, 2011 at 5:10 PM, Jan Urbański <wulczer@wulczer.org> wrote:
> On 12/01/11 19:57, Jan Urbański wrote:
>> On 11/01/11 21:21, Jan Urbański wrote:
>>> On 11/01/11 18:59, Tom Lane wrote:
>>>> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
>>>>> On 11/01/11 17:11, Tom Lane wrote:
>>>> Peter would probably be a better person than me to answer that, but I
>>>> imagine that what you want is similar to what src/backend/Makefile does
>>>> for parser/gram.h, only applied at the src/ level or maybe even the
>>>> root.
>>
>>> And actually, if I change my rule to read:
>>>
>>> $(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h
>>>
>>> it works. Now whether that's acceptable or not is another thing entirely...
>>
>> And so I came up with three patches to make errcodes.h, plerrcodes.h and
>> errcodes.sgml (respectively) generated files.
>
> Darn, forgot about MSVC again. See
> http://archives.postgresql.org/message-id/4D2DF996.9000100@wulczer.org
> for details on how this works.
>
> Attached are the previous 3 patches and a fourth one that adds MSVC support.

I think these look good.  I'm not sure there's any value in stripping
the duplicates out of plerrcodes.h, though, even if they were
undocumented:

-    "array_element_error", ERRCODE_ARRAY_ELEMENT_ERROR
-    "datetime_value_out_of_range", ERRCODE_DATETIME_VALUE_OUT_OF_RANGE
-    "undefined_cursor", ERRCODE_UNDEFINED_CURSOR
-    "undefined_database", ERRCODE_UNDEFINED_DATABASE
-    "undefined_pstatement", ERRCODE_UNDEFINED_PSTATEMENT
-    "undefined_schema", ERRCODE_UNDEFINED_SCHEMA

I'm attaching a few other proposed adjustments.

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

Robert Haas <robertmhaas@gmail.com> writes:
> I'll mark this one Ready for Committer.

If you don't want to commit it yourself, I'll take it.
        regards, tom lane

On 30/01/11 23:08, Robert Haas wrote:
> On Wed, Jan 12, 2011 at 5:10 PM, Jan Urbański <wulczer@wulczer.org> wrote:
>> On 12/01/11 19:57, Jan Urbański wrote:
>>> On 11/01/11 21:21, Jan Urbański wrote:
>>>> On 11/01/11 18:59, Tom Lane wrote:
>>>>> Jan Urbański <wulczer@wulczer.org> writes:
>>>>>> On 11/01/11 17:11, Tom Lane wrote:
>>>>> Peter would probably be a better person than me to answer that, but I
>>>>> imagine that what you want is similar to what src/backend/Makefile does
>>>>> for parser/gram.h, only applied at the src/ level or maybe even the
>>>>> root.
>>>
>>>> And actually, if I change my rule to read:
>>>>
>>>> $(SUBDIRS:%=all-%-recurse): $(top_builddir)/src/include/utils/errcodes.h
>>>>
>>>> it works. Now whether that's acceptable or not is another thing entirely...
>>>
>>> And so I came up with three patches to make errcodes.h, plerrcodes.h and
>>> errcodes.sgml (respectively) generated files.
>>
>> Darn, forgot about MSVC again. See
>> http://archives.postgresql.org/message-id/4D2DF996.9000100@wulczer.org
>> for details on how this works.
>>
>> Attached are the previous 3 patches and a fourth one that adds MSVC support.
> 
> I think these look good.  I'm not sure there's any value in stripping
> the duplicates out of plerrcodes.h, though, even if they were
> undocumented:

I think that if you don't strip them out, they will get documented (as
the SGML is generated).

For PL/pgSQL nothing horrible will happen, because if you write
EXCEPTION WHEN foo, it will look up the "foo" label and then compare the
exception's SQLSTATE to decide if it should be handled by that block.
But for PL/Python, the process is reverse: the exception object is
looked up by looking at the SQLSTATE and then injected into Python, so
if SQLSTATE 12345 is both foo_error and bar_error, and you write

try:   buggy()
except FooError:   handle_it()

you might be out of luck, if BarError is first on the list, as it will
match the SQLSTATE of the error from buggy() and your except clause will
be useless. AFAICS there's no way to do it another way, short of
dropping the idea of having names for specific exceptions, and forcing
the user to do:

try:   buggy()
except SPIError as e:   if e.sqlstate == plpy.condition_to_sqlstate("foo_error"):       handle_it()   else:
raise

which is ugly. It would be useful to have a 1-1 mapping between
condition names and SQLSTATE codes.

> I'm attaching a few other proposed adjustments.

Right, forgot about .gitignores.

Cheers,
Jan

On Sun, Jan 30, 2011 at 5:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I'll mark this one Ready for Committer.
>
> If you don't want to commit it yourself, I'll take it.

I'm happy to do it.  I would have done it straight off, but thought
I'd give everyone one last chance to kvetch.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jan 30, 2011 at 5:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If you don't want to commit it yourself, I'll take it.

> I'm happy to do it.  I would have done it straight off, but thought
> I'd give everyone one last chance to kvetch.

Just in a quick read-through of the patches, the only things I noticed
were (1) lack of a PGDG copyright notice in errcodes.txt, and (2)
in your proposed change to generate-errcodes.pl:

+    # Omit a comment for each section header
+    if (/^Section:(.*)/) {
+        my $header = $1;
+        $header =~ s/^\s+//;
+        print "\n/* $header */\n";
+        next;
+    }

ITYM "Emit" not "Omit"?
        regards, tom lane

I wrote:
> Just in a quick read-through of the patches, the only things I noticed

Oh, a third thing: the patch places errcodes.txt under src/include,
which does not seem even approximately right.  src/backend/utils
seems like a saner place for it.
        regards, tom lane

On Sun, Jan 30, 2011 at 6:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Just in a quick read-through of the patches, the only things I noticed
>
> Oh, a third thing: the patch places errcodes.txt under src/include,
> which does not seem even approximately right.  src/backend/utils
> seems like a saner place for it.

All right, committed after fixing merge conflicts and, I hope, taking
sensible actions based on your comments.

Oh, shoot, I forgot to add the copyright notice to errcodes.txt.  Let
me fix that...

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