Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
On Sun, Sep 1, 2013 at 10:36 AM, Stefan Kaltenbrunner ste...@kaltenbrunner.cc wrote: 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 patch has some definite coding-style issues, but those should be easy to fix. The bigger thing I worry about is whether distributing the decision as to what elevel ought to be used here all over the code base is indeed sane. Perhaps that ship has already sailed, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
On 09/03/2013 06:15 PM, Robert Haas wrote: On Sun, Sep 1, 2013 at 10:36 AM, Stefan Kaltenbrunner ste...@kaltenbrunner.cc wrote: 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 patch has some definite coding-style issues, but those should be easy to fix. The bigger thing I worry about is whether distributing the decision as to what elevel ought to be used here all over the code base is indeed sane. Perhaps that ship has already sailed, though. I can certainly fix the coding style thing up - but it was declared as rough mostly because I'm not entirely sure the way this is going is actually the right way to attack this... This whole stuff seems to be a bit messy and bolted on in some ways. There is ProcessGUCArray(), but also set_config_option() and its external wrapper SetConfigOption() - the division of labour between the caller deciding on what it wants vs what the function does with some combination of elevel and source internally is not very consistent at best. I also note that a lot of places are actually calling set_config_option() directly, so maybe there is an opportunity to unify here as well. Stefan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
* Tom Lane (t...@sss.pgh.pa.us) wrote: So I think a more reasonable fix is just to skip the ProcessGUCArray call altogether when check_function_bodies = false, and to document that validators mustn't assume anything about the GUC environment when check_function_bodies = false. If no objections, I'll go fix it that way. Works for me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
Robert Haas robertmh...@gmail.com writes: On Sun, Sep 1, 2013 at 10:36 AM, Stefan Kaltenbrunner ste...@kaltenbrunner.cc wrote: 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 patch has some definite coding-style issues, but those should be easy to fix. The bigger thing I worry about is whether distributing the decision as to what elevel ought to be used here all over the code base is indeed sane. Perhaps that ship has already sailed, though. I don't particularly care for this approach, for that reason and others. One such issue is that it would result in duplicate messages, because functioncmds.c will already have issued warnings thanks to the GUCArrayAdd calls it makes. (Those calls are made with context PGC_S_TEST, which is why they only throw warnings not errors, cf validate_option_array_item.) But here's the big-picture consideration: the reason that we call ProcessGUCArray in this place in ProcedureCreate is that we are trying to set up the correct GUC environment for the function validator. As an example, if the SET clause for a SQL-language function includes a setting for search_path, we had better adopt that search path while checking the function body, or we will come to incorrect conclusions. If we use this patch, we'd be saying that we're okay with failing to apply the SET clause before calling the validator, any time the requested GUC value happens to be invalid. That's not terribly comfortable: in general we'd be assuming that validators' results don't depend on the GUC environment, which is clearly false. Now we can probably get away with that in the context of check_function_bodies = false, because the validator shouldn't be doing much of anything then (although a few of them check some things anyway...). But if we're going to assume that the validator doesn't depend on GUC environment when check_function_bodies = false, why apply the SET clause at all? So I think a more reasonable fix is just to skip the ProcessGUCArray call altogether when check_function_bodies = false, and to document that validators mustn't assume anything about the GUC environment when check_function_bodies = false. If no objections, I'll go fix it that way. BTW, I notice the various comments about PGC_S_TEST context are out of date, because they don't mention that it is used for CREATE FUNCTION SET cases ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
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
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
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... Stefan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
* 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? 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. 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.. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] CREATE FUNCTION .. SET vs. pg_dump
Hi all! 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... Simple way to reproduce is using the following: 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 have not checked in any more detail but I suspect that this problem is not only affecting default_text_search_config. Stefan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE FUNCTION .. SET vs. pg_dump
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.) hmm yeah - just throwing a NOTICE with check_function_bodies=off seems like reasonable workaround to this problem area. Not sure it would be required to turn it into a NOTICE in general, though alter role/alter database seems like an established precedence for this. Stefan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
