Re: CREATE FUNCTION .. SET vs. pg_dump

On 09/01/2013 12:53 AM, Stephen Frost wrote:
> * Stefan Kaltenbrunner (stefan(at)kaltenbrunner(dot)cc) wrote:
>> On 08/18/2013 05:40 PM, Tom Lane wrote:
>>> Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)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(at)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