public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jeff Davis <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Joe Conway <[email protected]>
Cc: [email protected]
Subject: Re: [17] CREATE SUBSCRIPTION ... SERVER
Date: Wed, 17 Jan 2024 23:17:01 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALj2ACXw81_Cv36qL+twaLjX1_L59CctufKbYdvm9pyC_9GsWQ@mail.gmail.com>
References: <[email protected]>
<CAExHW5vv-78ixJs3arsuZ+12A4AyqehJnDPhejp0m4FAKG6JZA@mail.gmail.com>
<[email protected]>
<CAExHW5sAo4Gv3AwuxQNGSQ2Z45B8_MZEb-SG-B8D-hLMdrquAg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ@mail.gmail.com>
<[email protected]>
<CAExHW5sQVJeCgCGMkum_k4Z4VPaK5GkdAjHW-d+F3MngBZ9qBA@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CALj2ACXDua2Az15Kj3OZFaRm49G8-faemiEEv8t9GNCcsxv8Hw@mail.gmail.com>
<[email protected]>
<CAExHW5v5poohyfNRVQp3yjVE56ej0dWvVMbysSSVtg21aHT3uQ@mail.gmail.com>
<[email protected]>
<CAExHW5vF+cp9efQj=-W+Rhbr9f9ZZqt3XFnP0NB5jKsm=C=5tw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CALj2ACXw81_Cv36qL+twaLjX1_L59CctufKbYdvm9pyC_9GsWQ@mail.gmail.com>
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
view thread (63+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [17] CREATE SUBSCRIPTION ... SERVER
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox