Thread: Re: Server instrumentation patch
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 21 June 2005 18:06 > To: Dave Page > Cc: PostgreSQL-development; Andreas Pflug > Subject: Server instrumentation patch > > > OK, let me address this, but you might not like what I have > to say. ;-) > > Basically, Andreas' approach for 8.0 was to develop a patch (without > posting a proposal or interface), and then argue why pgadmin needs it, > but without addressing the real concerns about the patch. Saying > pgadmin needs it just isn't enough to get a patch in. There are the > issues of security and maintainability that have to be addressed, and > in the limited time we had to do this in 8.0, it was clear the patch > should not be applied. The reason it happen that way was because we already had the code as a contrib-style module for pgAdmin. It was posted because we recognised that it was becoming a PITA for pgAdmin users to compile a new server-side component and the functions seemed like they would be useful to other tools similar to pgAdmin. Yes, this is not the normal way to proprose new features, but I'm sure you appreciate that as picture speaks a thousand words, posting the *existing* code with minor changes to properly integrate it shows exactly what is being proposed, both in functional and impelmentation detail. > Now, in 8.1, the same thing has happened. Two weeks before feature > freeze, with no discussion, the patch appears, and makes no > reference to > concerns raised during the 8.0 discussion. OK, first it was the 10th of June which is a little more than two weeks, however, Andreas clearly did reference previous discussions on the subject - see his message http://archives.postgresql.org/pgsql-patches/2005-06/msg00226.php in which he points out that 2 functions are from the logger suprocess patch from 07/2004, that the file related stuff is based on discussions starting at http://archives.postgresql.org/pgsql-patches/2004-07/msg00287.php, including comments from yourself!! > pg_terminate_backend is even > in the patch, and there is no mention or attempt to address > concerns we > had in 8.0. No. I cannot argue with that, and for that reason I suggested that Andreas repost the patch without that function so it can be properly discussed and implemented in a safe way in the future. I'm sure you have see the reposted patch. > The move of dbsize into the backend is similar. He moves the parts of > dbsize the pgadmin needs into the backend, but makes no mention or > change to /contrib/dbsize to adjust it to the movement of the code. He > has since posted and updated version that fixes this, I think, but > again, we have to discuss how this is to be done --- do we > move all the > dbsize functions into the backend, some, or none? Do the other dbsize > functions stay in /contrib or get deleted? Well as far as I can see, Andreas did respond to all queries about it, and then posted his updated patch after it became apparent noone else was going to discuss the issue further - http://archives.postgresql.org/pgsql-patches/2005-06/msg00309.php. From what I can see, no-one has argued or disagreed with his opinion given a few days to do so, therefore there was nothing further to discuss. Unfortunately sometimes people don't respond - either because they don't care, or because they agree. Either way, *we* cannot force a discussion, and in this sort of development model we have no choice than to assume that if discussion of a issue stops and there are no outstanding queries, concerns or objections, it's because it's everyone is happy for the result of those discussions to be accepted into the project. > This needs discussion, not a patch. And because there are so many > assumptions made in the patch, the patch committers look unreasonable > asking for X changes to his patch, when in fact he made X > assumptions in > the patch and never asked anyone before developing the patch > about those > assumptions. With the exception of the now removed pg_terminate_backend, I am unaware of any issues that are outstanding. If the committers have issues they *must* raise them for *any* submitted patch otherwise developers will lose faith in the process when their hard work gets ignored. Now, to try to get this ball rolling again - do the committers or anyone else have any outstanding issues with the instrumentation or dbsize patches that haven't been answered in public discussion and addressed in the patches already? Regards, Dave.
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 22 June 2005 04:08 > To: Andreas Pflug > Cc: Dave Page; PostgreSQL-development > Subject: Re: Server instrumentation patch > > > > The move of dbsize into the backend is similar. He moves > the parts of > > > dbsize the pgadmin needs into the backend, but makes no mention or > > > change to /contrib/dbsize to adjust it to the movement of > the code. He > > > has since posted and updated version that fixes this, I think, but > > > again, we have to discuss how this is to be done --- do > we move all the > > > dbsize functions into the backend, some, or none? Do the > other dbsize > > > functions stay in /contrib or get deleted? > > > This needs discussion, not a patch. And because there are so many > > > assumptions made in the patch, the patch committers look > unreasonable > > > asking for X changes to his patch, when in fact he made X > assumptions in > > > the patch and never asked anyone before developing the > patch about those > > > assumptions. > > > > This was discussed lengthy starting May 11th, except for the broken > > dbsize functions. My post is the result from that. > > Really? Where? I don't remember anything about it. I imagine that would be this lengthy thread: http://archives.postgresql.org/pgsql-hackers/2005-05/msg00837.php Regards, Dave.
Dave Page wrote: > The reason it happen that way was because we already had the code as a > contrib-style module for pgAdmin. It was posted because we recognised > that it was becoming a PITA for pgAdmin users to compile a new > server-side component and the functions seemed like they would be useful > to other tools similar to pgAdmin. > > Yes, this is not the normal way to proprose new features, but I'm sure > you appreciate that as picture speaks a thousand words, posting the > *existing* code with minor changes to properly integrate it shows > exactly what is being proposed, both in functional and impelmentation > detail. Sure. > > Now, in 8.1, the same thing has happened. Two weeks before feature > > freeze, with no discussion, the patch appears, and makes no > > reference to > > concerns raised during the 8.0 discussion. > > OK, first it was the 10th of June which is a little more than two weeks, > however, Andreas clearly did reference previous discussions on the > subject - see his message > http://archives.postgresql.org/pgsql-patches/2005-06/msg00226.php in > which he points out that 2 functions are from the logger suprocess patch > from 07/2004, that the file related stuff is based on discussions > starting at > http://archives.postgresql.org/pgsql-patches/2004-07/msg00287.php, > including comments from yourself!! > > > pg_terminate_backend is even > > in the patch, and there is no mention or attempt to address > > concerns we > > had in 8.0. > > No. I cannot argue with that, and for that reason I suggested that > Andreas repost the patch without that function so it can be properly > discussed and implemented in a safe way in the future. I'm sure you have > see the reposted patch. OK. > > The move of dbsize into the backend is similar. He moves the parts of > > dbsize the pgadmin needs into the backend, but makes no mention or > > change to /contrib/dbsize to adjust it to the movement of the code. He > > has since posted and updated version that fixes this, I think, but > > again, we have to discuss how this is to be done --- do we > > move all the > > dbsize functions into the backend, some, or none? Do the other dbsize > > functions stay in /contrib or get deleted? > > Well as far as I can see, Andreas did respond to all queries about it, > and then posted his updated patch after it became apparent noone else > was going to discuss the issue further - > http://archives.postgresql.org/pgsql-patches/2005-06/msg00309.php. From > what I can see, no-one has argued or disagreed with his opinion given a > few days to do so, therefore there was nothing further to discuss. Well, I see Marc replying that dbsize should be moved completely from contrib: http://archives.postgresql.org/pgsql-patches/2005-06/msg00409.php The current version of the patch only moves those functions he wants. Marc says he wants them all moved, and I agree. > With the exception of the now removed pg_terminate_backend, I am unaware > of any issues that are outstanding. If the committers have issues they > *must* raise them for *any* submitted patch otherwise developers will > lose faith in the process when their hard work gets ignored. Well, from the May, 2005 discussion URL you posted, I see a clear reply to the idea of adding the I/O functions to the backend: http://archives.postgresql.org/pgsql-hackers/2005-05/msg00874.php Now, you can agree or disagree that there are issues with the I/O functions, but the concern was raised in May, and not addressed at all, either via email or the patch. > Now, to try to get this ball rolling again - do the committers or anyone > else have any outstanding issues with the instrumentation or dbsize > patches that haven't been answered in public discussion and addressed in > the patches already? OK, agreed, how can we move forward? The patch has three parts: o file I/Oo move dbsize from contribo backend terminate For the first, we need to re-discuss this on hackers. I found this as the conclusion from July of 2004 as it relates to the I/O functions: http://archives.postgresql.org/pgsql-patches/2004-07/msg00561.php However, the TODO items still exist so we can discuss it and hopefully resolve it by feature freeze. For the second, please supply a patch that moves _all_ of dbsize into the main server. I think we have agreement on that. For backend terminate, I agree with Tom that we have to get SIGTERM working, and then the function can just send a SIGTERM signal. Unless it is working 100%, we will not add a terminate function to SQL. I will post separately on this topic. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian wrote: >Dave Page wrote: > > >>The reason it happen that way was because we already had the code as a >>contrib-style module for pgAdmin. It was posted because we recognised >>that it was becoming a PITA for pgAdmin users to compile a new >>server-side component and the functions seemed like they would be useful >>to other tools similar to pgAdmin. >> >>Yes, this is not the normal way to proprose new features, but I'm sure >>you appreciate that as picture speaks a thousand words, posting the >>*existing* code with minor changes to properly integrate it shows >>exactly what is being proposed, both in functional and impelmentation >>detail. >> >> > >Sure. > > > >>>Now, in 8.1, the same thing has happened. Two weeks before feature >>>freeze, with no discussion, the patch appears, and makes no >>>reference to >>>concerns raised during the 8.0 discussion. >>> >>> >>OK, first it was the 10th of June which is a little more than two weeks, >>however, Andreas clearly did reference previous discussions on the >>subject - see his message >>http://archives.postgresql.org/pgsql-patches/2005-06/msg00226.php in >>which he points out that 2 functions are from the logger suprocess patch >>from 07/2004, that the file related stuff is based on discussions >>starting at >>http://archives.postgresql.org/pgsql-patches/2004-07/msg00287.php, >>including comments from yourself!! >> >> >> >>>pg_terminate_backend is even >>>in the patch, and there is no mention or attempt to address >>>concerns we >>>had in 8.0. >>> >>> >>No. I cannot argue with that, and for that reason I suggested that >>Andreas repost the patch without that function so it can be properly >>discussed and implemented in a safe way in the future. I'm sure you have >>see the reposted patch. >> >> > >OK. > > > >>>The move of dbsize into the backend is similar. He moves the parts of >>>dbsize the pgadmin needs into the backend, but makes no mention or >>>change to /contrib/dbsize to adjust it to the movement of the code. He >>>has since posted and updated version that fixes this, I think, but >>>again, we have to discuss how this is to be done --- do we >>>move all the >>>dbsize functions into the backend, some, or none? Do the other dbsize >>>functions stay in /contrib or get deleted? >>> >>> >>Well as far as I can see, Andreas did respond to all queries about it, >>and then posted his updated patch after it became apparent noone else >>was going to discuss the issue further - >>http://archives.postgresql.org/pgsql-patches/2005-06/msg00309.php. From >>what I can see, no-one has argued or disagreed with his opinion given a >>few days to do so, therefore there was nothing further to discuss. >> >> > >Well, I see Marc replying that dbsize should be moved completely from >contrib: > > http://archives.postgresql.org/pgsql-patches/2005-06/msg00409.php > >The current version of the patch only moves those functions he wants. >Marc says he wants them all moved, and I agree. > > > >>With the exception of the now removed pg_terminate_backend, I am unaware >>of any issues that are outstanding. If the committers have issues they >>*must* raise them for *any* submitted patch otherwise developers will >>lose faith in the process when their hard work gets ignored. >> >> > >Well, from the May, 2005 discussion URL you posted, I see a clear reply >to the idea of adding the I/O functions to the backend: > > http://archives.postgresql.org/pgsql-hackers/2005-05/msg00874.php > >Now, you can agree or disagree that there are issues with the I/O >functions, but the concern was raised in May, and not addressed at all, >either via email or the patch. > > > >>Now, to try to get this ball rolling again - do the committers or anyone >>else have any outstanding issues with the instrumentation or dbsize >>patches that haven't been answered in public discussion and addressed in >>the patches already? >> >> > >OK, agreed, how can we move forward? The patch has three parts: > > o file I/O > o move dbsize from contrib > o backend terminate > >For the first, we need to re-discuss this on hackers. I found this as >the conclusion from July of 2004 as it relates to the I/O functions: > > http://archives.postgresql.org/pgsql-patches/2004-07/msg00561.php > >However, the TODO items still exist so we can discuss it and hopefully >resolve it by feature freeze. > >For the second, please supply a patch that moves _all_ of dbsize into >the main server. I think we have agreement on that. > > I don't think so. As I mentioned, those views are broken. Do you want them to be in core anyway? Regards, Andreas
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 24 June 2005 14:00 > To: Dave Page > Cc: PostgreSQL-development; Andreas Pflug > Subject: Re: Server instrumentation patch > > Well, I see Marc replying that dbsize should be moved completely from > contrib: > > > http://archives.postgresql.org/pgsql-patches/2005-06/msg00409.php > > The current version of the patch only moves those functions he wants. > Marc says he wants them all moved, and I agree. OK - did you see Andreas' response to why he hadn't done that (it was actually posted in response to your original query, not Marcs)? In a nutshell, the functions that had not been moved returned values that were easily derived from the other functions, and thus could be considered bloat? The functions included in the patch were: int8 pg_tablespace_size(oid) - Return the size of the tablespace in bytes int8 pg_database_size(oid) - Return the size of the database in bytes int8 pg_relation_size(oid) - Return the size of the relation in bytes text pg_size_pretty(int8) - Pretty-print the byte value The ones left out were: int8 database_size(name) - Return the size of the database in bytes (by name) int8 relation_size(text) - Return the size of the relation in bytes (by name) int8 indexes_size(text) - Return the size of the indexes on the named relation int8 total_relation_size(text) - Return relation_size(text) + indexes_size(text) + relation_size(text->toast tables) setof record relation_size_components(text) - return a pretty-print set of values from above. I don't feel particularly strongly either way, but given the normal desire not to bloat the backend necessarily, I have to question the need to include the latter functions. > > With the exception of the now removed pg_terminate_backend, > I am unaware > > of any issues that are outstanding. If the committers have > issues they > > *must* raise them for *any* submitted patch otherwise > developers will > > lose faith in the process when their hard work gets ignored. > > Well, from the May, 2005 discussion URL you posted, I see a > clear reply > to the idea of adding the I/O functions to the backend: > > > http://archives.postgresql.org/pgsql-hackers/2005-05/msg00874.php > > Now, you can agree or disagree that there are issues with the I/O > functions, but the concern was raised in May, and not > addressed at all, > either via email or the patch. Maybe that's the wrong URL, but all I see there is a vague recollection from Tom that there were security issues. In the next message, Andreas recalls how you and he worked out the issues that were raised - I believe this is the thread http://archives.postgresql.org/pgsql-hackers/2004-07/msg00793.php. Mhonarc has made a mess of the thread so it does seem to break in a few places, and it is possible I've missed part. > > Now, to try to get this ball rolling again - do the > committers or anyone > > else have any outstanding issues with the instrumentation or dbsize > > patches that haven't been answered in public discussion and > addressed in > > the patches already? > > OK, agreed, how can we move forward? The patch has three parts: > > o file I/O > o move dbsize from contrib > o backend terminate > > For the first, we need to re-discuss this on hackers. I found this as > the conclusion from July of 2004 as it relates to the I/O functions: > > > http://archives.postgresql.org/pgsql-patches/2004-07/msg00561.php > I've just read through that thread, and the only mention of security concerns I can spot is one where you say yourself that they are a non-issue!! What are the actual outstanding concerns with these functions? > For the second, please supply a patch that moves _all_ of dbsize into > the main server. I think we have agreement on that. If that remains the case having seen my comments above echoing Andreas' concerns then sure, if that's what it takes to get them in, so be it. Please confirm either way. > For backend terminate, I agree with Tom that we have to get SIGTERM > working, and then the function can just send a SIGTERM signal. Unless > it is working 100%, we will not add a terminate function to > SQL. I will > post separately on this topic. Agreed. Regards, Dave.
Andreas Pflug wrote: >>For the second, please supply a patch that moves _all_ of dbsize into >>the main server. I think we have agreement on that. >> > > I don't think so. As I mentioned, those views are broken. Do you want them > to be in core anyway? Why is e.g. this one broken: int8 database_size(name) - Return the size of the database in bytes (by name) It should do the same as the one with the oid except that it will resolve the name first, no? If not it should be fixed, not dropped. I understand you'd like to have those functions for the GUI frontends, but what about psql users? For many people it will be hard work to type the subquery to get the database oid. I vote for all (possibly corrected) functions to be moved into core. Best Regards, Michael Paesold
> -----Original Message----- > From: Michael Paesold [mailto:mpaesold@gmx.at] > Sent: 24 June 2005 16:48 > To: Andreas Pflug > Cc: Dave Page; PostgreSQL-development > Subject: Re: [HACKERS] Server instrumentation patch > > Andreas Pflug wrote: > > >>For the second, please supply a patch that moves _all_ of > dbsize into > >>the main server. I think we have agreement on that. > >> > > > > I don't think so. As I mentioned, those views are broken. > Do you want them > > to be in core anyway? > > Why is e.g. this one broken: > int8 database_size(name) - Return the size of the database in > bytes (by name) > > It should do the same as the one with the oid except that it > will resolve > the name first, no? If not it should be fixed, not dropped. I > understand > you'd like to have those functions for the GUI frontends, but > what about > psql users? For many people it will be hard work to type the > subquery to get > the database oid. > > I vote for all (possibly corrected) functions to be moved into core. You have pg_database_size(oid) and database_size(name). Afaict, the latter is equivalent to: SELECT pg_database_size((SELECT oid FROM pg_database WHERE datname = 'foo')) My main concern is that the names are inconsistent for no obvious reason. I also questioned whether or not the bloat of an additional function is worthwhile for what is probably a very small number of psql users that might use it (probably quite rarely), however if people say they would use it and that it's wothwhile, I wouldn't argue with it's inclusion. Regards, Dave
Michael Paesold wrote: > Andreas Pflug wrote: > >>> For the second, please supply a patch that moves _all_ of dbsize into >>> the main server. I think we have agreement on that. >>> >> >> I don't think so. As I mentioned, those views are broken. Do you want >> them to be in core anyway? > > > Why is e.g. this one broken: > int8 database_size(name) - Return the size of the database in > bytes (by name) > > It should do the same as the one with the oid except that it will > resolve the name first, no? If not it should be fixed, not dropped. I > understand you'd like to have those functions for the GUI frontends, > but what about psql users? For many people it will be hard work to > type the subquery to get the database oid. > > I vote for all (possibly corrected) functions to be moved into core. You're correct about the functions, but I was talking about the views. "WHERE name = $1" won't respect the schema, contrary to the current doc. Regards, Andreas
On Fri, Jun 24, 2005 at 05:10:15PM +0100, Dave Page wrote: > You have pg_database_size(oid) and database_size(name). Afaict, the > latter is equivalent to: > > SELECT pg_database_size((SELECT oid FROM pg_database WHERE datname = > 'foo')) > > My main concern is that the names are inconsistent for no obvious > reason. I also questioned whether or not the bloat of an additional > function is worthwhile for what is probably a very small number of psql > users that might use it (probably quite rarely), however if people say > they would use it and that it's wothwhile, I wouldn't argue with it's > inclusion. ISTM it would be better to just add this info into newsysviews. Anyone who regularly queries against the catalog from psql is going to want to have them installed anyway. We could either add exact size info to pg_*_table_storage (http://lnk.nu/cvs.pgfoundry.org/39q.sql) or create a new view with the file sizes. -- Jim C. Nasby, Database Consultant decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828 Windows: "Where do you want to go today?" Linux: "Where do you want to go tomorrow?" FreeBSD: "Are you guys coming, or what?"
Andreas Pflug wrote: > Michael Paesold wrote: > >> Andreas Pflug wrote: >> >>>> For the second, please supply a patch that moves _all_ of dbsize into >>>> the main server. I think we have agreement on that. >>>> >>> >>> I don't think so. As I mentioned, those views are broken. Do you want >>> them to be in core anyway? >> >> >> Why is e.g. this one broken: >> int8 database_size(name) - Return the size of the database in >> bytes (by name) >> >> It should do the same as the one with the oid except that it will resolve >> the name first, no? If not it should be fixed, not dropped. I understand >> you'd like to have those functions for the GUI frontends, but what about >> psql users? For many people it will be hard work to type the subquery to >> get the database oid. >> >> I vote for all (possibly corrected) functions to be moved into core. > > You're correct about the functions, but I was talking about the views. > "WHERE name = $1" won't respect the schema, contrary to the current doc. Oh, I am sorry for not reading carefully enough. Didn't know there were views at all. So if they are broken and cannot be fixed, well... Best Regards, Michael Paesold
Dave Page wrote: > > You have pg_database_size(oid) and database_size(name). Afaict, the > latter is equivalent to: > > SELECT pg_database_size((SELECT oid FROM pg_database WHERE datname = > 'foo')) The typing is even more e.g. for tables or indexes, though. Of course you can use the raw form, but why do we have pg_tables if there is pg_class anyway. > My main concern is that the names are inconsistent for no obvious > reason. That could be fixed by having: pg_database_size(name) pg_database_size(oid) The original idea was probably to name "internal" functions with pg_ and more user friendly ones without pg_. That does not mean it's a good idea. > I also questioned whether or not the bloat of an additional > function is worthwhile for what is probably a very small number of psql > users that might use it (probably quite rarely), however if people say > they would use it and that it's wothwhile, I wouldn't argue with it's > inclusion. Well, I don't feel this is really bloat. I have been using them since the creation of the contrib module and have found them quite useful. Best Regards, Michael Paesold
Dave Page wrote: > > I vote for all (possibly corrected) functions to be moved into core. > > You have pg_database_size(oid) and database_size(name). Afaict, the > latter is equivalent to: > > SELECT pg_database_size((SELECT oid FROM pg_database WHERE datname = > 'foo')) > > My main concern is that the names are inconsistent for no obvious > reason. I also questioned whether or not the bloat of an additional > function is worthwhile for what is probably a very small number of psql > users that might use it (probably quite rarely), however if people say > they would use it and that it's wothwhile, I wouldn't argue with it's > inclusion. Well, this is a good time to figure out exactly what we want in the backend (perhaps with renaming), and which ones we want to keep in /contrib, or delete entirely. The point is that we have to discuss this item by item, _then_ we can look at a patch. A patch with assumptions is just confusing to me. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Dave Page wrote: > > The current version of the patch only moves those functions he wants. > > Marc says he wants them all moved, and I agree. > > OK - did you see Andreas' response to why he hadn't done that (it was > actually posted in response to your original query, not Marcs)? In a > nutshell, the functions that had not been moved returned values that > were easily derived from the other functions, and thus could be > considered bloat? > > The functions included in the patch were: > > int8 pg_tablespace_size(oid) - Return the size of the tablespace in > bytes > int8 pg_database_size(oid) - Return the size of the database in > bytes > int8 pg_relation_size(oid) - Return the size of the relation in > bytes > text pg_size_pretty(int8) - Pretty-print the byte value > > The ones left out were: > > int8 database_size(name) - Return the size of the database in > bytes (by name) > int8 relation_size(text) - Return the size of the relation in > bytes (by name) > int8 indexes_size(text) - Return the size of the indexes on the > named relation > int8 total_relation_size(text) - Return relation_size(text) + > indexes_size(text) + relation_size(text->toast tables) > setof record relation_size_components(text) - return a pretty-print set > of values from above. > > I don't feel particularly strongly either way, but given the normal > desire not to bloat the backend necessarily, I have to question the need > to include the latter functions. OK, well, let's talk about what we want done, then someone can work up a patch. Does someone want to make a proposal here on what to do? > > Well, from the May, 2005 discussion URL you posted, I see a > > clear reply > > to the idea of adding the I/O functions to the backend: > > > > > > http://archives.postgresql.org/pgsql-hackers/2005-05/msg00874.php > > > > Now, you can agree or disagree that there are issues with the I/O > > functions, but the concern was raised in May, and not > > addressed at all, > > either via email or the patch. > > Maybe that's the wrong URL, but all I see there is a vague recollection > from Tom that there were security issues. In the next message, Andreas > recalls how you and he worked out the issues that were raised - I > believe this is the thread > http://archives.postgresql.org/pgsql-hackers/2004-07/msg00793.php. > Mhonarc has made a mess of the thread so it does seem to break in a few > places, and it is possible I've missed part. The security issue is that we didn't want the backend to be able to read/write outside of /pgdata, and I think we have that working, except that I have no idea how it will handle config files outside /pgdata. Maybe that was in the patch --- I don't know. I think we need to see a new patch with just the i/o functions so we can review it. I personally think the I/O functions are a good idea, but I need to be considerate of others in the community who have concerns. > > For the second, please supply a patch that moves _all_ of dbsize into > > the main server. I think we have agreement on that. > > If that remains the case having seen my comments above echoing Andreas' > concerns then sure, if that's what it takes to get them in, so be it. > Please confirm either way. Let's discuss what to move/delete/keep in contrib. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
> -----Original Message----- > From: Michael Paesold [mailto:mpaesold@gmx.at] > Sent: 24 June 2005 17:53 > To: Dave Page; Andreas Pflug > Cc: PostgreSQL-development > Subject: Re: [HACKERS] Server instrumentation patch > > > My main concern is that the names are inconsistent for no obvious > > reason. > > That could be fixed by having: > pg_database_size(name) > pg_database_size(oid) > > The original idea was probably to name "internal" functions > with pg_ and > more user friendly ones without pg_. That does not mean it's > a good idea. Yes, agreed - it could be fixed that way easily. If the inclusion of /all/ functions is for backwards compatibility though, then that change is somewhat more of a problem. > > Well, I don't feel this is really bloat. I have been using > them since the > creation of the contrib module and have found them quite useful. Fair enough. Regards, Dave.
Dave Page wrote: > > > > -----Original Message----- > > From: Michael Paesold [mailto:mpaesold@gmx.at] > > Sent: 24 June 2005 17:53 > > To: Dave Page; Andreas Pflug > > Cc: PostgreSQL-development > > Subject: Re: [HACKERS] Server instrumentation patch > > > > > My main concern is that the names are inconsistent for no obvious > > > reason. > > > > That could be fixed by having: > > pg_database_size(name) > > pg_database_size(oid) > > > > The original idea was probably to name "internal" functions > > with pg_ and > > more user friendly ones without pg_. That does not mean it's > > a good idea. > > Yes, agreed - it could be fixed that way easily. If the inclusion of > /all/ functions is for backwards compatibility though, then that change > is somewhat more of a problem. We are moving the functions into the backend so if we are going to make them more consistent, now is the time. People are already going to not have to load them from /contrib, so a more consistent API change is fine at this stage --- it will be much harder later. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 24 June 2005 18:47 > To: Dave Page > Cc: PostgreSQL-development; Andreas Pflug > Subject: Re: [HACKERS] Server instrumentation patch > > The security issue is that we didn't want the backend to be able to > read/write outside of /pgdata, and I think we have that > working, except Andreas does indeed appear to be checking to ensure that only files under $PGDATA can be accessed, by disallowing any paths containing '..'. > that I have no idea how it will handle config files outside /pgdata. > Maybe that was in the patch --- I don't know. My reading of the code is that it should work OK if they are symlinked from other locations of course, however if hba_file or ident_file are set to locations outside $PGDATA, then that will not work. The log directory can be accessed if it is outside $PGDATA. I'm sure Andreas can confirm this. > I think we need to see a new patch with just the i/o > functions so we can > review it. Andreas, can you (re)post this please? > I personally think the I/O functions are a good > idea, but I > need to be considerate of others in the community who have concerns. Of course. I know we're pushing hard to get these included, but it's not to try to force in a sub-standard solution, it just seems to us like we're revisiting issues that we thought were resolved. We'll get there in the end :-) /D