Re: [17] CREATE SUBSCRIPTION ... SERVER

On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2024-01-23 at 15:21 +0530, Ashutosh Bapat wrote:
> > I am with the prefix. The changes it causes make review difficult. If
> > you can separate those changes into a patch that will help.
>
> I ended up just removing the dummy FDW. Real users are likely to want
> to use postgres_fdw, and if not, it's easy enough to issue a CREATE
> FOREIGN DATA WRAPPER. Or I can bring it back if desired.
>
> Updated patch set (patches are renumbered):
>
>   * removed dummy FDW and test churn
>   * made a new pg_connection_validator function which leaves
> postgresql_fdw_validator in place. (I didn't document the new function
> -- should I?)
>   * included your tests improvements
>   * removed dependency from the subscription to the user mapping -- we
> don't depend on the user mapping for foreign tables, so we shouldn't
> depend on them here. Of course a change to a user mapping still
> invalidates the subscription worker and it will restart.
>   * general cleanup
>
> Overall it's simpler and hopefully easier to review. The patch to
> introduce the pg_create_connection role could use some more discussion,
> but I believe 0001 and 0002 are nearly ready.

Thanks for the patches. I have some comments on v9-0001:

1.
+SELECT pg_conninfo_from_server('testserver1', CURRENT_USER, false);
+      pg_conninfo_from_server
+-----------------------------------
+ user = 'value' password = 'value'

Isn't this function an unsafe one as it shows the password? I don't
see its access being revoked from the public. If it seems important
for one to understand how the server forms a connection string by
gathering bits and pieces from foreign server and user mapping, why
can't it look for the password in the result string and mask it before
returning it as output?

2.
+ */
+typedef const struct ConnectionOption *(*walrcv_conninfo_options_fn) (void);
+

struct here is unnecessary as the structure definition of
ConnectionOption is typedef-ed already.

3.
+  OPTIONS (user 'publicuser', password $pwd$'\"$# secret'$pwd$);

Is pwd here present working directory name? If yes, isn't it going to
be different on BF animals making test output unstable?

4.
-struct ConnectionOption
+struct TestConnectionOption
 {

How about say PgFdwConnectionOption instead of TestConnectionOption?

5. Comment #4 makes me think - why not get rid of
postgresql_fdw_validator altogether and use pg_connection_validator
instead for testing purposes? The tests don't complain much, see the
patch Remove-deprecated-postgresql_fdw_validator.diff created on top
of v9-0001.

I'll continue to review the other patches.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com