Re: [17] CREATE SUBSCRIPTION ... SERVER

On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> > I think 0004 needs a bit more work, so I'm leaving it off for now,
> > but
> > I'll bring it back in the next patch set.
>
> Here's the next patch set. 0001 - 0003 are mostly the same with some
> improved error messages and some code fixes. I am looking to start
> committing 0001 - 0003 soon, as they have received some feedback
> already and 0004 isn't required for the earlier patches to be useful.

Thanks. Here are some comments on 0001. I'll look at other patches very soon.

1.
+    /* Load the library providing us libpq calls. */
+    load_file("libpqwalreceiver", false);

At first glance, it looks odd that libpqwalreceiver library is being
linked to every backend that uses postgresql_fdw_validator. After a
bit of grokking, this feels/is a better and easiest way to not link
libpq to the main postgresql executable as specified at the beginning
of libpqwalreceiver.c file comments. May be a more descriptive note is
worth here instead of just saying "Load the library providing us libpq
calls."?

2. Why not typedef keyword before the ConnectionOption structure? This
way all the "struct ConnectionOption" can be remvoed, no? I know the
previously there is no typedef, but we can add it now so that the code
looks cleaner.

typedef struct ConnectionOption
{
    const char *optname;
    bool        issecret;        /* is option for a password? */
    bool        isdebug;        /* is option a debug option? */
} ConnectionOption;

FWIW, with the above change and removal of struct before every use of
ConnectionOption, the code compiles cleanly for me.

3.
+static const struct ConnectionOption *
+libpqrcv_conninfo_options(void)

Why is libpqrcv_conninfo_options returning the const ConnectionOption?
Is it that we don't expect callers to modify the result? I think it's
not needed given the fact that PQconndefaults doesn't constify the
return value.

4.
+    /* skip options that must be overridden */
+    if (strcmp(option, "client_encoding") == 0)
+        return false;
+

Options that must be overriden or disallow specifiing
"client_encoding" in the SERVER/USER MAPPING definition (just like the
dblink)?

    /* Disallow "client_encoding" */
    if (strcmp(opt->keyword, "client_encoding") == 0)
        return false;

5.
"By using the correct libpq options, it no longer needs to be
deprecated, and can be used by the upcoming pg_connection_fdw."

Use of postgresql_fdw_validator for pg_connection_fdw seems a bit odd
to me. I don't mind pg_connection_fdw having its own validator
pg_connection_fdw_validator even if it duplicates the code. To avoid
code duplication we can move the guts to an internal function in
foreign.c so that both postgresql_fdw_validator and
pg_connection_fdw_validator can use it. This way the code is cleaner
and we can just leave postgresql_fdw_validator as deprecated.

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