fdw validation function vs zero catalog id

Поиск
Список
Период
Сортировка
От Martin Pihlak
Тема fdw validation function vs zero catalog id
Дата
Msg-id 4B2E28FE.3040800@gmail.com
обсуждение исходный текст
Ответы Re: fdw validation function vs zero catalog id  (Tom Lane <tgl@sss.pgh.pa.us>)
Список pgsql-hackers
It appears that the function for validating generic options to a FDW,
SERVER and USER MAPPING is always passed a catalog oid of 0. Whereas
it should actually be passed the oid of the catalog that the options
apply to.

Attached patch fixes the issue by passing the proper catalog id from
transformGenericOptions().

PS. I personally would like this applied to 8.4 as well -- it'd enable
us to write a proper validator for pl/proxy fdw.

regards,
Martin

*** 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/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');

В списке pgsql-hackers по дате отправления:

Предыдущее
От: James William Pye
Дата:
Сообщение: Re: alpha3 bundled -- please verify
Следующее
От: Pavel Stehule
Дата:
Сообщение: Re: Aggregate ORDER BY patch