Thread: REVIEW: Extensions support for pg_dump
I used the patch from CommitFest application and applied the following commit to fix a known issue: http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=d4991d35283ae0ceeb7f9e4203cf6a9dfb5d128d Is the patch in context diff format? Yes. Does the patch apply cleanly? No: patching file src/include/commands/defrem.h Hunk #2 succeeded at 107 with fuzz 2 (offset 27 lines). Hunk #3 FAILED at 105. 1 out of 5 hunks FAILED -- saving rejects to file src/include/commands/defrem.h.rej I have used the git head of http://git.postgresql.org/gitweb?p=postgresql-extension.git branch extensions to do the rest of reviewing. There is one compiler warning: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_ SOURCE -c -o pg_dump.o pg_dump.c pg_dump.c: In function ‘getTables’: pg_dump.c:3748: warning: too many arguments for format And, make check gives me the following errors: test sanity_check ... FAILED test rules ... FAILED Does it include reasonable tests, necessary doc patches, etc? There is enough documentation, but I think the documentation needs some polishing. I am not a native English speaker, so it might be it is my English that is failing. The data is there, but the representation might be a little off. I didn't spot any tests. This could be that I don't know what to look for... Usability review: The patch implements a way to create extensions. While the patch is labeled extensions support for pg_dump, it actually implements more. It implements a new way to package and install extension, and changes contrib extensions to use the new way. I do think we want these features, and that we do not have those already. As far as I understand, there is nothing in the standard regarding this feature. I wonder if the structure of having all the extensions in share/contrib/ is a good idea. It might be better if one could separate the contrib extensions in one place, and user created extensions in another place. This could make it easy to see what user created extensions is installed in (or installable to) the cluster. I think this might be helpful to DBAs upgrading PostgreSQL. Overall, the system seems intuitive to use. It is relatively easy to create extension using this feature, and loading contrib extensions is really easy. I haven't tested how easy it is to create C-language extensions using this. If I am not mistaken it just adds the compile & install the C-functions before installing the extension. Feature test: The packaging feature works as advertised, expect for the following bugs and inconsistencies. When using the plain CREATE EXTENSION foobar command without schema qualifier, the extension is created in schema public (by name) without regard to the current search path. This is inconsistent with create table, and if the public schema is renamed, the command gives error: ERROR: schema "public" does not exist This makes the name "public" have a special meaning, and I suspect that is not wanted. When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is not relocatable and it's control file uses the SET search_path TO @extschema@, the search_path is set to bar for the session. That is, it is not changed to the original search_path after the command has completed. When trying to load extensions which contain identical signatured functions, if the loading will succeed is dependent on the search path. If there is a conflicting function in search path (first extension loaded to schema public), then the second extension load will fail. But if the order is reversed (first extension loaded to schema foo which is not in search path), then the second extension can be loaded to the public schema. While it is not possible to drop functions in extensions, it is possible to rename a function, and also to CREATE OR REPLACE a function in an extension. After renaming or CORing a function, it is possible to drop the function. I also wonder if alter function ... set parameter should be allowed? There is no validation for the extension names in share/contrib/. It is possible to have extensions control files named ".control", ".name.control", "name''.control" etc. While it is stupid to name them so, it might be better to give an explicit warning / error in these cases. It is of course possible to use these extensions. If there is no comment for a given extension in the .control file, then \dx will not show the extension installed even if it is installed. I was able to make it crash: postgres=# alter extension foo.bar set schema baz; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. Log contains this: TRAP: FailedAssertion("!(list_length(name) == 1)", File: "extension.c", Line: 1163) It doesn't matter if the schema foo exists or not, and if the extension is there or not. I haven't done anything that could be called review on the code level, but I have quickly glanced over the patch. Some things that caught my eye: In file /src/backend/commands/extension.c: + * The extension control file format is the most simple name = value, we + * don't need anything more there. The SQL file to execute commands from is + * hardcoded to `pg_config --sharedir`/<extension>.install.sql. This seems to be outdated. In file src/bin/pg_dump/pg_dump.c: ! /* ! * So we want the namespaces, but we want to filter out any ! * namespace created by an extension's script. That's unless the ! * user went over his head and created objects into the extension's ! * schema: we now want the schema not to be filtered out to avoid: ! * ! * pg_dump: schema with OID 77869 does not exist ! */ I wonder what that last line is doing there... I haven't had time to review the pg_dump part of the patch yet, will do that next (tomorrow). I hope it is OK to post a partial review... - Anssi
Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011: > While it is not possible to drop functions in extensions, it is possible > to rename a function, and also to CREATE OR REPLACE a function in an > extension. After renaming or CORing a function, it is possible to drop > the function. Hmm, this seems a serious problem. I imagine that what's going on is that the function cannot be dropped because the extension depends on it; but after the rename, the dependencies of the function are dropped and recreated, but the dependency that relates it to the extension is forgotten. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Mon, Jan 17, 2011 at 10:41 AM, Anssi Kääriäinen <anssi.kaariainen@thl.fi> wrote: > I haven't had time to review the pg_dump part of the patch yet, will do that > next (tomorrow). I hope it is OK to post a partial review... It is, and this is a very good and detailed review! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011:
>
>> While it is not possible to drop functions in extensions, it is possible
>> to rename a function, and also to CREATE OR REPLACE a function in an
>> extension. After renaming or CORing a function, it is possible to drop
>> the function.
>
> Hmm, this seems a serious problem. I imagine that what's going on is
> that the function cannot be dropped because the extension depends on it;
> but after the rename, the dependencies of the function are dropped and
> recreated, but the dependency that relates it to the extension is
> forgotten.
Well I'm not seeing that here:
~:5490=# drop function utils.lo_manage_d();
ERROR: cannot drop function utils.lo_manage_d() because extension lo requires it
HINT: You can drop extension lo instead.
src/backend/commands/functioncmds.c
/* rename */namestrcpy(&(procForm->proname), newname);simple_heap_update(rel, &tup->t_self,
tup);CatalogUpdateIndexes(rel,tup);
But here:
src/backend/catalog/pg_proc.c
/* * Create dependencies for the new function. If we are updating an * existing function, first delete any existing
pg_dependentries. * (However, since we are not changing ownership or permissions, the * shared dependencies do *not*
needto change, and we leave them alone.) */if (is_update) deleteDependencyRecordsFor(ProcedureRelationId, retval);
[ ... adding all dependencies back ... ]
/* dependency on extension */if (create_extension){ recordDependencyOn(&myself, &CreateExtensionAddress,
DEPENDENCY_INTERNAL);}
Will investigate some more later.
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Hi, Thanks for your review! Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes: > Does the patch apply cleanly? > No: That was some bitrot, has been fixed, thanks you for working from the git repository meanwhile. > pg_dump.c:3748: warning: too many arguments for format Fixed in v25 already sent this morning. > And, make check gives me the following errors: > test sanity_check ... FAILED > test rules ... FAILED Fixed too. Was due to git conflict solving where it adds a new line in the tests and keep the embedded rowcount the same. I think. > Does it include reasonable tests, necessary doc patches, etc? > > There is enough documentation, but I think the documentation needs some > polishing. I am not a native English speaker, so it might be it is my > English that is failing. The data is there, but the representation might be > a little off. We already made lots of improvements there thanks to David Wheeler reviews in the previous commitfest (which shows up already), and I'll be happy to continue improving as much as I can. If all it needs is english native review, I guess that'll be part of the usual marathon Bruce runs every year there? > I didn't spot any tests. This could be that I don't know what to look for... make -C contrib installcheck will exercise CREATE EXTENSION for each contrib module. > Usability review: > > The patch implements a way to create extensions. While the patch is labeled > extensions support for pg_dump, it actually implements more. It implements a > new way to package and install extension, and changes contrib extensions to > use the new way. Well, all there's in the patch is infrastructure to be able to issue those single lines in your dump :) > I do think we want these features, and that we do not have those already. As > far as I understand, there is nothing in the standard regarding this > feature. > > I wonder if the structure of having all the extensions in share/contrib/ is > a good idea. It might be better if one could separate the contrib extensions > in one place, and user created extensions in another place. This could make > it easy to see what user created extensions is installed in (or installable > to) the cluster. I think this might be helpful to DBAs upgrading PostgreSQL. It's always been this way and I though it wouldn't be in this patch scope to re-organise things. Also I think we should include the UPGRADE needs when we discuss file system level layout. > Overall, the system seems intuitive to use. It is relatively easy to create > extension using this feature, and loading contrib extensions is really > easy. I haven't tested how easy it is to create C-language extensions using > this. If I am not mistaken it just adds the compile & install the > C-functions before installing the extension. It's using PGXS which existed before, all you have to do that's new is care about the Makefile EXTENSION variable and the control file. Even when doing C coded extension work. > When using the plain CREATE EXTENSION foobar command without schema > qualifier, the extension is created in schema public (by name) without > regard to the current search path. This is inconsistent with create table, > and if the public schema is renamed, the command gives error: > > ERROR: schema "public" does not exist > > This makes the name "public" have a special meaning, and I suspect that is > not wanted. Fixed in git, thanks for reporting! ~:5490=# set client_min_messages to debug1; SET ~:5490=# set search_path to utils; SET ~:5490=# create extension unaccent; DEBUG: parse_extension_control_file(unaccent, '/home/dfontaine/pgsql/exts/share/contrib/unaccent.control') DEBUG: CreateExtensionStmt: with user data, schema = 'utils', encoding = '' DEBUG: Installing extension 'unaccent' from '/home/dfontaine/pgsql/exts/share/contrib/unaccent.sql', in schema utils, withuser data CREATE EXTENSION ~:5490=# \dx List of extensionsSchema | Name | Version | Description --------+---------------+----------+-------------------------------------------------utils | citext | 9.1devel |case-insensitive character string typeutils | hstore | 9.1devel | storing sets of key/value pairsutils | int_aggregate| 9.1devel | integer aggregator and an enumerator (obsolete)utils | lo | 9.1devel | managing LargeObjectsutils | unaccent | 9.1devel | text search dictionary that removes accents (5 rows) > When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is not > relocatable and it's control file uses the SET search_path TO @extschema@, > the search_path is set to bar for the session. That is, it is not changed to > the original search_path after the command has completed. It used to work this way with \i, obviously. Should the extension patch care about that and how? Do we want to RESET search_path or to manually manage it like we do for the client_min_messages and log_min_messages? > When trying to load extensions which contain identical signatured functions, > if the loading will succeed is dependent on the search path. If there is a > conflicting function in search path (first extension loaded to schema > public), then the second extension load will fail. But if the order is > reversed (first extension loaded to schema foo which is not in search path), > then the second extension can be loaded to the public schema. Well I think that's an expected limitation here. In the future we might want to add suport for inter-extension dependencies and conflicts, but we're certainly not there yet. > While it is not possible to drop functions in extensions, it is possible to > rename a function, and also to CREATE OR REPLACE a function in an > extension. After renaming or CORing a function, it is possible to drop the > function. I also wonder if alter function ... set parameter should be > allowed? Well there's no specific restrictions in what you can put in an extension's SQL file. I see that as a feature… think upgrade scripts, too. > There is no validation for the extension names in share/contrib/. It is > possible to have extensions control files named ".control", ".name.control", > "name''.control" etc. While it is stupid to name them so, it might be better > to give an explicit warning / error in these cases. It is of course possible > to use these extensions. I don't have a strong opinion here, will wait for some votes. > If there is no comment for a given extension in the .control file, then \dx > will not show the extension installed even if it is installed. Fixed in the git repository. > I was able to make it crash: > > postgres=# alter extension foo.bar set schema baz; Fixed: ~:5490=# alter extension foo.bar set schema baz; ERROR: syntax error at or near "." LINE 1: alter extension foo.bar set schema baz; ^ ~:5490=# alter extension bar set schema baz; ERROR: extension "bar" does not exist > I haven't done anything that could be called review on the code level, but I > have quickly glanced over the patch. Some things that caught my eye: > > In file /src/backend/commands/extension.c: > + * The extension control file format is the most simple name = value, we > + * don't need anything more there. The SQL file to execute commands from is > + * hardcoded to `pg_config --sharedir`/<extension>.install.sql. > > This seems to be outdated. Yes it is. Updated, but now that is covered extensively in the documentation, maybe it could just be removed from the file here? > In file src/bin/pg_dump/pg_dump.c: > > ! /* > ! * So we want the namespaces, but we want to filter out any > ! * namespace created by an extension's script. That's unless the > ! * user went over his head and created objects into the extension's > ! * schema: we now want the schema not to be filtered out to avoid: > ! * > ! * pg_dump: schema with OID 77869 does not exist > ! */ > > I wonder what that last line is doing there... That's the error message you can easily get when you want to have something more simple than the provided SQL… > I haven't had time to review the pg_dump part of the patch yet, will do that > next (tomorrow). I hope it is OK to post a partial review... From here, it's very good! Will you continue from the git repository, where the fixes are available, or do you want a v26 already? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
> Well I'm not seeing that here I am not at work at the moment and I don't have the possibility to compile PostgreSQL on this computer, so the example hereis from memory. The issue I saw was this: assume you have an extension foo, containing one function, test(). CREATE EXTENSION foo; DROP FUNCTION test(); -- restricted due to dependency ALTER FUNCTION test() RENAME TO test2; DROP FUNCTION test2(); -- not restricted! The same can be done using CREATE OR REPLACE. I hope this is not an error on my part. It is possible because I had a lot of schemas and my search_path might have beenwrong... - Anssi PS: Using web email client, I hope this comes out in somewhat sane format.
On 01/17/2011 07:58 PM, Kääriäinen Anssi wrote: > The issue I saw was this: assume you have an extension foo, containing one function, test(). > > CREATE EXTENSION foo; > DROP FUNCTION test(); > -- restricted due to dependency > > ALTER FUNCTION test() RENAME TO test2; > DROP FUNCTION test2(); > -- not restricted! > > The same can be done using CREATE OR REPLACE. > > I hope this is not an error on my part. It is possible because I had a lot of schemas and my search_path might have beenwrong... The rename is an error on my part, sorry for that. Renaming can be done, but dropping is not possible even after rename. But a function in a package can be CREATE OR REPLACEd, and after that the function can be dropped. COR should be restricted in the same way as DROP is. I will check if this is still a problem with the latest patch. Another problem is that you can ALTER FUNCTION test() SET SCHEMA = something_else, and you can alter the functions search_path, which could be a problem for non-relocatable extensions. Even if the schema is changed, dropping extension / altering extension will work as expected. The function is just in different schema than the extension. But, both of these IMO fall in the category "don't do that". - Anssi
On 01/17/2011 06:53 PM, Dimitri Fontaine wrote: >> Usability review: >> >> The patch implements a way to create extensions. While the patch is labeled >> extensions support for pg_dump, it actually implements more. It implements a >> new way to package and install extension, and changes contrib extensions to >> use the new way. > Well, all there's in the patch is infrastructure to be able to issue > those single lines in your dump :) Is this supposed to be used mainly by contrib and PGXN extensions? When I saw the documentation, I immediately thought that this is a nice way to package my application's stored procedures. If this is not one of the intended usages, it should be documented. I can see that this could be problematic when updating PostgreSQL and when recovering from backups. When recovering from backup, you need to have the locally created extension available. But it might be that the extension files are lost when the system went down in flames. Now, the backup is unusable (right?) until extension files can be recovered from source control or where ever they might be stored. This is why I suggested having multiple locations for the extensions. It would be easy to backup locally created extensions if those were in a single directory. All in all, I have a nervous feeling that somebody someday will have an unusable dump because they used this feature, but do not have the extension files available... Also, this part of documentation: The goal of using extensions is so that <application>pg_dump</> knows not to dump all the object definitions into your backups, but rather issue a single <xref linkend="SQL-CREATEEXTENSION"> command. From that, it is not entirely clear to me why this is actually wanted in PostgreSQL. I suppose this will make dump/restore easier to use. But from that paragraph, I get the feeling the only advantage is that your dump will be smaller. And I still have a feeling this implements more. Not that it is a bad thing at all. > It used to work this way with \i, obviously. Should the extension patch > care about that and how? Do we want to RESET search_path or to manually > manage it like we do for the client_min_messages and log_min_messages? It was unintuitive to me to have search_path changed by SQL command as a side effect. When using \i, not so much. >> When trying to load extensions which contain identical signatured functions, >> if the loading will succeed is dependent on the search path. If there is a >> conflicting function in search path (first extension loaded to schema >> public), then the second extension load will fail. But if the order is >> reversed (first extension loaded to schema foo which is not in search path), >> then the second extension can be loaded to the public schema. > Well I think that's an expected limitation here. In the future we might > want to add suport for inter-extension dependencies and conflicts, but > we're certainly not there yet. OK with this as is. It is just a bit surprising that you can create the extensions in one order but not in another. >> There is no validation for the extension names in share/contrib/. It is >> possible to have extensions control files named ".control", ".name.control", >> "name''.control" etc. While it is stupid to name them so, it might be better >> to give an explicit warning / error in these cases. It is of course possible >> to use these extensions. > I don't have a strong opinion here, will wait for some votes. Yeah, this is not a big problem in practice. Just don't name them like that. And if you do, you will find out soon enough that you made a mistake. By the way, in my comment above "It is of course possible to use these extensions." -> "It is of course NOT possible ...". >> I haven't had time to review the pg_dump part of the patch yet, will do that >> next (tomorrow). I hope it is OK to post a partial review... > From here, it's very good! Will you continue from the git repository, > where the fixes are available, or do you want a v26 already? It is easy for me to continue from the Git repo. I will next continue doing the pg_dump part of the review. I hope I have time to complete that today. - Anssi
Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes:
> The rename is an error on my part, sorry for that. Renaming can be done, but
> dropping is not possible even after rename.
Ok, that matches my tests and reading fo the code.
> But a function in a package can
> be CREATE OR REPLACEd, and after that the function can be dropped. COR
> should be restricted in the same way as DROP is. I will check if this is
> still a problem with the latest patch.
There, I've been missing exactly what Alvaro has been telling us about
here, that CREATE OR REPLACE on pg_proc will drop all known dependencies
then recreate them, using deleteDependencyRecordsFor(). As Tom
explained this week, DEPENDENCY_INTERNAL are working in the reverse way
than usual, which means that the function/extension dependency were lost
here.
I've fixed the case by having the code remember the function's extension
if any, and restore it along with the other dependencies. Here's my
test case that now passes:
~:5490=# \dx+ lo
Objects in extension "lo"
Object Class | Object OID | Object Description
--------------+------------+---------------------------------
pg_proc | 33082 | function utils.lo_manage()
pg_proc | 33081 | function utils.lo_oid(utils.lo)
pg_type | 33080 | type utils.lo
(3 rows)
~:5490=# CREATE OR REPLACE FUNCTION utils.lo_manage()
RETURNS pg_catalog.trigger
AS '$libdir/lo'
LANGUAGE C;
CREATE FUNCTION
~:5490=# \dx+ lo
Objects in extension "lo"
Object Class | Object OID | Object Description
--------------+------------+---------------------------------
pg_proc | 33082 | function utils.lo_manage()
pg_proc | 33081 | function utils.lo_oid(utils.lo)
pg_type | 33080 | type utils.lo
(3 rows)
(before the fix it would have missed the utils.lo_manage() function in
this second listing).
The fix is in my git repository and in the attached v26 patch,
containing also the fixes from yesterday. Thanks again for your
complete testing!
> Another problem is that you can ALTER FUNCTION test() SET SCHEMA =
> something_else, and you can alter the functions search_path, which could be
> a problem for non-relocatable extensions. Even if the schema is changed,
> dropping extension / altering extension will work as expected. The function
> is just in different schema than the extension. But, both of these IMO fall
> in the category "don't do that".
Agreed. And on the other hand, I can image cases where as a work around
you'll be glad that you still have "full power" on the extension's objects.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On 01/18/2011 11:42 AM, Dimitri Fontaine wrote: > I've fixed the case by having the code remember the function's extension > if any, and restore it along with the other dependencies. The only question here is should CREATE OR REPLACE be allowed. I just realized this could present a new problem. If I am not mistaken, when loading from dump, you suddenly get the extension's version back, not the one you defined in CREATE OR REPLACE. If this is the case, this should NOT be allowed. And by the same reasoning, ALTER FUNCTION [anything] should not be allowed either. Or at least then the function/(or any object for that matter) should be restored somehow from the backup, not from the extension files. I still haven't had the time to start pg_dump reviewing, so I haven't verified if this is really a problem. But I suspect so... - Anssi
Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes: > Is this supposed to be used mainly by contrib and PGXN extensions? When I > saw the documentation, I immediately thought that this is a nice way to > package my application's stored procedures. If this is not one of the > intended usages, it should be documented. I can see that this could be > problematic when updating PostgreSQL and when recovering from backups. Sure, private application's stored procedure are meant to be fully supported by the extension's facility. > When recovering from backup, you need to have the locally created extension > available. But it might be that the extension files are lost when the system > went down in flames. Now, the backup is unusable (right?) until extension > files can be recovered from source control or where ever they might be > stored. This is why I suggested having multiple locations for the > extensions. It would be easy to backup locally created extensions if those > were in a single directory. All in all, I have a nervous feeling that > somebody someday will have an unusable dump because they used this feature, > but do not have the extension files available... Well, as said in the documentation, extensions are to be used for objects you are *not* maintaining in your database, but elsewhere. Typically you are maintaining your stored procedure code in some VCS, and you have some "packaging" (cat *.sql > my-ext.sql in the Makefile would be the simpler to imagine). So yes if you tell PostgreSQL that your procedures are managed elsewhere so that their code won't be part of your dumps, and then fail to manage them anywhere else, you're hosed. My viewpoint here is that when you want to use extensions, you want to package them for your OS of choice (mine is debian, and I've been working on easing things on this side too with pg_buildext to be found in the postgresql-server-dev-all package). If you're an occasional user just wanting to use new shining facilities… well, think twice… > Also, this part of documentation: > > The goal of using extensions is so that <application>pg_dump</> knows > not to dump all the object definitions into your backups, but rather > issue a single <xref linkend="SQL-CREATEEXTENSION"> command. So maybe we want to extend this little sentence to add the warnings around it, that if you're not packaging your extension's delivery to your servers, you're likely shooting yourself in the foot? > From that, it is not entirely clear to me why this is actually wanted in > PostgreSQL. I suppose this will make dump/restore easier to use. But from > that paragraph, I get the feeling the only advantage is that your dump will > be smaller. And I still have a feeling this implements more. Not that it is > a bad thing at all. Well try to upgrade from 8.4 to 9.0 with some "extensions" installed in there and used in your tables. Pick any contrib, such as hstore or ltree or cube, or some external code, such as ip4r or prefix or such. Then compare to upgrade with the extension facility, and tell me what's best :) Hint: the dump contains the extension's script, but does not contain the shared object file. If you're upgrading the OSand the contribs, as you often do when upgrading major versions, you're hosed. You would think that pg_upgrade alleviatethe concerns here, but you still have to upgrade manually the extension's .so. All in all, those extensions (contribs, ip4r, etc) are *not* maintained in your database and pretending they are by puttingtheir scripts in your dumps is only building problems. This patch aims to offer a solution here. >> It used to work this way with \i, obviously. Should the extension patch >> care about that and how? Do we want to RESET search_path or to manually >> manage it like we do for the client_min_messages and log_min_messages? > It was unintuitive to me to have search_path changed by SQL command as a > side effect. When using \i, not so much. Agreed. Will code the manual management way (that is already used for log settings) later today, unless told to see RESET and how to do that at the statement level rather than the transaction one. > It is easy for me to continue from the Git repo. I will next continue doing > the pg_dump part of the review. I hope I have time to complete that today. Excellent, will try to continue following your pace :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 01/18/2011 12:11 PM, Anssi Kääriäinen wrote: > The only question here is should CREATE OR REPLACE be allowed. I just > realized this could present a new problem. If I am not mistaken, when > loading from dump, you suddenly get the extension's version back, not > the one you defined in CREATE OR REPLACE. If this is the case, this > should NOT be allowed. And by the same reasoning, ALTER FUNCTION > [anything] should not be allowed either. Or at least then the > function/(or any object for that matter) should be restored somehow from > the backup, not from the extension files. > > I still haven't had the time to start pg_dump reviewing, so I haven't > verified if this is really a problem. But I suspect so... Ok, verified at least for CREATE OR REPLACE, ALTER FUNCTION RENAME and ALTER FUNCTION SET search_path. You will get the extensions version back when restoring from plain sql dump, not the CORed function, rename is lost and same for search_path. I suspect this is a problem for any object type supported in extensions. But unfortunately I do not have time to verify that. One more problem with pg_dump. If you have CREATE EXTENSION in you extensions .sql file, you will have problems restoring. I know it is stupid to do so, but still CREATE EXTENSION inside CREATE EXTENSION should be disallowed, as it is possible you find out too late that this is stupid thing to do. Also, the functions created in the "recursive" CREATE EXTENSION will be dumped, and the dependencies are not created correctly. Unfortunately I have used up all the time I have for reviewing this patch. I can arrange some more time, maybe late this week, maybe a bit later. So, I do not have time to do the pg_dump part review in full detail right now. Still, I hope the work I have done is helpful. Should I write up a post that contains all the current outstanding issues in one post, or is it enough to just link this thread in the CommitFest application? - Anssi
Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes: > The only question here is should CREATE OR REPLACE be allowed. I just Yes. Think ALTER EXTENSION UPGRADE, the next patch in the series (already proposed for this CF too). > realized this could present a new problem. If I am not mistaken, when > loading from dump, you suddenly get the extension's version back, not the > one you defined in CREATE OR REPLACE. If this is the case, this should NOT > be allowed. And by the same reasoning, ALTER FUNCTION [anything] should not > be allowed either. Or at least then the function/(or any object for that > matter) should be restored somehow from the backup, not from the extension > files. Well ideally those will get into extension's upgrade scripts, not be typed interactively by superusers. But I don't think we should limit the capability of superusers to quickly fix a packaging mistake… > I still haven't had the time to start pg_dump reviewing, so I haven't > verified if this is really a problem. But I suspect so... Both a problem when badly used and a good thing to have sometime, as in the upgrade scripts :) -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes:
> Ok, verified at least for CREATE OR REPLACE, ALTER FUNCTION RENAME and ALTER
> FUNCTION SET search_path. You will get the extensions version back when
> restoring from plain sql dump, not the CORed function, rename is lost and
> same for search_path. I suspect this is a problem for any object type
> supported in extensions. But unfortunately I do not have time to verify
> that.
Yes it's the same, if you edit an extension's object directly the edited
version is not in the dump. There's provision to have those objects in
the dump but the function to make it so is currently restricted to only
be called from the extension's script.
pg_extension_flag_dump() changes the dependency type between an extension's and one of its object, given by oid. The
defaultbehavior of an extension is to skip objects in pg_dump, but in some cases that's not what you want to achieve,
asan extension author. If your extension provides user data (in a table, for example), then flag this table so that
it'spart of the backups, like so:
SELECT pg_extension_flag_dump('schema.tablename'::regclass);
Maybe we should open this function so that it's usable even outside of
the extension's script, but I'm not keen on doing so.
Again, editing the extension's objects out of the scripts is still
limited to superusers and not the only way to shoot yourself in the
foot…
> One more problem with pg_dump. If you have CREATE EXTENSION in you
> extensions .sql file, you will have problems restoring. I know it is stupid
> to do so, but still CREATE EXTENSION inside CREATE EXTENSION should be
> disallowed, as it is possible you find out too late that this is stupid
> thing to do. Also, the functions created in the "recursive" CREATE EXTENSION
> will be dumped, and the dependencies are not created correctly.
That will be handled later, it's called inter-extension dependencies.
We said we won't address that yet…
> Unfortunately I have used up all the time I have for reviewing this patch. I
> can arrange some more time, maybe late this week, maybe a bit later. So, I
> do not have time to do the pg_dump part review in full detail right
> now. Still, I hope the work I have done is helpful.
Very much so, thanks a lot for the time you already spent on it!
> Should I write up a post that contains all the current outstanding issues in
> one post, or is it enough to just link this thread in the CommitFest
> application?
I'd appreciate a list of yet-to-fix items. What I have is the
search_path issue where CREATE EXTENSION foo; can leave it changed for
the current session, I intend to fix that later today.
Other than that, I have no further already agreed on code fix to make.
What's your list?
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 01/18/2011 01:03 PM, Dimitri Fontaine wrote: > I'd appreciate a list of yet-to-fix items. What I have is the > search_path issue where CREATE EXTENSION foo; can leave it changed for > the current session, I intend to fix that later today. > > Other than that, I have no further already agreed on code fix to make. > What's your list? There is only documentation fixes, and I am not sure if even those are agreed to be necessary. It might be good if the documentation contained: - A warning that you need to have the files for your extensions readily available to be able to restore from a dump. This might be obvious, but better safe than sorry... - A warning that you will be restored to the extension's version if you ALTER or CREATE OR REPLACE a function. From the current documentation, it is maybe too easy to miss these risks. I am seeing this from non-experienced user's angle, and thus see these as potential foot guns. Other than that, I don't think there is anything more. I am a little nervous of restoring to extension's version of a function when the function has been CREATE OR REPLACEd, but that might be just me over thinking this. Also, from the previous posts, there is just the control file naming issue, and the issue of load order if two extensions contain similarly named and signatured functions. But these were agreed to be issues not needing any further work. Now, I need help what to do next. Should I leave the status as Needs Review as the pg_dump part is almost completely non-reviewed? And then attach this thread as a comment? Or as a review? - Anssi
Excerpts from Dimitri Fontaine's message of mar ene 18 07:01:55 -0300 2011: > Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes: > >> It used to work this way with \i, obviously. Should the extension patch > >> care about that and how? Do we want to RESET search_path or to manually > >> manage it like we do for the client_min_messages and log_min_messages? > > It was unintuitive to me to have search_path changed by SQL command as a > > side effect. When using \i, not so much. If the CREATE EXTENSION stuff all works in a transaction, perhaps it would be easier if you used SET LOCAL. At the end of the transaction it would reset to the original value automatically. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes: > On 01/18/2011 01:03 PM, Dimitri Fontaine wrote: >> I'd appreciate a list of yet-to-fix items. What I have is the >> search_path issue where CREATE EXTENSION foo; can leave it changed for >> the current session, I intend to fix that later today. After some reading of backend/catalog/namespace.c and some testing, my take would be to have extension authors use the following form when that is necessary: SET LOCAL search_path TO public; Now, none of the contribs use that form (they all are relocatable except for adminpack which forces its objects to get installed in pg_catalog), so I've only updated the documentation. > There is only documentation fixes, and I am not sure if even those are > agreed to be necessary. It might be good if the documentation contained: > > - A warning that you need to have the files for your extensions readily > available to be able to restore from a dump. This might be obvious, but > better safe than sorry... Added a § about that in the docs. > - A warning that you will be restored to the extension's version if you > ALTER or CREATE OR REPLACE a function. Well I don't want to encourage people to manually edit any extension objects directly, so I've added a note about not doing that. > Other than that, I don't think there is anything more. I am a little nervous > of restoring to extension's version of a function when the function has been > CREATE OR REPLACEd, but that might be just me over thinking this. Also, from Well with the next patch in the series, ALTER EXTENSION UPGRADE, you will have a way to edit extension's objects and have the new version installed next time, but there's no magic bullet here, it means work to do by the extension's author (preparing upgrade scripts and new version's install-from-scratch scripts). > the previous posts, there is just the control file naming issue, and the > issue of load order if two extensions contain similarly named and signatured > functions. But these were agreed to be issues not needing any further work. Yes, extension dependencies and conflicts are not meant to be covered yet. > Now, I need help what to do next. Should I leave the status as Needs Review > as the pg_dump part is almost completely non-reviewed? And then attach this > thread as a comment? Or as a review? My guess would be to leave it as Needs Review and add this thread as a review. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support