logfile subprocess and Fancy File Functions

Updated patch which leaves postmaster runnable after the syslogger
terminated due to pipe problems.
Also includes an updated pg_proc.h which was totally f*cked up in my
previous posting.

Regards,
Andreas

Magnus Hagander wrote:

Super-minor nitpicking from just eyeing over the patch, not actually
checking how it works.

Reviewing the own code the most obvious things are overlooked.

This patch changes the error message for pg_signal_backend() to "only
superuser may access generic file functions".

I'm sure that was not intended.. You probably need to pass a parameter
to requireSuperuser() about what should go in the err msg.

Yes, seems I was a bit overenthusiastic...

Also, I think you forgot to attach syslogger.h.

Indeed, attached is include/postmaster/syslogger.h

Regards,
Andreas

Andreas Pflug wrote:

Updated patch which leaves postmaster runnable after the syslogger
terminated due to pipe problems.

Very nice. You did a nice trick of reading the log filenames into a
timestamp field:

count = sscanf(de->d_name, "%04d-%02d-%02d_%02d%02d%02d_%05d.log", &yea$

You only process files that match that pattern for pg_logfiles_ls()
(perhaps this should be pg_logdir_ls for consistency). And you can then
process the timestamp field in queries. Good idea. What happens if a
filename matches the above pattern but isn't a valid timestamp? Does
the function fail?

My only question is whether we need to allow a custom prefix for the
log filenames so they can be distinguished from other file names in a
user-supplied log directory, like /var/log, or would they always go into
a separate directory under there. I think a prefix would be nice.

Of course this needs docs but I assume you are waiting to see it applied
first.

-- 
  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, Pennsylvania 19073

Bruce Momjian wrote:

Andreas Pflug wrote:

Very nice. You did a nice trick of reading the log filenames into a
timestamp field:

count = sscanf(de->d_name, "%04d-%02d-%02d_%02d%02d%02d_%05d.log", &yea$

You only process files that match that pattern for pg_logfiles_ls()
(perhaps this should be pg_logdir_ls for consistency).

Yup.

And you can then

process the timestamp field in queries. Good idea. What happens if a
filename matches the above pattern but isn't a valid timestamp? Does
the function fail?

Right now, BuildTupleFromCString will fail for invalid timestamps.

I'm going to change that to pgsql's internal function (strptime seems a
bad idea though).

My only question is whether we need to allow a custom prefix for the
log filenames so they can be distinguished from other file names in a
user-supplied log directory, like /var/log, or would they always go into
a separate directory under there. I think a prefix would be nice.

How should the prefix be named? pgsql_ ?

Of course this needs docs but I assume you are waiting to see it applied
first.

Not necessarily, but I'd like names etc. fixed before.

Regards,
Andreas

Andreas Pflug wrote:

process the timestamp field in queries. Good idea. What happens if a
filename matches the above pattern but isn't a valid timestamp? Does
the function fail?

Right now, BuildTupleFromCString will fail for invalid timestamps.

I'm going to change that to pgsql's internal function (strptime seems a
bad idea though).

Tom used abstimein() in xlog.c.

My only question is whether we need to allow a custom prefix for the
log filenames so they can be distinguished from other file names in a
user-supplied log directory, like /var/log, or would they always go into
a separate directory under there. I think a prefix would be nice.

How should the prefix be named? pgsql_ ?

I would default to that, yes, but allow the user to set it via GUC
variable. You want to hard-code the time part of the file name so you
can load it into a timestamp, but we should give users control over a
prefix just in case they mix the files in a directory with log files
from other applications.

Of course this needs docs but I assume you are waiting to see it applied
first.

Not necessarily, but I'd like names etc. fixed before.

Sure.

-- 
  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, Pennsylvania 19073

Andreas Pflug wrote:

How should the prefix be named? pgsql_ ?

Make the file names configurable.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:

Andreas Pflug wrote:

How should the prefix be named? pgsql_ ?

Make the file names configurable.

He has code to interpret the file names as timestamps that can be used
in queries. If we allowed full user control over the file name, he
couldn't do that.

-- 
  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, Pennsylvania 19073

Bruce Momjian wrote:

Peter Eisentraut wrote:

Andreas Pflug wrote:

How should the prefix be named? pgsql_ ?

Make the file names configurable.

He has code to interpret the file names as timestamps that can be
used in queries. If we allowed full user control over the file name,
he couldn't do that.

I can't see this working. As you know, there are constantly people who
want to install and configure PostgreSQL in the weirdest ways. If we
tell everybody, you log files must be named like this, it will start
all over again.

Maybe it would be better if the time stamps of the files are used as
time stamps in queries.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:

Bruce Momjian wrote:

Peter Eisentraut wrote:

Andreas Pflug wrote:

How should the prefix be named? pgsql_ ?

Make the file names configurable.

He has code to interpret the file names as timestamps that can be
used in queries. If we allowed full user control over the file name,
he couldn't do that.

I can't see this working. As you know, there are constantly people who
want to install and configure PostgreSQL in the weirdest ways. If we
tell everybody, you log files must be named like this, it will start
all over again.

Maybe it would be better if the time stamps of the files are used as
time stamps in queries.

I guess you could use the creation time of the inode as the start time,
yes, and then they could call the file names whatever they want. We
could even add the modification time as the end time.

-- 
  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, Pennsylvania 19073

Bruce Momjian wrote:

Peter Eisentraut wrote:

Bruce Momjian wrote:

Peter Eisentraut wrote:

Andreas Pflug wrote:

How should the prefix be named? pgsql_ ?

Make the file names configurable.

He has code to interpret the file names as timestamps that can be
used in queries. If we allowed full user control over the file name,
he couldn't do that.

I can't see this working. As you know, there are constantly people who
want to install and configure PostgreSQL in the weirdest ways. If we
tell everybody, you log files must be named like this, it will start
all over again.

Maybe it would be better if the time stamps of the files are used as
time stamps in queries.

Imagine an older logfile was edited with lets say emacs, which will
rename the old and create a new file. Or after log_directory was
changed, the files from the old location are copied to the new location.
This would garble the log_dir_ls output badly.

The logfilename currently also includes the postmaster's pid, there's no
file metadata that could take this information safely.

Apparently it's best to invent a log_file_prefix = 'pgsql_' guc variable.

In fact one idea would be to add new stat() columns for
creation/mod/access file times to the directory listing command.

Actually, a preliminary version of pg_dir_ls did also return some stat
data. I removed this, in favor of functions like pg_file_length.

SELECT fn, pg_file_length(fn)
FROM pg_dir_ls('/etc', true) AS fn
WHERE fn like '/etc/p%'

I certainly could supply a record-returning pg_dir_ls
(fn text, fullfn text, len int8, ctime timestamp, atime timestamp, mtime
timestamp)

Regards,
Andreas

Andreas Pflug wrote:

Apparently it's best to invent a log_file_prefix = 'pgsql_' guc
variable.

In any case, the prefix "postgresql-" is more in line with current
practice.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:

Andreas Pflug wrote:

Apparently it's best to invent a log_file_prefix = 'pgsql_' guc
variable.

In any case, the prefix "postgresql-" is more in line with current
practice.

For logs I think pgsql_ is best because that filename is already going
to be long, and I don't usually like dashes in file names. They look
too much like arguments, but tarballs use them and it looks OK there, I
guess.

-- 
  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, Pennsylvania 19073

Bruce Momjian wrote:

For logs I think pgsql_ is best because that filename is already
going to be long, and I don't usually like dashes in file names.
They look too much like arguments, but tarballs use them and it looks
OK there, I guess.

I wasn't talking about what looks best, I was talking about current
practice for log files. From that you might be able to extrapolate
what other people have previously found to look best.

In any case, we're not using DOS and 12 inch monitors any more. File
names can be as long as we want.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:

Bruce Momjian wrote:

For logs I think pgsql_ is best because that filename is already
going to be long, and I don't usually like dashes in file names.
They look too much like arguments, but tarballs use them and it looks
OK there, I guess.

I wasn't talking about what looks best, I was talking about current
practice for log files. From that you might be able to extrapolate
what other people have previously found to look best.

In any case, we're not using DOS and 12 inch monitors any more. File
names can be as long as we want.

Before the thread concentrates too much on a decent default value, I'm
posting a fresh version of the patch, for some more discussion. Current
default for pg_logfile_prefix is 'postgresql-', may the committers
decide which name is The Perfect One.

All previous suggestions have been included, (nb: abstimein is not
usable, because it ereports(ERROR) on failure; we want to skip wrong
files gracefully, so I'm using ParseDateTime and DecodeDateTime instead).

I'd still need feedback on pg_dir_ls: should it merely return a setof
text, or should I enrich it to a record returning all stat data? After
spending another thought on it, I believe the more sql-like approach is
to deliver a full-featured record which is selected for the desired
data, not adding columns with functions.

Regards,
Andreas

Andreas Pflug wrote:

I wasn't talking about what looks best, I was talking about current
practice for log files. From that you might be able to extrapolate
what other people have previously found to look best.

In any case, we're not using DOS and 12 inch monitors any more. File
names can be as long as we want.

Before the thread concentrates too much on a decent default value, I'm
posting a fresh version of the patch, for some more discussion. Current
default for pg_logfile_prefix is 'postgresql-', may the committers
decide which name is The Perfect One.

All previous suggestions have been included, (nb: abstimein is not
usable, because it ereports(ERROR) on failure; we want to skip wrong
files gracefully, so I'm using ParseDateTime and DecodeDateTime instead).

I'd still need feedback on pg_dir_ls: should it merely return a setof
text, or should I enrich it to a record returning all stat data? After
spending another thought on it, I believe the more sql-like approach is
to deliver a full-featured record which is selected for the desired
data, not adding columns with functions.

This patch looks good to me.  As far as your question about pg_dir_ls
--- you already return multiple columns from pg_logdir_ls, so it seems
you would do the same for returning stat() information from pg_dir_ls,
right?

-- 
  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, Pennsylvania 19073

Andreas Pflug wrote:

Peter Eisentraut wrote:

Bruce Momjian wrote:

For logs I think pgsql_ is best because that filename is already
going to be long, and I don't usually like dashes in file names.
They look too much like arguments, but tarballs use them and it looks
OK there, I guess.

I wasn't talking about what looks best, I was talking about current
practice for log files. From that you might be able to extrapolate
what other people have previously found to look best.

In any case, we're not using DOS and 12 inch monitors any more. File
names can be as long as we want.

Before the thread concentrates too much on a decent default value, I'm
posting a fresh version of the patch, for some more discussion. Current
default for pg_logfile_prefix is 'postgresql-', may the committers
decide which name is The Perfect One.

All previous suggestions have been included, (nb: abstimein is not
usable, because it ereports(ERROR) on failure; we want to skip wrong
files gracefully, so I'm using ParseDateTime and DecodeDateTime instead).

I'd still need feedback on pg_dir_ls: should it merely return a setof
text, or should I enrich it to a record returning all stat data? After
spending another thought on it, I believe the more sql-like approach is
to deliver a full-featured record which is selected for the desired
data, not adding columns with functions.

Now that I look at it, you could remove pg_file_length() and allow
pg_dir_ls to show you the file sizes. The only problem is that the
length is needed for the read API so you would need to use a WHERE
clause to pick the file where you want the size, rather than just pass
the file name.

-- 
  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, Pennsylvania 19073

Andreas Pflug wrote:

Before the thread concentrates too much on a decent default value, I'm
posting a fresh version of the patch, for some more discussion. Current
default for pg_logfile_prefix is 'postgresql-', may the committers
decide which name is The Perfect One.

All previous suggestions have been included, (nb: abstimein is not
usable, because it ereports(ERROR) on failure; we want to skip wrong
files gracefully, so I'm using ParseDateTime and DecodeDateTime instead).

I'd still need feedback on pg_dir_ls: should it merely return a setof
text, or should I enrich it to a record returning all stat data? After
spending another thought on it, I believe the more sql-like approach is
to deliver a full-featured record which is selected for the desired
data, not adding columns with functions.

OK, new idea. Forget about modifying pg_dir_ls(). Instead add
pg_file_stat the returns the file size, times. You can then easily use
that for file size and times. Also, if you want, add an is_dir boolean
so people can write functions that walk the directory tree.

I noticed we had a big logging discussion during 7.4 beta about logging
and log rotation. This patch is clearly superior to the ideas we had at
that time.

-- 
  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, Pennsylvania 19073

Bruce Momjian wrote:

Andreas Pflug wrote:

OK, new idea. Forget about modifying pg_dir_ls(). Instead add
pg_file_stat the returns the file size, times. You can then easily use
that for file size and times. Also, if you want, add an is_dir boolean
so people can write functions that walk the directory tree.

I now replaced pg_logfile_length, instead pg_logfile_stat(text) will
return a record (len int8, ctime timestamp, atime timestamp, mtime
timestamp, isdir bool).

For convenience, I'd like to have the function

CREATE FUNCTION pg_file_length(text) RETURNS int8
AS
$BODY$
SELECT len
FROM pg_file_stat($1) AS stat
(len int8, ctime timestamp,
atime timestamp, mtime timestamp, isdir bool)
$BODY$ LANGUAGE SQL STRICT;

Where is the right place to put it?

Also, I wonder how to join pg_file_stat and pg_dir_ls to get a ls -l
like listing. Apparently I can't do that, unless I don't code pg_dir_ls
as returning records too, right?

I noticed we had a big logging discussion during 7.4 beta about logging
and log rotation. This patch is clearly superior to the ideas we had at
that time.

Currently, the discussion circles around file functions, not logging. If
you think that part is clean, how about committing it separately so it
can be tested/used (no problem if pg_logfile_rotate() isn't available
right from the start). I'll supply docs RSN.

Regards,
Andreas

Andreas Pflug wrote:

Bruce Momjian wrote:

Andreas Pflug wrote:

OK, new idea. Forget about modifying pg_dir_ls(). Instead add
pg_file_stat the returns the file size, times. You can then easily use
that for file size and times. Also, if you want, add an is_dir boolean
so people can write functions that walk the directory tree.

I now replaced pg_logfile_length, instead pg_logfile_stat(text) will
return a record (len int8, ctime timestamp, atime timestamp, mtime
timestamp, isdir bool).

You mean pg_file_stat(text), right? That's what I see in your code.

For convenience, I'd like to have the function

CREATE FUNCTION pg_file_length(text) RETURNS int8
AS
$BODY$
SELECT len
FROM pg_file_stat($1) AS stat
(len int8, ctime timestamp,
atime timestamp, mtime timestamp, isdir bool)
$BODY$ LANGUAGE SQL STRICT;

Where is the right place to put it?

Take a look at obj_description in include/catalog/pg_proc.h. That
should be a good example.

Also, I wonder how to join pg_file_stat and pg_dir_ls to get a ls -l
like listing. Apparently I can't do that, unless I don't code pg_dir_ls
as returning records too, right?

Ideally you want:

select filename, pg_file_stat(filename)
from pg_dir_ls()

or something like that. However, I don't think you can have a function
call returning multiple values in the target list, and I can't figure
out how to pass an argument to the function if it is in the target list.
Ideas?

I noticed we had a big logging discussion during 7.4 beta about logging
and log rotation. This patch is clearly superior to the ideas we had at
that time.

Currently, the discussion circles around file functions, not logging. If
you think that part is clean, how about committing it separately so it
can be tested/used (no problem if pg_logfile_rotate() isn't available
right from the start). I'll supply docs RSN.

Is pg_logfile_rotate() not working? You mean pg_file_length().

Seems we should get this stat() idea working first. Adjusting catalog
entries once they are in CVS means a catalog bump for every catalog
change.

-- 
  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, Pennsylvania 19073