Re: Improve the HINT message of the ALTER command for postgres_fdw

On Wed, Oct 13, 2021 at 11:06 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/10/13 14:00, Bharath Rupireddy wrote:
> > On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
> > <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >> BTW, I found file_fdw.c, dblink.c, postgres_fdw/option.c and foreign.c
> >> use different error codes for the same error message as follows.
> >> They should use the same error code? If yes, ISTM that
> >> ERRCODE_FDW_INVALID_OPTION_NAME is better because
> >> the error message is "invalid option ...".
> >>
> >> - ERRCODE_FDW_INVALID_OPTION_NAME (file_fdw.c)
> >> - ERRCODE_FDW_OPTION_NAME_NOT_FOUND (dblink.c)
> >> - ERRCODE_FDW_INVALID_OPTION_NAME (postgres_fdw/option.c)
> >> - ERRCODE_SYNTAX_ERROR (foreign.c)
> >
> > Good catch. ERRCODE_FDW_INVALID_OPTION_NAME seems reasonable to me. I
> > think we can remove the error code ERRCODE_FDW_OPTION_NAME_NOT_FOUND
> > (it is being used only by dblink.c), instead use
> > ERRCODE_FDW_INVALID_OPTION_NAME for all option parsing and
> > validations.
>
> Alvaro told me the difference of those error codes as follows at [1].
> This makes me think that ERRCODE_FDW_OPTION_NAME_NOT_FOUND
> is more proper for the error message. Thought?
>
> -----------------------
> in SQL/MED compare GetServerOptByName: INVALID OPTION NAME is used
> when the buffer length does not match the option name length;
> OPTION NAME NOT FOUND is used when an option of that name is not found
> -----------------------
>
> [1] https://twitter.com/alvherre/status/1447991206286348302

I'm fine with the distinction that's made, now I'm thinking about the
appropriate areas where ERRCODE_FDW_INVALID_OPTION_NAME can be used.
Is it correct to use ERRCODE_FDW_INVALID_OPTION_NAME in
postgresImportForeignSchema where we don't check buffer length and
option name length but throw the error when we don't find what's being
expected for IMPORT FOREIGN SCHEMA command? Isn't it the
ERRCODE_FDW_OPTION_NAME_NOT_FOUND right choice there? I've seen some
of the option parsing logic(with the search text "stmt->options)" in
the code base), they are mostly using "option \"%s\" not recognized"
without an error code or "unrecognized XXXX option \"%s\"" with
ERRCODE_SYNTAX_ERROR. I'm not sure which is right. If not in
postgresImportForeignSchema, where else can
ERRCODE_FDW_INVALID_OPTION_NAME be used?

If we were to retain the error code ERRCODE_FDW_INVALID_OPTION_NAME,
do we need to maintain the difference in documentation or in code
comments or in the commit message at least?

Regards,
Bharath Rupireddy.