Re: I am done

I have finished going through my email box and applying patches from the
patches queue. There is one patch left in the queue related to tcl
notification of connection failure. Tom wants to look at that.

I am running tests now on the code.

Tomorrow, I will run pgindent and create the HISTORY file for 7.3.

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

You can probably nail some TODOs:

* Add OR REPLACE clauses to non-FUNCTION object creation
* Allow autocommit so always in a transaction block
* Cache most recent query plan(s) (Neil) [prepare]

Chris

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org]On Behalf Of Bruce Momjian
> Sent: Monday, 2 September 2002 2:30 PM
> To: PostgreSQL-development
> Subject: [HACKERS] I am done
>
>
> I have finished going through my email box and applying patches from the
> patches queue. There is one patch left in the queue related to tcl
> notification of connection failure. Tom wants to look at that.
>
> I am running tests now on the code.
>
> Tomorrow, I will run pgindent and create the HISTORY file for 7.3.
>
> --
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>

Thanks. Done.

---------------------------------------------------------------------------

Christopher Kings-Lynne wrote:
> You can probably nail some TODOs:
>
> * Add OR REPLACE clauses to non-FUNCTION object creation
> * Allow autocommit so always in a transaction block
> * Cache most recent query plan(s) (Neil) [prepare]
>
> Chris
>
> > -----Original Message-----
> > From: pgsql-hackers-owner(at)postgresql(dot)org
> > [mailto:pgsql-hackers-owner(at)postgresql(dot)org]On Behalf Of Bruce Momjian
> > Sent: Monday, 2 September 2002 2:30 PM
> > To: PostgreSQL-development
> > Subject: [HACKERS] I am done
> >
> >
> > I have finished going through my email box and applying patches from the
> > patches queue. There is one patch left in the queue related to tcl
> > notification of connection failure. Tom wants to look at that.
> >
> > I am running tests now on the code.
> >
> > Tomorrow, I will run pgindent and create the HISTORY file for 7.3.
> >
> > --
> > 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
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 5: Have you checked our extensive FAQ?
> >
> > http://www.postgresql.org/users-lounge/docs/faq.html
> >
>
>

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

"Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> You can probably nail some TODOs:

> * Allow autocommit so always in a transaction block

This isn't really done; the backend side is probably okay, but we have
a ton of client-side code that will malfunction if you try to run it in
autocommit-off state. I'm willing to ship it that way for 7.3, but we
should certainly have a TODO item indicating that client libraries,
psql, etc need work.

Other TODO items that are done, or at least better than 7.2:

* Show location of syntax error in query [yacc]

The character-position hack addresses this, though surely it's not complete.

* Allow logging of query durations

Didn't Bruce just commit that?

* Make single-user local access permissions the default by limiting
permissions on the socket file (Peter E)

I believe we have decided *not* to do this.

* Reserve last few process slots for super-user if max_connections reached
* Add GUC parameter to print queries that generate errors

Both done, no?

* Declare typein/out functions in pg_proc with a special "C string" data type
* Functions returning sets do not totally work

Both done (the remaining work on sets is covered by another item)

* Allow bytea to handle LIKE with non-TEXT patterns

I didn't want to apply Joe's patch at this late hour, but I think Bruce
did it anyway.

o Store binary-compatible type information in the system

Done, see pg_cast.

o -SELECT col FROM tab WHERE numeric_col = 10.1 fails, requires quotes

This should not be marked done; the problem is still there, just this
particular symptom went away.

o Ensure we have array-eq operators for every built-in array type

Did that; there's even a regression test to catch the omission in
future.

* Allow setting database character set without multibyte enabled

This is probably irrelevant now that multibyte can't be disabled.

* Have UPDATE/DELETE clean out indexes

This entry makes no sense to me; unless we abandon the entire concept of
MVCC, this is not gonna happen.

o ALTER TABLE ADD COLUMN to inherited table put column in wrong place
[inheritance]

While this isn't done, its urgency has dropped an awful lot now that
pg_dump knows to use COPY column lists; you don't have to worry about
dump/restore breakage. Accordingly, I doubt we're ever gonna try to
change it.

o Add ALTER FUNCTION

What is ALTER FUNCTION? How does it differ from CREATE OR REPLACE
FUNCTION?

o -ALTER TABLE ADD PRIMARY KEY (Tom)
o -ALTER TABLE ADD UNIQUE (Tom)

AFAIR, I didn't do either of those; I think Chris K-L gets the credit.

o ALTER TABLE ADD COLUMN column SERIAL doesn't create sequence

This is not a problem. The actual problem with adding a serial column
is covered by the next entry:
o ALTER TABLE ADD COLUMN column DEFAULT should fill existing
rows with DEFAULT value

o -Cluster all tables at once using pg_index.indisclustered set during
previous CLUSTER

This is not done, unless we are going to accept Alvaro's last-minute
patch for it; which I vote we don't. It's too big a change.

o Prevent DROP of table being referenced by our own open cursor

Huh? There is no such bug that I know of.

o -Disallow missing columns in INSERT ... VALUES, per ANSI

What is this, and why is it marked done?

o -Remove SET KSQO option now that OR processing is improved (Tom)

I don't think I get the credit (blame?) for this one, either.

* Have pg_dump use LEFT OUTER JOIN in multi-table SELECTs
or multiple SELECTS to avoid bad system catalog entries

Isn't this pretty much done?

* Add config file check for $ODBCINI, $HOME/.odbc.ini, installpath/etc/odbc.ini

With ODBC out of the main distro, this isn't our problem anymore.

* Fix foreign key constraints to not error on intermediate db states (Stephan)

Isn't this done?

* Have SERIAL generate non-colliding sequence names when we have
auto-destruction

They should be pretty well non-colliding now. What's the gripe exactly?

* Propagate column or table renaming to foreign key constraints

This is done.

* Remove wal_files postgresql.conf option because WAL files are now recycled

Done, no?

* Improve dynamic memory allocation by introducing tuple-context memory
allocation (Tom)

Uh, wasn't that done long ago?

* Nested FULL OUTER JOINs don't work (Tom)

Fixed.

regards, tom lane

On Mon, 2 Sep 2002, Tom Lane wrote:

> Date: Mon, 02 Sep 2002 10:33:49 -0400
> From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> To: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
> Cc: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>,
> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
> Subject: Re: [HACKERS] I am done
>
> "Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > You can probably nail some TODOs:
>

Maybe add a TODO item:

* fix a possibility of DoS attacks in the backend before the user is
authenticated

? Since I guess my patch for that is too late and I don't even know if
it's any good approach at all.

-s

Tom Lane dijo:

> "Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > You can probably nail some TODOs:
>

> o -Cluster all tables at once using pg_index.indisclustered set during
> previous CLUSTER
>
> This is not done, unless we are going to accept Alvaro's last-minute
> patch for it; which I vote we don't. It's too big a change.

I agree, but there's the (ugly) clusterdb script. Perhaps replace with
another TODO item.

--
Alvaro Herrera (<alvherre[a]atentus.com>)

Tom Lane wrote:
> "Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > You can probably nail some TODOs:
>
> > * Allow autocommit so always in a transaction block
>
> This isn't really done; the backend side is probably okay, but we have
> a ton of client-side code that will malfunction if you try to run it in
> autocommit-off state. I'm willing to ship it that way for 7.3, but we
> should certainly have a TODO item indicating that client libraries,
> psql, etc need work.

My feeling is that we have to fix this during beta.

Added to open items:

Fix client apps for autocommit = off

> Other TODO items that are done, or at least better than 7.2:
>
> * Show location of syntax error in query [yacc]
>
> The character-position hack addresses this, though surely it's not complete.

Yep, still on the TODO.

>
> * Allow logging of query durations
>
> Didn't Bruce just commit that?

Thanks. Marked as done now.

> * Make single-user local access permissions the default by limiting
> permissions on the socket file (Peter E)
>
> I believe we have decided *not* to do this.

Yes, removed.

> * Reserve last few process slots for super-user if max_connections reached
> * Add GUC parameter to print queries that generate errors
>
> Both done, no?

Yep, marked, but right now there is no good way to turn off the printing
of queries on errors. We have to address that before 7.3 final.

> * Declare typein/out functions in pg_proc with a special "C string" data type
> * Functions returning sets do not totally work
>
> Both done (the remaining work on sets is covered by another item)

Marked.

>
> * Allow bytea to handle LIKE with non-TEXT patterns
>
> I didn't want to apply Joe's patch at this late hour, but I think Bruce
> did it anyway.

I think he made the deadline and no one objected.

> o Store binary-compatible type information in the system
>
> Done, see pg_cast.

Marked.

>
> o -SELECT col FROM tab WHERE numeric_col = 10.1 fails, requires quotes
>
> This should not be marked done; the problem is still there, just this
> particular symptom went away.

Someone reported to me it was done. I have removed the item because
though it isn't a bug query anymore, it isn't as fixed as we would like.
We still have:

o Allow better handling of numeric constants, type conversion

Can someone give me another example failing query?

> o Ensure we have array-eq operators for every built-in array type
>
> Did that; there's even a regression test to catch the omission in
> future.

Marked.

>
> * Allow setting database character set without multibyte enabled
>
> This is probably irrelevant now that multibyte can't be disabled.

Item removed.

> * Have UPDATE/DELETE clean out indexes
>
> This entry makes no sense to me; unless we abandon the entire concept of
> MVCC, this is not gonna happen.

Good point. Now that we have light vacuum, we are fine.

> o ALTER TABLE ADD COLUMN to inherited table put column in wrong place
> [inheritance]
>
> While this isn't done, its urgency has dropped an awful lot now that
> pg_dump knows to use COPY column lists; you don't have to worry about
> dump/restore breakage. Accordingly, I doubt we're ever gonna try to
> change it.

OK, removed. Can we dump/reload the regression database now? What
areas still need this fix?

> o Add ALTER FUNCTION
>
> What is ALTER FUNCTION? How does it differ from CREATE OR REPLACE
> FUNCTION?

Removed. I think it was thought of before the CREATE OR REPLACE idea
came around.

> o -ALTER TABLE ADD PRIMARY KEY (Tom)
> o -ALTER TABLE ADD UNIQUE (Tom)
>
> AFAIR, I didn't do either of those; I think Chris K-L gets the credit.

Done.

> o ALTER TABLE ADD COLUMN column SERIAL doesn't create sequence
>
> This is not a problem. The actual problem with adding a serial column
> is covered by the next entry:
> o ALTER TABLE ADD COLUMN column DEFAULT should fill existing
> rows with DEFAULT value

Well, the lack of sequence for a SERIAL is an issue if only related to
the DEFAULT issue so I will keep it.

> o -Cluster all tables at once using pg_index.indisclustered set during
> previous CLUSTER
>
> This is not done, unless we are going to accept Alvaro's last-minute
> patch for it; which I vote we don't. It's too big a change.

Well, we have clusterdb. One thing that bothers me is that we have
clusterdb and /contrib/reindexdb going out new in 7.3, only to be
removed in 7.4 when we get the table scan done in the backend code ---
not a great API solution but we may have to live with it.

>
> o Prevent DROP of table being referenced by our own open cursor
>
> Huh? There is no such bug that I know of.

Well, actually, there is or was. The issue is that if you open a cursor
in a transaction, then drop the table while the cursor is open, all
sorts of weird things happen:

test=> create table test (x int);
CREATE TABLE
test=> insert into test values (1);
INSERT 149484 1
test=> begin;
BEGIN
test=> declare xx cursor for select * from test;
DECLARE CURSOR
test=> fetch xx;
x
---
1
(1 row)

test=> drop table test;
WARNING: FlushRelationBuffers(test, 0): block 0 is referenced (private 2, global 1)
ERROR: heap_drop_with_catalog: FlushRelationBuffers returned -2

> o -Disallow missing columns in INSERT ... VALUES, per ANSI
>
> What is this, and why is it marked done?

We used to allow INSERT INTO tab VALUES (...) to skip the trailing
columns and automatically fill in null's. That is fixed, per ANSI.

> o -Remove SET KSQO option now that OR processing is improved (Tom)
>
> I don't think I get the credit (blame?) for this one, either.

I think your name was on it because you were the contact for it. I
think I did the dirty work. That is how some of these got your name.

> * Have pg_dump use LEFT OUTER JOIN in multi-table SELECTs
> or multiple SELECTS to avoid bad system catalog entries
>
> Isn't this pretty much done?

Yes, I suspected it was but wasn't sure. Marked as done now.

>
> * Add config file check for $ODBCINI, $HOME/.odbc.ini, installpath/etc/odbc.ini
>
> With ODBC out of the main distro, this isn't our problem anymore.

Yep, removed the ODBC section too.

> * Fix foreign key constraints to not error on intermediate db states (Stephan)
>
> Isn't this done?

Yes, I think Stephan took care of it, but wasn't sure. Marked as done.

> * Have SERIAL generate non-colliding sequence names when we have
> auto-destruction
>
> They should be pretty well non-colliding now. What's the gripe exactly?

The issue was that when there were name collisions, we threw an error
instead of trying other sequence names. We had to do that because we
needed the sequence name to be predictable so it could be auto-deleted.
Now with dependency, we don't need to have it be predictable. However,
we still use nextval() on the sequence name, so we can't say it is
arbitrary either. Should we just remove the item?

> * Propagate column or table renaming to foreign key constraints
>
> This is done.

Marked.

>
> * Remove wal_files postgresql.conf option because WAL files are now recycled
>
> Done, no?

Yep.

>
> * Improve dynamic memory allocation by introducing tuple-context memory
> allocation (Tom)
>
> Uh, wasn't that done long ago?

Yep, I think so, but I wasn't sure.

>
> * Nested FULL OUTER JOINs don't work (Tom)
>
> Fixed.

Done.

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

Serguei A. Mokhov wrote:
>
>
> On Mon, 2 Sep 2002, Tom Lane wrote:
>
> > Date: Mon, 02 Sep 2002 10:33:49 -0400
> > From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> > To: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
> > Cc: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>,
> > PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
> > Subject: Re: [HACKERS] I am done
> >
> > "Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > > You can probably nail some TODOs:
> >
>
> Maybe add a TODO item:
>
> * fix a possibility of DoS attacks in the backend before the user is
> authenticated
>
> ? Since I guess my patch for that is too late and I don't even know if
> it's any good approach at all.

It is a security issue and will have to be fixed before 7.3 final, one
way or the other.

--
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:
>> This isn't really done; the backend side is probably okay, but we have
>> a ton of client-side code that will malfunction if you try to run it in
>> autocommit-off state.

> My feeling is that we have to fix this during beta.

No we don't.

I don't think it *can* be fixed in a reasonable fashion until we have
notification to the client side about what the backend's transaction
state is; which is one of the protocol-change items for 7.4.

> Added to open items:
> Fix client apps for autocommit = off

Put it on TODO for 7.4, instead.

BTW, why is there not a TODO item (or maybe a whole section) for all the
protocol changes we want?

> Yep, marked, but right now there is no good way to turn off the printing
> of queries on errors. We have to address that before 7.3 final.

Oh, didn't you put in that patch to provide a GUC level control?

> Can we dump/reload the regression database now?

Yes.

> Well, the lack of sequence for a SERIAL is an issue if only related to
> the DEFAULT issue so I will keep it.

I think you should merge the two items into one: the DEFAULT is the
actual problem, but it'd be okay to note that it blocks adding a serial
column.

>> o Prevent DROP of table being referenced by our own open cursor
>>
>> Huh? There is no such bug that I know of.

> Well, actually, there is or was.

Oh, wait, our *own* open cursor. Okay --- I was testing it with a
different backend trying to drop the table, and of course it blocked
waiting to acquire exclusive lock on the table. But if it's our own
cursor we won't block.

>> o -Disallow missing columns in INSERT ... VALUES, per ANSI
>>
>> What is this, and why is it marked done?

> We used to allow INSERT INTO tab VALUES (...) to skip the trailing
> columns and automatically fill in null's. That is fixed, per ANSI.

Oh, I see: we compromised on being strict if you have an explicit list
of column names. You can still omit trailing columns if you just
say INSERT INTO foo VALUES(...), which is what the TODO item seems to
say you can't do anymore.

>> * Have SERIAL generate non-colliding sequence names when we have
>> auto-destruction
>>
>> They should be pretty well non-colliding now. What's the gripe exactly?

> The issue was that when there were name collisions, we threw an error
> instead of trying other sequence names. We had to do that because we
> needed the sequence name to be predictable so it could be auto-deleted.
> Now with dependency, we don't need to have it be predictable.

The system may not need that, but I think unpredictable sequence names
are a bad idea from the user's point of view anyway. Also, the main
reason why there was a problem before was the failure to auto-drop the
sequence --- so you were certain to get a collision if you dropped and
remade the table. So I think this is a non-problem now, and we should
just remove the item.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> This isn't really done; the backend side is probably okay, but we have
> >> a ton of client-side code that will malfunction if you try to run it in
> >> autocommit-off state.
>
> > My feeling is that we have to fix this during beta.
>
> No we don't.
>
> I don't think it *can* be fixed in a reasonable fashion until we have
> notification to the client side about what the backend's transaction
> state is; which is one of the protocol-change items for 7.4.

Why can't we just turn on auto-commit when we start the session:

SET autocommit = true;

> > Added to open items:
> > Fix client apps for autocommit = off
>
> Put it on TODO for 7.4, instead.
>
> BTW, why is there not a TODO item (or maybe a whole section) for all the
> protocol changes we want?

A few are mentioned in TODO, the others are mentioned in comments in the
code. I can gather them if you it would help.

> > Yep, marked, but right now there is no good way to turn off the printing
> > of queries on errors. We have to address that before 7.3 final.
>
> Oh, didn't you put in that patch to provide a GUC level control?

Yes, but what level do you set it at to turn it off? It goes from
DEBUG5 all the way up to ERROR but all of those print the query on an
error.

>
> > Can we dump/reload the regression database now?
>
> Yes.

Oh, OK, then removed.

> > Well, the lack of sequence for a SERIAL is an issue if only related to
> > the DEFAULT issue so I will keep it.
>
> I think you should merge the two items into one: the DEFAULT is the
> actual problem, but it'd be okay to note that it blocks adding a serial
> column.

OK, new text:

o ALTER TABLE ADD COLUMN column DEFAULT should fill existing
rows with DEFAULT value
o ALTER TABLE ADD COLUMN column SERIAL doesn't create sequence because
of the item above

> >> o Prevent DROP of table being referenced by our own open cursor
> >>
> >> Huh? There is no such bug that I know of.
>
> > Well, actually, there is or was.
>
> Oh, wait, our *own* open cursor. Okay --- I was testing it with a
> different backend trying to drop the table, and of course it blocked
> waiting to acquire exclusive lock on the table. But if it's our own
> cursor we won't block.

Don't know how I can improve that wording. :-)

> >> o -Disallow missing columns in INSERT ... VALUES, per ANSI
> >>
> >> What is this, and why is it marked done?
>
> > We used to allow INSERT INTO tab VALUES (...) to skip the trailing
> > columns and automatically fill in null's. That is fixed, per ANSI.
>
> Oh, I see: we compromised on being strict if you have an explicit list
> of column names. You can still omit trailing columns if you just
> say INSERT INTO foo VALUES(...), which is what the TODO item seems to
> say you can't do anymore.

Yes, that was a point not addressed in the TODO because that distinction
didn't exist at the time it was added. Text updated:

o -Disallow missing columns in INSERT ... (col) VALUES, per ANSI

> >> * Have SERIAL generate non-colliding sequence names when we have
> >> auto-destruction
> >>
> >> They should be pretty well non-colliding now. What's the gripe exactly?
>
> > The issue was that when there were name collisions, we threw an error
> > instead of trying other sequence names. We had to do that because we
> > needed the sequence name to be predictable so it could be auto-deleted.
> > Now with dependency, we don't need to have it be predictable.
>
> The system may not need that, but I think unpredictable sequence names
> are a bad idea from the user's point of view anyway. Also, the main
> reason why there was a problem before was the failure to auto-drop the
> sequence --- so you were certain to get a collision if you dropped and
> remade the table. So I think this is a non-problem now, and we should
> just remove the item.

OK, removed. Again, thank's for the review. I am working on HISTORY today.

--
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 don't think it *can* be fixed in a reasonable fashion until we have
>> notification to the client side about what the backend's transaction
>> state is; which is one of the protocol-change items for 7.4.

> Why can't we just turn on auto-commit when we start the session:
> SET autocommit = true;

How's that help? If the user turns it off again, we still break.

More to the point, we can hardly claim any level of SQL compliance if
either libpq or psql try to prevent you from using the autocommit-off
mode ... but both contain problematic code (mostly in the startup and
large-object-support areas).

The real problem here is that the client code cannot know how to behave
(ie, whether to issue BEGIN and/or COMMIT) unless it knows the current
autocommit setting and current transaction state. And getting that info
in any reliable fashion requires a protocol change, AFAICS.

There are some things we can tweak to make the clients less broken than
they are now --- for instance, all of libpq's startup-time SET commands
could be switched to "BEGIN; SET ...; COMMIT;" which will work the same
with or without autocommit --- but I don't think we can expect to fix
large-object support, for example, without the protocol change.

>> Oh, didn't you put in that patch to provide a GUC level control?

> Yes, but what level do you set it at to turn it off?

FATAL? PANIC?

regards, tom lane

> > * Have SERIAL generate non-colliding sequence names when we have
> > auto-destruction
> >
> > They should be pretty well non-colliding now. What's the gripe exactly?
>
> The issue was that when there were name collisions, we threw an error
> instead of trying other sequence names. We had to do that because we
> needed the sequence name to be predictable so it could be auto-deleted.
> Now with dependency, we don't need to have it be predictable. However,
> we still use nextval() on the sequence name, so we can't say it is
> arbitrary either. Should we just remove the item?

The names are relied on by pg_dump for setting the next value of the
sequence. That is, it relies on the names being generated the exact
same way every time.

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> I don't think it *can* be fixed in a reasonable fashion until we have
> >> notification to the client side about what the backend's transaction
> >> state is; which is one of the protocol-change items for 7.4.
>
> > Why can't we just turn on auto-commit when we start the session:
> > SET autocommit = true;
>
> How's that help? If the user turns it off again, we still break.
>
> More to the point, we can hardly claim any level of SQL compliance if
> either libpq or psql try to prevent you from using the autocommit-off
> mode ... but both contain problematic code (mostly in the startup and
> large-object-support areas).
>
> The real problem here is that the client code cannot know how to behave
> (ie, whether to issue BEGIN and/or COMMIT) unless it knows the current
> autocommit setting and current transaction state. And getting that info
> in any reliable fashion requires a protocol change, AFAICS.
>
> There are some things we can tweak to make the clients less broken than
> they are now --- for instance, all of libpq's startup-time SET commands
> could be switched to "BEGIN; SET ...; COMMIT;" which will work the same
> with or without autocommit --- but I don't think we can expect to fix
> large-object support, for example, without the protocol change.

I was considering the original report that createlang doesn't work.
Surely we can do some things to fix those at least. Yes, we clearly
aren't going to get this working 100% in 7.3. I don't even know what to
put on the TODO list because we don't know what cases have problems.

> >> Oh, didn't you put in that patch to provide a GUC level control?
>
> > Yes, but what level do you set it at to turn it off?
>
> FATAL? PANIC?

He doesn't support those levels:

test=> set log_min_error_statement = fatal;
ERROR: invalid value for option 'log_min_error_statement': 'fatal'
test=> set log_min_error_statement = error;
SET

and in fact, the default is ERROR. I think the default has to be
something higher, but even FATAL seems wrong. We have to be able to
turn it off, and have it off by default, rather than saying it only
happens with fatal errors or something like that.

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

> There are some things we can tweak to make the clients less broken than
> they are now --- for instance, all of libpq's startup-time SET commands
> could be switched to "BEGIN; SET ...; COMMIT;" which will work the same
> with or without autocommit --- but I don't think we can expect to fix
> large-object support, for example, without the protocol change.

Perhaps one should consider removing the autocommit option. It's no use
if it's there but everything breaks when you turn it on.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Perhaps one should consider removing the autocommit option. It's no use
> if it's there but everything breaks when you turn it on.

As far as I'm concerned, it's in there for one reason only (as far as
7.3 goes): so that we can run the NIST SQL compliance tests. Anyone who
wants to use it in production at this point is doing so at their own
risk.

In practice, as long as we fix libpq's startup SET commands, the major
problems will just be with large object support, which is not mainstream
usage either; so I'm prepared to live with it for a release or so. It's
not like there aren't any other combinations of PG features that don't
play nice together.

regards, tom lane

> o -ALTER TABLE ADD PRIMARY KEY (Tom)
> o -ALTER TABLE ADD UNIQUE (Tom)
>
> AFAIR, I didn't do either of those; I think Chris K-L gets the credit.

Actually, I did ADD UNIQUE originally after lots of coding and then you went
and made it work by changing a couple of lines in the grammar. You then got
ADD PRIMARY KEY working as well, so I had Bruce change it to you. In fact,
you even removed all the code I had in there that was no longer reachable :)

Chris

Christopher Kings-Lynne wrote:
> > o -ALTER TABLE ADD PRIMARY KEY (Tom)
> > o -ALTER TABLE ADD UNIQUE (Tom)
> >
> > AFAIR, I didn't do either of those; I think Chris K-L gets the credit.
>
> Actually, I did ADD UNIQUE originally after lots of coding and then you went
> and made it work by changing a couple of lines in the grammar. You then got
> ADD PRIMARY KEY working as well, so I had Bruce change it to you. In fact,
> you even removed all the code I had in there that was no longer reachable :)

Are you guys competing for the modesty award? ;-)

I heard Stallman is trying to win it this year. :-)

--
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 Mon, 2 Sep 2002, Bruce Momjian wrote:

> > >> Oh, didn't you put in that patch to provide a GUC level control?
> >
> > > Yes, but what level do you set it at to turn it off?
> >
> > FATAL? PANIC?
>
> He doesn't support those levels:
>
> test=> set log_min_error_statement = fatal;
> ERROR: invalid value for option 'log_min_error_statement': 'fatal'
> test=> set log_min_error_statement = error;
> SET
>
> and in fact, the default is ERROR. I think the default has to be
> something higher, but even FATAL seems wrong. We have to be able to
> turn it off, and have it off by default, rather than saying it only
> happens with fatal errors or something like that.

Okay, my bad. From my reading of the email exchange, I thought people
wanted this on -- always. The best solution for this, in my opinion, is to
have a magic value "off" which the error code lookup translates to some
number > PANIC.

Secondly, there is a flaw in the patch. I merged all the
assign_server_min_messages() and assign_client_min_messages() code to make
things pretty. Perhaps I shouldn't have (since I left off FATAL and PANIC
from the list, which I shouldn't have for the prior but should have for
the latter). So there are a few ways to fix it: allow both functions (+
the log_min_error_state function) to accept all possible error codes +
"off" (which does nothing for the first two functions); pass a unique
number for each function to assign_msglvl() so that we can determine the
a legal error code for that GUC variable is being assigned; or, just have
different lists.

Now, the first solution is a hack, but it shouldn't actually break
anything. The second is overkill. The third is the best way to do it but
as we add more of these kinds of functions (log_min_parse,
log_min_rewritten? -- I can a use for that) the amount of assign_ code
will grow linearly and be pretty similar.

Ideas?

Gavin

Gavin Sherry wrote:
> Okay, my bad. From my reading of the email exchange, I thought people
> wanted this on -- always. The best solution for this, in my opinion, is to
> have a magic value "off" which the error code lookup translates to some
> number > PANIC.

What do people think? I thought we needed a way to turn this off,
especially if the queries can be large. Because ERROR is above LOG in
server_min_messages, I don't think that is a way to fix it.

> Secondly, there is a flaw in the patch. I merged all the
> assign_server_min_messages() and assign_client_min_messages() code to make
> things pretty. Perhaps I shouldn't have (since I left off FATAL and PANIC
> from the list, which I shouldn't have for the prior but should have for
> the latter). So there are a few ways to fix it: allow both functions (+
> the log_min_error_state function) to accept all possible error codes +
> "off" (which does nothing for the first two functions); pass a unique
> number for each function to assign_msglvl() so that we can determine the
> a legal error code for that GUC variable is being assigned; or, just have
> different lists.

I thought it was good you could merge them, but now I remember why I
didn't --- they take different args.

>
> Now, the first solution is a hack, but it shouldn't actually break
> anything. The second is overkill. The third is the best way to do it but

You can't do the hack.

> as we add more of these kinds of functions (log_min_parse,
> log_min_rewritten? -- I can a use for that) the amount of assign_ code
> will grow linearly and be pretty similar.

I think the second, passing an arg to say whether it is server or
client, will do the trick, though now you need an error one too. I
guess you have to use #define and set it, or pass a string down with the
GUC variable and test that with strcmp.

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

> Are you guys competing for the modesty award? ;-)
> I heard Stallman is trying to win it this year. :-)

Hah, that's a good one.

For doing what - telling you not to call it GNU/Linux, only Linux/GNU ?
:-)

--
Kaare Rasmussen --Linux, spil,-- Tlf: 3816 2582
Kaki Data tshirts, merchandize Fax: 3816 2501
Howitzvej 75 Åben 12.00-18.00 Email: kar(at)webline(dot)dk
2000 Frederiksberg Lørdag 11.00-17.00 Web: www.suse.dk

On Tue, 3 Sep 2002, Kaare Rasmussen wrote:

> > Are you guys competing for the modesty award? ;-)
> > I heard Stallman is trying to win it this year. :-)
>
> Hah, that's a good one.
>
> For doing what - telling you not to call it GNU/Linux, only Linux/GNU ?
> :-)

SELECT FreeProject FROM History ORDER BY FreeProject

Seems quite logical : GNU/Linux ;-)

--
Alexandre Dulaunoy -- http://www.foo.be/
3B12 DCC2 82FA 2931 2F5B 709A 09E2 CD49 44E6 CBCD --- AD993-6BONE
"People who fight may lose.People who do not fight have already lost."
Bertolt Brecht

Hi all,

Does anyone else have an opinion on this? If not, I will implement it per
Bruce's commentary.

Gavin

On Mon, 2 Sep 2002, Bruce Momjian wrote:

> Gavin Sherry wrote:
> > Okay, my bad. From my reading of the email exchange, I thought people
> > wanted this on -- always. The best solution for this, in my opinion, is to
> > have a magic value "off" which the error code lookup translates to some
> > number > PANIC.
>
> What do people think? I thought we needed a way to turn this off,
> especially if the queries can be large. Because ERROR is above LOG in
> server_min_messages, I don't think that is a way to fix it.
>
>
> > Secondly, there is a flaw in the patch. I merged all the
> > assign_server_min_messages() and assign_client_min_messages() code to make
> > things pretty. Perhaps I shouldn't have (since I left off FATAL and PANIC
> > from the list, which I shouldn't have for the prior but should have for
> > the latter). So there are a few ways to fix it: allow both functions (+
> > the log_min_error_state function) to accept all possible error codes +
> > "off" (which does nothing for the first two functions); pass a unique
> > number for each function to assign_msglvl() so that we can determine the
> > a legal error code for that GUC variable is being assigned; or, just have
> > different lists.
>
>
> I thought it was good you could merge them, but now I remember why I
> didn't --- they take different args.
>
>
> >
> > Now, the first solution is a hack, but it shouldn't actually break
> > anything. The second is overkill. The third is the best way to do it but
>
> You can't do the hack.
>
> > as we add more of these kinds of functions (log_min_parse,
> > log_min_rewritten? -- I can a use for that) the amount of assign_ code
> > will grow linearly and be pretty similar.
>
> I think the second, passing an arg to say whether it is server or
> client, will do the trick, though now you need an error one too. I
> guess you have to use #define and set it, or pass a string down with the
> GUC variable and test that with strcmp.
>
>

Yes, I would like to know if it should be enabled by default, and
whether we need a way to turn it off. I assume, considering the size of
some of the queries, that we have to have a way to turn it off, and it
is possible the admin may not want queries in the log, even if the
generate errors.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> Hi all,
>
> Does anyone else have an opinion on this? If not, I will implement it per
> Bruce's commentary.
>
> Gavin
>
> On Mon, 2 Sep 2002, Bruce Momjian wrote:
>
> > Gavin Sherry wrote:
> > > Okay, my bad. From my reading of the email exchange, I thought people
> > > wanted this on -- always. The best solution for this, in my opinion, is to
> > > have a magic value "off" which the error code lookup translates to some
> > > number > PANIC.
> >
> > What do people think? I thought we needed a way to turn this off,
> > especially if the queries can be large. Because ERROR is above LOG in
> > server_min_messages, I don't think that is a way to fix it.
> >
> >
> > > Secondly, there is a flaw in the patch. I merged all the
> > > assign_server_min_messages() and assign_client_min_messages() code to make
> > > things pretty. Perhaps I shouldn't have (since I left off FATAL and PANIC
> > > from the list, which I shouldn't have for the prior but should have for
> > > the latter). So there are a few ways to fix it: allow both functions (+
> > > the log_min_error_state function) to accept all possible error codes +
> > > "off" (which does nothing for the first two functions); pass a unique
> > > number for each function to assign_msglvl() so that we can determine the
> > > a legal error code for that GUC variable is being assigned; or, just have
> > > different lists.
> >
> >
> > I thought it was good you could merge them, but now I remember why I
> > didn't --- they take different args.
> >
> >
> > >
> > > Now, the first solution is a hack, but it shouldn't actually break
> > > anything. The second is overkill. The third is the best way to do it but
> >
> > You can't do the hack.
> >
> > > as we add more of these kinds of functions (log_min_parse,
> > > log_min_rewritten? -- I can a use for that) the amount of assign_ code
> > > will grow linearly and be pretty similar.
> >
> > I think the second, passing an arg to say whether it is server or
> > client, will do the trick, though now you need an error one too. I
> > guess you have to use #define and set it, or pass a string down with the
> > GUC variable and test that with strcmp.
> >
> >
>
>

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

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> Does anyone else have an opinion on this? If not, I will implement it per
> Bruce's commentary.

> On Mon, 2 Sep 2002, Bruce Momjian wrote:
>> I think the second, passing an arg to say whether it is server or
>> client, will do the trick, though now you need an error one too. I
>> guess you have to use #define and set it, or pass a string down with the
>> GUC variable and test that with strcmp.

I think you're going to end up un-merging the routines. There is no way
to pass an extra parameter to the set/check routines (at least not
without uglifying all the rest of the GUC code). The design premise is
that the per-variable hook routines know what they're supposed to do,
and in that case this means one hook for each variable.

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Yes, I would like to know if it should be enabled by default, and
> whether we need a way to turn it off.

I feel it should be off by default --- if enough people say "hey, this
is great" then maybe we could turn it on by default, but we don't yet
have that market testing to prove the demand is there. I'm also worried
about log bloat.

regards, tom lane

On Wed, 4 Sep 2002, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > Does anyone else have an opinion on this? If not, I will implement it per
> > Bruce's commentary.
>
> > On Mon, 2 Sep 2002, Bruce Momjian wrote:
> >> I think the second, passing an arg to say whether it is server or
> >> client, will do the trick, though now you need an error one too. I
> >> guess you have to use #define and set it, or pass a string down with the
> >> GUC variable and test that with strcmp.
>
> I think you're going to end up un-merging the routines. There is no way
> to pass an extra parameter to the set/check routines (at least not

There is a wrapper around the generic function:

const char *
assign_min_error_statement(const char *newval, bool doit, bool
interactive)
{
return(assign_msglvl(&log_min_error_statement,newval,doit,interactive));
}

I would simply define some macros:

#define MSGLVL_MIN_ERR_STMT (1<<0)
#define MSGLVL_MIN_CLI_MSGS (1<<1)
#define MSGLVL_MIN_SVR_MSGS (1<<2)

And assign_msglvl(), having been passed the variable 'caller', determined
by the calling function, will do something like this:

/* everyone has likes debug */
if (strcasecmp(newval, "debug") == 0 &&
caller & (MSGLVL_MIN_ERR_STMT | MSGLVL_MIN_CLI_MSGS | MSGLVL_MIN_SVR_MSGS))
{ if (doit) (*var) = DEBUG1; }

/* ... */

else if (strcasecmp(newval, "fatal") == 0 &&
caller & (MSGLVL_MIN_ERR_STMT | MSGLVL_MIN_SVR_MSGS))
{ if (doit) (*var) = FATAL; }
else if (strcasecmp(newval, "off") == 0 &&
caller & MSGLVL_MIN_ERR_STMT)
{ if (doit) (*var) = MIN_ERR_STMT_OFF; }

Personally, I've never liked coding like this. The reason I merged the
base code for each function was so that the GUC code didn't get uglier as
more minimum-level-of-logging parameters were added. But with the code
above, the bitwise operations and appearance of the
assign_msglvl() routine will suffer the same fate, I'm afraid.

Since the flawed code is now in beta, it will need to be fixed. Do people
like the above solution or should I just revert to having a seperate
function for each GUC variable affected?

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> Since the flawed code is now in beta, it will need to be fixed. Do people
> like the above solution or should I just revert to having a seperate
> function for each GUC variable affected?

I do not see a good reason why "fatal" and "off" shouldn't be allowed
values for all three message variables. If we just did that, then you'd
be back to sharable code.

BTW, is it a good idea for server_min_messages and
log_min_error_statement to be PGC_USERSET? I could see an argument that
they should be PGC_SIGHUP, ie, settable only by the admin. As it is,
any user can hide his activity from the logs. OTOH, in the past we've
allowed anyone to change the debug level, and there haven't been
complaints about it.

There's some value in being able to kick the log level up a notch for
a specific session, but knocking it down from the admin's default could
be considered a bad thing. I suppose we could invent a PGC_SIGHUP
"min_server_min_messages" variable that sets a minimum value below which
the user can't set server_min_messages. Does that seem like a good
idea, or overkill?

A compromise position would be to make these two variables PG_SUSET,
ie settable per-session but only if you're superuser.

regards, tom lane

Tom Lane wrote:
> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > Since the flawed code is now in beta, it will need to be fixed. Do people
> > like the above solution or should I just revert to having a seperate
> > function for each GUC variable affected?
>
> I do not see a good reason why "fatal" and "off" shouldn't be allowed
> values for all three message variables. If we just did that, then you'd
> be back to sharable code.

I recommended he only allow valid values for each variable. I think if
we say we only support values X,Y,Z we had better throw an error if it
anything else.

> BTW, is it a good idea for server_min_messages and
> log_min_error_statement to be PGC_USERSET? I could see an argument that
> they should be PGC_SIGHUP, ie, settable only by the admin. As it is,
> any user can hide his activity from the logs. OTOH, in the past we've
> allowed anyone to change the debug level, and there haven't been
> complaints about it.
>
> There's some value in being able to kick the log level up a notch for
> a specific session, but knocking it down from the admin's default could
> be considered a bad thing. I suppose we could invent a PGC_SIGHUP
> "min_server_min_messages" variable that sets a minimum value below which
> the user can't set server_min_messages. Does that seem like a good
> idea, or overkill?

Seems a new GUC variable seems like overkill to me, and I think we need
to allow it to be raised. I think we can make server_min_messages
PGC_SUSET so only the admin can change 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

Tom Lane wrote:
> There's some value in being able to kick the log level up a notch for
> a specific session, but knocking it down from the admin's default could
> be considered a bad thing. I suppose we could invent a PGC_SIGHUP
> "min_server_min_messages" variable that sets a minimum value below which
> the user can't set server_min_messages. Does that seem like a good
> idea, or overkill?
>
> A compromise position would be to make these two variables PG_SUSET,
> ie settable per-session but only if you're superuser.

Oh, I just saw your compromise position. Yes, I think that is the way
to go.

In fact, aside from the security issue, allowing users to throw
voluminous debug info into the server logs doesn't seem like a good idea
anyway.

--
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:
>> I do not see a good reason why "fatal" and "off" shouldn't be allowed
>> values for all three message variables. If we just did that, then you'd
>> be back to sharable code.

> I recommended he only allow valid values for each variable. I think if
> we say we only support values X,Y,Z we had better throw an error if it
> anything else.

That's not what I said: I said allow all the values for each variable.
And document it. Why shouldn't we let people turn off error logging
if they want to?

> Seems a new GUC variable seems like overkill to me, and I think we need
> to allow it to be raised. I think we can make server_min_messages
> PGC_SUSET so only the admin can change it.

Okay, and log_min_error_statement too.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> I do not see a good reason why "fatal" and "off" shouldn't be allowed
> >> values for all three message variables. If we just did that, then you'd
> >> be back to sharable code.
>
> > I recommended he only allow valid values for each variable. I think if
> > we say we only support values X,Y,Z we had better throw an error if it
> > anything else.
>
> That's not what I said: I said allow all the values for each variable.
> And document it. Why shouldn't we let people turn off error logging
> if they want to?

But the client side doesn't make any sense to support FATAL. Am I
missing something?

> > Seems a new GUC variable seems like overkill to me, and I think we need
> > to allow it to be raised. I think we can make server_min_messages
> > PGC_SUSET so only the admin can change it.
>
> Okay, and log_min_error_statement too.

Yes.

--
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:
> But the client side doesn't make any sense to support FATAL. Am I
> missing something?

Hm. I suppose a client setting above ERROR might break some application
programs that expect either ERROR or a command-complete response ...
but do we need to go out of our way to prohibit people from choosing
settings that break their clients? If so, I've got a long list of
things we'd better worry about ...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > But the client side doesn't make any sense to support FATAL. Am I
> > missing something?
>
> Hm. I suppose a client setting above ERROR might break some application
> programs that expect either ERROR or a command-complete response ...
> but do we need to go out of our way to prohibit people from choosing
> settings that break their clients? If so, I've got a long list of
> things we'd better worry about ...

client_min_messages currently shows:

#client_min_messages = notice # Values, in order of decreasing detail:
# debug5, debug4, debug3, debug2, debug1,
# log, info, notice, warning, error

so it is only fatal and panic that are not allowed for clients. If you
want to allow them, that is fine with me. It would make it more
consistent, but of course I don't think a fatal or panic ever makes it
to the client side.

Your point that there should be a way of eliminating even ERROR coming
to a client seems valid to me. Let's make the change.

--
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:
> of course I don't think a fatal or panic ever makes it
> to the client side.

Of course it does. Try entering a bad password as a trivial example.
We punt *after* we send the elog.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > of course I don't think a fatal or panic ever makes it
> > to the client side.
>
> Of course it does. Try entering a bad password as a trivial example.
> We punt *after* we send the elog.

Oh, that's good. I guess it was PANIC I assumed never made it to the
client. Well, anyway, client should support the same values as 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

On Thu, 5 Sep 2002, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > Since the flawed code is now in beta, it will need to be fixed. Do people
> > like the above solution or should I just revert to having a seperate
> > function for each GUC variable affected?
>
> I do not see a good reason why "fatal" and "off" shouldn't be allowed
> values for all three message variables. If we just did that, then you'd
> be back to sharable code.

This was one of my other suggestions: does it matter if people can set
client_min_messages to, say, PANIC -- since they wont get it anyway.

> BTW, is it a good idea for server_min_messages and
> log_min_error_statement to be PGC_USERSET? I could see an argument that
> they should be PGC_SIGHUP, ie, settable only by the admin. As it is,
> any user can hide his activity from the logs. OTOH, in the past we've
> allowed anyone to change the debug level, and there haven't been
> complaints about it.
>
> There's some value in being able to kick the log level up a notch for
> a specific session, but knocking it down from the admin's default could
> be considered a bad thing. I suppose we could invent a PGC_SIGHUP
> "min_server_min_messages" variable that sets a minimum value below which
> the user can't set server_min_messages. Does that seem like a good
> idea, or overkill?

I think it would be important to implement it this way. I'm surprised this
hasn't come up before. Still, it'd be a 7.4 item.

>
> A compromise position would be to make these two variables PG_SUSET,
> ie settable per-session but only if you're superuser.

Sounds like a reasonably compromise. I cannot think of a reason why
people would be setting server_min_messages per session in
production. Perhaps this should be changed for 7.3?

>
> regards, tom lane
>

Gavin

Gavin Sherry wrote:
> On Thu, 5 Sep 2002, Tom Lane wrote:
>
> > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > Since the flawed code is now in beta, it will need to be fixed. Do people
> > > like the above solution or should I just revert to having a seperate
> > > function for each GUC variable affected?
> >
> > I do not see a good reason why "fatal" and "off" shouldn't be allowed
> > values for all three message variables. If we just did that, then you'd
> > be back to sharable code.
>
> This was one of my other suggestions: does it matter if people can set
> client_min_messages to, say, PANIC -- since they wont get it anyway.

Seems it is OK and is equivalent to off.

> > BTW, is it a good idea for server_min_messages and
> > log_min_error_statement to be PGC_USERSET? I could see an argument that
> > they should be PGC_SIGHUP, ie, settable only by the admin. As it is,
> > any user can hide his activity from the logs. OTOH, in the past we've
> > allowed anyone to change the debug level, and there haven't been
> > complaints about it.
> >
> > There's some value in being able to kick the log level up a notch for
> > a specific session, but knocking it down from the admin's default could
> > be considered a bad thing. I suppose we could invent a PGC_SIGHUP
> > "min_server_min_messages" variable that sets a minimum value below which
> > the user can't set server_min_messages. Does that seem like a good
> > idea, or overkill?
>
> I think it would be important to implement it this way. I'm surprised this
> hasn't come up before. Still, it'd be a 7.4 item.

I think restricting it to super-user is better.

> > A compromise position would be to make these two variables PG_SUSET,
> > ie settable per-session but only if you're superuser.
>
> Sounds like a reasonably compromise. I cannot think of a reason why
> people would be setting server_min_messages per session in
> production. Perhaps this should be changed for 7.3?

I can imagine doing it so you can log something and look at it later.

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