Re: [HACKERS] For review: Server instrumentation patch

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin(at)pse-consulting(dot)de]
> Sent: 01 August 2005 15:05
> To: Dave Page
> Cc: Bruce Momjian; PostgreSQL-patches
> Subject: Re: [PATCHES] [HACKERS] For review: Server
> instrumentation patch
>
> Dave Page wrote:
>
> >>
> >>pg_dir_ls isn't necessary for reading the logfiles;
> >>pg_logdir_ls will do
> >>this.
> >
> >
> > Err, yes, sorry - that was a thinko.
>
> The list isn't complete. pgadmin uses these three functions
> for logfile
> tracking:
>
> - pg_logdir_ls to list logfiles
> - pg_file_length to check for changes of the current logfile
> - pg_file_read to retrieve a logfile

Yes you're right, I didn't check thoroughly (in my defence, the coffee
machine broke this morning). Anyhoo, pg_file_stat is used by
pg_file_length, so that would be required as well.

None of those allow any modification of the filesystem, so do not suffer
the potential security issues that Tom was concerned about, so hopefully
there is no problem with them going in?

Regards, Dave.

Dave Page wrote:
> > The list isn't complete. pgadmin uses these three functions
> > for logfile
> > tracking:
> >
> > - pg_logdir_ls to list logfiles
> > - pg_file_length to check for changes of the current logfile
> > - pg_file_read to retrieve a logfile
>
> Yes you're right, I didn't check thoroughly (in my defence, the coffee
> machine broke this morning). Anyhoo, pg_file_stat is used by
> pg_file_length, so that would be required as well.
>
> None of those allow any modification of the filesystem, so do not suffer
> the potential security issues that Tom was concerned about, so hopefully
> there is no problem with them going in?

OK, I have modified the patch to include these functions:

pg_reload_conf()
pg_file_stat()
pg_file_read()
pg_file_length()
pg_dir_ls()
pg_logfile_rotate()
pg_logdir_ls()

These can only be run by the super-user, and can only access files
inside PGDATA, or in the logdir directory if that is in a different
place from PGDATA.

The only part I didn't like about the patch is the stat display:

test=> select pg_file_stat('postgresql.conf');
pg_file_stat
-----------------------------------------------------------------------------

(12287,"2005-08-11 00:06:30","2005-08-11 00:06:43","2005-08-11 00:06:30",f)
(1 row)

Shouldn't this return multiple labeled columns rather than an array?

The patch is attached and genfile.c goes in utils/adt.

--
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

Bruce Momjian wrote:
> Dave Page wrote:
>
>
> The only part I didn't like about the patch is the stat display:
>
> test=> select pg_file_stat('postgresql.conf');
> pg_file_stat
> -----------------------------------------------------------------------------
>
> (12287,"2005-08-11 00:06:30","2005-08-11 00:06:43","2005-08-11 00:06:30",f)
> (1 row)
>
> Shouldn't this return multiple labeled columns rather than an array?

pg_show_all_settings output is equally unreadable, designed not to be
used directly.

Regards,
Andreas

Andreas Pflug wrote:
> Bruce Momjian wrote:
> > Dave Page wrote:
> >
> >
> > The only part I didn't like about the patch is the stat display:
> >
> > test=> select pg_file_stat('postgresql.conf');
> > pg_file_stat
> > -----------------------------------------------------------------------------
> >
> > (12287,"2005-08-11 00:06:30","2005-08-11 00:06:43","2005-08-11 00:06:30",f)
> > (1 row)
> >
> > Shouldn't this return multiple labeled columns rather than an array?
>
> pg_show_all_settings output is equally unreadable, designed not to be
> used directly.

True, but we have SELECT * FROM pg_settings() that does display the
titles. Right now the values aren't even documented.

--
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

On Thu, Aug 11, 2005 at 11:34:43AM -0400, Bruce Momjian wrote:
> Andreas Pflug wrote:
> > Bruce Momjian wrote:
> > > Dave Page wrote:
> > >
> > >
> > > The only part I didn't like about the patch is the stat display:
> > >
> > > test=> select pg_file_stat('postgresql.conf');
> > > pg_file_stat
> > > -----------------------------------------------------------------------------
> > >
> > > (12287,"2005-08-11 00:06:30","2005-08-11 00:06:43","2005-08-11 00:06:30",f)
> > > (1 row)
> > >
> > > Shouldn't this return multiple labeled columns rather than an array?
> >
> > pg_show_all_settings output is equally unreadable, designed not to be
> > used directly.
>
> True, but we have SELECT * FROM pg_settings() that does display the
> titles. Right now the values aren't even documented.

So, did you try

select * from pg_file_stats('postgresql.conf');

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
Si no sabes adonde vas, es muy probable que acabes en otra parte.

Alvaro Herrera wrote:
> On Thu, Aug 11, 2005 at 11:34:43AM -0400, Bruce Momjian wrote:
> > Andreas Pflug wrote:
> > > Bruce Momjian wrote:
> > > > Dave Page wrote:
> > > >
> > > >
> > > > The only part I didn't like about the patch is the stat display:
> > > >
> > > > test=> select pg_file_stat('postgresql.conf');
> > > > pg_file_stat
> > > > -----------------------------------------------------------------------------
> > > >
> > > > (12287,"2005-08-11 00:06:30","2005-08-11 00:06:43","2005-08-11 00:06:30",f)
> > > > (1 row)
> > > >
> > > > Shouldn't this return multiple labeled columns rather than an array?
> > >
> > > pg_show_all_settings output is equally unreadable, designed not to be
> > > used directly.
> >
> > True, but we have SELECT * FROM pg_settings() that does display the
> > titles. Right now the values aren't even documented.
>
> So, did you try
>
> select * from pg_file_stats('postgresql.conf');

Yes, I got:

ERROR: a column definition list is required for functions returning "record"

--
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

On Thu, Aug 11, 2005 at 01:19:29PM -0400, Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > So, did you try
> >
> > select * from pg_file_stats('postgresql.conf');
>
> Yes, I got:
>
> ERROR: a column definition list is required for functions returning "record"

Interesting. I wonder why the function is not defined instead with OUT
parameters. That way you don't have to declare the record at execution
time. See my patch to pgstattuple for an example.

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"No es bueno caminar con un hombre muerto"

Alvaro Herrera wrote:
> On Thu, Aug 11, 2005 at 01:19:29PM -0400, Bruce Momjian wrote:
> > Alvaro Herrera wrote:
>
> > > So, did you try
> > >
> > > select * from pg_file_stats('postgresql.conf');
> >
> > Yes, I got:
> >
> > ERROR: a column definition list is required for functions returning "record"
>
> Interesting. I wonder why the function is not defined instead with OUT
> parameters. That way you don't have to declare the record at execution
> time. See my patch to pgstattuple for an example.

Uh, where is that patch? I can't find it.

--
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

On Thu, Aug 11, 2005 at 02:26:16PM -0400, Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > Interesting. I wonder why the function is not defined instead with OUT
> > parameters. That way you don't have to declare the record at execution
> > time. See my patch to pgstattuple for an example.
>
> Uh, where is that patch? I can't find it.

Hmm, seems it never made it to the list, I can't find it in the
archives either. Here it is.

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"¿Qué importan los años? Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo" (Mafalda)

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Interesting. I wonder why the function is not defined instead with OUT
> parameters.

Because bootstrap mode isn't capable of dealing with array columns,
so you can't define stuff in pg_proc.h that sets up an array of OUT
parameter types. I tried to apply that idea for the pg_locks function
a month or two ago, but it blew up in my face :-(.

It'd be nice to fix this sometime, but not while we are so far past
feature freeze.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Interesting. I wonder why the function is not defined instead with OUT
> > parameters.
>
> Because bootstrap mode isn't capable of dealing with array columns,
> so you can't define stuff in pg_proc.h that sets up an array of OUT
> parameter types. I tried to apply that idea for the pg_locks function
> a month or two ago, but it blew up in my face :-(.
>
> It'd be nice to fix this sometime, but not while we are so far past
> feature freeze.

The patch includes documentation about the meaning the return values,
and I think that is good enough.

--
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

Here is an updated patch I have just applied (catalog version updated).
I have added these functions:

pg_stat_file()
pg_read_file()
pg_ls_dir()
pg_reload_conf()
pg_rotate_logfile()

I did not include pg_logdir_ls because that was basically pg_ls_dir with
logic to decode the log file name and convert it to a timestamp. That
seemed best done in the client.

I also renamed a number of the functions to be use verb-noun, rather
than noun-verb.

--
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

Bruce Momjian wrote:

>
> I did not include pg_logdir_ls because that was basically pg_ls_dir with
> logic to decode the log file name and convert it to a timestamp. That
> seemed best done in the client.

IMHO omitting pg_logdir_ls is a bad idea, because the function is
intended to hide server internal's naming scheme from the user. We want
as few server side implementation specific client side code as possible.
In addition, pg_logdir_ls filters files that are not log files.
You'll probably have trouble writing a query that performes this task.

Regards,
Andreas

On Fri, Aug 12, 2005 at 11:28:22AM +0000, Andreas Pflug wrote:
> Bruce Momjian wrote:
>
> >
> >I did not include pg_logdir_ls because that was basically pg_ls_dir with
> >logic to decode the log file name and convert it to a timestamp. That
> >seemed best done in the client.
>
> IMHO omitting pg_logdir_ls is a bad idea, because the function is
> intended to hide server internal's naming scheme from the user. We want
> as few server side implementation specific client side code as possible.

BTW, it surprised me that one of the functions (don't remember which
one) expected the log files to be named in a very specific fashion. So
there's no flexibility for changing the log_prefix. Probably it's not
so bad, but strange anyway. Is this for "security" reasons?

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
Y dijo Dios: "Que sea Satanás, para que la gente no me culpe de todo a mí."
"Y que hayan abogados, para que la gente no culpe de todo a Satanás"

Alvaro Herrera wrote:
> On Fri, Aug 12, 2005 at 11:28:22AM +0000, Andreas Pflug wrote:
> > Bruce Momjian wrote:
> >
> > >
> > >I did not include pg_logdir_ls because that was basically pg_ls_dir with
> > >logic to decode the log file name and convert it to a timestamp. That
> > >seemed best done in the client.
> >
> > IMHO omitting pg_logdir_ls is a bad idea, because the function is
> > intended to hide server internal's naming scheme from the user. We want
> > as few server side implementation specific client side code as possible.
>
> BTW, it surprised me that one of the functions (don't remember which
> one) expected the log files to be named in a very specific fashion. So
> there's no flexibility for changing the log_prefix. Probably it's not
> so bad, but strange anyway. Is this for "security" reasons?

Righ, pg_logdir_ls() was the function. My feeling is that the
application has access to the log_directory and log_filename values and
can better and move flexibly filter pg_ls_dir() on the client end than
we can do on the server. It just seemed like something that we better
done outside the server.

--
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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Here is an updated patch I have just applied (catalog version updated).

Actually, you forgot the catversion bump.

I read over this and fixed most of the problems I could see, but there
is still one left:

/*
* Prevent reference to the parent directory.
* "..a.." is a valid file name though.
*
* XXX this is BROKEN because it fails to prevent "C:.." on Windows.
* Need access to "skip_drive" functionality to do it right. (There
* is no actual security hole because we'll prepend the DataDir below,
* resulting in a just-plain-broken path, but we should give the right
* error message instead.)
*/

I'm not sure whether to export skip_drive from path.c or just duplicate
it. If we do export it, a different name would probably be a good idea,
as it seems too generic for a global symbol.

regards, tom lane

I wrote:
> I'm not sure whether to export skip_drive from path.c or just duplicate
> it. If we do export it, a different name would probably be a good idea,
> as it seems too generic for a global symbol.

On reflection, there's another possibility, which is to put the test
code into path.c in the first place, defined something like

bool path_contains_parent_reference(const char *path);

This is probably better since the whole business of looking for ".."
seems like something that should be in path.c and not elsewhere.

regards, tom lane

Bruce Momjian wrote:

>> BTW, it surprised me that one of the functions (don't remember
>> which one) expected the log files to be named in a very specific
>> fashion. So there's no flexibility for changing the log_prefix.
>> Probably it's not so bad, but strange anyway. Is this for
>> "security" reasons?

The logger subprocess patch originally didn't allow changing the the
logfile name pattern, to make sure it can be interpreted safely at a
later time. There's simply no way to mark the file with a timestamp
without the risk of it being arbitrarily modified by file commands, thus
screwing up the order of logfiles. Later, there was the request to
alternatively append a timestamp instead of a date pattern, to use
apache logging tools that will probably access the logfiles directly
anyway. This ended up in the log_filename GUC variable.

>
> Righ, pg_logdir_ls() was the function. My feeling is that the
> application has access to the log_directory and log_filename values
> and can better and move flexibly filter pg_ls_dir() on the client end
> than we can do on the server. It just seemed like something that we
> better done outside the server.

Outside the server means pure SQL, if you don't want to drop psql as
client. So how would your query to display all all available _logfiles_
look like? You'd need to check for a valid date, besides interpreting
pg_strfime's patterns. Doesn't sound exactly like fun, but I'm keen to
see how your equivalent to

SELECT *, pg_file_length(filename) AS len
FROM pg_logdir_ls

looks like.

Regards,
Andreas

Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
> So how would your query to display all all available _logfiles_
> look like?

I think it's perfectly reasonable to assume that all the files in the
log directory are logfiles --- more so than assuming that the admin
hasn't exercised his option to change the log filename pattern, anyway.

I also don't have a problem with using the file mod times to sort them.
Yes, you can break that if you go and poke the files --- but there's no
reason to do so, whereas there are many reasons for changing the log
filename pattern.

regards, tom lane

Tom Lane wrote:
> Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
>
>>So how would your query to display all all available _logfiles_
>>look like?
>
>
> I think it's perfectly reasonable to assume that all the files in the
> log directory are logfiles --- more so than assuming that the admin
> hasn't exercised his option to change the log filename pattern, anyway.
>
> I also don't have a problem with using the file mod times to sort them.

... until you copy the database cluster. See discussion from last year.

Regards,
Andreas

Andreas Pflug wrote:
> Bruce Momjian wrote:
>
> >> BTW, it surprised me that one of the functions (don't remember
> >> which one) expected the log files to be named in a very specific
> >> fashion. So there's no flexibility for changing the log_prefix.
> >> Probably it's not so bad, but strange anyway. Is this for
> >> "security" reasons?
>
> The logger subprocess patch originally didn't allow changing the the
> logfile name pattern, to make sure it can be interpreted safely at a
> later time. There's simply no way to mark the file with a timestamp
> without the risk of it being arbitrarily modified by file commands, thus
> screwing up the order of logfiles. Later, there was the request to
> alternatively append a timestamp instead of a date pattern, to use
> apache logging tools that will probably access the logfiles directly
> anyway. This ended up in the log_filename GUC variable.
>
> >
> > Righ, pg_logdir_ls() was the function. My feeling is that the
> > application has access to the log_directory and log_filename values
> > and can better and move flexibly filter pg_ls_dir() on the client end
> > than we can do on the server. It just seemed like something that we
> > better done outside the server.
>
> Outside the server means pure SQL, if you don't want to drop psql as
> client. So how would your query to display all all available _logfiles_
> look like? You'd need to check for a valid date, besides interpreting
> pg_strfime's patterns. Doesn't sound exactly like fun, but I'm keen to
> see how your equivalent to

I don't assume people using psql will care about the current log files ---
it would be something done in C or another application language. Aren't
the file names already ordered based on their file names, given the
default pattern, postgresql-%Y-%m-%d_%H%M%S.log?

--
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

Bruce Momjian wrote:

>
>
> I don't assume people using psql will care about the current log files ---

Hm. Probably because you think these users will have direct file access?
Which in turn means they can edit *.conf directly too and don't need an
interface for that either.

> it would be something done in C or another application language. Aren't
> the file names already ordered based on their file names, given the
> default pattern, postgresql-%Y-%m-%d_%H%M%S.log?

The issue is _filtering_, not ordering. Since the log directory might be
directed to a different location, non-pgsql logfiles might be there too.
You'd probably won't expect to retrieve these files over a pgsql connection.

Regards,
Andreas

Andreas Pflug wrote:
> Bruce Momjian wrote:
>
> >
> >
> > I don't assume people using psql will care about the current log files ---
>
> Hm. Probably because you think these users will have direct file access?
> Which in turn means they can edit *.conf directly too and don't need an
> interface for that either.

I don't see how listing the log files relates to editing the confuration
files.

> > it would be something done in C or another application language. Aren't
> > the file names already ordered based on their file names, given the
> > default pattern, postgresql-%Y-%m-%d_%H%M%S.log?
>
> The issue is _filtering_, not ordering. Since the log directory might be
> directed to a different location, non-pgsql logfiles might be there too.
> You'd probably won't expect to retrieve these files over a pgsql connection.

Well, if they mix log files and non-log files in the same directory, we
would have to filter based on the log_filename directive in the
application, or use LIKE in a query.

--
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

Bruce Momjian wrote:

>
>
> I don't see how listing the log files relates to editing the confuration
> files.

Both are remote administration. While we've seen the discussion that one
aspect (config file editing) should be performed in psql, you assume the
other aspect (viewing the logfile) to be not interesting. Your
argumentation doesn't seem consequent to me.

>>>it would be something done in C or another application language. Aren't
>>>the file names already ordered based on their file names, given the
>>>default pattern, postgresql-%Y-%m-%d_%H%M%S.log?
>>
>>The issue is _filtering_, not ordering. Since the log directory might be
>>directed to a different location, non-pgsql logfiles might be there too.
>>You'd probably won't expect to retrieve these files over a pgsql connection.
>
>
> Well, if they mix log files and non-log files in the same directory, we
> would have to filter based on the log_filename directive in the
> application, or use LIKE in a query.

.. which is what pg_logdir_ls does. And it's robust against filenames
that don't have valid dates too; imagine postgresql-2005-01-01_crash1.log.

Regards,
Andreas

Andreas Pflug wrote:
> Bruce Momjian wrote:
>
> >
> >
> > I don't see how listing the log files relates to editing the confuration
> > files.
>
> Both are remote administration. While we've seen the discussion that one
> aspect (config file editing) should be performed in psql, you assume the
> other aspect (viewing the logfile) to be not interesting. Your
> argumentation doesn't seem consequent to me.

To me monitoring (logfiles) and configuration (postgresql.conf) are
different, but if I can make it easy to do both from psql, great.

I can see making postgresql.conf easy (SET GLOBAL), I am unsure about
making pg_hba.conf easy (and many feel that way), and I am not sure how
adding a log directory listing took makes things significantly easier to
monitor log files.

What I can imagine making things very easy is a readonly GUC that returns
the current log file name. That I think should be in the backend, and I
can see a query that combines that with pg_read_file() that prints the
last 1000 bytes from the file.

SELECT pg_read_file(t1.setting, -1000, 1000);
FROM (SELECT setting FROM pg_settings WHERE NAME = 'log_current_name') AS t1

> >>>it would be something done in C or another application language. Aren't
> >>>the file names already ordered based on their file names, given the
> >>>default pattern, postgresql-%Y-%m-%d_%H%M%S.log?
> >>
> >>The issue is _filtering_, not ordering. Since the log directory might be
> >>directed to a different location, non-pgsql logfiles might be there too.
> >>You'd probably won't expect to retrieve these files over a pgsql connection.
> >
> >
> > Well, if they mix log files and non-log files in the same directory, we
> > would have to filter based on the log_filename directive in the
> > application, or use LIKE in a query.
>
> .. which is what pg_logdir_ls does. And it's robust against filenames
> that don't have valid dates too; imagine postgresql-2005-01-01_crash1.log.

True, but that is more for the application. I don't imagine a user
looking at that from psql would have a problem.

However, you asked for a query that looks like pg_ls_logdir() and here
it is:

SELECT pg_ls_dir
FROM (
SELECT pg_ls_dir(t1.setting)
FROM (SELECT setting FROM pg_settings WHERE NAME = 'log_directory') AS t1
) AS t2,
(SELECT setting FROM pg_settings WHERE NAME = 'log_filename') AS t3
WHERE t2.pg_ls_dir LIKE regexp_replace(t3.setting, '%.*', '') || '%';

The one thing it doesn't do, as you mentioned, is check for valid dates,
but it is certainly more flexible than embedding something in the backend.

--
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

Bruce Momjian wrote:

>
> True, but that is more for the application. I don't imagine a user
> looking at that from psql would have a problem.
>
> However, you asked for a query that looks like pg_ls_logdir() and here
> it is:
>
> SELECT pg_ls_dir
> FROM (
> SELECT pg_ls_dir(t1.setting)
> FROM (SELECT setting FROM pg_settings WHERE NAME = 'log_directory') AS t1
> ) AS t2,
> (SELECT setting FROM pg_settings WHERE NAME = 'log_filename') AS t3
> WHERE t2.pg_ls_dir LIKE regexp_replace(t3.setting, '%.*', '') || '%';
>
> The one thing it doesn't do, as you mentioned, is check for valid dates,
> but it is certainly more flexible than embedding something in the backend.

The interesting part of pg_logdir_ls is the filetime, to enable

SELECT pg_file_unlink(filename)
FROM pg_logdir_ls()
WHERE filetime < now() - '30 days'::interval

Regards,
Andreas

Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
> Bruce Momjian wrote:
>> Well, if they mix log files and non-log files in the same directory, we
>> would have to filter based on the log_filename directive in the
>> application, or use LIKE in a query.

> .. which is what pg_logdir_ls does. And it's robust against filenames
> that don't have valid dates too; imagine postgresql-2005-01-01_crash1.log.

The proposed version of pg_logdir_ls could not be called "robust" in any
way at all, considering that it fails as soon as you modify the log_filename
pattern.

I concur with Bruce that this is better left to the application side.
I don't see any basic functionality gain from doing it in the server.
The client code can look at log_filename and do the filtering just as
well (or badly) as it could possibly be done in the server. Moreover,
having a restriction like "this doesn't work unless you use this
log_filename setting" feels more reasonable on the client side than
inside the server.

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> What I can imagine making things very easy is a readonly GUC that returns
> the current log file name.

... which unfortunately is not going to happen since the backends can't
see inside the syslogger process to know what it's doing.

ATM I think the best you can do is look for the newest mod date among
the files in the log directory.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > What I can imagine making things very easy is a readonly GUC that returns
> > the current log file name.
>
> ... which unfortunately is not going to happen since the backends can't
> see inside the syslogger process to know what it's doing.

That's a shame.

> ATM I think the best you can do is look for the newest mod date among
> the files in the log directory.

I guess, but then you have the pattern problem, especially if you are
putting things in /var/log where there are other log files too.

One idea would be to implement pg_ls_logdir() as a system view, and then
build a GUC on that, but I am not sure that is possible. Here is an
updated version of the query that also checks the file extension:

SELECT pg_ls_dir
FROM (
SELECT pg_ls_dir(t1.setting)
FROM (SELECT setting FROM pg_settings
WHERE NAME = 'log_directory') AS t1
) AS t2,
(SELECT setting FROM pg_settings
WHERE NAME = 'log_filename') AS t3
WHERE t2.pg_ls_dir LIKE regexp_replace(t3.setting, '%.*', '') ||
'%' || regexp_replace(t3.setting, '.*\\.', '.') ;

pg_ls_dir
----------------------------------
postgresql-2005-08-12_211251.log
postgresql-2005-08-13_000000.log
(2 rows)

Also, do we have a way to return columns from a system-installed
function? I really don't like that pg_stat_file() to returns a record
rather than named columns. How do I even access the individual record
values?

test=> select pg_stat_file('.');
pg_stat_file
---------------------------------------------------------------------------

(512,"2005-08-12 21:13:01","2005-08-13 07:08:54","2005-08-12 21:13:01",t)
(1 row)

test=> select pg_stat_file('.')[1];
ERROR: syntax error at or near "[" at character 25

We have system _tables_ that return columns, like pg_settings, but of
course that doesn't take any arguments, just a WHERE clause, so that
wouldn't work here.

--
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

Tom Lane wrote:
> Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
>
>>Bruce Momjian wrote:
>>
>>>Well, if they mix log files and non-log files in the same directory, we
>>>would have to filter based on the log_filename directive in the
>>>application, or use LIKE in a query.
>
>
>>.. which is what pg_logdir_ls does. And it's robust against filenames
>>that don't have valid dates too; imagine postgresql-2005-01-01_crash1.log.
>
>
> The proposed version of pg_logdir_ls could not be called "robust" in any
> way at all, considering that it fails as soon as you modify the log_filename
> pattern.

This is caused by the exposure of log_filename, I never proposed to do
that for good reasons. Any try to interpret it and read files back will
break finally when log_filename is changed at runtime, i.e. it's a
'break me' option by design.

Regards,
Andreas

Bruce Momjian wrote:
>
> Also, do we have a way to return columns from a system-installed
> function? I really don't like that pg_stat_file() to returns a record
> rather than named columns. How do I even access the individual record
> values?

As in pg_settings:

SELECT length, mtime FROM pg_file_stat('postgresql.conf') AS st(length
int4, ctime timestamp, atime timestamp, mtime timestamp, isdir bool)

Regards,
Andreas

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> SELECT pg_ls_dir
> FROM (
> SELECT pg_ls_dir(t1.setting)
> FROM (SELECT setting FROM pg_settings
> WHERE NAME = 'log_directory') AS t1
> ) AS t2,
> (SELECT setting FROM pg_settings
> WHERE NAME = 'log_filename') AS t3
> WHERE t2.pg_ls_dir LIKE regexp_replace(t3.setting, '%.*', '') ||
> '%' || regexp_replace(t3.setting, '.*\\.', '.') ;

This is unnecessarily complicated --- use current_setting, eg,

select * from pg_ls_dir(current_setting('log_directory'))
where pg_ls_dir like
regexp_replace(current_setting('log_filename'), '%.', '%', 'g');

> I really don't like that pg_stat_file() to returns a record
> rather than named columns. How do I even access the individual record
> values?

"select * from ...". See the documentation:

Use it like this:

SELECT *
FROM pg_stat_file('filename')
AS s(length int8, atime timestamptz, mtime timestamptz,
ctime timestamptz, isdir bool);

I suppose as long it's just this one function at stake, we could imagine
fixing the pg_proc row after-the-fact (later in the initdb sequence).
Pretty klugy but something nicer could get done in the 8.2 time frame.

regards, tom lane

Andreas Pflug wrote:
> Bruce Momjian wrote:
> >
> > Also, do we have a way to return columns from a system-installed
> > function? I really don't like that pg_stat_file() to returns a record
> > rather than named columns. How do I even access the individual record
> > values?
>
> As in pg_settings:
>
> SELECT length, mtime FROM pg_file_stat('postgresql.conf') AS st(length
> int4, ctime timestamp, atime timestamp, mtime timestamp, isdir bool)

Ewe, that is ugly. How does the user even know the data types? int4 or
int8? timestamp or timestamptz? \df doesn't show it:

pg_catalog | pg_stat_file | record | text

and it isn't in the documenation. However, internally, it knows. In
fact your example is wrong because the size it int8, not int4:

test=> SELECT length, mtime FROM pg_stat_file('postgresql.conf') AS st(length
test(> int4, ctime timestamp, atime timestamp, mtime timestamp, isdir bool);
ERROR: function return row and query-specified return row do not match
DETAIL: Returned type bigint at ordinal position 1, but query expects integer.

test=> SELECT length, mtime FROM pg_stat_file('postgresql.conf') AS st(length
test(> int8, ctime timestamp, atime timestamp, mtime timestamp, isdir bool);
length | mtime
--------+---------------------
12576 | 2005-08-12 21:12:36
(1 row)

Let me repond to Tom's email.

--
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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > SELECT pg_ls_dir
> > FROM (
> > SELECT pg_ls_dir(t1.setting)
> > FROM (SELECT setting FROM pg_settings
> > WHERE NAME = 'log_directory') AS t1
> > ) AS t2,
> > (SELECT setting FROM pg_settings
> > WHERE NAME = 'log_filename') AS t3
> > WHERE t2.pg_ls_dir LIKE regexp_replace(t3.setting, '%.*', '') ||
> > '%' || regexp_replace(t3.setting, '.*\\.', '.') ;
>
> This is unnecessarily complicated --- use current_setting, eg,
>
> select * from pg_ls_dir(current_setting('log_directory'))
> where pg_ls_dir like
> regexp_replace(current_setting('log_filename'), '%.', '%', 'g');

Nice.

> > I really don't like that pg_stat_file() to returns a record
> > rather than named columns. How do I even access the individual record
> > values?
>
> "select * from ...". See the documentation:
>
> Use it like this:
>
> SELECT *
> FROM pg_stat_file('filename')
> AS s(length int8, atime timestamptz, mtime timestamptz,
> ctime timestamptz, isdir bool);
>
> I suppose as long it's just this one function at stake, we could imagine
> fixing the pg_proc row after-the-fact (later in the initdb sequence).
> Pretty klugy but something nicer could get done in the 8.2 time frame.

Yes, see my earlier email --- we don't even document the return type of
the function, nor does \df show it. This seems too hard to use.

I am worried that if we improve things in 8.2, we would then be changing
the API of the function. Are the other functions returning records usable?
Could we fix it in a way that later improvements would maintain the same
API?

--
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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> I suppose as long it's just this one function at stake, we could imagine
>> fixing the pg_proc row after-the-fact (later in the initdb sequence).
>> Pretty klugy but something nicer could get done in the 8.2 time frame.

> Yes, see my earlier email --- we don't even document the return type of
> the function, nor does \df show it. This seems too hard to use.

> I am worried that if we improve things in 8.2, we would then be changing
> the API of the function.

Yeah, we would.

> Are the other functions returning records usable?

All the other ones are meant to be used via views, so it doesn't matter
so much. pg_stat_file can't very usefully be called through a view, so
we have a problem.

I'll see about installing an initdb-time kluge to make it use OUT
parameters.

regards, tom lane

I wrote:
> I'll see about installing an initdb-time kluge to make it use OUT
> parameters.

Done:

regression=# SELECT * FROM pg_stat_file('postgresql.conf');
length | atime | mtime | ctime | isdir
--------+------------------------+------------------------+------------------------+-------
12578 | 2005-08-13 14:51:03-04 | 2005-08-13 14:50:32-04 | 2005-08-13 14:50:32-04 | f
(1 row)

I removed the separate pg_file_length() function, as it doesn't have any
significant notational advantage anymore; you can do

regression=# select (pg_stat_file('postgresql.conf')).length;
length
--------
12578
(1 row)

BTW, \df is no real help when it comes to stuff with OUT parameters;
it still says

regression=# \df pg_stat_file
List of functions
Schema | Name | Result data type | Argument data types
------------+--------------+------------------+---------------------
pg_catalog | pg_stat_file | record | text
(1 row)

Possibly we should try to improve that.

regards, tom lane

Tom Lane wrote:
> I wrote:
> > I'll see about installing an initdb-time kluge to make it use OUT
> > parameters.
>
> Done:
>
> regression=# SELECT * FROM pg_stat_file('postgresql.conf');
> length | atime | mtime | ctime | isdir
> --------+------------------------+------------------------+------------------------+-------
> 12578 | 2005-08-13 14:51:03-04 | 2005-08-13 14:50:32-04 | 2005-08-13 14:50:32-04 | f
> (1 row)

Great.

> I removed the separate pg_file_length() function, as it doesn't have any
> significant notational advantage anymore; you can do

Perfect. I was going to suggest that could be removed once pg_stat_file
was more usable.

> regression=# select (pg_stat_file('postgresql.conf')).length;
> length
> --------
> 12578
> (1 row)

Great. I was also wondering if that would work. One more closed item!

> BTW, \df is no real help when it comes to stuff with OUT parameters;
> it still says
>
> regression=# \df pg_stat_file
> List of functions
> Schema | Name | Result data type | Argument data types
> ------------+--------------+------------------+---------------------
> pg_catalog | pg_stat_file | record | text
> (1 row)
>
> Possibly we should try to improve that.

Good point. Let's see if people ask for it. Because they don't need to
know the data types to use the function, we might be fine.

--
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

Tom Lane wrote:
>
> I removed the separate pg_file_length() function, as it doesn't have any
> significant notational advantage anymore; you can do

Please note that there are pg_file_length functions in use for 8.0 on
probably >95 % of win32 installations, so you're breaking backwards
compatibility.

Regards,
Andreas

Andreas Pflug wrote:

> Tom Lane wrote:
>
>>
>> I removed the separate pg_file_length() function, as it doesn't have any
>> significant notational advantage anymore; you can do
>
>
> Please note that there are pg_file_length functions in use for 8.0 on
> probably >95 % of win32 installations, so you're breaking backwards
> compatibility.
>
>

If the windows packagers have added extra functions into their build of
postgres, then they can maintain them, no? Of course, this would
illustrate the danger of such a practice. Postgres is surely only
obligated to try not to break backwards compatibility with its own
released code.

cheers

andrew

Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
> Tom Lane wrote:
>> I removed the separate pg_file_length() function, as it doesn't have any
>> significant notational advantage anymore; you can do

> Please note that there are pg_file_length functions in use for 8.0 on
> probably >95 % of win32 installations, so you're breaking backwards
> compatibility.

What backwards compatibility? Bruce already renamed several of these
functions.

regards, tom lane

Tom Lane wrote:
> Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
>
>>Tom Lane wrote:
>>
>>>I removed the separate pg_file_length() function, as it doesn't have any
>>>significant notational advantage anymore; you can do
>
>
>>Please note that there are pg_file_length functions in use for 8.0 on
>>probably >95 % of win32 installations, so you're breaking backwards
>>compatibility.
>
>
> What backwards compatibility? Bruce already renamed several of these
> functions.

You're right. These arbitrary renames brake nearly all of the existing
code. Great.

Regards,
Andreas