Horton

Thread

Commits

  1. Make error logging in extended statistics more consistent

  1. error messages in extended statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-03T15:44:04Z

    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/

  2. Re: error messages in extended statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-03T16:21:36Z

    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

  3. Re: error messages in extended statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-05-03T19:42:17Z

    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

  4. Re: error messages in extended statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-05-04T23:42:33Z

    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

  5. Re: error messages in extended statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-15T16:17:29Z

    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

  6. Re: error messages in extended statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-05-15T16:35:47Z

    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

  7. Re: error messages in extended statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-05-30T15:10:29Z

    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

  8. Re: error messages in extended statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-30T16:21:59Z

    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