Re: fdw validation function vs zero catalog id - Mailing list pgsql-hackers

I wrote:
> The validator is run for the generic options specified to CREATE/ALTER FDW,
> SERVER and USER MAPPING (+ possible future SQL/MED objects). In this case the
> catalog is always known. Also we can assume that the catalog is known, if a user
> runs the validator directly. So yes, AFAICS there is no case for the "or zero".
>

Updated patch attached. This now also removes the "or zero" note from
the documentation and modifies postgresql_fdw_validator() to assume that
a valid catalog oid is always passed.

regards,
Martin

*** a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
--- b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
***************
*** 74,83 **** CREATE FOREIGN DATA WRAPPER <replaceable class="parameter">name</replaceable>
        take two arguments: one of type <type>text[]</type>, which will
        contain the array of options as stored in the system catalogs,
        and one of type <type>oid</type>, which will be the OID of the
!       system catalog containing the options, or zero if the context is
!       not known.  The return type is ignored; the function should
!       indicate invalid options using
!       the <function>ereport()</function> function.
       </para>
      </listitem>
     </varlistentry>
--- 74,82 ----
        take two arguments: one of type <type>text[]</type>, which will
        contain the array of options as stored in the system catalogs,
        and one of type <type>oid</type>, which will be the OID of the
!       system catalog containing the options. The return type is ignored;
!       the function should indicate invalid options using the
!       <function>ereport()</function> function.
       </para>
      </listitem>
     </varlistentry>
*** a/src/backend/commands/foreigncmds.c
--- b/src/backend/commands/foreigncmds.c
***************
*** 88,94 **** optionListToArray(List *options)
   * This is used by CREATE/ALTER of FOREIGN DATA WRAPPER/SERVER/USER MAPPING.
   */
  static Datum
! transformGenericOptions(Datum oldOptions,
                          List *options,
                          Oid fdwvalidator)
  {
--- 88,95 ----
   * This is used by CREATE/ALTER of FOREIGN DATA WRAPPER/SERVER/USER MAPPING.
   */
  static Datum
! transformGenericOptions(Oid catalogId,
!                         Datum oldOptions,
                          List *options,
                          Oid fdwvalidator)
  {
***************
*** 162,168 **** transformGenericOptions(Datum oldOptions,
      result = optionListToArray(resultOptions);

      if (fdwvalidator)
!         OidFunctionCall2(fdwvalidator, result, (Datum) 0);

      return result;
  }
--- 163,169 ----
      result = optionListToArray(resultOptions);

      if (fdwvalidator)
!         OidFunctionCall2(fdwvalidator, result, ObjectIdGetDatum(catalogId));

      return result;
  }
***************
*** 384,390 **** CreateForeignDataWrapper(CreateFdwStmt *stmt)

      nulls[Anum_pg_foreign_data_wrapper_fdwacl - 1] = true;

!     fdwoptions = transformGenericOptions(PointerGetDatum(NULL), stmt->options,
                                           fdwvalidator);

      if (PointerIsValid(DatumGetPointer(fdwoptions)))
--- 385,393 ----

      nulls[Anum_pg_foreign_data_wrapper_fdwacl - 1] = true;

!     fdwoptions = transformGenericOptions(ForeignDataWrapperRelationId,
!                                          PointerGetDatum(NULL),
!                                          stmt->options,
                                           fdwvalidator);

      if (PointerIsValid(DatumGetPointer(fdwoptions)))
***************
*** 501,507 **** AlterForeignDataWrapper(AlterFdwStmt *stmt)
              datum = PointerGetDatum(NULL);

          /* Transform the options */
!         datum = transformGenericOptions(datum, stmt->options, fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
              repl_val[Anum_pg_foreign_data_wrapper_fdwoptions - 1] = datum;
--- 504,513 ----
              datum = PointerGetDatum(NULL);

          /* Transform the options */
!         datum = transformGenericOptions(ForeignDataWrapperRelationId,
!                                         datum,
!                                         stmt->options,
!                                         fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
              repl_val[Anum_pg_foreign_data_wrapper_fdwoptions - 1] = datum;
***************
*** 667,673 **** CreateForeignServer(CreateForeignServerStmt *stmt)
      nulls[Anum_pg_foreign_server_srvacl - 1] = true;

      /* Add server options */
!     srvoptions = transformGenericOptions(PointerGetDatum(NULL), stmt->options,
                                           fdw->fdwvalidator);

      if (PointerIsValid(DatumGetPointer(srvoptions)))
--- 673,681 ----
      nulls[Anum_pg_foreign_server_srvacl - 1] = true;

      /* Add server options */
!     srvoptions = transformGenericOptions(ForeignServerRelationId,
!                                          PointerGetDatum(NULL),
!                                          stmt->options,
                                           fdw->fdwvalidator);

      if (PointerIsValid(DatumGetPointer(srvoptions)))
***************
*** 765,771 **** AlterForeignServer(AlterForeignServerStmt *stmt)
              datum = PointerGetDatum(NULL);

          /* Prepare the options array */
!         datum = transformGenericOptions(datum, stmt->options,
                                          fdw->fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
--- 773,781 ----
              datum = PointerGetDatum(NULL);

          /* Prepare the options array */
!         datum = transformGenericOptions(ForeignServerRelationId,
!                                         datum,
!                                         stmt->options,
                                          fdw->fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
***************
*** 936,942 **** CreateUserMapping(CreateUserMappingStmt *stmt)
      values[Anum_pg_user_mapping_umserver - 1] = ObjectIdGetDatum(srv->serverid);

      /* Add user options */
!     useoptions = transformGenericOptions(PointerGetDatum(NULL), stmt->options,
                                           fdw->fdwvalidator);

      if (PointerIsValid(DatumGetPointer(useoptions)))
--- 946,954 ----
      values[Anum_pg_user_mapping_umserver - 1] = ObjectIdGetDatum(srv->serverid);

      /* Add user options */
!     useoptions = transformGenericOptions(UserMappingRelationId,
!                                          PointerGetDatum(NULL),
!                                          stmt->options,
                                           fdw->fdwvalidator);

      if (PointerIsValid(DatumGetPointer(useoptions)))
***************
*** 1031,1037 **** AlterUserMapping(AlterUserMappingStmt *stmt)
              datum = PointerGetDatum(NULL);

          /* Prepare the options array */
!         datum = transformGenericOptions(datum, stmt->options,
                                          fdw->fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
--- 1043,1051 ----
              datum = PointerGetDatum(NULL);

          /* Prepare the options array */
!         datum = transformGenericOptions(UserMappingRelationId,
!                                         datum,
!                                         stmt->options,
                                          fdw->fdwvalidator);

          if (PointerIsValid(DatumGetPointer(datum)))
*** a/src/backend/foreign/foreign.c
--- b/src/backend/foreign/foreign.c
***************
*** 372,378 **** is_conninfo_option(const char *option, Oid context)
      struct ConnectionOption *opt;

      for (opt = libpq_conninfo_options; opt->optname; opt++)
!         if ((context == opt->optcontext || context == InvalidOid) && strcmp(opt->optname, option) == 0)
              return true;
      return false;
  }
--- 372,378 ----
      struct ConnectionOption *opt;

      for (opt = libpq_conninfo_options; opt->optname; opt++)
!         if (context == opt->optcontext && strcmp(opt->optname, option) == 0)
              return true;
      return false;
  }
***************
*** 409,415 **** postgresql_fdw_validator(PG_FUNCTION_ARGS)
               */
              initStringInfo(&buf);
              for (opt = libpq_conninfo_options; opt->optname; opt++)
!                 if (catalog == InvalidOid || catalog == opt->optcontext)
                      appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
                                       opt->optname);

--- 409,415 ----
               */
              initStringInfo(&buf);
              for (opt = libpq_conninfo_options; opt->optname; opt++)
!                 if (catalog == opt->optcontext)
                      appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
                                       opt->optname);

*** a/src/test/regress/expected/foreign_data.out
--- b/src/test/regress/expected/foreign_data.out
***************
*** 284,290 **** CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna
  CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b');
  CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR
  ERROR:  invalid option "foo"
! HINT:  Valid options in this context are: authtype, service, user, password, connect_timeout, dbname, host, hostaddr,
port,tty, options, requiressl, sslmode, gsslib 
  CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db');
  \des+
                                                  List of foreign servers
--- 284,290 ----
  CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b');
  CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR
  ERROR:  invalid option "foo"
! HINT:  Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty,
options,requiressl, sslmode, gsslib 
  CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db');
  \des+
                                                  List of foreign servers
***************
*** 395,401 **** ERROR:  permission denied for foreign-data wrapper foo
  RESET ROLE;
  ALTER SERVER s8 OPTIONS (foo '1');                          -- ERROR option validation
  ERROR:  invalid option "foo"
! HINT:  Valid options in this context are: authtype, service, user, password, connect_timeout, dbname, host, hostaddr,
port,tty, options, requiressl, sslmode, gsslib 
  ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host);
  SET ROLE regress_test_role;
  ALTER SERVER s1 OWNER TO regress_test_indirect;             -- ERROR
--- 395,401 ----
  RESET ROLE;
  ALTER SERVER s8 OPTIONS (foo '1');                          -- ERROR option validation
  ERROR:  invalid option "foo"
! HINT:  Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty,
options,requiressl, sslmode, gsslib 
  ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host);
  SET ROLE regress_test_role;
  ALTER SERVER s1 OWNER TO regress_test_indirect;             -- ERROR
***************
*** 534,540 **** ERROR:  user mapping "foreign_data_user" already exists for server s4
  CREATE USER MAPPING FOR public SERVER s4 OPTIONS (mapping 'is public');
  CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret');    -- ERROR
  ERROR:  invalid option "username"
! HINT:  Valid options in this context are: authtype, service, user, password, connect_timeout, dbname, host, hostaddr,
port,tty, options, requiressl, sslmode, gsslib 
  CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret');
  ALTER SERVER s5 OWNER TO regress_test_role;
  ALTER SERVER s6 OWNER TO regress_test_indirect;
--- 534,540 ----
  CREATE USER MAPPING FOR public SERVER s4 OPTIONS (mapping 'is public');
  CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret');    -- ERROR
  ERROR:  invalid option "username"
! HINT:  Valid options in this context are: user, password
  CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret');
  ALTER SERVER s5 OWNER TO regress_test_role;
  ALTER SERVER s6 OWNER TO regress_test_indirect;
***************
*** 573,579 **** ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true');            -- E
  ERROR:  user mapping "public" does not exist for the server
  ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test');    -- ERROR
  ERROR:  invalid option "username"
! HINT:  Valid options in this context are: authtype, service, user, password, connect_timeout, dbname, host, hostaddr,
port,tty, options, requiressl, sslmode, gsslib 
  ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public');
  SET ROLE regress_test_role;
  ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1');
--- 573,579 ----
  ERROR:  user mapping "public" does not exist for the server
  ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test');    -- ERROR
  ERROR:  invalid option "username"
! HINT:  Valid options in this context are: user, password
  ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public');
  SET ROLE regress_test_role;
  ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1');