Re: [17] CREATE SUBSCRIPTION ... SERVER

On Tue, 2024-01-16 at 09:23 +0530, Bharath Rupireddy wrote:
> 1.
>  May be a more descriptive note is
> worth here instead of just saying "Load the library providing us
> libpq calls."?

OK, will be in the next patch set.

> 2. Why not typedef keyword before the ConnectionOption structure?

Agreed. An earlier unpublished iteration had the struct more localized,
but here it makes more sense to be typedef'd.

> 3.
> +static const struct ConnectionOption *
> +libpqrcv_conninfo_options(void)
> 
> Why is libpqrcv_conninfo_options returning the const
> ConnectionOption?

I did that so I could save the result, and each subsequent call would
be free (just returning the same pointer). That also means that the
caller doesn't need to free the result, which would require another
entry point in the API.

> 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.

The result of PQconndefaults() can change from call to call when the
defaults change. libpqrcv_conninfo_options() only depends on the
available option names (and dispchar), which should be a static list.

> 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)?

I'm not quite sure of your question, but I'll try to improve the
comment.

> 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.

Will do so in the next patch set.

Thank you for taking a look.

Regards,
	Jeff Davis