Обсуждение: pgsql 8.1 instrumentation status

I've been testing the current status of instrumentation function
integration in pgsql8.1 and found the result quite frustrating.

Only pg_reload_conf and pg_postmaster_start_time (formerly
pg_postmaster_starttime) are implemented in a way comparable to the
admin package (pg_logfile_rotate does work now, when calling
pg_rotate_logfile; this needed backend support that wasn't present in 8.0).

All other functions are either omitted completely, or stripped down to
near or complete unusability:
The file functions, if present at all, don't accept absolute paths, even
if the log directory is addressed. pg_read_file isn't a replacement for

pg_file_read for this reason.

pg_ls_dir lost its second bool parameter.

pg_stat_file isn't implemented, although announced by Bruce. This makes
pg_file_length impossible.

Naming:
While only an annoying source of unnecessary work, the example of
pg_stat_file (if it would exist) shows how arbitrary the change of names
is: it follows the pattern of pg_stat_% statistics functions.

Consequently, only a small fraction of the 8.0 admin package will be
obsoleted for 8.1.

sigh...

Regards,
Andreas


PS AFAICS we now could add pg_terminate_backend safely too, IIRC the
on-exit lock tables cleanup is corrected now.

> -----Original Message-----
> From: pgadmin-hackers-owner@postgresql.org
> [mailto:pgadmin-hackers-owner@postgresql.org] On Behalf Of
> Andreas Pflug
> Sent: 28 August 2005 21:48
> To: pgadmin-hackers
> Subject: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
> I've been testing the current status of instrumentation function
> integration in pgsql8.1 and found the result quite frustrating.

We should talk more - I've been looking at that! (thought you were on
Slony).

> Only pg_reload_conf and pg_postmaster_start_time (formerly
> pg_postmaster_starttime) are implemented in a way comparable to the
> admin package (pg_logfile_rotate does work now, when calling
> pg_rotate_logfile; this needed backend support that wasn't
> present in 8.0).

Yup.

> All other functions are either omitted completely, or
> stripped down to
> near or complete unusability:
> The file functions, if present at all, don't accept absolute
> paths, even
> if the log directory is addressed. pg_read_file isn't a
> replacement for
>
> pg_file_read for this reason.

No, just hit that. We knew absolute paths outside the data dir were
going to be rejected, but they seem to have gone further. They should be
allow if:

1) The path starts with $PGDATA
2) The path exactly matches one of the config files.

I would call this a bug. If you've got time to produce a simple patch,
I'll try to argue it in.

> pg_ls_dir lost its second bool parameter.
>
> pg_stat_file isn't implemented, although announced by Bruce.
> This makes
> pg_file_length impossible.

Yes it is. I've just been using it. Don't forget, Tom made it into a
procedure to allow things like SELECT
(pg_file_stat('pg_hba.conf')).size; to work

> Naming:
> While only an annoying source of unnecessary work, the example of
> pg_stat_file (if it would exist) shows how arbitrary the
> change of names
> is: it follows the pattern of pg_stat_% statistics functions.
>
> Consequently, only a small fraction of the 8.0 admin package will be
> obsoleted for 8.1.
>

No, only write, unllink and rename should now be needed, with a suitable
patch to fix the path issue.

>
> PS AFAICS we now could add pg_terminate_backend safely too, IIRC the
> on-exit lock tables cleanup is corrected now.

I'm pretty sure we won't argue that one in now. I think Tom will demand
more proof of proper testing.

Regards, Dave.

Dave Page wrote:
>
>
>
>>-----Original Message-----
>>From: pgadmin-hackers-owner@postgresql.org
>>[mailto:pgadmin-hackers-owner@postgresql.org] On Behalf Of
>>Andreas Pflug
>>Sent: 28 August 2005 21:48
>>To: pgadmin-hackers
>>Subject: [pgadmin-hackers] pgsql 8.1 instrumentation status
>>
>>I've been testing the current status of instrumentation function
>>integration in pgsql8.1 and found the result quite frustrating.
>
>
> We should talk more - I've been looking at that! (thought you were on
> Slony).

Yes, but need those functions to work with logfiles etc.


>
>>All other functions are either omitted completely, or
>>stripped down to
>>near or complete unusability:
>>The file functions, if present at all, don't accept absolute
>>paths, even
>>if the log directory is addressed. pg_read_file isn't a
>>replacement for
>>
>>pg_file_read for this reason.
>
>
> No, just hit that. We knew absolute paths outside the data dir were
> going to be rejected, but they seem to have gone further. They should be
> allow if:
>
> 1) The path starts with $PGDATA
> 2) The path exactly matches one of the config files.

logfiles may go somewhere else too.
>
> I would call this a bug. If you've got time to produce a simple patch,
> I'll try to argue it in.

I won't. A perfectly working and tightly restricted version is available
for more than a year now.

>
>>pg_ls_dir lost its second bool parameter.
>>
>>pg_stat_file isn't implemented, although announced by Bruce.
>>This makes
>>pg_file_length impossible.
>
>
> Yes it is. I've just been using it. Don't forget, Tom made it into a
> procedure to allow things like SELECT
> (pg_file_stat('pg_hba.conf')).size; to work

'k. pg_file_length may use it.


> No, only write, unllink and rename should now be needed, with a suitable
> patch to fix the path issue.

pg_logdir_ls...
I won't duplicate pgsql's formatting/interpreting code in pgadmin.

>>PS AFAICS we now could add pg_terminate_backend safely too, IIRC the
>>on-exit lock tables cleanup is corrected now.
>
>
> I'm pretty sure we won't argue that one in now. I think Tom will demand
> more proof of proper testing.

I'm certainly not talking about arguing to commit this to core, but to
support this in our package, which will be needed anyway, e.g. for
pg_logdir_ls.

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 28 August 2005 23:02
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
> >
> > No, just hit that. We knew absolute paths outside the data dir were
> > going to be rejected, but they seem to have gone further.
> They should be
> > allow if:
> >
> > 1) The path starts with $PGDATA
> > 2) The path exactly matches one of the config files.
>
> logfiles may go somewhere else too.

Yup.

> > I would call this a bug. If you've got time to produce a
> simple patch,
> > I'll try to argue it in.
>
> I won't. A perfectly working and tightly restricted version
> is available
> for more than a year now.

Yes, but that wasn't accepted into core exactly as written, and seems to
have got modified incorrectly at some point (there was no discussion of
removing the functionality we're missing - I think it's an oversight),
and will not be going into pgInstaller unless there is absolutely no
usable alternative. The attached patch should give us the same
capabilities as we had before. Does it look OK to you before I post it?
(note that it doesn't try to match the config file names, just dup the
code we had before).

> pg_logdir_ls...
> I won't duplicate pgsql's formatting/interpreting code in pgadmin.

Eh? I still think what you're trying to there is overkill, and limits
flexibility by forcing the user to use a specific filename format. I
think we should just look at the log_filename, and use all characters up
until the first % and from the last % (+1 char) as a pattern match in
the log directory. Eg,

postgresql-%Y-%m-%d_%H%M%S.log gives us a search pattern of
postgresql-*.log

If anyone is a) mixing logs in one directory and b) using ambiguous
names for different app logs in that directory, then they deserve any
bizarre effects they might see *if* they try to view the wrong log. For
everyone else, name format changes are now possible to suit their needs.

Regards, Dave.

Dave Page wrote:

>
>
>
>
>>-----Original Message-----
>>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
>>Sent: 28 August 2005 23:02
>>To: Dave Page
>>Cc: pgadmin-hackers
>>Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>>
>>
>>
>>>No, just hit that. We knew absolute paths outside the data dir were
>>>going to be rejected, but they seem to have gone further.
>>>
>>>
>>They should be
>>
>>
>>>allow if:
>>>
>>>1) The path starts with $PGDATA
>>>2) The path exactly matches one of the config files.
>>>
>>>
>>logfiles may go somewhere else too.
>>
>>
>
>Yup.
>
>
>
>>>I would call this a bug. If you've got time to produce a
>>>
>>>
>>simple patch,
>>
>>
>>>I'll try to argue it in.
>>>
>>>
>>I won't. A perfectly working and tightly restricted version
>>is available
>>for more than a year now.
>>
>>
>
>Yes, but that wasn't accepted into core exactly as written, and seems to
>have got modified incorrectly at some point (there was no discussion of
>removing the functionality we're missing - I think it's an oversight),
>and will not be going into pgInstaller unless there is absolutely no
>usable alternative. The attached patch should give us the same
>capabilities as we had before. Does it look OK to you before I post it?
>(note that it doesn't try to match the config file names, just dup the
>code we had before).
>
>
>
>>pg_logdir_ls...
>>I won't duplicate pgsql's formatting/interpreting code in pgadmin.
>>
>>
>
>Eh? I still think what you're trying to there is overkill, and limits
>flexibility by forcing the user to use a specific filename format. I
>think we should just look at the log_filename, and use all characters up
>until the first % and from the last % (+1 char) as a pattern match in
>the log directory. Eg,
>
>

I won't implement date interpretation in the client, full stop.
You, as well as all others complaining about pg_logdir_ls, fail to
explain how a query like
select pg_file_unlink(filename) FROM pg_logdir_ls
where filetime < now() - '10 days'::interval
should work reliably. Try it with an alternate implementation, in the
presence of "bizarre filenames"!




>postgresql-%Y-%m-%d_%H%M%S.log gives us a search pattern of
>postgresql-*.log
>
>If anyone is a) mixing logs in one directory and b) using ambiguous
>names for different app logs in that directory, then they deserve any
>bizarre effects they might see *if* they try to view the wrong log. For
>everyone else, name format changes are now possible to suit their needs.
>
>
>
This logfile configuration stuff is a bummer. Anybody who changes it
will probably use his own logfile reader.

I'll be committing a 8.1 version of the admin package soon, which may be
switched over step by step to use core functions if applicable. For
now,  most of them do not deliver the same functionality as present for 8.0.

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 29 August 2005 10:21
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
>
> I won't implement date interpretation in the client, full stop.

Seriously Andreas, you need to get over this annoyance about these
functions not getting in as is. It's not healthy, and is only hurting
you and the end users who might miss out on features. I realise the end
result wasn't exactly what you wanted, but sometimes we just have to
compromise, otherwise we won't get anywhere at all.

Please, let's move on and make things work with what we have.

> You, as well as all others complaining about pg_logdir_ls, fail to
> explain how a query like
> select pg_file_unlink(filename) FROM pg_logdir_ls
> where filetime < now() - '10 days'::interval
> should work reliably. Try it with an alternate implementation, in the
> presence of "bizarre filenames"!
>

There are various reasons for this, including:

- None of our code does that, so it is a non-issue at the moment.
- There is no pg_file_unlink in the server, so why should the
pgsql-hackers care about making it easy to use?
- pg_logdir_ls only worked with log_filename set to a specific value.
You know as well as anyone that this is not good software design.
- Virtually identical results can be achieved using ctime or mtime from
pg_stat_file with pg_dir_ls (as I think Bruce suggested).

If you are not happy with this, please let me know and I will fix the
logfile viewer for 8.1.

> This logfile configuration stuff is a bummer. Anybody who changes it
> will probably use his own logfile reader.

Possibly, but not necessarily. What about people who rotate their logs
daily for example? They might simply want to lose the unnecessary
characters from the filenames.

> I'll be committing a 8.1 version of the admin package soon,
> which may be
> switched over step by step to use core functions if applicable. For
> now,  most of them do not deliver the same functionality as
> present for 8.0.

I already created an 8.1 admin pack and committed it to SVN. Currently
it contains rename, write and unlink. If we need to add more, I'd like
it to be because we really have no choice to get sane functionality,
rather than just because we would prefer something work slightly
differently.

Regards, Dave.

Dave Page wrote:
>
>
>
>>-----Original Message-----
>>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
>>Sent: 29 August 2005 10:21
>>To: Dave Page
>>Cc: pgadmin-hackers
>>Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>>
>>
>>I won't implement date interpretation in the client, full stop.
>
>
> Seriously Andreas, you need to get over this annoyance about these
> functions not getting in as is. It's not healthy, and is only hurting
> you and the end users who might miss out on features. I realise the end
> result wasn't exactly what you wanted, but sometimes we just have to
> compromise, otherwise we won't get anywhere at all.
>
> Please, let's move on and make things work with what we have.
>
>
>>You, as well as all others complaining about pg_logdir_ls, fail to
>>explain how a query like
>>select pg_file_unlink(filename) FROM pg_logdir_ls
>>where filetime < now() - '10 days'::interval
>>should work reliably. Try it with an alternate implementation, in the
>>presence of "bizarre filenames"!
>>
>
>
> There are various reasons for this, including:
>
> - None of our code does that, so it is a non-issue at the moment.
> - There is no pg_file_unlink in the server, so why should the
> pgsql-hackers care about making it easy to use?

This condenses the problem. They don't care.

> - pg_logdir_ls only worked with log_filename set to a specific value.
> You know as well as anyone that this is not good software design.

I never intended to open this to configuration. MSSQL doesn't offer this
too.

> - Virtually identical results can be achieved using ctime or mtime from
> pg_stat_file with pg_dir_ls (as I think Bruce suggested).

No, not reliably. See the archives.

> If you are not happy with this, please let me know and I will fix the
> logfile viewer for 8.1.

This is not fixing, this is working around.
pg_logdir_ls works reliably or not at all, if you take the break option
(editing log_filename). All other proposals can be damaged in less
obvious ways.

>>This logfile configuration stuff is a bummer. Anybody who changes it
>>will probably use his own logfile reader.
>
>
> Possibly, but not necessarily. What about people who rotate their logs
> daily for example? They might simply want to lose the unnecessary
> characters from the filenames.

Which would be really short sighted, and is error prone. When max
logfile size is reached, those people will be bitten.

>>I'll be committing a 8.1 version of the admin package soon,
>>which may be
>>switched over step by step to use core functions if applicable. For
>>now,  most of them do not deliver the same functionality as
>>present for 8.0.
>
>
> I already created an 8.1 admin pack and committed it to SVN. Currently
> it contains rename, write and unlink. If we need to add more, I'd like
> it to be because we really have no choice to get sane functionality,
> rather than just because we would prefer something work slightly
> differently.

Adapting the 8.0 admin package to 8.1 is much less effort than adapting
to the pgsql functions.

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 29 August 2005 22:40
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
> > - There is no pg_file_unlink in the server, so why should the
> > pgsql-hackers care about making it easy to use?
>
> This condenses the problem. They don't care.

Exactly.

> > - pg_logdir_ls only worked with log_filename set to a
> specific value.
> > You know as well as anyone that this is not good software design.
>
> I never intended to open this to configuration. MSSQL doesn't
> offer this
> too.

No, you didn't, but the overall opinion on on -hackers was different,
and it went in. Like it or not, we have to work with it. We cannot
afford to start forcing people to use configurations that they might not
want because they'll simply say 'sod this' and go an try another admin
package.

> > - Virtually identical results can be achieved using ctime
> or mtime from
> > pg_stat_file with pg_dir_ls (as I think Bruce suggested).
>
> No, not reliably. See the archives.

I have. The only argument you gave was 'what if someone copies the
cluster'. Well, in my experience it's far more likely that someone will
change the logfile name format.

Anyway, I'll point out again - this is irrelevant as we do not have any
functionality that this will break at present.

> > If you are not happy with this, please let me know and I
> will fix the
> > logfile viewer for 8.1.
>
> This is not fixing, this is working around.
> pg_logdir_ls works reliably or not at all, if you take the
> break option
> (editing log_filename). All other proposals can be damaged in less
> obvious ways.

The worst that will happen with any sane feature that might be added, is
that an old logfile might not be deleted because it's mtime or ctime is
too recent. That's merely an inconvenience - no data will be lost.

Am I missing some other, more disasterous potential problem?

> >>This logfile configuration stuff is a bummer. Anybody who
> changes it
> >>will probably use his own logfile reader.
> >
> >
> > Possibly, but not necessarily. What about people who rotate
> their logs
> > daily for example? They might simply want to lose the unnecessary
> > characters from the filenames.
>
> Which would be really short sighted, and is error prone. When max
> logfile size is reached, those people will be bitten.

That was only an example. What if people change the prefix to match some
site specific detail, say 'devpostmaster-[whatever]'. That may well be
perfectly sensible in some situations. The bottom line is, it's a
documented feature of the server, so we must assume that some people
will have a legitimate use for it.

> >>I'll be committing a 8.1 version of the admin package soon,
> >>which may be
> >>switched over step by step to use core functions if applicable. For
> >>now,  most of them do not deliver the same functionality as
> >>present for 8.0.
> >
> >
> > I already created an 8.1 admin pack and committed it to
> SVN. Currently
> > it contains rename, write and unlink. If we need to add
> more, I'd like
> > it to be because we really have no choice to get sane functionality,
> > rather than just because we would prefer something work slightly
> > differently.
>
> Adapting the 8.0 admin package to 8.1 is much less effort
> than adapting
> to the pgsql functions.

Maybe, but given the effort I put in, and Magnus put in to get the code
included in the server in as much of it's original form as possible, I'm
damned if I'm just going to take that route now 'because it's less
effort'.

BTW, dunno if you noticed, but the absolute path thing should be fixed
in CVS HEAD now, giving access to Logdir and Datadir as per the original
admin functions.

Regards, Dave.

Dave Page wrote:

>>Adapting the 8.0 admin package to 8.1 is much less effort
>>than adapting
>>to the pgsql functions.
>
>
> Maybe, but given the effort I put in, and Magnus put in to get the code
> included in the server in as much of it's original form as possible, I'm
> damned if I'm just going to take that route now 'because it's less
> effort'.

I can post my code that restores full functionality now, with the option
to switch them to LANGUAGE INTERNAL for backend functions that work, or
do nothing. I won't implement code that belongs on the server in pgadmin.

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 30 August 2005 09:08
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
> Dave Page wrote:
>
> >>Adapting the 8.0 admin package to 8.1 is much less effort
> >>than adapting
> >>to the pgsql functions.
> >
> >
> > Maybe, but given the effort I put in, and Magnus put in to
> get the code
> > included in the server in as much of it's original form as
> possible, I'm
> > damned if I'm just going to take that route now 'because it's less
> > effort'.
>
> I can post my code that restores full functionality now, with
> the option
> to switch them to LANGUAGE INTERNAL for backend functions
> that work, or
> do nothing. I won't implement code that belongs on the server
> in pgadmin.

LANGUAGE INTERNAL??

/D

Dave Page wrote:

>>to switch them to LANGUAGE INTERNAL for backend functions
>>that work, or
>>do nothing. I won't implement code that belongs on the server
>>in pgadmin.
>
>
> LANGUAGE INTERNAL??

Yes, effectively replacing the admin code with backend code, reducing
the affected admin functions to stubs into official code where
applicable, remaining backward compatibility to 8.0 (and pgadmin 1.2)

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 30 August 2005 10:14
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] pgsql 8.1 instrumentation status
>
> Dave Page wrote:
>
> >>to switch them to LANGUAGE INTERNAL for backend functions
> >>that work, or
> >>do nothing. I won't implement code that belongs on the server
> >>in pgadmin.
> >
> >
> > LANGUAGE INTERNAL??
>
> Yes, effectively replacing the admin code with backend code, reducing
> the affected admin functions to stubs into official code where
> applicable, remaining backward compatibility to 8.0 (and pgadmin 1.2)

<grumble> Go on then, but I reserve the right to get picky about it
later if there's any gratuitous duplication of core code!

:-)

(and don't forget I already committed a cut down admin pack for 8.1 that
can be updated if required).

/D