Re: [HACKERS] Massimo patch - Mailing list pgsql-hackers

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