Thread: error messages in extended statistics
Hello
Error reporting in extended statistics is inconsistent -- many messages
that are ereport() in mvdistinct.c are elog() in the other modules. I
think what happened is that I changed them from elog to ereport when
committing mvdistinct, but Tomas and Simon didn't follow suit when
committing the other two modules. As a result, some messages that
should be essentially duplicates only show up once, because the elog()
ones are not marked translatable.
I think this should be cleaned up, while at the same time not giving too
much hassle for translators; for example, this message
dependencies.c: elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)",
should not only be turned into an ereport(), but also the MVDependencies
part turned into a %s. (Alternatively, we could decide I was wrong and
turn them all back into elogs, but I obviously vote against that.)
$ git grep 'elog\|errmsg' src/backend/statistics
dependencies.c: elog(ERROR, "cache lookup failed for ordering operator for type %u",
dependencies.c: elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)",
dependencies.c: elog(ERROR, "invalid dependency magic %d (expected %d)",
dependencies.c: elog(ERROR, "invalid dependency type %d (expected %d)",
dependencies.c: errmsg("invalid zero-length item array in MVDependencies")));
dependencies.c: elog(ERROR, "invalid dependencies size %zd (expected at least %zd)",
dependencies.c: elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
dependencies.c: elog(ERROR,
dependencies.c: errmsg("cannot accept a value of type %s", "pg_dependencies")));
dependencies.c: errmsg("cannot accept a value of type %s", "pg_dependencies")));
extended_stats.c: errmsg("statistics object \"%s.%s\" could not be computed for relation
\"%s.%s\"",
extended_stats.c: elog(ERROR, "unexpected statistics type requested: %d", type);
extended_stats.c: elog(ERROR, "stxkind is not a 1-D char array");
extended_stats.c: elog(ERROR, "cache lookup failed for statistics object %u", statOid);
mcv.c: elog(ERROR, "cache lookup failed for ordering operator for type %u",
mcv.c: elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
mcv.c: elog(ERROR,
mcv.c: elog(ERROR, "invalid MCV size %zd (expected at least %zu)",
mcv.c: elog(ERROR, "invalid MCV magic %u (expected %u)",
mcv.c: elog(ERROR, "invalid MCV type %u (expected %u)",
mcv.c: elog(ERROR, "invalid zero-length dimension array in MCVList");
mcv.c: elog(ERROR, "invalid length (%d) dimension array in MCVList",
mcv.c: elog(ERROR, "invalid zero-length item array in MCVList");
mcv.c: elog(ERROR, "invalid length (%u) item array in MCVList",
mcv.c: elog(ERROR, "invalid MCV size %zd (expected %zu)",
mcv.c: elog(ERROR, "invalid MCV size %zd (expected %zu)",
mcv.c: errmsg("function returning record called in context "
mcv.c: errmsg("cannot accept a value of type %s", "pg_mcv_list")));
mcv.c: errmsg("cannot accept a value of type %s", "pg_mcv_list")));
mcv.c: elog(ERROR, "unknown clause type: %d", clause->type);
mvdistinct.c: elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
mvdistinct.c: elog(ERROR,
mvdistinct.c: elog(ERROR, "invalid MVNDistinct size %zd (expected at least %zd)",
mvdistinct.c: errmsg("invalid ndistinct magic %08x (expected %08x)",
mvdistinct.c: errmsg("invalid ndistinct type %d (expected %d)",
mvdistinct.c: errmsg("invalid zero-length item array in MVNDistinct")));
mvdistinct.c: errmsg("invalid MVNDistinct size %zd (expected at least %zd)",
mvdistinct.c: errmsg("cannot accept a value of type %s", "pg_ndistinct")));
mvdistinct.c: errmsg("cannot accept a value of type %s", "pg_ndistinct")));
mvdistinct.c: elog(ERROR, "cache lookup failed for ordering operator for type %u",
--
Álvaro Herrera Developer, https://www.PostgreSQL.org/
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Error reporting in extended statistics is inconsistent -- many messages
> that are ereport() in mvdistinct.c are elog() in the other modules.
> ...
> I think this should be cleaned up, while at the same time not giving too
> much hassle for translators; for example, this message
> dependencies.c: elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)",
> should not only be turned into an ereport(), but also the MVDependencies
> part turned into a %s. (Alternatively, we could decide I was wrong and
> turn them all back into elogs, but I obviously vote against that.)
FWIW, I'd vote the other way: that seems like a clear "internal error",
so making translators deal with it is just make-work. It should be an
elog. If there's a reasonably plausible way for a user to trigger an
error condition, then yes ereport, but if we're reporting situations
that couldn't happen without a server bug then elog seems fine.
regards, tom lane
On Fri, May 03, 2019 at 12:21:36PM -0400, Tom Lane wrote: >Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> Error reporting in extended statistics is inconsistent -- many messages >> that are ereport() in mvdistinct.c are elog() in the other modules. >> ... >> I think this should be cleaned up, while at the same time not giving too >> much hassle for translators; for example, this message >> dependencies.c: elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)", >> should not only be turned into an ereport(), but also the MVDependencies >> part turned into a %s. (Alternatively, we could decide I was wrong and >> turn them all back into elogs, but I obviously vote against that.) > >FWIW, I'd vote the other way: that seems like a clear "internal error", >so making translators deal with it is just make-work. It should be an >elog. If there's a reasonably plausible way for a user to trigger an >error condition, then yes ereport, but if we're reporting situations >that couldn't happen without a server bug then elog seems fine. > Yeah, I agree. Most of (peshaps all) those errors are internal errors, and thus should be elog. I'll take care of clening this up a bit. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 03, 2019 at 09:42:17PM +0200, Tomas Vondra wrote: >On Fri, May 03, 2019 at 12:21:36PM -0400, Tom Lane wrote: >>Alvaro Herrera <alvherre@2ndquadrant.com> writes: >>>Error reporting in extended statistics is inconsistent -- many messages >>>that are ereport() in mvdistinct.c are elog() in the other modules. >>>... >>>I think this should be cleaned up, while at the same time not giving too >>>much hassle for translators; for example, this message >>>dependencies.c: elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)", >>>should not only be turned into an ereport(), but also the MVDependencies >>>part turned into a %s. (Alternatively, we could decide I was wrong and >>>turn them all back into elogs, but I obviously vote against that.) >> >>FWIW, I'd vote the other way: that seems like a clear "internal error", >>so making translators deal with it is just make-work. It should be an >>elog. If there's a reasonably plausible way for a user to trigger an >>error condition, then yes ereport, but if we're reporting situations >>that couldn't happen without a server bug then elog seems fine. >> > >Yeah, I agree. Most of (peshaps all) those errors are internal errors, >and thus should be elog. I'll take care of clening this up a bit. > OK, so here is a patch, using elog() for all places except for the input function, where we simply report we don't accept those values. I agree those are internal errors, usually meaning the statistics object was either corrupted or there's a bug in how it's built/serialized. Users should not be able to trigger those cases (the only thing I can think of is sending a bogus value through send/recv functions, that simply do byteaout/byteain). Now, what about backpatch? It's a small tweak, but it makes the life a bit easier for translators ... regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2019-May-05, Tomas Vondra wrote: > OK, so here is a patch, using elog() for all places except for the > input function, where we simply report we don't accept those values. Hmm, does this actually work? I didn't know that elog() supported errcode()/errmsg()/etc. I thought the macro definition didn't allow for that. Anyway, since the messages are still passed with errmsg(), they would still end up in the message catalog, so this patch doesn't help my case. I would suggest that instead of changing ereport to elog, you should change errmsg() to errmsg_internal(). That prevents the translation marking, and achieves the desired effect. (You can verify by running "make update-po" in src/backend/ and seeing that the msgid no longer appears in postgres.pot). > Now, what about backpatch? It's a small tweak, but it makes the life a > bit easier for translators ... +1 for backpatching. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 15, 2019 at 12:17:29PM -0400, Alvaro Herrera wrote: >On 2019-May-05, Tomas Vondra wrote: > >> OK, so here is a patch, using elog() for all places except for the >> input function, where we simply report we don't accept those values. > >Hmm, does this actually work? I didn't know that elog() supported >errcode()/errmsg()/etc. I thought the macro definition didn't allow for >that. > D'oh, it probably does not. I might not have tried to compile it before sending it to the mailing list, not sure ... :-( >Anyway, since the messages are still passed with errmsg(), they would >still end up in the message catalog, so this patch doesn't help my case. >I would suggest that instead of changing ereport to elog, you should >change errmsg() to errmsg_internal(). That prevents the translation >marking, and achieves the desired effect. (You can verify by running >"make update-po" in src/backend/ and seeing that the msgid no longer >appears in postgres.pot). > >> Now, what about backpatch? It's a small tweak, but it makes the life a >> bit easier for translators ... > >+1 for backpatching. > OK. -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 15, 2019 at 06:35:47PM +0200, Tomas Vondra wrote: >On Wed, May 15, 2019 at 12:17:29PM -0400, Alvaro Herrera wrote: >>On 2019-May-05, Tomas Vondra wrote: >> >>>OK, so here is a patch, using elog() for all places except for the >>>input function, where we simply report we don't accept those values. >> >>Hmm, does this actually work? I didn't know that elog() supported >>errcode()/errmsg()/etc. I thought the macro definition didn't allow for >>that. >> > >D'oh, it probably does not. I might not have tried to compile it before >sending it to the mailing list, not sure ... :-( > >>Anyway, since the messages are still passed with errmsg(), they would >>still end up in the message catalog, so this patch doesn't help my case. >>I would suggest that instead of changing ereport to elog, you should >>change errmsg() to errmsg_internal(). That prevents the translation >>marking, and achieves the desired effect. (You can verify by running >>"make update-po" in src/backend/ and seeing that the msgid no longer >>appears in postgres.pot). >> >>>Now, what about backpatch? It's a small tweak, but it makes the life a >>>bit easier for translators ... >> >>+1 for backpatching. >> Pushed and backpatched, changing most places to elog(). -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-May-30, Tomas Vondra wrote: > Pushed and backpatched, changing most places to elog(). Thanks :-) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services