2ndQuadrant / pglogical

Logical Replication extension for PostgreSQL 17, 16, 15, 14, 13, 12, 11, 10, 9.6, 9.5, 9.4 (Postgres), providing much faster replication than Slony, Bucardo or Londiste, as well as cross-version upgrades.
http://2ndquadrant.com/en/resources/pglogical/
Other
1.01k stars 154 forks source link

Postgres 16 support #437

Closed eulerto closed 1 year ago

eulerto commented 1 year ago

It includes support for Postgres 16. Since this new release includes -Wshadow=compatible-local, there is an additional commit to rename a variable that shadows another variable. It also includes -fvisibility=hidden that requires adding PGDLLEXPORT to the worker main functions.

It passes the tests from version 9.5 to 16 beta3 (The add_table test always fails. The row_filter test has a different error message that causes a failure for 9.5 and 9.6. We could add an alternative test but it is a deprecated version so ...).

df7cb commented 1 year ago

Fwiw on top of this, I had to apply this patch to get PG16 to compile:

--- a/compat16/pglogical_compat.h
+++ b/compat16/pglogical_compat.h
@@ -111,3 +111,14 @@
    EvalPlanQualInit(epqstate, parentestate, subplan, auxrowmarks, epqParam, NIL)

 #endif
+
+#define PGL_PG_TRY(...) PG_TRY()
+#define PGL_PG_CATCH(...) PG_CATCH()
+#define PGL_PG_FINALLY(...) PG_FINALLY()
+#define PGL_PG_END_TRY(...) PG_END_TRY()
+
+#define PGL_PG_ENSURE_ERROR_CLEANUP(cleanup_function, arg, suffix) \
+   PG_ENSURE_ERROR_CLEANUP(cleanup_function, arg)
+
+#define PGL_PG_END_ENSURE_ERROR_CLEANUP(cleanup_function, arg, suffix) \
+   PG_END_ENSURE_ERROR_CLEANUP(cleanup_function, arg)

Could you merge this and tag a new release so we can sanely update the Debian packages?

eulerto commented 1 year ago

Due to a new compiler option introduced in Postgres 16 (-Wshadow=compatible-local), it prints:

pglogical_sync.c: In function ‘pglogical_sync_subscription’:
/home/euler/pg16/include/postgresql/server/utils/elog.h:386:15: warning: declaration of ‘_save_exception_stack’ shadows a previous local [-Wshadow=compatible-local]
  386 |   sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
      |               ^~~~~~~~~~~~~~~~~~~~~
/home/euler/pg16/include/postgresql/server/storage/ipc.h:50:3: note: in expansion of macro ‘PG_TRY’
   50 |   PG_TRY()
      |   ^~~~~~
pglogical_sync.c:918:4: note: in expansion of macro ‘PG_ENSURE_ERROR_CLEANUP’
  918 |    PG_ENSURE_ERROR_CLEANUP(pglogical_sync_tmpfile_cleanup_cb,
      |    ^~~~~~~~~~~~~~~~~~~~~~~
/home/euler/pg16/include/postgresql/server/utils/elog.h:386:15: note: shadowed declaration is here
  386 |   sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
      |               ^~~~~~~~~~~~~~~~~~~~~
/home/euler/pg16/include/postgresql/server/storage/ipc.h:50:3: note: in expansion of macro ‘PG_TRY’
   50 |   PG_TRY()
      |   ^~~~~~
pglogical_sync.c:909:3: note: in expansion of macro ‘PG_ENSURE_ERROR_CLEANUP’
  909 |   PG_ENSURE_ERROR_CLEANUP(pglogical_sync_worker_cleanup_error_cb,
      |   ^~~~~~~~~~~~~~~~~~~~~~~

The reason is that PG_ENSURE_ERROR_CLEANUP is called inside a previous PG_ENSURE_ERROR_CLEANUP block. Is there a simple way to avoid this warning? Maybe we should leave it as is.

eulerto commented 1 year ago

I removed the PGL_PG_ENSURE_ERROR_CLEANUP because the goal was to avoid the warning mentioned in the previous comment. However, it does not work. It seems we can avoid such errors if we copy that part from Postgres code and have a way to distinguish the variable name (using an additional parameter that compounds it?). Another option is to rewrite this code to avoid the nested PG_ENSURE_ERROR_CLEANUP call.