Thread: alternative to PG_CATCH
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
Attachment
At Thu, 13 Dec 2018 11:33:39 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<c170919d-c78b-3dac-5ff6-9bd12f7a38bc@2ndquadrant.com>
> 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
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
On 12/13/18 7:26 AM, Kyotaro HORIGUCHI wrote:
> At Thu, 13 Dec 2018 11:33:39 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<c170919d-c78b-3dac-5ff6-9bd12f7a38bc@2ndquadrant.com>
>>
>> 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
david@pgmasters.net
Attachment
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 2018-12-14 16:49, Tom Lane wrote:
> 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();
Here is a new implementation that works just like that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Mon, Oct 28, 2019 at 4:43 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Here is a new implementation that works just like that.
This looks like a marked notational improvement.
With the patch:
[rhaas pgsql]$ git grep PG_CATCH | wc -l
102
[rhaas pgsql]$ git grep PG_FINALLY | wc -l
55
I'm actually a bit surprised that the percentage of cases that could
be converted to use PG_FINALLY wasn't even higher than that.
In theory, the do_rethrow variable could conflict with a symbol
declared in the surrounding scope, but that doesn't seem like it's a
problem worth getting worked up about.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2019-10-28 13:45, Robert Haas wrote: > In theory, the do_rethrow variable could conflict with a symbol > declared in the surrounding scope, but that doesn't seem like it's a > problem worth getting worked up about. Right. A PG_TRY block also declares other local variables for internal use without much care about namespacing. If it becomes a problem, it's easy to address. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-10-28 13:45, Robert Haas wrote:
>> In theory, the do_rethrow variable could conflict with a symbol
>> declared in the surrounding scope, but that doesn't seem like it's a
>> problem worth getting worked up about.
> Right. A PG_TRY block also declares other local variables for internal
> use without much care about namespacing. If it becomes a problem, it's
> easy to address.
Although we haven't been terribly consistent about it, some of our macros
address this problem by using local variable names with a leading and/or
trailing underscore, or otherwise making them names you'd be quite
unlikely to use in normal code. I suggest doing something similar
here. (Wouldn't be a bad idea to make PG_TRY's variables follow suit.)
regards, tom lane
On 2019-10-29 17:10, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 2019-10-28 13:45, Robert Haas wrote: >>> In theory, the do_rethrow variable could conflict with a symbol >>> declared in the surrounding scope, but that doesn't seem like it's a >>> problem worth getting worked up about. > >> Right. A PG_TRY block also declares other local variables for internal >> use without much care about namespacing. If it becomes a problem, it's >> easy to address. > > Although we haven't been terribly consistent about it, some of our macros > address this problem by using local variable names with a leading and/or > trailing underscore, or otherwise making them names you'd be quite > unlikely to use in normal code. I suggest doing something similar > here. (Wouldn't be a bad idea to make PG_TRY's variables follow suit.) committed with a leading underscore -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> committed with a leading underscore
I hadn't actually tested this patch before commit, but now that
it's in, I'm seeing assorted compiler warnings:
plpy_exec.c: In function 'PLy_procedure_call':
plpy_exec.c:1028: warning: 'rv' may be used uninitialized in this function
pltcl.c: In function 'pltcl_handler':
pltcl.c:721: warning: 'retval' may be used uninitialized in this function
plpy_typeio.c: In function 'PLyObject_ToBytea':
plpy_typeio.c:904: warning: 'rv' may be used uninitialized in this function
plperl.c: In function 'plperl_call_handler':
plperl.c:1843: warning: 'retval' may be used uninitialized in this function
I'm not happy about that.
regards, tom lane
On 2019-11-02 15:36, Tom Lane wrote: > I hadn't actually tested this patch before commit, but now that > it's in, I'm seeing assorted compiler warnings: I've fixed the ones that I could reproduce on CentOS 6. I haven't seen any on a variety of newer systems. It's not clear why only a handful of cases cause warnings, but my guess is that the functions are above some size/complexity threshold beyond which those older compilers give up doing a full analysis. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-02 15:36, Tom Lane wrote:
>> I hadn't actually tested this patch before commit, but now that
>> it's in, I'm seeing assorted compiler warnings:
> I've fixed the ones that I could reproduce on CentOS 6. I haven't seen
> any on a variety of newer systems.
I'd hoped for a way to fix PG_FINALLY rather than having to band-aid
the individual callers :-(. But maybe there isn't one.
Now that I've actually looked at the patched code, there's a far
more severe problem with it. Namely, that use of PG_FINALLY
means that the "finally" segment is run without having popped
the error context stack, which means that any error thrown
within that segment will sigsetjmp right back to the top,
resulting in an infinite loop. (Well, not infinite, because
it'll crash out once the error nesting depth limit is hit.)
We *must* pop the stack before running recovery code.
Possibly this could be fixed like so:
#define PG_FINALLY() \
} \
else \
{ \
PG_exception_stack = _save_exception_stack; \
error_context_stack = _save_context_stack; \
_do_rethrow = true
#define PG_END_TRY() \
} \
if (_do_rethrow) \
PG_RE_THROW(); \
PG_exception_stack = _save_exception_stack; \
error_context_stack = _save_context_stack; \
} while (0)
But I haven't tested that.
regards, tom lane
On 2019-11-04 16:01, Tom Lane wrote: > Now that I've actually looked at the patched code, there's a far > more severe problem with it. Namely, that use of PG_FINALLY > means that the "finally" segment is run without having popped > the error context stack, which means that any error thrown > within that segment will sigsetjmp right back to the top, > resulting in an infinite loop. (Well, not infinite, because > it'll crash out once the error nesting depth limit is hit.) > We *must* pop the stack before running recovery code. I can confirm that that indeed happens. :( Here is a patch to fix it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-04 16:01, Tom Lane wrote:
>> Now that I've actually looked at the patched code, there's a far
>> more severe problem with it. Namely, that use of PG_FINALLY
>> means that the "finally" segment is run without having popped
>> the error context stack, which means that any error thrown
>> within that segment will sigsetjmp right back to the top,
>> resulting in an infinite loop. (Well, not infinite, because
>> it'll crash out once the error nesting depth limit is hit.)
>> We *must* pop the stack before running recovery code.
> I can confirm that that indeed happens. :(
> Here is a patch to fix it.
This seems all right from here. Since PG_RE_THROW() is guaranteed
noreturn, I personally wouldn't have bothered with an "else" after it,
but that's just stylistic.
regards, tom lane
On 2019-11-06 15:49, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 2019-11-04 16:01, Tom Lane wrote: >>> Now that I've actually looked at the patched code, there's a far >>> more severe problem with it. Namely, that use of PG_FINALLY >>> means that the "finally" segment is run without having popped >>> the error context stack, which means that any error thrown >>> within that segment will sigsetjmp right back to the top, >>> resulting in an infinite loop. (Well, not infinite, because >>> it'll crash out once the error nesting depth limit is hit.) >>> We *must* pop the stack before running recovery code. > >> I can confirm that that indeed happens. :( > >> Here is a patch to fix it. > > This seems all right from here. Since PG_RE_THROW() is guaranteed > noreturn, I personally wouldn't have bothered with an "else" after it, > but that's just stylistic. Committed, without the "else". -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services