Re: [PATCHES] Re: some patches

>
> Massimo, this is a fine set of patches. I want to comment on a few.
>
> > > async.patch
> > >
> > > pathes for debugging and to reduce conflicts and locks between users
> > > of pg_listeners.
> >
> > Appears to be massive changes since patch created...Not Applied.
>
> That is a shame. Sounds like a nice patch. Massimo, I am curious about
> your comments on my changes to the lock source code. Was it a help?
>
> Hard to imagine what has changed since August 21st.

fixed now

> > > lock.patch
> > >
> > > I have 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. There is also a new
> > > function which returns an array of pids owning a lock.
> > > I'm using this code from over six months and it works fine.
> >
> > Applied...
>
> Again, any work on locks is good. Did my changes help?

I'm not completely convinced of the new lock priority. I will study your
code and make some test. I suspect it may introduce some deadlocks in the
listen/notify code.

> > > ps-status.patch
> > >
> > > macros for ps status, used by postgres.c and utility.c.
> > > Unfortunately ps status is system dependent and the current
> > > code doesn't work on linux. The use of macros confines system
> > > dependency to into one file (ps-status.h). Users of other
> > > operating systems should check this code and submit new macros.
> >
> > Applied...but...
> >
> > ...is there a reason why we aren't using setproctitle() where
> > applicable, or is this what you mean with the 'submit new macros' comment?
> > Or is this totally unrelated? :)
>
> I think so. Someone with setproctitle needs to give us code that works
> on their machine. We can then add a configure check so OS's with the
> feature can use it. One issue is whether we want the function called
> for each backend query because it may be too large a performance hit.
> No idea how to deal with this.

ps status is optional. If we can't do it on some OS we simply define empty
macros and all is ok. We must however be careful with side effects inside
ps macros in utility.c.

> > > 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.
> > > I don't know if anybody is working on SI, but until another
> > > solution is found this patch fixes the problem. I have received
> > > messages from other people reporting the same problem which I
> > > fixed many months ago.
> >
> > Applied...
>
> I was hoping you would submit this one. Sounds like a needed patch, and
> something few of use know how to debug.

Is Vadim working on shared cache? Maybe this patch could become obsolete
in the next version. But for now it is necessary. I had very nasty problems
with this.

> > > socket-flock.patch
> > >
> > > use advisory locks to check if the unix socket can be deleted.
> > > A running postmaster keeps a lock on that file. A starting
> > > postmaster exits if the file exists and is locked, otherwise
> > > it deletes the sockets and proceeds.
> > > This avoid the need to remove manually the file after a postmaster
> > > or system crash.
> > > I don't know if flock is available on any system. If not we could
> > > define a HAVE_FLOCK set by configure.
> >
> > Definitely applied...
>
> This is certainly a great idea. Deleting that lock file is a pain.
>
> I am almost certain we are going to need a configure check on this one.
> flock/lockf() are supported on almost all systems, but usually not both.
> Perhaps someone with lockf and not flock can give us some code.

Yes, this must be checked on every OS. I have only linux to test.

> > > tprintf.patch
> > >
> > > 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.
> >
> > Applied....
> >
> > Just curious, but any documentation forthcoming on some of this
> > stuff? pg_options and the assert stuff are the one that jump to mind...
>
> Good idea. If you can give me a comment about it, I can put something
> in the FAQ under debugging. Sounds like we need manual page update, if
> that was not already in the patch.

I will write some documentation. Any comments on the new signal handling ?

I would like also to discuss on the different ways used to print debug
messages. At the moment we have stdout, stderr, tprintf with syslog and
elog. Maybe we could establish some standard ways to print messages.
And we could put all the various debug flags and options in pg_options.

I have also another idea. When the postmaster is started it should delete
all tuples left in pg_listener because all pids refer to processes now dead.
I'm thinking of doing an ftruncate on all data/*/pg_listener at postmaster
startup. This should work because there aren't any indices on that table.
I'm doing this in the shell script which starts postgres and it semms to
work fine. Any comments ?

--
Massimo Dal Zotto

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