Massimo patch

On Sat, 14 Feb 1998, Bruce Momjian wrote:

Here is a description of the major patch from Massimo. The irony of
this is that the mail message is dated January 27th, when application of
the patch would have been easier because we were not in beta testing. I
have a copy of the patch here on my machine.

What do people want to do with this? I have reviewed the patch, and it
looks good, but it may take work to merge in because it is against
6.2.1p7, not 6.3 beta, and it does introduce quite a bit of new code.

I sent Massimo and email (CC'd to the list) explaining the fact
that we are in Beta mode, and that some of these patches are currently
questionable for v6.3 (the lock patch being one)...with another 2 weeks of
beta testing before release, my opinion is that these involve changes that
will not have sufficient time to test prior to release, especially with at
least one major bug still existing...

The other question that I do have is how appropriate some of these
patches are to v6.3...2 weeks, in my opinion, isn't a suitable amount of
time, in which to accurately test these, and should wait until after the
release...

Unless anyone can really argue this, those that add features
(assert.patch) should be omitted...those that fix a bug are appropriate.
Not that I haven't looked at the patch itself, only at the descriptions
presented...

assert.patch

adds a switch to turn on/off the assert checking if enabled at compile
time. You can now compile postgres with assert checking and disable it
at runtime in a production environment.

New feature...not a bug fix...

async-unlisten.patch

declares Async_Unlisten() external so that it can be called by user
modules.

New feature...not a bug fix...

exec-limit.patch

removes the #ifdef NOT_USED around ExecutorLimit(). It is used.

if it has a "#ifdef NOT_USED" around it, then how is it used?

exitpg.patch

limits recursive calls to exitpg() preventing an infinite loop
if an error is found inside exitpg.

Potential bug...but how is it triggered?

libpgtcl-listen.patch

Just a change from upper to lowercase of an sql command in libpgtcl,
totally harmless.

??

new-locks.patch

After long studying and many debugging sessions I have finally
understood how the low level locks work.
I have completely rewritten lock.c cleaning up the code and adding
better assert checking. I have also added some fields to the lock
and xid tags for better support of user locks. This patch includes
also a patch submitted by Bruce Momjian which changes the handling
of lock priorities. It can however be disabled if an option is set
in pg_options, see tprintf.patch (Bruce patch works by building
the queue in reverse priority order, my old patch kept the queue in
decreasing order and traversed it from the other side).

I don't understand this one, but it sounds like its negates the
work you just did? Again, sounds like a feature change, not a bug fix,
since you have recently re-written this...

pg-flush.patch

removes an unnecessary flush in libpq reducing network traffic and
increasing performance.

Didn't we just do a protocol rewrite?

relname.patch

an utility which returns the relname corresponding to a given oid.
Useful for debug messages (see vacum.patch).

Is this a new program? src/bin/relname?

sequence.patch

added a setval() function which enables othe owner of a sequence
to set its value without need to delete and recreate it.

Feature change, not a bug...

sinval.patch

fixes a problem in SI cache which causes table overflow if some
backend is idle for a long time while other backends keep adding
entries.
It uses the new signal handling implemented in tprintf.patch.
I have also increacasesed the max number of backends from 32 to 64 and
the table size from 1000 to 5000.

Bug fix...

spin-lock.patch

I'm not sure if this is really useful, but it seems stupid to have
a backend wasting cpu cycles in a busy loop while the process which
should release the lock is waiting for the cpu. So I added a call
to process_yield() if the spin lock can't obtained.
This has been implemented and tested only on Linux. I don't know if
other OS have process_yield(). If someone can check please do it.

Sounds like a bug fix to me...

tprintf.patch

adds functions and macros which implement a conditional trace package
with the ability to change flags and numeric options of running
backends at runtime.
Options/flags can be specified in the command line and/or read from
the file pg_options in the data directory.
Running backends can be forced to update their options from this file
by sending them a SIGHUP signal (this is the convention used by most
unix daemons so I changed the meaning of SIGHUP).
Options can be debugging flags used by the trace package or any other
numeric value used by the backend, for example the deadlock_timeout.
Having flags and options specified at runtime and changed while the
backends are running can greatly simplify the debugging and tuning
of the database. New options can be defined in utils/misc/trace.c and
include/utils/trace.h. As an example of the usage of this package
see lock.c and proc.c which make use of new runtime options.

Definitely have to say new feature...

vacuum.patch

adds a debug message to vacuum that prints the name of a table or
index *before* vacuuming it, if the verbose keyword is set.
This is useful to know which table is causing troubles if a
vacuum all crashes. Currently table information is printed only
at the end of each vacuum operation and is never printed if the
vacuum crashes.

Again, a feature more then a bug fix...

Marc G. Fournier
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org

Here is a description of the major patch from Massimo. The irony of
this is that the mail message is dated January 27th, when application of
the patch would have been easier because we were not in beta testing. I
have a copy of the patch here on my machine.

What do people want to do with this? I have reviewed the patch, and it
looks good, but it may take work to merge in because it is against
6.2.1p7, not 6.3 beta, and it does introduce quite a bit of new code.

Jeesh. Sure looks nice. Without knowing how fundamental the code changes are, it
is hard to say for sure, but I would think we would want to put this in, unless
it leads to major breakage. Someone would need to take the patches, merge them
all in as a group, and validate the regression tests to see where we are on them.
I'd even think it would be worth doing if it delays release a couple of weeks,
which it may not.

More comments??

- Tom

On Sat, 14 Feb 1998, Bruce Momjian wrote:

Here is a description of the major patch from Massimo. The irony of
this is that the mail message is dated January 27th, when application of
the patch would have been easier because we were not in beta testing. I
have a copy of the patch here on my machine.

What do people want to do with this? I have reviewed the patch, and it
looks good, but it may take work to merge in because it is against
6.2.1p7, not 6.3 beta, and it does introduce quite a bit of new code.

I sent Massimo and email (CC'd to the list) explaining the fact
that we are in Beta mode, and that some of these patches are currently
questionable for v6.3 (the lock patch being one)...with another 2 weeks of
beta testing before release, my opinion is that these involve changes that
will not have sufficient time to test prior to release, especially with at
least one major bug still existing...

The other question that I do have is how appropriate some of these
patches are to v6.3...2 weeks, in my opinion, isn't a suitable amount of
time, in which to accurately test these, and should wait until after the
release...

Unless anyone can really argue this, those that add features
(assert.patch) should be omitted...those that fix a bug are appropriate.
Not that I haven't looked at the patch itself, only at the descriptions
presented...

assert.patch

adds a switch to turn on/off the assert checking if enabled at compile
time. You can now compile postgres with assert checking and disable it
at runtime in a production environment.

New feature...not a bug fix...

Old feature, I submitted this patch more than six months ago, I'm using it
from that date without problems. It simply adds a global variable to a macro.

async-unlisten.patch

declares Async_Unlisten() external so that it can be called by user
modules.

New feature...not a bug fix...

Just declaring external an existing function shouldn't break anything.

exec-limit.patch

removes the #ifdef NOT_USED around ExecutorLimit(). It is used.

if it has a "#ifdef NOT_USED" around it, then how is it used?

It is used by a loadable module in contrib.

exitpg.patch

limits recursive calls to exitpg() preventing an infinite loop
if an error is found inside exitpg.

Potential bug...but how is it triggered?

I don't know exactly how it is triggered but I have seen it happen. This
patch tries to limit damages.

libpgtcl-listen.patch

Just a change from upper to lowercase of an sql command in libpgtcl,
totally harmless.

??

Not really important, it just changes 'LISTEN' to 'listen' so that when I
look at the postgres log the query is shown in lowercase like the others.
Totally harmless.

new-locks.patch

After long studying and many debugging sessions I have finally
understood how the low level locks work.
I have completely rewritten lock.c cleaning up the code and adding
better assert checking. I have also added some fields to the lock
and xid tags for better support of user locks. This patch includes
also a patch submitted by Bruce Momjian which changes the handling
of lock priorities. It can however be disabled if an option is set
in pg_options, see tprintf.patch (Bruce patch works by building
the queue in reverse priority order, my old patch kept the queue in
decreasing order and traversed it from the other side).

I don't understand this one, but it sounds like its negates the
work you just did? Again, sounds like a feature change, not a bug fix,
since you have recently re-written this...

I rewrote part of the work I did one year ago, cleaned up the original lock
code and did some small changes. I admit that this should be handled with
care, but, again, I'm using it from months and it seems to work well.

pg-flush.patch

removes an unnecessary flush in libpq reducing network traffic and
increasing performance.

Didn't we just do a protocol rewrite?

I don't know about it, but Bruce approved the patch some months ago, so it
has probably been included in the new protocol.

relname.patch

an utility which returns the relname corresponding to a given oid.
Useful for debug messages (see vacum.patch).

Is this a new program? src/bin/relname?

It is just a new utility function which returns the name of a relation given
its oid. It may be used to show relname information in user messages and
traces.

sequence.patch

added a setval() function which enables othe owner of a sequence
to set its value without need to delete and recreate it.

Feature change, not a bug...

sinval.patch

fixes a problem in SI cache which causes table overflow if some
backend is idle for a long time while other backends keep adding
entries.
It uses the new signal handling implemented in tprintf.patch.
I have also increacasesed the max number of backends from 32 to 64 and
the table size from 1000 to 5000.

Bug fix...

Yes, and very nasty.

spin-lock.patch

I'm not sure if this is really useful, but it seems stupid to have
a backend wasting cpu cycles in a busy loop while the process which
should release the lock is waiting for the cpu. So I added a call
to process_yield() if the spin lock can't obtained.
This has been implemented and tested only on Linux. I don't know if
other OS have process_yield(). If someone can check please do it.

Sounds like a bug fix to me...

No, it is just a performance optimization. And I'm not sure how much we could
gain from this patch, so I ask a discussion about it.

tprintf.patch

adds functions and macros which implement a conditional trace package
with the ability to change flags and numeric options of running
backends at runtime.
Options/flags can be specified in the command line and/or read from
the file pg_options in the data directory.
Running backends can be forced to update their options from this file
by sending them a SIGHUP signal (this is the convention used by most
unix daemons so I changed the meaning of SIGHUP).
Options can be debugging flags used by the trace package or any other
numeric value used by the backend, for example the deadlock_timeout.
Having flags and options specified at runtime and changed while the
backends are running can greatly simplify the debugging and tuning
of the database. New options can be defined in utils/misc/trace.c and
include/utils/trace.h. As an example of the usage of this package
see lock.c and proc.c which make use of new runtime options.

Definitely have to say new feature...

Yes, but very handy. I highly recommend it, maybe after 6.3.

vacuum.patch

adds a debug message to vacuum that prints the name of a table or
index *before* vacuuming it, if the verbose keyword is set.
This is useful to know which table is causing troubles if a
vacuum all crashes. Currently table information is printed only
at the end of each vacuum operation and is never printed if the
vacuum crashes.

Again, a feature more then a bug fix...

Just a debug message, should not break anything.

I would like also to add a new patch for the oracle_compat patch added in
version 6.2.1p7, which is definitely broken. Try this and you will see:

test=> create table t (x text);
CREATE
test=> insert into t values('1');
INSERT 17422 1
test=> insert into t values('12');
INSERT 17423 1
test=> insert into t values('123');
INSERT 17424 1
test=> insert into t values('1234');
INSERT 17425 1
test=> insert into t values('12345');
INSERT 17426 1
test=> insert into t values('123456');
INSERT 17427 1
test=> insert into t values('1234567');
INSERT 17428 1
test=> insert into t values('12345678');
INSERT 17429 1
test=> insert into t values('123456789');
INSERT 17430 1
test=> insert into t values('1234567890');
INSERT 17431 1
test=> insert into t values('12345678901');
INSERT 17432 1
test=> insert into t values('123456789012');
INSERT 17433 1
test=> insert into t values('1234567890123');
INSERT 17434 1
test=> insert into t values('12345678901234');
INSERT 17435 1
test=> insert into t values('123456789012345');
INSERT 17436 1
test=> insert into t values('1234567890123456');
INSERT 17437 1
test=> select x from t;
x
----------------
1
12
123
1234
12345
123456
1234567
12345678
123456789
1234567890
12345678901
123456789012
1234567890123
12345678901234
123456789012345
1234567890123456
(16 rows)

test=> select substr(x,1) from t;
substr
----------------
1
12
123
12347
12345
123456
1234567
12345678
123456789
1234567890
12345678901
123456789012?
1234567890123
12345678901234
123456789012345
1234567890123456
(16 rows)

Note the extra character after 1234 and 123456789012. The bug is random and
is present also in the latest version sent me by Bruce. The following patch
corrects the problem. BTW, why are the results aligned differently by psql?

*** src/backend/utils/adt/oracle_compat-621p7.c	Wed Jan 28 00:11:16 1998
--- src/backend/utils/adt/oracle_compat.c	Tue Feb  3 14:48:42 1998
***************
*** 510,526 ****
  			   *ptr_ret;
  	int			len;

if ((string == (text *) NULL) ||
! (m <= 0) || (n <= 0) ||
! ((len = VARSIZE(string) - VARHDRSZ - m + 1) <= 0))
return string;

! len = len + 1 < n ? len + 1 : n;

ret = (text *) palloc(VARHDRSZ + len);
VARSIZE(ret) = VARHDRSZ + len;

! ptr = VARDATA(string) + m - 1;
ptr_ret = VARDATA(ret);

  	while (len--)
--- 510,529 ----
  			   *ptr_ret;
  	int			len;

+ /* Make offset 0-based */
+ m--;
+
if ((string == (text *) NULL) ||
! (m < 0) || (n <= 0) ||
! ((len = VARSIZE(string) - VARHDRSZ - m) <= 0))
return string;

! len = (len < n) ? len : n;

ret = (text *) palloc(VARHDRSZ + len);
VARSIZE(ret) = VARHDRSZ + len;

! ptr = VARDATA(string) + m;
ptr_ret = VARDATA(ret);

while (len--)

--
Massimo Dal Zotto

+----------------------------------------------------------------------+
|  Massimo Dal Zotto                e-mail:  dz@cs.unitn.it            |
|  Via Marconi, 141                 phone:  ++39-461-534251            |
|  38057 Pergine Valsugana (TN)     www:  http://www.cs.unitn.it/~dz/  |
|  Italy                            pgp:  finger dz@tango.cs.unitn.it  |
+----------------------------------------------------------------------+

Peanut Gallery Vote on tprintf patch:

I would vote to put this into 6.3.

RATIONALE: It seems to offer database _users_ the potential of
providing more useful information about (potential) bugs to
the development team. Seems like it could help speed up
the development process. Getting it into the code base
sooner rather than later seems desireable.

Marc Zuckman
marc@fallon.classyad.com

On Mon, 16 Feb 1998, Marc Howard Zuckman wrote:

Peanut Gallery Vote on tprintf patch:

I would vote to put this into 6.3.

RATIONALE: It seems to offer database _users_ the potential of
providing more useful information about (potential) bugs to
the development team. Seems like it could help speed up
the development process. Getting it into the code base
sooner rather than later seems desireable.

Except, it is a new feature, not a bug fix...except for a *very*
select few features that overlapped the beta freeze (subselects being the
only one that I can thnk of), nothing new is going into this release...

To add to it, the patches are against v6.2.1, which would mean
having to spend time away from current bug fixes to safely be able to
merge them in.

As I said before, all of the patches look good and I want to get
them in, but not until after v6.3 is released.

Massimo, it would really help things if you could submit a new
patch, after v6.3 is released, against v6.3...I've kept the original
patch, and will manually apply it if I have to, but...:)