Обсуждение: PG_RE_THROW is mandatory (was Re: jsonpath)
In https://postgr.es/m/1676.1548726280@sss.pgh.pa.us Tom Lane wrote: > Sure: every errcode we have is unsafe to treat this way. > > The backend coding rule from day one has been that a thrown error requires > (sub)transaction cleanup to be done to make sure that things are back in a > good state. You can *not* just decide that it's okay to ignore that, > especially not when invoking code outside the immediate area of what > you're doing. elog.h claims that PG_RE_THROW is "optional": /*---------- * API for catching ereport(ERROR) exits. Use these macros like so: * * PG_TRY(); * { * ... code that might throw ereport(ERROR) ... * } * PG_CATCH(); * { * ... error recovery code ... * } * PG_END_TRY(); * * (The braces are not actually necessary, but are recommended so that * pgindent will indent the construct nicely.) The error recovery code * can optionally do PG_RE_THROW() to propagate the same error outwards. This is obviously wrong; while we have a couple of codesites that omit it, it's not a generally available coding pattern. I think we should amend that comment. I propose: "The error recovery code must normally do PG_RE_THROW() to propagate the error outwards; failure to do so may leave the system in an inconsistent state for further processing." Other wording proposals welcome, but I don't want to leave it as is. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-02-06 13:09:59 -0300, Alvaro Herrera wrote: > In https://postgr.es/m/1676.1548726280@sss.pgh.pa.us Tom Lane wrote: > > > Sure: every errcode we have is unsafe to treat this way. > > > > The backend coding rule from day one has been that a thrown error requires > > (sub)transaction cleanup to be done to make sure that things are back in a > > good state. You can *not* just decide that it's okay to ignore that, > > especially not when invoking code outside the immediate area of what > > you're doing. > > elog.h claims that PG_RE_THROW is "optional": > > /*---------- > * API for catching ereport(ERROR) exits. Use these macros like so: > * > * PG_TRY(); > * { > * ... code that might throw ereport(ERROR) ... > * } > * PG_CATCH(); > * { > * ... error recovery code ... > * } > * PG_END_TRY(); > * > * (The braces are not actually necessary, but are recommended so that > * pgindent will indent the construct nicely.) The error recovery code > * can optionally do PG_RE_THROW() to propagate the same error outwards. > > This is obviously wrong; while we have a couple of codesites that omit > it, it's not a generally available coding pattern. I think we should > amend that comment. I propose: "The error recovery code must normally > do PG_RE_THROW() to propagate the error outwards; failure to do so may > leave the system in an inconsistent state for further processing." Well, but it's ok not to rethrow if you do a [sub]transaction rollback. I assume that's why it's framed as optional. We probably should reference that fact? Greetings, Andres Freund
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> elog.h claims that PG_RE_THROW is "optional":
> * (The braces are not actually necessary, but are recommended so that
> * pgindent will indent the construct nicely.) The error recovery code
> * can optionally do PG_RE_THROW() to propagate the same error outwards.
> This is obviously wrong; while we have a couple of codesites that omit
> it, it's not a generally available coding pattern. I think we should
> amend that comment. I propose: "The error recovery code must normally
> do PG_RE_THROW() to propagate the error outwards; failure to do so may
> leave the system in an inconsistent state for further processing."
Well, it can either do PG_RE_THROW or do a (sub)transaction abort.
Some level of throw-catching code has to do the latter eventually.
regards, tom lane
> On 2019-02-06 13:09:59 -0300, Alvaro Herrera wrote: > > This is obviously wrong; while we have a couple of codesites that omit > > it, it's not a generally available coding pattern. I think we should > > amend that comment. I propose: "The error recovery code must normally > > do PG_RE_THROW() to propagate the error outwards; failure to do so may > > leave the system in an inconsistent state for further processing." On 2019-Feb-06, Andres Freund wrote: > Well, but it's ok not to rethrow if you do a [sub]transaction > rollback. I assume that's why it's framed as optional. We probably > should reference that fact? On 2019-Feb-06, Tom Lane wrote: > Well, it can either do PG_RE_THROW or do a (sub)transaction abort. > Some level of throw-catching code has to do the latter eventually. So, "The error recovery code can either do PG_RE_THROW to propagate the error outwards, or do a (sub)transaction abort. Failure to do so may leave the system in an inconsistent state for further processing." -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> So,
> "The error recovery code can either do PG_RE_THROW to propagate the
> error outwards, or do a (sub)transaction abort. Failure to do so may
> leave the system in an inconsistent state for further processing."
WFM.
regards, tom lane
On 2019-Feb-07, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > So, > > > "The error recovery code can either do PG_RE_THROW to propagate the > > error outwards, or do a (sub)transaction abort. Failure to do so may > > leave the system in an inconsistent state for further processing." > > WFM. Thanks, pushed. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services