PG_ENSURE_ERROR_CLEANUP and nested blocks

Started by Euler Taveira14 days ago2 messageshackers
Jump to latest
#1Euler Taveira
euler@eulerto.com

Hi,

If you use PG_ENSURE_ERROR_CLEANUP with nested blocks, the compiler warns about
shadow locals. There is no harm since each PG_TRY and PG_CATCH uses its own
context stack. The current code doesn't have an example of such usage but
pglogical [1]https://github.com/2ndQuadrant/pglogical/issues/521#issuecomment-4925354899 does. I don't know if other extensions have the same issue too.

The PG_TRY and PG_CATCH already accept an optional parameter (commit
112f0225dbfe) to avoid this exact issue. Since PG_ENSURE_ERROR_CLEANUP /
PG_END_ENSURE_ERROR_CLEANUP already use PG_TRY / PG_CATCH, add an optional
parameter too. (I opted to a variadic argument argument -- mimicking how PG_TRY
already handles its optional suffix parameter -- because it avoids changing existing callers).

[1]: https://github.com/2ndQuadrant/pglogical/issues/521#issuecomment-4925354899

--
Euler Taveira
EDB https://www.enterprisedb.com/

Attachments:

v1-0001-Avoid-shadowing-variables-for-nested-PG_ENSURE_ER.patchtext/x-patch; name="=?UTF-8?Q?v1-0001-Avoid-shadowing-variables-for-nested-PG=5FENSURE=5FER.?= =?UTF-8?Q?patch?="Download+7-7
#2Michael Paquier
michael@paquier.xyz
In reply to: Euler Taveira (#1)
Re: PG_ENSURE_ERROR_CLEANUP and nested blocks

On Mon, Jul 27, 2026 at 11:56:46AM -0300, Euler Taveira wrote:

If you use PG_ENSURE_ERROR_CLEANUP with nested blocks, the compiler warns about
shadow locals. There is no harm since each PG_TRY and PG_CATCH uses its own
context stack. The current code doesn't have an example of such usage but
pglogical [1] does. I don't know if other extensions have the same issue too.

The PG_TRY and PG_CATCH already accept an optional parameter (commit
112f0225dbfe) to avoid this exact issue. Since PG_ENSURE_ERROR_CLEANUP /
PG_END_ENSURE_ERROR_CLEANUP already use PG_TRY / PG_CATCH, add an optional
parameter too. (I opted to a variadic argument argument -- mimicking how PG_TRY
already handles its optional suffix parameter -- because it avoids changing existing callers).

This line of arguments makes sense here.

If I check out the pglogical code at the top of its REL2_x_STABLE
branch and try to compile it with a Postgres at the top of
REL_19_STABLE and your patch applied, then I still check shadow
variable warnings:
In file included from /home/popo/pgsql/include/server/postgres.h:49,
from pglogical_sync.c:14:
pglogical_sync.c: In function ‘pglogical_sync_subscription’:
/home/popo/pgsql/include/server/utils/elog.h:390:29: warning:
declaration of ‘_save_exception_stack’ shadows a previous local
[-Wshadow=compatible-local]
390 | sigjmp_buf *_save_exception_stack##__VA_ARGS__
= PG_exception_stack; \
| ^~~~~~~~~~~~~~~~~~~~~

Something I am missing or a change on the pglogical side I am missing
perhaps?
--
Michael