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

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