PG_RE_THROW is mandatory (was Re: jsonpath)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

PG_RE_THROW is mandatory (was Re: jsonpath)

Alvaro Herrera-9
In https://postgr.es/m/1676.1548726280@... 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

Reply | Threaded
Open this post in threaded view
|

Re: PG_RE_THROW is mandatory (was Re: jsonpath)

Andres Freund
On 2019-02-06 13:09:59 -0300, Alvaro Herrera wrote:

> In https://postgr.es/m/1676.1548726280@... 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

Reply | Threaded
Open this post in threaded view
|

Re: PG_RE_THROW is mandatory (was Re: jsonpath)

Tom Lane-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: PG_RE_THROW is mandatory (was Re: jsonpath)

Alvaro Herrera-9
> 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

Reply | Threaded
Open this post in threaded view
|

Re: PG_RE_THROW is mandatory (was Re: jsonpath)

Tom Lane-2
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: PG_RE_THROW is mandatory (was Re: jsonpath)

Alvaro Herrera-9
On 2019-Feb-07, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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