alternative to PG_CATCH

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

alternative to PG_CATCH

Peter Eisentraut-6
This is a common pattern:

    PG_TRY();
    {
        ... code that might throw ereport(ERROR) ...
    }
    PG_CATCH();
    {
        cleanup();
        PG_RE_THROW();
    }
    PG_END_TRY();
    cleanup();  /* the same as above */

I've played with a way to express this more compactly:

    PG_TRY();
    {
        ... code that might throw ereport(ERROR) ...
    }
    PG_FINALLY({
        cleanup();
    });

See attached patch for how this works out in practice.

Thoughts?  Other ideas?

One problem is that this currently makes pgindent crash.  That's
probably worth fixing anyway.  Or perhaps find a way to write this
differently.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-PG_FINALLY.patch (37K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

Kyotaro HORIGUCHI-2
At Thu, 13 Dec 2018 11:33:39 +0100, Peter Eisentraut <[hidden email]> wrote in <[hidden email]>

> This is a common pattern:
>
>     PG_TRY();
>     {
>         ... code that might throw ereport(ERROR) ...
>     }
>     PG_CATCH();
>     {
>         cleanup();
>         PG_RE_THROW();
>     }
>     PG_END_TRY();
>     cleanup();  /* the same as above */
>
> I've played with a way to express this more compactly:
>
>     PG_TRY();
>     {
>         ... code that might throw ereport(ERROR) ...
>     }
>     PG_FINALLY({
>         cleanup();
>     });
>
> See attached patch for how this works out in practice.
>
> Thoughts?  Other ideas?
>
> One problem is that this currently makes pgindent crash.  That's
> probably worth fixing anyway.  Or perhaps find a way to write this
> differently.

Though I didn't look into individual change, this kind of
refactoring looks good to me. But the syntax looks
somewhat.. uh..

I'm not sure it is actually workable, but I suspect that what we
need here is just a shortcut of 'PG_CATCH();{PG_RE_THROW();}'.
Something like this:

#define PG_FINALLY()    \
        } \
        else \
        { \
            PG_exception_stack = save_exception_stack; \
            error_context_stack = save_context_stack; \
            PG_RE_THROW();        \
        } \
        PG_exception_stack = save_exception_stack;    \
        error_context_stack = save_context_stack; \
        {

Which can be used as:

PG_TRY();
{
    ... code that might throw ereport(ERROR) ...
}
PG_FINALLY();
{
    cleanup();
}
PG_TRY_END();


Or


#define PG_END_TRY_CATCHING_ALL()  \
                PG_FINALLY(); \
                PG_END_TRY();

PG_TRY();
{
    ... code that might throw ereport(ERROR) ...
}
PG_END_TRY_CATCHING_ALL();

cleanup();



regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

Andrew Dunstan-8
In reply to this post by Peter Eisentraut-6

On 12/13/18 5:33 AM, Peter Eisentraut wrote:

> This is a common pattern:
>
>     PG_TRY();
>     {
>         ... code that might throw ereport(ERROR) ...
>     }
>     PG_CATCH();
>     {
>         cleanup();
>         PG_RE_THROW();
>     }
>     PG_END_TRY();
>     cleanup();  /* the same as above */
>
> I've played with a way to express this more compactly:
>
>     PG_TRY();
>     {
>         ... code that might throw ereport(ERROR) ...
>     }
>     PG_FINALLY({
>         cleanup();
>     });
>
> See attached patch for how this works out in practice.
>
> Thoughts?  Other ideas?
>
> One problem is that this currently makes pgindent crash.  That's
> probably worth fixing anyway.  Or perhaps find a way to write this
> differently.
>


The pgindent issue isn't at all surprising. If we wanted to fix it the
way would be to get the perl script to rewrite it with something indent
would accept (e.g. move the block outside the parentheses) and then undo
that after the indent had run.


But the block inside parentheses feels kinda funny, doesn't it?


cheers


andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

David Steele
In reply to this post by Kyotaro HORIGUCHI-2
On 12/13/18 7:26 AM, Kyotaro HORIGUCHI wrote:

> At Thu, 13 Dec 2018 11:33:39 +0100, Peter Eisentraut <[hidden email]> wrote in <[hidden email]>
>>
>> I've played with a way to express this more compactly:
>>
>>     PG_TRY();
>>     {
>>         ... code that might throw ereport(ERROR) ...
>>     }
>>     PG_FINALLY({
>>         cleanup();
>>     });
>>
>> See attached patch for how this works out in practice.
+1

> Though I didn't look into individual change, this kind of
> refactoring looks good to me. But the syntax looks
> somewhat.. uh..
>
> I'm not sure it is actually workable, but I suspect that what we
> need here is just a shortcut of 'PG_CATCH();{PG_RE_THROW();}'.
> Something like this:
>
> #define PG_FINALLY()    \
>         } \
>         else \
>         { \
>             PG_exception_stack = save_exception_stack; \
>             error_context_stack = save_context_stack; \
>             PG_RE_THROW();        \
>         } \
>         PG_exception_stack = save_exception_stack;    \
>         error_context_stack = save_context_stack; \
>         {
>
> Which can be used as:
>
> PG_TRY();
> {
>     ... code that might throw ereport(ERROR) ...
> }
> PG_FINALLY();
> {
>     cleanup();
> }
> PG_TRY_END();
I like this syntax better.  We use something very similar in the
pgBackRest project:

TRY_BEGIN()
{
    <Do something that might throw an error>
}
CATCH(Error1)
{
    <Handle Error1>
}
CATCH(Error2)
{
    <Handle Error2>
}
CATCH_ANY()
{
    <Handle errors that are not Error1 or Error2>
}
FINALLY()
{
    <Cleanup code that runs in all cases>
}
TRY_END();

The syntax works out simpler if the FINALLY is part of the TRY block.

See attached for the implementation.

Regards,
--
-David
[hidden email]

error.h (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

Tom Lane-2
In reply to this post by Andrew Dunstan-8
Andrew Dunstan <[hidden email]> writes:
> But the block inside parentheses feels kinda funny, doesn't it?

+1.  I think this is a good concept, but let's put in enough effort to
not require weird syntax.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

Peter Eisentraut-6
In reply to this post by Kyotaro HORIGUCHI-2
On 13/12/2018 13:26, Kyotaro HORIGUCHI wrote:

> Though I didn't look into individual change, this kind of
> refactoring looks good to me. But the syntax looks
> somewhat.. uh..
>
> I'm not sure it is actually workable, but I suspect that what we
> need here is just a shortcut of 'PG_CATCH();{PG_RE_THROW();}'.
> Something like this:
>
> #define PG_FINALLY()    \
>         } \
>         else \
>         { \
>             PG_exception_stack = save_exception_stack; \
>             error_context_stack = save_context_stack; \
>             PG_RE_THROW();        \
>         } \
>         PG_exception_stack = save_exception_stack;    \
>         error_context_stack = save_context_stack; \
>         {

I don't think this works, because the "finally" code needs to be run in
the else branch before the rethrow.

The fundamental problem, as I see it, is that the macro expansion needs
to produce the "finally" code twice: Once in the else (error) branch of
the setjmp, and once in the non-error code path, either after the
if-setjmp, or in the try block.  But to be able to do that, we need to
capture the block as a macro argument.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: alternative to PG_CATCH

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> The fundamental problem, as I see it, is that the macro expansion needs
> to produce the "finally" code twice: Once in the else (error) branch of
> the setjmp, and once in the non-error code path, either after the
> if-setjmp, or in the try block.  But to be able to do that, we need to
> capture the block as a macro argument.

I don't especially like the MACRO({...}) proposal, because it looks way
too much like gcc's special syntax for "statement expressions".  If we
have to go this way, I'd rather see if MACRO((...)) can be made to work.
But I question your assumption that we have to have two physical copies
of the "finally" code.  There's nothing wrong with implementing this
sort of infrastructure with some goto's, or whatever else we have to
have to make it work.  Also, it'd look more natural as an extension
to the existing PG_TRY infrastructure if the source code were like

        PG_TRY();
        {
                ...
        }
        PG_FINALLY();
        {
                ...
        }
        PG_END_TRY();

So even if Kyotaro-san's initial sketch isn't right in detail,
I think he has the right idea about where we want to go.

                        regards, tom lane