Re: Server instrumentation patch

> -----Original Message-----
> From: Bruce Momjian [mailto:pgman(at)candle(dot)pha(dot)pa(dot)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.

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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073