Re: Patch to implement pg_current_logfile() function

From: Christoph Berg <myon(at)debian(dot)org>
To: Gilles Darold <gilles(dot)darold(at)dalibo(dot)com>
Cc: "Karl O(dot) Pinc" <kop(at)meme(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Shulgin, Oleksandr" <oleksandr(dot)shulgin(at)zalando(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Patch to implement pg_current_logfile() function
Date: 2016-10-14 12:54:06
Message-ID: 20161014125406.albrfj3qldiwjnrr@msg.df7cb.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Gilles,

Re: Gilles Darold 2016-10-07 <0731a353-8d2f-0f2f-fcd5-fde77114cb7e(at)dalibo(dot)com>
> > What bugs me is the new file "pg_log_file" in PGDATA. It clutters the
> > directory listing. I wouldn't know where else to put it, but you might
> > want to cross-check that with the thread that is trying to reshuffle
> > the directory layout to make it easier to exclude files from backups.
> > (Should this file be part of backups?)
> >
> > It's probably correct to leave the file around on shutdown (given it's
> > still a correct pointer). But there might be a case for removing it on
> > startup if logging_collector isn't active anymore.
>
> The file has been renamed into current_logfile and is now removed at
> startup if logging_collector is not active. The file can be excluded
> from a backup but otherwise if it is restored it will be removed or
> overridden at startup. Perhaps the file can give a useful information in
> a backup to know the last log file active at backup time, but not sure
> it has any interest.
>
> I'm not sure which thread is talking about reshuffling the directory
> layout, please give me a pointer if this is not the thread talking about
> renaming of pg_xlog and pg_clog. In the future if we have a directory to
> store files that must be excluded from backup or status files, it will
> be easy to move this file here. I will follow such a change.

I meant "exclude from backup", but let's not worry here because it
will be very easy to move the file to this location once it exists.

Thanks for renaming the file, the new name makes the purpose of the
file very clear.

I'm still not very happy about the file being in PGDATA (modulo the
move mentioned above) - how about the idea of putting it into global/?
That location isn't ideal either, but maybe a better place for now?
(pg_control is a similar write-by-rename file in there as well, so
we'd only be constantly rewriting one subdirectory instead of two.)

> > # select pg_read_file(pg_current_logfile());
> > pg_read_file
> > ───────────────────────────────────────────────────────────────
> > LOG: ending log output to stderr ↵
> > HINT: Future log output will go to log destination "csvlog".↵
> >
> > -rw------- 1 cbe staff 1011 Okt 3 15:06 postgresql-2016-10-03_150602.csv
> > -rw------- 1 cbe staff 96 Okt 3 15:06 postgresql-2016-10-03_150602.log
> >
> > ... though it's unclear what to do if both stderr and csvlog are
> > selected.
> >
> > Possibly NULL should be returned if only "syslog" is selected.
> > (Maybe remove pg_log_file once 'HINT: Future log output will go to
> > log destination "syslog".' is logged?)
>
> I've totally missed that we can have log_destination set to stderr and
> csvlog at the same time, so pg_current_logfile() might return two
> filenames in this case. I've changed the function to return a setof
> record to report the last stderr or csv log file or both. One another
> major change is that the current log filename list is also updated after
> a configuration reload and not just after a startup or a log rotation.
> So in the case of you are switching from stderr to csvlog or both you
> will see immediately the change in current_logfile instead of waiting
> for the next log rotation.

I'm unsure if having two lines of output there is helpful. In the case
where both log destinations are active, pg_read_file(pg_current_logfile())
prints the log contents twice. If I want a specific format, I'll have
to use limit and/or offset which seems fragile.

A better design might be to return two columns instead:

postgres=# select * from pg_current_logfile();
stderr | csvlog
---------------------------------------+---------------------------------------
pg_log/postgresql-2016-10-07_1646.log | pg_log/postgresql-2016-10-07_1646.csv

("stderr" is not a nice column name, but it would at least match the
log_destination setting.)

The on-disk format in current_logfile could then be to always write
two lines, and leave the first or second line empty for the disabled
format.

The downside of that idea is that "pg_read_file(pg_current_logfile())"
won't work, so I'm not sure this change is an improvement in
usability.

(The alternative could be to return an extra column:

postgres=# select * from pg_current_logfile();
type | logfile
---------------------------------------
stderr | pg_log/postgresql-2016-10-07_1646.log
csvlog | pg_log/postgresql-2016-10-07_1646.csv

... but this makes (interactive) querying even harder, though it
would scale better to any future added log formats/channels.)

> I can change the return type to a single text[] if that's looks better.

Arrays would be worse, I think.

There's a bug in the SRF code, on rm'ing current_logfile, invoking
pg_current_logfile() crashes the server.

Christoph

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christoph Berg 2016-10-14 12:58:33 Re: Renaming of pg_xlog and pg_clog
Previous Message Craig Ringer 2016-10-14 12:53:34 Re: PATCH: Batch/pipelining support for libpq