On 09/01/2013 12:53 AM, Stephen Frost wrote:
> * Stefan Kaltenbrunner (ste...@kaltenbrunner.cc) wrote:
>> On 08/18/2013 05:40 PM, Tom Lane wrote:
>>> Stefan Kaltenbrunner <ste...@kaltenbrunner.cc> writes:
>>>> While working on upgrading the database of the search system on
>>>> postgresql.org to 9.2 I noticed that the dumps that pg_dump generates on
>>>> that system are actually invalid and cannot be reloaded without being
>>>> hacked on manually...
>>>
>>>> CREATE TEXT SEARCH CONFIGURATION pg (
>>>> PARSER = pg_catalog."default" );
>>>
>>>> CREATE FUNCTION test() RETURNS INTEGER
>>>> LANGUAGE sql SET default_text_search_config TO 'public.pg' AS $$
>>>> SELECT 1;
>>>> $$;
>>>
>>>> once you dump that you will end up with an invalid dump because the
>>>> function will be dumped before the actual text search configuration is
>>>> (re)created.
>>>
>>> I don't think it will work to try to fix this by reordering the dump;
>>> it's too easy to imagine scenarios where that would lead to circular
>>> ordering constraints. What seems like a more workable answer is for
>>> CREATE FUNCTION to not attempt to validate SET clauses when
>>> check_function_bodies is off, or at least not throw a hard error when
>>> the validation fails. (I see for instance that if you try
>>> ALTER ROLE joe SET default_text_search_config TO nonesuch;
>>> you just get a notice and not an error.)
>>>
>>> However, I don't recall if there are any places where we assume the
>>> SET info was pre-validated by CREATE/ALTER FUNCTION.
>>
>> any further insights into that issue? - seems a bit silly to have an
>> open bug that actually prevents us from taking (restorable) backups of
>> the search system on our own website...
>
> It would seem that a simple solution would be to add an elevel argument
> to ProcessGUCArray and then call it with NOTICE in the case that
> check_function_bodies is true. None of the contrib modules call
> ProcessGUCArray, but should we worry that some external module does?
attached is a rough patch that does exactly that, if we are worried
about an api change we could simple do a ProcessGUCArrayNotice() in the
backbranches if that approach is actually sane.
>
> This doesn't address Tom's concern that we may trust in the SET to
> ensure that the value stored is valid. That seems like it'd be pretty
> odd given how we typically handle GUCs, but I've not done a
> comprehensive review to be sure.
well the whole per-database/per-user GUC handling is already pretty
weird/inconsistent, if you for example alter a database with an invalid
default_text_search_config you get a NOTICE about it, every time you
connect to that database later on you get a WARNING.
mastermind=# alter database mastermind set default_text_search_config to
'foo';
NOTICE: text search configuration "foo" does not exist
ALTER DATABASE
mastermind=# \q
mastermind@powerbrain:~$ psql
WARNING: invalid value for parameter "default_text_search_config": "foo"
psql (9.1.9)
Type "help" for help.
>
> Like Stefan, I'd really like to see this fixed, and sooner rather than
> later, so we can continue the process of upgrading our systems to 9.2..
well - we can certainly work around it but others might not...
Stefan
diff --git a/src/backend/catalog/pg_db_role_setting.c b/src/backend/catalog/pg_db_role_setting.c
index 6e19736..7fda64c
*** a/src/backend/catalog/pg_db_role_setting.c
--- b/src/backend/catalog/pg_db_role_setting.c
*************** ApplySetting(Snapshot snapshot, Oid data
*** 262,268 ****
* right to insert an option into pg_db_role_setting was checked
* when it was inserted.
*/
! ProcessGUCArray(a, PGC_SUSET, source, GUC_ACTION_SET);
}
}
--- 262,268 ----
* right to insert an option into pg_db_role_setting was checked
* when it was inserted.
*/
! ProcessGUCArray(a, PGC_SUSET, source, GUC_ACTION_SET,0);
}
}
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 2a98ca9..5ecc630
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
*************** ProcedureCreate(const char *procedureNam
*** 679,688 ****
if (set_items) /* Need a new GUC nesting level */
{
save_nestlevel = NewGUCNestLevel();
ProcessGUCArray(set_items,
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
! GUC_ACTION_SAVE);
}
else
save_nestlevel = 0; /* keep compiler quiet */
--- 679,699 ----
if (set_items) /* Need a new GUC nesting level */
{
save_nestlevel = NewGUCNestLevel();
+ /* reduce elevel to NOTICE if check_function_bodies is disabled */
+ if (check_function_bodies) {
ProcessGUCArray(set_items,
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
! GUC_ACTION_SAVE,
! 0);
! }
! else {
! ProcessGUCArray(set_items,
! (superuser() ? PGC_SUSET : PGC_USERSET),
! PGC_S_SESSION,
! GUC_ACTION_SAVE,
! NOTICE);
! }
}
else
save_nestlevel = 0; /* keep compiler quiet */
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 42de04c..7d323eb
*** a/src/backend/utils/fmgr/fmgr.c
--- b/src/backend/utils/fmgr/fmgr.c
*************** fmgr_security_definer(PG_FUNCTION_ARGS)
*** 951,957 ****
ProcessGUCArray(fcache->proconfig,
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
! GUC_ACTION_SAVE);
}
/* function manager hook */
--- 951,958 ----
ProcessGUCArray(fcache->proconfig,
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
! GUC_ACTION_SAVE,
! 0);
}
/* function manager hook */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0b23c38..15e4aab
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** ParseLongOption(const char *string, char
*** 7820,7832 ****
/*
* Handle options fetched from pg_db_role_setting.setconfig,
! * pg_proc.proconfig, etc. Caller must specify proper context/source/action.
*
* The array parameter must be an array of TEXT (it must not be NULL).
*/
void
ProcessGUCArray(ArrayType *array,
! GucContext context, GucSource source, GucAction action)
{
int i;
--- 7820,7832 ----
/*
* Handle options fetched from pg_db_role_setting.setconfig,
! * pg_proc.proconfig, etc. Caller must specify proper context/source/action/elevel.
*
* The array parameter must be an array of TEXT (it must not be NULL).
*/
void
ProcessGUCArray(ArrayType *array,
! GucContext context, GucSource source, GucAction action,int elevel)
{
int i;
*************** ProcessGUCArray(ArrayType *array,
*** 7868,7874 ****
(void) set_config_option(name, value,
context, source,
! action, true, 0);
free(name);
if (value)
--- 7868,7874 ----
(void) set_config_option(name, value,
context, source,
! action, true, elevel);
free(name);
if (value)
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d497b1f..fb4b8f5
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** extern void ExecSetVariableStmt(Variable
*** 334,340 ****
extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
extern void ProcessGUCArray(ArrayType *array,
! GucContext context, GucSource source, GucAction action);
extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
extern ArrayType *GUCArrayReset(ArrayType *array);
--- 334,340 ----
extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
extern void ProcessGUCArray(ArrayType *array,
! GucContext context, GucSource source, GucAction action, int elevel);
extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
extern ArrayType *GUCArrayReset(ArrayType *array);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
