On Mon, Jan 29, 2024 at 11:11 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis <pg...@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.
I forgot to attach the diff patch as specified in comment #5, please find the attached. Sorry for the noise. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Remove-deprecated-postgresql_fdw_validator.diff
Description: Binary data