SSI error messages

Some of these new error messages from the SSI code are a mouthful:

not enough elements in RWConflictPool to record a rw-conflict
not enough elements in RWConflictPool to record a potential rw-conflict

These are basically "out of shared memory" conditions that could be
moved to a DETAIL message.

Canceled on identification as a pivot, during conflict out checking.
Canceled on conflict out to old pivot %u.
Canceled on identification as a pivot, with conflict out to old committed transaction %u.
Canceled on conflict out to old pivot.
Canceled on identification as a pivot, during conflict in checking.
Canceled on identification as a pivot, during write.
Canceled on conflict out to pivot %u, during read.
Canceled on identification as a pivot, during commit attempt.
Canceled on commit attempt with conflict in from prepared pivot.

These are DETAIL messages, with the rest of the report saying

ERROR: could not serialize access due to read/write dependencies among transactions
HINT: The transaction might succeed if retried.

AFAICT, the documentation doesn't mention anything about this "pivot"
that keeps coming up. Is it useful that have the user face this
information? Is there anything a user can derive from seeing one of
these messages versus another, and in addition to the error and hint,
that would help them address the situation?

Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Some of these new error messages from the SSI code are a mouthful:
>
> not enough elements in RWConflictPool to record a rw-conflict
> not enough elements in RWConflictPool to record a potential
> rw-conflict
>
> These are basically "out of shared memory" conditions that could
> be moved to a DETAIL message.

Yes and no. The resource which is exhausted at this point *is* in
shared memory, but its allocation of space is fixed at startup --
it's not competing with other users of shared memory for the shared
memory "slush space". I have some concern that an "out of shared
memory" message might confuse people on that aspect of the issue.

That said, if people find it helps more than it hurts, I have no
objection to a wording change.

> Canceled on identification as a pivot, during conflict out
> checking.
> Canceled on conflict out to old pivot %u.
> Canceled on identification as a pivot, with conflict out to
> old committed transaction %u.
> Canceled on conflict out to old pivot.
> Canceled on identification as a pivot, during conflict in
> checking.
> Canceled on identification as a pivot, during write.
> Canceled on conflict out to pivot %u, during read.
> Canceled on identification as a pivot, during commit attempt.
> Canceled on commit attempt with conflict in from prepared
> pivot.
>
> These are DETAIL messages, with the rest of the report saying
>
> ERROR: could not serialize access due to read/write
> dependencies among transactions
> HINT: The transaction might succeed if retried.
>
> AFAICT, the documentation doesn't mention anything about this
> "pivot" that keeps coming up.

Good point. It's in the README-SSI and the Wiki page, but not in
the user docs; so using it in the error detail seems likely to
confuse. Also, some of the information is referring to phases of
processing which can only be understood by looking at the source
code, like "during conflict out checking." While it might not be
entirely unreasonable to document what a pivot is, documenting some
of the other language is really out of the question.

> Is it useful that have the user face this information? Is there
> anything a user can derive from seeing one of these messages
> versus another, and in addition to the error and hint, that would
> help them address the situation?

A user with a firm grasp of SSI might be better able to figure out
mitigation techniques for frequently occurring rollback scenarios
with that detail.

We did have one thread where someone was hoping for as much
information as we could provide, but I don't know how typical that
will be:

http://archives.postgresql.org/pgsql-hackers/2010-09/msg01133.php

I find the differentiation of causes helpful when working through
test cases, but moving this information to DEBUG messages might
satisfy that need. I've also wondered whether that HINT line is
worthwhile.

I have a suspicion that we might sometimes find the information
conveyed by the detail useful when responding to users with
questions; but the language as it stands seems confusing for users.

-Kevin

On Jul 15, 2011, at 12:06 PM, "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> I have a suspicion that we might sometimes find the information
> conveyed by the detail useful when responding to users with
> questions; but the language as it stands seems confusing for users.

I think removing info here or making it harder to get would be a mistake. SSI is a complicated technology and we are likely to find that we need more debugging and instrumentation, not less.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Jul 15, 2011, at 12:06 PM, "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> I have a suspicion that we might sometimes find the information
>> conveyed by the detail useful when responding to users with
>> questions; but the language as it stands seems confusing for users.

> I think removing info here or making it harder to get would be a mistake.

Agreed. If we can clarify the messages, and/or make sure the
terminology they use is documented, that'd be a good thing; but let's
not just remove them.

I think that Peter's real concern is whether these are worth
translating, and I share that doubt. Perhaps we should invent an
errdetail_internal, parallel to errmsg_internal, that works like
errdetail but doesn't treat the message as a candidate for translation?

regards, tom lane

Excerpts from Tom Lane's message of vie jul 15 14:33:34 -0400 2011:

> I think that Peter's real concern is whether these are worth
> translating, and I share that doubt. Perhaps we should invent an
> errdetail_internal, parallel to errmsg_internal, that works like
> errdetail but doesn't treat the message as a candidate for translation?

Yeah. The other point is that translated versions of those phrases are
likely to introduce randomly diverging terms, which is not going to help
with debugging. As long as the technology is new enough that a user is
going to need help from -hackers (or at least someone who read and
grokked README.SSI) to debug a problem, there's no benefit to
translating those errdetails.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Tom Lane's message:
>
>> I think that Peter's real concern is whether these are worth
>> translating, and I share that doubt. Perhaps we should invent an
>> errdetail_internal, parallel to errmsg_internal, that works like
>> errdetail but doesn't treat the message as a candidate for
>> translation?
>
> Yeah. The other point is that translated versions of those
> phrases are likely to introduce randomly diverging terms, which is
> not going to help with debugging. As long as the technology is
> new enough that a user is going to need help from -hackers (or at
> least someone who read and grokked README.SSI) to debug a problem,
> there's no benefit to translating those errdetails.

OK, since that seems to be the consensus, I put a patch together.
Testing it now. Will post once I've confirmed it doesn't break
anything.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>> Excerpts from Tom Lane's message:
>>
>>> I think that Peter's real concern is whether these are worth
>>> translating, and I share that doubt. Perhaps we should invent
>>> an errdetail_internal, parallel to errmsg_internal, that works
>>> like errdetail but doesn't treat the message as a candidate for
>>> translation?
>>
>> Yeah. The other point is that translated versions of those
>> phrases are likely to introduce randomly diverging terms, which
>> is not going to help with debugging. As long as the technology
>> is new enough that a user is going to need help from -hackers (or
>> at least someone who read and grokked README.SSI) to debug a
>> problem, there's no benefit to translating those errdetails.
>
> OK, since that seems to be the consensus, I put a patch together.
> Testing it now. Will post once I've confirmed it doesn't break
> anything.

OK, after getting distracted by test failures caused by an unrelated
commit, I've confirmed that this passes my usual tests. I don't
know anything about the tools used for extracting the text for the
translators, so if that needs any corresponding adjustment, someone
will need to point me in the right direction or cover that part
separately.

The leading whitespace changes in predicate.c are from pgindent.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> OK, after getting distracted by test failures caused by an unrelated
> commit, I've confirmed that this passes my usual tests. I don't
> know anything about the tools used for extracting the text for the
> translators, so if that needs any corresponding adjustment, someone
> will need to point me in the right direction or cover that part
> separately.

Well, the point is that this function *isn't* going to be known to the
NLS code, so AFAICS no adjustments should be needed there. You did miss
some places that ought to be updated (mumble sources.sgml mumble) but
unless I hear objections to the whole idea, I'll fix and apply this
tomorrow.

regards, tom lane

On 16.07.2011 03:14, Tom Lane wrote:
> "Kevin Grittner"<Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> OK, after getting distracted by test failures caused by an unrelated
>> commit, I've confirmed that this passes my usual tests. I don't
>> know anything about the tools used for extracting the text for the
>> translators, so if that needs any corresponding adjustment, someone
>> will need to point me in the right direction or cover that part
>> separately.
>
> Well, the point is that this function *isn't* going to be known to the
> NLS code, so AFAICS no adjustments should be needed there. You did miss
> some places that ought to be updated (mumble sources.sgml mumble) but
> unless I hear objections to the whole idea, I'll fix and apply this
> tomorrow.

I find it strange to simply leave those strings untranslated. It's going
to look wrong, like someone just forgot to translate them. However, I
agree it's perhaps a bit too much detail to translate all of those
messages, and the translations would probably sound weird because there
isn't established terms for these things yet.

I think I would prefer something like this:

ERROR: could not serialize access due to read/write dependencies among
transactions
DETAIL: Reason code: %s
HINT: The transaction might succeed if retried.

Where %s gets the current detail field, untranslated, like:

Canceled on commit attempt with conflict in from prepared pivot.

Or perhaps shorten that to just "conflict in from prepared pivot", as
the fact that it happened on commit attempt should be clear from the
context - the error happened at a COMMIT statement.

That would be similar to what we do with OS error messages, with %m. It
would be more obvious that the untranslated message is some internal
information that the user is not expect to understand, and that it is
untranslated on purpose.

That's my 2c, anyway. I see you committed this already, I don't
violently object to that either...

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> I think I would prefer something like this:

> ERROR: could not serialize access due to read/write dependencies among
> transactions
> DETAIL: Reason code: %s
> HINT: The transaction might succeed if retried.

> That's my 2c, anyway. I see you committed this already, I don't
> violently object to that either...

Well, as I mentioned in the commit message, I've thought for some time
that there were use cases for errdetail_internal. Whether these
particular places in predicate.c use it or not doesn't affect that.

I don't have a strong opinion about whether to do it like you suggest,
other than that the proposed wording doesn't meet the message style
guideline about detail messages being complete sentences.

regards, tom lane

On lör, 2011-07-16 at 15:01 -0400, Tom Lane wrote:
> Well, as I mentioned in the commit message, I've thought for some time
> that there were use cases for errdetail_internal. Whether these
> particular places in predicate.c use it or not doesn't affect that.

Looking at commit 1af37ec96d97722aeb527f5f43d6f6f2304f0861, not all of
these are strings that don't need to be translated. For example, you
can't assume that the translation of "%s: %s" is "%s: %s" or that the
translation of "%s (%x)" is "%s (%x)". I'll review those in detail when
I find some time, but some of them will probably have to be reverted.

On lör, 2011-07-16 at 21:55 +0300, Heikki Linnakangas wrote:
> I find it strange to simply leave those strings untranslated. It's
> going
> to look wrong, like someone just forgot to translate them. However, I
> agree it's perhaps a bit too much detail to translate all of those
> messages, and the translations would probably sound weird because
> there
> isn't established terms for these things yet.
>
> I think I would prefer something like this:
>
> ERROR: could not serialize access due to read/write dependencies
> among
> transactions
> DETAIL: Reason code: %s
> HINT: The transaction might succeed if retried.

Yes, I think that would be better. I will work on that.

Excerpts from Peter Eisentraut's message of mié jul 27 16:19:22 -0400 2011:
> On lör, 2011-07-16 at 15:01 -0400, Tom Lane wrote:
> > Well, as I mentioned in the commit message, I've thought for some time
> > that there were use cases for errdetail_internal. Whether these
> > particular places in predicate.c use it or not doesn't affect that.
>
> Looking at commit 1af37ec96d97722aeb527f5f43d6f6f2304f0861, not all of
> these are strings that don't need to be translated. For example, you
> can't assume that the translation of "%s: %s" is "%s: %s" or that the
> translation of "%s (%x)" is "%s (%x)". I'll review those in detail when
> I find some time, but some of them will probably have to be reverted.

Hmm, if "%s: %s" has different uses in different places, we're going to
need a more involved solution because they might not translate
identically.

In any case, we need /* translator: */ comments to explain what's going
on.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On lör, 2011-07-16 at 21:55 +0300, Heikki Linnakangas wrote:
> I think I would prefer something like this:
>
> ERROR: could not serialize access due to read/write dependencies
> among
> transactions
> DETAIL: Reason code: %s
> HINT: The transaction might succeed if retried.
>
> Where %s gets the current detail field, untranslated, like:
>
> Canceled on commit attempt with conflict in from prepared pivot.

Do you have an idea how to address this case:

@@ -3865,7 +3865,7 @@ CheckForSerializableConflictOut(bool visible, Relation relation,
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to read/write dependencies among transactions"),
- errdetail_internal("Canceled on conflict out to old pivot %u.", xid),
+ errdetail("Reason code: %s.", "canceled on conflict out to old pivot %u", xid), // XXX bogus
errhint("The transaction might succeed if retried.")));

if (SxactHasSummaryConflictIn(MySerializableXact)

Excerpts from Peter Eisentraut's message of vie jul 29 14:46:20 -0400 2011:
> On lör, 2011-07-16 at 21:55 +0300, Heikki Linnakangas wrote:
> > I think I would prefer something like this:
> >
> > ERROR: could not serialize access due to read/write dependencies
> > among
> > transactions
> > DETAIL: Reason code: %s
> > HINT: The transaction might succeed if retried.
> >
> > Where %s gets the current detail field, untranslated, like:
> >
> > Canceled on commit attempt with conflict in from prepared pivot.
>
> Do you have an idea how to address this case:

Call sprintf to expand the %u before ereport()?

> @@ -3865,7 +3865,7 @@ CheckForSerializableConflictOut(bool visible, Relation relation,
> ereport(ERROR,
> (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
> errmsg("could not serialize access due to read/write dependencies among transactions"),
> - errdetail_internal("Canceled on conflict out to old pivot %u.", xid),
> + errdetail("Reason code: %s.", "canceled on conflict out to old pivot %u", xid), // XXX bogus
> errhint("The transaction might succeed if retried.")));

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Peter Eisentraut's message of vie jul 29 14:46:20 -0400 2011:
>> On lr, 2011-07-16 at 21:55 +0300, Heikki Linnakangas wrote:
>> Do you have an idea how to address this case:

> Call sprintf to expand the %u before ereport()?

That sounds like way too much work for a wording "improvement" that is
exceedingly marginal in the first place.

IMO the only thing really wrong with those messages is that they aren't
complete sentences, which is a style guideline that we ought to try to
adhere to even if we're not bothering to translate them. Perhaps
"Transaction canceled because of ..." would read better than "Canceled
on ..."?

regards, tom lane

On lör, 2011-07-16 at 21:55 +0300, Heikki Linnakangas wrote:
> I think I would prefer something like this:
>
> ERROR: could not serialize access due to read/write dependencies
> among
> transactions
> DETAIL: Reason code: %s
> HINT: The transaction might succeed if retried.

I've done it this way now, except that "Reason code" is not marked for
translation, for the implementation reasons discussed downthread.

I'm looking forward to see how many of these we'll see in practice and
how users will react to them.