Обсуждение: pgsql: CREATE SUBSCRIPTION ... SERVER.
CREATE SUBSCRIPTION ... SERVER.
Allow CREATE SUBSCRIPTION to accept a foreign server using the SERVER
clause instead of a raw connection string using the CONNECTION clause.
* Enables a user with sufficient privileges to create a subscription
using a foreign server by name without specifying the connection
details.
* Integrates with user mappings (and other FDW infrastructure) using
the subscription owner.
* Provides a layer of indirection to manage multiple subscriptions
to the same remote server more easily.
Also add CREATE FOREIGN DATA WRAPPER ... CONNECTION clause to specify
a connection_function. To be eligible for a subscription, the foreign
server's foreign data wrapper must specify a connection_function.
Add connection_function support to postgres_fdw, and bump postgres_fdw
version to 1.3.
Bump catversion.
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/61831790a0a937038f78ce09f8dd4cef7de7456a.camel@j-davis.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/8185bb53476378443240d57f7d844347d5fae1bf
Modified Files
--------------
contrib/postgres_fdw/Makefile | 2 +-
contrib/postgres_fdw/connection.c | 299 +++++++++++++---------
contrib/postgres_fdw/expected/postgres_fdw.out | 8 +
contrib/postgres_fdw/meson.build | 2 +
contrib/postgres_fdw/postgres_fdw--1.2--1.3.sql | 12 +
contrib/postgres_fdw/postgres_fdw.control | 2 +-
contrib/postgres_fdw/sql/postgres_fdw.sql | 7 +
contrib/postgres_fdw/t/010_subscription.pl | 71 +++++
doc/src/sgml/logical-replication.sgml | 4 +-
doc/src/sgml/postgres-fdw.sgml | 26 ++
doc/src/sgml/ref/alter_foreign_data_wrapper.sgml | 20 ++
doc/src/sgml/ref/alter_subscription.sgml | 18 +-
doc/src/sgml/ref/create_foreign_data_wrapper.sgml | 20 ++
doc/src/sgml/ref/create_server.sgml | 7 +
doc/src/sgml/ref/create_subscription.sgml | 16 +-
src/backend/catalog/dependency.c | 11 +
src/backend/catalog/pg_subscription.c | 38 ++-
src/backend/catalog/system_views.sql | 2 +-
src/backend/commands/foreigncmds.c | 58 ++++-
src/backend/commands/subscriptioncmds.c | 203 +++++++++++++--
src/backend/foreign/foreign.c | 86 +++++++
src/backend/parser/gram.y | 22 ++
src/backend/replication/logical/worker.c | 24 +-
src/bin/pg_dump/pg_dump.c | 39 ++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 6 +-
src/bin/psql/tab-complete.in.c | 11 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_foreign_data_wrapper.h | 3 +
src/include/catalog/pg_subscription.h | 8 +-
src/include/foreign/foreign.h | 3 +
src/include/nodes/parsenodes.h | 3 +
src/test/regress/expected/oidjoins.out | 2 +
src/test/regress/expected/subscription.out | 222 +++++++++-------
src/test/regress/regress.c | 7 +
src/test/regress/sql/subscription.sql | 55 ++++
36 files changed, 1070 insertions(+), 250 deletions(-)
On 06.03.26 17:44, Jeff Davis wrote:
> CREATE SUBSCRIPTION ... SERVER.
In src/backend/foreign/foreign.c, this
volatile text *connection_text = NULL;
should probably be
text *volatile connection_text = NULL;
similar to commit 6307b096e25.
Hi, On 2026-03-11 08:37:13 +0100, Peter Eisentraut wrote: > On 06.03.26 17:44, Jeff Davis wrote: > > CREATE SUBSCRIPTION ... SERVER. > > In src/backend/foreign/foreign.c, this > > volatile text *connection_text = NULL; > > should probably be > > text *volatile connection_text = NULL; > > similar to commit 6307b096e25. Seems like the issue is a bit bigger to me. Isn't the whole way the function uses PG_TRY / PG_FINALLY just plain odd? The only reason connection_text needs to be volatile is because it's modified in PG_TRY and then accessed in PG_FINALLY. But what's the point of the latter? If an error was thrown, why would we want to construct the 'result' string, as the error isn't caught, there's no PG_CATCH. So now the function builds the result string in case of an error, just to throw it away. Am I missing something? I'm also rather unconvinced by ForeignServerConnectionString() creating a temporary memory context. When would you ever use the function in a long lived memory context? Greetings, Andres
On Wed, 2026-03-11 at 17:49 -0400, Andres Freund wrote:
> The only reason connection_text needs to be volatile is because it's
> modified
> in PG_TRY and then accessed in PG_FINALLY. But what's the point of
> the
> latter? If an error was thrown, why would we want to construct the
> 'result'
> string, as the error isn't caught, there's no PG_CATCH. So now the
> function
> builds the result string in case of an error, just to throw it away.
I believe it would be correct if the variable was not volatile at all,
because it's only set in the last statement of PG_TRY(), so the
PG_FINALLY() will never need to read the modified variable on the error
path. I added the volatile to follow the expected pattern, in case the
code is adjusted later.
That leaves some awkwardness, but I don't immediately see a cleaner way
to do it. Suggestions welcome.
> I'm also rather unconvinced by ForeignServerConnectionString()
> creating a
> temporary memory context. When would you ever use the function in a
> long lived
> memory context?
It's called by GetSubscription(), and the context is ApplyContext.
Regards,
Jeff Davis
On Wed, 2026-03-11 at 17:59 -0700, Jeff Davis wrote:
> That leaves some awkwardness, but I don't immediately see a cleaner
> way
> to do it. Suggestions welcome.
I did see a way to avoid the volatile entirely, it just needs a
redundant MemoryContextSwtichTo(). I think this is cleaner overall,
attached.
Regards,
Jeff Davis
Вложения
Hi, On 2026-03-11 17:59:40 -0700, Jeff Davis wrote: > On Wed, 2026-03-11 at 17:49 -0400, Andres Freund wrote: > > The only reason connection_text needs to be volatile is because it's > > modified > > in PG_TRY and then accessed in PG_FINALLY. But what's the point of > > the > > latter? If an error was thrown, why would we want to construct the > > 'result' > > string, as the error isn't caught, there's no PG_CATCH. So now the > > function > > builds the result string in case of an error, just to throw it away. > > I believe it would be correct if the variable was not volatile at all, > because it's only set in the last statement of PG_TRY(), so the > PG_FINALLY() will never need to read the modified variable on the error > path. I added the volatile to follow the expected pattern, in case the > code is adjusted later. > > That leaves some awkwardness, but I don't immediately see a cleaner way > to do it. Suggestions welcome. > > > I'm also rather unconvinced by ForeignServerConnectionString() > > creating a > > temporary memory context. When would you ever use the function in a > > long lived > > memory context? > > It's called by GetSubscription(), and the context is ApplyContext. GetSubscription() does a dozen allocations or so, so I'm not sure it matters that ForeignServerConnectionString() would do another few. There is FreeSubscription(), but it only seems to free a subset of the allocations, and none of the temporary allocations that are surely made below GetSubscription(). But regardless of that, even if you want to make ForeignServerConnectionString() not leak, I don't think that means you need to use PG_TRY/FINALLY. If there's an error, afaict the callers will just throw away the apply context, no? Otherwise there'd likely be way more problems. So just create the context without a PG_TRY and delete it in the success case. In the failure case it'll be cleaned up by error handling. And if you do need the PG_TRY for some reason, why not do the text_to_cstring() call inside the PG_TRY, since it never can have a value if an error was thrown? Greetings, Andres Freund
On Fri, 2026-03-13 at 09:54 -0400, Andres Freund wrote: > GetSubscription() does a dozen allocations or so, so I'm not sure it > matters > that ForeignServerConnectionString() would do another few. There is > FreeSubscription(), but it only seems to free a subset of the > allocations, and > none of the temporary allocations that are surely made below > GetSubscription(). That by itself seems like a problem. If FreeSubscription() is needed, then it should do its job; and if it's not needed, then it should be eliminated. To me it looks like it's needed. Otherwise there will be leaks for every invalidation. > But regardless of that, even if you want to make > ForeignServerConnectionString() not leak, I don't think that means > you need to > use PG_TRY/FINALLY. If there's an error, afaict the callers will just > throw > away the apply context, no? Otherwise there'd likely be way more > problems. I believe that's correct. While there's no delete or reset, it looks like an error will cause the worker to exit/restart. > So > just create the context without a PG_TRY and delete it in the success > case. > In the failure case it'll be cleaned up by error handling. Thank you, patch attached. > And if you do need the PG_TRY for some reason, why not do the > text_to_cstring() call inside the PG_TRY, since it never can have a > value if > an error was thrown? That's what I did in the patch I posted here: https://www.postgresql.org/message-id/f48610c90e69de4b30841361c568c3765e8f3dfe.camel%40j-davis.com but I think you are right that we don't need the try/catch at all. Regards, Jeff Davis
Вложения
Hi, On 2026-03-13 13:37:29 -0700, Jeff Davis wrote: > On Fri, 2026-03-13 at 09:54 -0400, Andres Freund wrote: > > GetSubscription() does a dozen allocations or so, so I'm not sure it > > matters > > that ForeignServerConnectionString() would do another few. There is > > FreeSubscription(), but it only seems to free a subset of the > > allocations, and > > none of the temporary allocations that are surely made below > > GetSubscription(). > > That by itself seems like a problem. If FreeSubscription() is needed, > then it should do its job; and if it's not needed, then it should be > eliminated. No argument there. > To me it looks like it's needed. Otherwise there will be leaks for > every invalidation. It does indeed look like it'd leak. I suspect the more scalable approach would be to create a dedicated memory context for each GetSubscription() call that's then torn down during invalidation. > > So > > just create the context without a PG_TRY and delete it in the success > > case. > > In the failure case it'll be cleaned up by error handling. > > Thank you, patch attached. LGTM on a quick scan. > > And if you do need the PG_TRY for some reason, why not do the > > text_to_cstring() call inside the PG_TRY, since it never can have a > > value if > > an error was thrown? > > That's what I did in the patch I posted here: > > https://www.postgresql.org/message-id/f48610c90e69de4b30841361c568c3765e8f3dfe.camel%40j-davis.com > > but I think you are right that we don't need the try/catch at all. I was behind on my email, so I hadn't yet seen that :) Greetings, Andres
On Fri, 2026-03-13 at 16:48 -0400, Andres Freund wrote:
> I suspect the more scalable approach would be to create a dedicated
> memory
> context for each GetSubscription() call that's then torn down during
> invalidation.
Attached. This is posted as an alternative to the previous patch.
Regards,
Jeff Davis