Обсуждение: patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
Hello, The attached patch changes the location of the dumpUserConfig call in the dumpRoles function of pg_dumpall. This is related to this thread: http://archives.postgresql.org/pgsql-hackers/2011-02/msg02359.php Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement. Sometimes this will cause a conflict when a dependent role is not yet created: -- -- Roles -- CREATE ROLE a; ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION; ALTER ROLE a SET role TO 'b'; CREATE ROLE b; ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION; CREATE ROLE postgres; ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN NOREPLICATION; As you can see, role a is set to role b before role b is created. This patch moves the call to dumpUserConfig to after the loop where all the roles are created. This produces output like the this: -- -- Roles -- CREATE ROLE a; ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION; CREATE ROLE b; ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION; CREATE ROLE postgres; ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN NOREPLICATION; ALTER ROLE a SET role TO 'b'; Now this dump will succeed upon restore. This passed all regression tests. Thanks.
Вложения
Phil Sorber <phil@omniti.com> writes: > Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will > dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement. I think pg_dumpall is the very least of your problems if you do something like that. We probably ought to forbid it entirely. regards, tom lane
On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will >> dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement. > > I think pg_dumpall is the very least of your problems if you do > something like that. We probably ought to forbid it entirely. Well, we had a long discussion of that on the thread Phil linked to, and I don't think there was any consensus that forbidding it was the right thing to do. Phil appears to be trying to implement the proposal you made here: http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php ...although I don't think that what he did quite matches what you asked for. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think pg_dumpall is the very least of your problems if you do >> something like that. �We probably ought to forbid it entirely. > Well, we had a long discussion of that on the thread Phil linked to, > and I don't think there was any consensus that forbidding it was the > right thing to do. You're right, I was half-remembering that thread and thinking that there are a lot of gotchas in doing an ALTER ROLE SET ROLE. Florian claimed in the thread that he'd never hit one before, but that doesn't convince me much. > Phil appears to be trying to implement the > proposal you made here: > http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php > ...although I don't think that what he did quite matches what you asked for. No, the proposed patch doesn't go nearly far enough to address Florian's problem. What I was speculating about was moving all the role (and database) alters to the *end*, so they'd not take effect until after we'd completed all the restore actions. regards, tom lane
I have included two patches in this email. The first (dump_user_config_last_with_set_role.patch) is an extension of my first patch. In addition to moving the ALTER ROLE statements after the CREATE ROLE statements it also inserts a SET ROLE after every connect. It takes the role parameter from the --role command line option. This fixes the problem of not being able to restore to a database because of lack of permissions. This is similar to the idea proposed here: http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php I think this patch is the better one for several reasons. It keeps the ALTER ROLE and ALTER DATABASE statements in the same sections as their related CREATE ROLE and CREATE DATABASE statements, thus not dramatically altering the output of the command. It is a much simpler non-invasive patch. It's concise and easy to understand. It solves all the known scenario's that we have discussed previously. That being said, I understand you have reservations about how it works. You would prefer to move all of these type's of ALTER statements to the end of the output. I also would prefer to see the correct behavior included in the main tree over having my preference over the changes. To that end I have also included a second patch (dump_all_configs_last.patch) which handles it the way you prefer. This creates new sections at the bottom for the ALTER statements. These two patches are mutually exclusive. I would ask that you at least consider the first patch before dismissing the idea. However, I think applying either would would be an acceptable approach. Thanks for your consideration. On Thu, Jul 28, 2011 at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I think pg_dumpall is the very least of your problems if you do >>> something like that. We probably ought to forbid it entirely. > >> Well, we had a long discussion of that on the thread Phil linked to, >> and I don't think there was any consensus that forbidding it was the >> right thing to do. > > You're right, I was half-remembering that thread and thinking that > there are a lot of gotchas in doing an ALTER ROLE SET ROLE. Florian > claimed in the thread that he'd never hit one before, but that doesn't > convince me much. > >> Phil appears to be trying to implement the >> proposal you made here: >> http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php >> ...although I don't think that what he did quite matches what you asked for. > > No, the proposed patch doesn't go nearly far enough to address Florian's > problem. What I was speculating about was moving all the role (and > database) alters to the *end*, so they'd not take effect until after > we'd completed all the restore actions. > > regards, tom lane >
Вложения
Phil Sorber <phil@omniti.com> writes: > I have included two patches in this email. The first > (dump_user_config_last_with_set_role.patch) is an extension of my > first patch. In addition to moving the ALTER ROLE statements after the > CREATE ROLE statements it also inserts a SET ROLE after every connect. > It takes the role parameter from the --role command line option. This > fixes the problem of not being able to restore to a database because > of lack of permissions. This is similar to the idea proposed here: > http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php I don't understand why you think that that will fix anything? The problem that Florian originally pointed out is that settings established by ALTER DATABASE/ROLE could interfere with the restoration script's actions. That seems to be just as much of a risk for the --role role as the one originally used to connect. I don't see a way around that other than not applying those settings until we are done reconnecting to the target database. Also, given that the --role switch is only defined to select the role to be used at *dump* time, I'm unconvinced that forcing it to be used at *restore* time is a good idea. You'd really need to invent a separate switch if you were to go down this path. regards, tom lane
On Tue, Aug 2, 2011 at 5:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> I have included two patches in this email. The first >> (dump_user_config_last_with_set_role.patch) is an extension of my >> first patch. In addition to moving the ALTER ROLE statements after the >> CREATE ROLE statements it also inserts a SET ROLE after every connect. >> It takes the role parameter from the --role command line option. This >> fixes the problem of not being able to restore to a database because >> of lack of permissions. This is similar to the idea proposed here: >> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php > > I don't understand why you think that that will fix anything? > > The problem that Florian originally pointed out is that settings > established by ALTER DATABASE/ROLE could interfere with the restoration > script's actions. That seems to be just as much of a risk for the > --role role as the one originally used to connect. I don't see a way > around that other than not applying those settings until we are done > reconnecting to the target database. > > Also, given that the --role switch is only defined to select the role > to be used at *dump* time, I'm unconvinced that forcing it to be used > at *restore* time is a good idea. You'd really need to invent a > separate switch if you were to go down this path. > > regards, tom lane > Ok, here is the patch that just moves the ALTER/SET pieces to the end. Can we get this included in the next commit fest?
Вложения
On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote: > Ok, here is the patch that just moves the ALTER/SET pieces to the end. > Can we get this included in the next commit fest? Yep, just make yourself an account and add it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote: >> Ok, here is the patch that just moves the ALTER/SET pieces to the end. >> Can we get this included in the next commit fest? > > Yep, just make yourself an account and add it. Unfortunately, it doesn't look like anyone ever replied to this thread, but Tom posted some thoughts on another thread that seem to me to be a serious problem for this patch: http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us I don't see any easy way around that problem, so I'm going to mark this patch Returned with Feedback for now. It strikes me as craziness to try to guess which settings we should restore at the beginning and which at the end, so I think we need a better idea. I don't really understand why it's not OK to just have pg_dump issue RESET ROLE at appropriate points in the process; that seems like it would be sufficient and not particularly ugly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I don't really > understand why it's not OK to just have pg_dump issue RESET ROLE at > appropriate points in the process; that seems like it would be > sufficient and not particularly ugly. Well, it was alleged that that would fix this problem: http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php but if it does fix it, I think that's a bug in itself: http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php But more to the point, I think the specific case of "ALTER DATABASE SET ROLE" is just one element of a class of problems, namely that settings attached to either databases or roles could create issues for restoring a dump. Issuing RESET ROLE would fix only that one single case. regards, tom lane
On Mon, Oct 10, 2011 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I don't really >> understand why it's not OK to just have pg_dump issue RESET ROLE at >> appropriate points in the process; that seems like it would be >> sufficient and not particularly ugly. > > Well, it was alleged that that would fix this problem: > http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php > but if it does fix it, I think that's a bug in itself: > http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php Hmm. > But more to the point, I think the specific case of "ALTER DATABASE SET > ROLE" is just one element of a class of problems, namely that settings > attached to either databases or roles could create issues for restoring > a dump. Issuing RESET ROLE would fix only that one single case. It's not very clear to me that we're going to find a fix that reaches across every setting, though. I mean, for something like maintenance_work_mem, there's no correctness issue regardless of when the new setting takes effect, but there might very well be a performance issue, and it's not really all that clear when the "right" time to put the old setting back is. And that ambiguity about what's actually correct is, perhaps, the root of the problem. There are related cases where we have a clear-cut policy. For example, we are clear that you must use the newer pg_dump against the older server for best results. That's not always convenient, but at least it's a line in the sand. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Oct 10, 2011 at 11:54 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote: >>> Ok, here is the patch that just moves the ALTER/SET pieces to the end. >>> Can we get this included in the next commit fest? >> >> Yep, just make yourself an account and add it. > > Unfortunately, it doesn't look like anyone ever replied to this > thread, but Tom posted some thoughts on another thread that seem to me > to be a serious problem for this patch: > > http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us > > I don't see any easy way around that problem, so I'm going to mark > this patch Returned with Feedback for now. It strikes me as craziness > to try to guess which settings we should restore at the beginning and > which at the end, so I think we need a better idea. I don't really > understand why it's not OK to just have pg_dump issue RESET ROLE at > appropriate points in the process; that seems like it would be > sufficient and not particularly ugly. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > I am going to remove that patch from the commit fest because we all agree that it does not solve the problem satisfactorily. I would like to re-iterate a few points, and submit a new patch. First off, there are two distinct problems here that we have been lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' and then there is the 'ALTER ROLE SET ROLE' case. The former is the one that has been causing us so many problems, and the latter is the one that I really care about. Also, there are more people that are hitting this issue as well: http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php My proposal would be to table the discussion about the 'ALTER DATABASE SET ROLE' case because there seems to be a bit of a philosophical debate behind this that needs to be sorted out first. If we focus only on the 'ALTER ROLE' case I think there is a trivial solution. We already separate the CREATE from the ALTER in a single loop. We also already separate out the user config in a separate function called from this same loop. The problem is that the user config can be dependent upon a later CREATE. So all I suggest is to move the user config dumping into a new loop afterward so that the user config ALTER's come after all the other CREATE's and ALTER's. It amounts to a 7 line change and solves our problem rather elegantly.
Вложения
On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil@omniti.com> wrote: > I am going to remove that patch from the commit fest because we all > agree that it does not solve the problem satisfactorily. I would like > to re-iterate a few points, and submit a new patch. > > First off, there are two distinct problems here that we have been > lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' > and then there is the 'ALTER ROLE SET ROLE' case. The former is the > one that has been causing us so many problems, and the latter is the > one that I really care about. > > Also, there are more people that are hitting this issue as well: > > http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php > > My proposal would be to table the discussion about the 'ALTER DATABASE > SET ROLE' case because there seems to be a bit of a philosophical > debate behind this that needs to be sorted out first. > > If we focus only on the 'ALTER ROLE' case I think there is a trivial > solution. We already separate the CREATE from the ALTER in a single > loop. We also already separate out the user config in a separate > function called from this same loop. The problem is that the user > config can be dependent upon a later CREATE. So all I suggest is to > move the user config dumping into a new loop afterward so that the > user config ALTER's come after all the other CREATE's and ALTER's. It > amounts to a 7 line change and solves our problem rather elegantly. I'm not sure I completely follow this explanation of the problem, but it does seem better to me to set all the role attributes after dumping all the create role statements, and I don't see how that can break anything that works now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Oct 12, 2011 at 3:48 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil@omniti.com> wrote: >> I am going to remove that patch from the commit fest because we all >> agree that it does not solve the problem satisfactorily. I would like >> to re-iterate a few points, and submit a new patch. >> >> First off, there are two distinct problems here that we have been >> lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' >> and then there is the 'ALTER ROLE SET ROLE' case. The former is the >> one that has been causing us so many problems, and the latter is the >> one that I really care about. >> >> Also, there are more people that are hitting this issue as well: >> >> http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php >> >> My proposal would be to table the discussion about the 'ALTER DATABASE >> SET ROLE' case because there seems to be a bit of a philosophical >> debate behind this that needs to be sorted out first. >> >> If we focus only on the 'ALTER ROLE' case I think there is a trivial >> solution. We already separate the CREATE from the ALTER in a single >> loop. We also already separate out the user config in a separate >> function called from this same loop. The problem is that the user >> config can be dependent upon a later CREATE. So all I suggest is to >> move the user config dumping into a new loop afterward so that the >> user config ALTER's come after all the other CREATE's and ALTER's. It >> amounts to a 7 line change and solves our problem rather elegantly. > > I'm not sure I completely follow this explanation of the problem, but > it does seem better to me to set all the role attributes after dumping > all the create role statements, and I don't see how that can break > anything that works now. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > Just to be clear, this doesn't move all the ALTER's. It will still do CREATE/ALTER pair's for attributes that could go in the CREATE. Example: CREATE ROLE bob; ALTER ROLE bob WITH LOGIN PASSWORD 'blah'; You could turn those in to one statement, but for various reasons you do not want to. None of these have any dependencies other than the actual creation of the role. Then there is a separate section of code that is called as a separate function 'dumpUserConfig()' that does other role attributes like 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can have dependencies on other roles. I agree this is a little confusing, and if you prefer to see all the ALTER's in one section together directly after the CREATE statements I can make the patch do that, but it isn't necessary to solve the problem.
Phil Sorber <phil@omniti.com> writes: > Then there is a separate section of code that is called as a separate > function 'dumpUserConfig()' that does other role attributes like > 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can > have dependencies on other roles. Right. Phrased that way, this is an obvious improvement, and I concur that it doesn't look like it could break anything that works now. The more general problem remains to be solved, but we might as well apply this. regards, tom lane
Re: patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
От
Martijn van Oosterhout
Дата:
On Wed, Oct 12, 2011 at 02:27:51PM -0400, Phil Sorber wrote: > My proposal would be to table the discussion about the 'ALTER DATABASE > SET ROLE' case because there seems to be a bit of a philosophical > debate behind this that needs to be sorted out first. Note: "to table the discussion" means opposite things in American and British english. I think the rest of the sentence makes it clear what you mean though :). Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > He who writes carelessly confesses thereby at the very outset that he does > not attach much importance to his own thoughts. -- Arthur Schopenhauer
On Wed, Oct 12, 2011 at 7:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> Then there is a separate section of code that is called as a separate >> function 'dumpUserConfig()' that does other role attributes like >> 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can >> have dependencies on other roles. > > Right. Phrased that way, this is an obvious improvement, and I concur > that it doesn't look like it could break anything that works now. > The more general problem remains to be solved, but we might as well > apply this. OK, done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company