Обсуждение: Massimo patch
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. --------------------------------------------------------------------------- Forwarded message: > From owner-pgsql-patches@hub.org Sat Feb 14 09:31:23 1998 > From: Massimo Dal Zotto <dz@cs.unitn.it> > Message-Id: <199802132233.XAA01003@nikita.wizard.it> > Subject: [PATCHES] patches for 6.2.1p6 > To: pgsql-patches@postgreSQL.org (Pgsql Patches) > Date: Tue, 27 Jan 1998 22:46:38 +0100 (MET) > X-Mailer: ELM [version 2.4 PL24 ME4] > MIME-Version: 1.0 > Content-Type: multipart/mixed; boundary=%#%record%#% > Sender: owner-pgsql-patches@hub.org > Precedence: bulk > > --%#%record%#% > Content-Type: text/plain; charset=iso-8859-1 > Content-Transfer-Encoding: 8bit > Content-Length: 6638 > > Hi hackers, > > I have old patches for version 6.2.1p6 which fix some problems and add > new features. Here is a short description of each patch file: > > > 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. > > async-unlisten.patch > > declares Async_Unlisten() external so that it can be called by user > modules. > > exec-limit.patch > > removes the #ifdef NOT_USED around ExecutorLimit(). It is used. > > exitpg.patch > > limits recursive calls to exitpg() preventing an infinite loop > if an error is found inside exitpg. > > 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). > > pg-flush.patch > > removes an unnecessary flush in libpq reducing network traffic and > increasing performance. > > relname.patch > > an utility which returns the relname corresponding to a given oid. > Useful for debug messages (see vacum.patch). > > sequence.patch > > added a setval() function which enables othe owner of a sequence > to set its value without need to delete and recreate it. > > 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. > > 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. > > 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. > > Old files using int flags or variables can be easily changed to > use the new package by substituting the old variable with a #define > like in the following example: > > /* int my_flag = 0; */ > #include "trace.h" > #define my_flag pg_options[OPT_MYFLAG] > > I have done it in postgres.c and some other files and now I can turn > on/off any single debug flag on the fly with a simple shell script. > I have removed the IpcConfigTip() from ipc.c, it should better be > described in the postgres manual instead of being printed on stderr. > > This patch provides also a new format of debugging messages which > are always in a single line with a timestamp and the backend pid: > > #timestamp #pid #message > 980127.17:52:14.173 [29271] StartTransactionCommand > 980127.17:52:14.174 [29271] ProcessUtility: drop table t; > 980127.17:52:14.186 [29271] SIIncNumEntries: table is 70% full > 980127.17:52:14.186 [29286] Async_NotifyHandler > 980127.17:52:14.186 [29286] Waking up sleeping backend process > 980127.19:52:14.292 [29286] Async_NotifyFrontEnd > 980127.19:52:14.413 [29286] Async_NotifyFrontEnd done > 980127.19:52:14.466 [29286] Async_NotifyHandler done > > This improves the readability of the log and allows one to understand > exactly which backend is doing what and at which time. It also makes > easier to write simple awk or perl scripts which monitor the log to > detect database errors or problem, or to compute transaction times. > > The patch changes also the meaning of signals used by postgres, as > described by the following table: > > postmaster backend > > SIGHUP kill(*,sighup) read_pg_options > SIGINT kill(*,sigint), die die > SIGCHLD reaper - > SIGTTIN ignored - > SIGTTOU ignored - > SIGQUIT die handle_warn > SIGTERM kill(*,sigterm), kill(*,9), die die > SIGCONT dumpstatus - > SIGPIPE ignored die > SIGFPE - FloatExceptionHandler > SIGTSTP - ignored (alive test) > SIGUSR1 kill(*,sigusr1), die quickdie > SIGUSR2 kill(*,sigusr2) Async_NotifyHandler > (also SI buffer flush) > > The main changes to the old implementation are SIGQUIT instead of > SIGHUP to handle warns, SIGHUP to reread pg_options and redirection > to all backends of SIGHUP, SIGINT, SIGTERM, SIGUSR1 and SIGUSR2. > In this way some of the signals sent to the postmaster can be sent > automatically to all the backends. To shut down postgres one needs > only to send a SIGTERM to postmaster and it will stop automatically > all the backends. This new signal handling mechanism is also used > to prevent SI cache table overflows: when a backend detects the SI > table full at 70% it simply sends a signal to the postmaster which > will wake up all idle backends and make them flush the cache. > > 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. > > -- > Massimo Dal Zotto -- Bruce Momjian maillist@candle.pha.pa.us -- Bruce Momjian maillist@candle.pha.pa.us
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...:)