Verbosity of Function Return Type Checks

Volkan YAZICI wrote:

Yesterday I needed to fiddle with PostgreSQL internals to be able to
debug a PL/pgSQL procedure returning a set of records. I attached the
patch I used to increase the verbosity of error messages related with
function return type checks. I'll be appreciated if any developer could
commit this patch (or a similar one) into the core.

I think this is a good idea, but the new error messages need more work.
Have a look at the message style guidelines please,
http://www.postgresql.org/docs/8.3/static/error-style-guide.html

Particularly I think you need to keep the original errmsg() and add the
new messages as errdetail(). (I notice that there's the slight problem
that the error messages are different for the different callers.)

Also, please use context diffs.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, 8 Aug 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

I think this is a good idea, but the new error messages need more work.
Have a look at the message style guidelines please,
http://www.postgresql.org/docs/8.3/static/error-style-guide.html

Right. Done -- I hope.

Particularly I think you need to keep the original errmsg() and add the
new messages as errdetail(). (I notice that there's the slight problem
that the error messages are different for the different callers.)

Done.

Also, please use context diffs.

Done.

Regards.

[Please ignore the previous reply.]

On Fri, 8 Aug 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

I think this is a good idea, but the new error messages need more work.
Have a look at the message style guidelines please,
http://www.postgresql.org/docs/8.3/static/error-style-guide.html

Right. Done -- I hope.

Particularly I think you need to keep the original errmsg() and add the
new messages as errdetail().

Made callers pass related error message as a string parameter, and
appended required details using errdetail().

(I notice that there's the slight problem
that the error messages are different for the different callers.)

Above mentioned change should have addressed this issue too.

Also, please use context diffs.

Done.

Regards.

Volkan YAZICI wrote:

Made callers pass related error message as a string parameter, and
appended required details using errdetail().

Cool, thanks. I had a look and you had some of the expected vs.
returned reversed. This patch should be OK. Amazingly, none of the
regression tests need changing, which is a bit worrisome.

I wasn't able to run the tests in contrib, I don't know why, and I have
to go out now. I'll commit this tomorrow.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

I wasn't able to run the tests in contrib, I don't know why, and I have
to go out now. I'll commit this tomorrow.

This is not ready to go: you've lost the ability to localize most of the
error message strings. Also, "char *msg" should be "const char *msg"
if you're going to pass literal constants to it, and this gives me
the willies even though the passed-in strings are supposedly all fixed:
errmsg(msg),
Use
errmsg("%s", msg),
to be safe.

Actually, the entire concept of varying the main message to suit the
context exactly, while the detail messages are *not* changing, seems
pretty bogus...

Another problem with it is it's likely going to fail completely on
dropped columns (which will have atttypid = 0).

regards, tom lane

On Thu, 4 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

Cool, thanks. I had a look and you had some of the expected vs.
returned reversed.

I'll happy to fix the reversed ones if you can report them in more
details.

Regards.

On Thu, 04 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:

This is not ready to go: you've lost the ability to localize most of
the error message strings.

How can I make this available? What's your suggestion?

Also, "char *msg" should be "const char *msg"

Done.

if you're going to pass literal constants to it, and this gives me the
willies even though the passed-in strings are supposedly all fixed:
errmsg(msg),
Use
errmsg("%s", msg),
to be safe.

Done.

Actually, the entire concept of varying the main message to suit the
context exactly, while the detail messages are *not* changing, seems
pretty bogus...

I share your concerns but couldn't come up with a better approach. I'd
be happy to hear your suggestions.

Another problem with it is it's likely going to fail completely on
dropped columns (which will have atttypid = 0).

Is it ok if I'd replace

if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)

line in validate_tupdesc_compat with

if (td1->attrs[i]->atttypid &&
td2->attrs[i]->atttypid &&
td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)

expression to fix this?

Regards.

Volkan YAZICI wrote:

On Thu, 4 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

Cool, thanks. I had a look and you had some of the expected vs.
returned reversed.

I'll happy to fix the reversed ones if you can report them in more
details.

Please use the patch I posted yesterday, as it had all the issues I
found fixed. There were other changes in that patch too.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Volkan YAZICI <yazicivo@ttmail.com> writes:

On Thu, 04 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:

This is not ready to go: you've lost the ability to localize most of
the error message strings.

How can I make this available? What's your suggestion?

I think the best way is to use

subroutine(..., gettext_noop("special error message here"))

at the call sites, and then

errmsg("%s", _(msg))

when throwing the error. gettext_noop() is needed to have the string
be put into the message catalog, but it doesn't represent any run-time
effort. The _() macro is what actually makes the translation lookup
happen. We don't want to incur that cost in the normal no-error case,
which is why you shouldn't just do _() at the call site and pass an
already-translated string to the subroutine.

Another problem with it is it's likely going to fail completely on
dropped columns (which will have atttypid = 0).

Is it ok if I'd replace

if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)

line in validate_tupdesc_compat with

if (td1->attrs[i]->atttypid &&
td2->attrs[i]->atttypid &&
td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)

expression to fix this?

No, that's weakening the compatibility check. (There's a separate issue
here of teaching plpgsql to actually cope nicely with rowtypes
containing dropped columns, but that's well beyond the scope of this
patch.)

What I'm on about is protecting format_type_be() from being passed
a zero and then failing. Perhaps it would be good enough to do
something like

OidIsValid(td1->attrs[i]->atttypid) ?
format_type_with_typemod(td1->attrs[i]->atttypid,
td1->attrs[i]->atttypmod) :
"dropped column"

while throwing the error.

BTW, since what's actually being looked at is just the type OID and not
the typmod, it seems inappropriate to me to show the typmod in the
error. I'd go with just format_type_be(td1->attrs[i]->atttypid) here
I think.

regards, tom lane

On Fri, 05 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:

I think the best way is to use

subroutine(..., gettext_noop("special error message here"))

at the call sites, and then

errmsg("%s", _(msg))

when throwing the error. gettext_noop() is needed to have the string
be put into the message catalog, but it doesn't represent any run-time
effort. The _() macro is what actually makes the translation lookup
happen. We don't want to incur that cost in the normal no-error case,
which is why you shouldn't just do _() at the call site and pass an
already-translated string to the subroutine.

Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?

if (td1->attrs[i]->atttypid &&
td2->attrs[i]->atttypid &&
td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)

expression to fix this?

No, that's weakening the compatibility check. (There's a separate issue
here of teaching plpgsql to actually cope nicely with rowtypes
containing dropped columns, but that's well beyond the scope of this
patch.)

What I'm on about is protecting format_type_be() from being passed
a zero and then failing. Perhaps it would be good enough to do
something like

OidIsValid(td1->attrs[i]->atttypid) ?
format_type_with_typemod(td1->attrs[i]->atttypid,
td1->attrs[i]->atttypmod) :
"dropped column"

while throwing the error.

BTW, since what's actually being looked at is just the type OID and not
the typmod, it seems inappropriate to me to show the typmod in the
error. I'd go with just format_type_be(td1->attrs[i]->atttypid) here
I think.

Done with format_type_be() usage.

BTW, Alvaro fixed my string concatenations which yielded in lines
exceeding 80 characters width, but I'd want to ask twice if you're sure
with this. Because, IMHO, PostgreSQL is also famous with the quality and
readability of its source code -- that I'm quite proud of as a user,
kudos to developers -- and I think it'd be better to stick with 80
characters width convention as much as one can.

Regards.

Volkan YAZICI wrote:

BTW, Alvaro fixed my string concatenations which yielded in lines
exceeding 80 characters width, but I'd want to ask twice if you're sure
with this. Because, IMHO, PostgreSQL is also famous with the quality and
readability of its source code -- that I'm quite proud of as a user,
kudos to developers -- and I think it'd be better to stick with 80
characters width convention as much as one can.

Yeah, I'm quite anal about that. What will happen is that pgindent will
"push back" the strings so that they start earlier in the line to keep
the 80 char limit. IMHO that's actually clearer than truncating the
string.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Volkan YAZICI wrote:

On Fri, 05 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:

at the call sites, and then

errmsg("%s", _(msg))

when throwing the error.

Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?

Yes, you need _() around those too.

Done with format_type_be() usage.

BTW you forgot to remove the quotes around the type names (I know I told
you to add them but Tom gave the reasons why it's not needed) :-)

Those are minor problems that are easily fixed. However there's a
larger issue that Tom mentioned earlier and I concur, which is that the
caller is forming the primary error message and passing it down. It
gets a bit silly if you consider the ways the messages end up worded:

errmsg("returned record type does not match expected record type"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
---> this is the case where it's OK

errmsg("wrong record type supplied in RETURN NEXT"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
--> this is strange

errmsg("returned tuple structure does not match table of trigger event"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
--> this is not OK

I've been thinking how to pass down the context information without
feeding the complete string, but I don't find a way without doing
message construction. Construction is to be avoided because it's a pain
for translators.

Maybe we should just use something generic like errmsg("mismatching record type")
and have the caller pass two strings specifying what's the "returned"
tuple and what's the "expected", but I don't see how ... (BTW this is
worth fixing, because every case seems to have appeared independently
without much thought as to other callsites with the same pattern.)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, 8 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?

Yes, you need _() around those too.

For this purpose, I introduced a dropped_column_type variable in
validate_tupdesc_compat() function:

const char dropped_column_type[] = _("n/a (dropped column)");

And used this in below fashion:

OidIsValid(returned->attrs[i]->atttypid)
? format_type_be(returned->attrs[i]->atttypid)
: dropped_column_type

Done with format_type_be() usage.

BTW you forgot to remove the quotes around the type names (I know I told
you to add them but Tom gave the reasons why it's not needed) :-)

Done.

Those are minor problems that are easily fixed. However there's a
larger issue that Tom mentioned earlier and I concur, which is that the
caller is forming the primary error message and passing it down. It
gets a bit silly if you consider the ways the messages end up worded:

errmsg("returned record type does not match expected record type"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
---> this is the case where it's OK

errmsg("wrong record type supplied in RETURN NEXT"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
--> this is strange

errmsg("returned tuple structure does not match table of trigger event"));
errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
--> this is not OK

What we're trying to do is to avoid code duplication while checking the
returned tuple types against expected tuple types. For this purpose we
implemented a generic function (validate_tupdesc_compat) to handle all
possible cases. But, IMHO, it's important to remember that there is no
perfect generic function to satisfy all possible cases at best.

I've been thinking how to pass down the context information without
feeding the complete string, but I don't find a way without doing
message construction. Construction is to be avoided because it's a
pain for translators.

Maybe we should just use something generic like errmsg("mismatching
record type") and have the caller pass two strings specifying what's
the "returned" tuple and what's the "expected", but I don't see how
... (BTW this is worth fixing, because every case seems to have
appeared independently without much thought as to other callsites with
the same pattern.)

I considered the subject with identical constraints but couldn't come up
with a more rational solution. Nevertheless, I'm open to any suggestion.

Regards.

Volkan YAZICI wrote:

On Mon, 8 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:

Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?

Yes, you need _() around those too.

For this purpose, I introduced a dropped_column_type variable in
validate_tupdesc_compat() function:

const char dropped_column_type[] = _("n/a (dropped column)");

And used this in below fashion:

OidIsValid(returned->attrs[i]->atttypid)
? format_type_be(returned->attrs[i]->atttypid)
: dropped_column_type

I changed it to gettext_noop("the text") and _(dropped_column_type) in
errdetail, and committed it.

I'd still like to have a better way to word the message, and maybe have
this error appear in a regression test somewhere at least once ...

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support