Re: [PATCHES] Minor fix in lwlock.c
"Qingqing Zhou" <[EMAIL PROTECTED]> writes:
> "Tom Lane" <[EMAIL PROTECTED]> writes
>> Maybe we *should* make it a PANIC. Thoughts?
> Reasonable. Since this should *never* happen. Once happened, that's means we
> have a serious bug in our design/coding.
Plan C would be something like
if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
{
release the acquired lock;
elog(ERROR, "too many LWLocks taken");
}
But we couldn't just call LWLockRelease, since it expects the lock to
be recorded in held_lwlocks[]. We'd have to duplicate a lot of code,
or split LWLockRelease into multiple routines, neither of which seem
attractive answers considering that this must be a can't-happen
case anyway.
PANIC it will be, unless someone thinks of a reason why not by
tomorrow...
regards, tom lane
---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [PATCHES] Minor fix in lwlock.c
"Tom Lane" <[EMAIL PROTECTED]> writes > > Maybe we *should* make it a PANIC. Thoughts? > Reasonable. Since this should *never* happen. Once happened, that's means we have a serious bug in our design/coding. Regards, Qingqing ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Minor fix in lwlock.c
"Qingqing Zhou" <[EMAIL PROTECTED]> writes: > Another alternative might use PG_TRY/PG_CATCH to make sure that the > semaphore is released. But seems this costs too much ... I agree. LWLockAcquire is a hot-spot already. Maybe we *should* make it a PANIC. Thoughts? regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Minor fix in lwlock.c
"Tom Lane" <[EMAIL PROTECTED]> writes> > The alternative would be to move the Unlock loop in front of the > addition of the LWLock to held_lwlocks[], but I think that cure > is probably worse than the disease --- the chance of an error during > Unlock seems nonzero. > Another alternative might use PG_TRY/PG_CATCH to make sure that the semaphore is released. But seems this costs too much ... Regards, Qingqing ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] Minor fix in lwlock.c
"Qingqing Zhou" <[EMAIL PROTECTED]> writes: > I guess the problem is here: > /* > * Fix the process wait semaphore's count for any absorbed wakeups. > */ > while (extraWaits-- > 0) > PGSemaphoreUnlock(&proc->sem); Mmm. Could be a problem, but the chances of having extraWaits>0 is really pretty small. In any case, FATAL doesn't fix this, because it will still try to go through normal backend exit cleanup which requires having working LWLock support. If you take the above risk seriously then you need a PANIC error. The alternative would be to move the Unlock loop in front of the addition of the LWLock to held_lwlocks[], but I think that cure is probably worse than the disease --- the chance of an error during Unlock seems nonzero. regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] Minor fix in lwlock.c
"Tom Lane" <[EMAIL PROTECTED]> writes > I don't see any reason to think we'd be unable to recover normally from such a > bug --- do you? > I guess the problem is here: /* * Fix the process wait semaphore's count for any absorbed wakeups. */ while (extraWaits-- > 0) PGSemaphoreUnlock(&proc->sem); elog(ERROR) won't recover semaphore count. ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] Minor fix in lwlock.c
"Qingqing Zhou" <[EMAIL PROTECTED]> writes: > The chance that num_held_lwlocks is beyond MAX_SIMUL_LWLOCKS is similar to > the chance that failed to grasp a spinlock in 1 minute, so they should be > treated in the same way. This is mainly to prevent programming error (e.g., > forget to release the LWLocks). Hmm ... yeah, it's not too hard to imagine a bug leading to trying to grab content locks on more than 100 buffers, for example. Patch applied, although I reduced the severity from FATAL to ERROR. I don't see any reason to think we'd be unable to recover normally from such a bug --- do you? regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
[PATCHES] Minor fix in lwlock.c
The chance that num_held_lwlocks is beyond MAX_SIMUL_LWLOCKS is similar to
the chance that failed to grasp a spinlock in 1 minute, so they should be
treated in the same way. This is mainly to prevent programming error (e.g.,
forget to release the LWLocks).
Regards,
Qingqing
---
Index: lwlock.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.25
diff -c -r1.25 lwlock.c
*** lwlock.c31 Dec 2004 22:01:05 - 1.25
--- lwlock.c8 Apr 2005 02:19:31 -
***
*** 328,334
SpinLockRelease_NoHoldoff(&lock->mutex);
/* Add lock to list of locks held by this backend */
! Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
held_lwlocks[num_held_lwlocks++] = lockid;
/*
--- 328,335
SpinLockRelease_NoHoldoff(&lock->mutex);
/* Add lock to list of locks held by this backend */
! if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
! elog(FATAL, "Too many LWLocks");
held_lwlocks[num_held_lwlocks++] = lockid;
/*
***
*** 397,403
else
{
/* Add lock to list of locks held by this backend */
! Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
held_lwlocks[num_held_lwlocks++] = lockid;
}
--- 398,405
else
{
/* Add lock to list of locks held by this backend */
! if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
! elog(FATAL, "Too many LWLocks");
held_lwlocks[num_held_lwlocks++] = lockid;
}
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork_process() for pgstat
Neil Conway wrote: Much to my chagrin, I recently noticed that I forgot to convert the pgstat fork() code to use fork_process() when I applied the original fork_process() patch a few months ago. This patch does that. Applied. -Neil ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] add_missing_from = false
Neil Conway wrote: Now that DELETE has a USING clause, we should be able to make add_missing_from=false the default in 8.1; the attached patch implements this and updates the documentation. Applied. -Neil ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [GENERAL] [PATCHES] A way to let Vacuum warn if FSM settings are
Ron Mayer wrote: > Tom Lane wrote: > > Bruce Momjian writes: > >>I never heard any discussion on whether this should be backpatched to > >>8.0.X. Should it? > > I personally think it should _not_ be backpatched. Since it > doesn't fix any bugs, it's not really the kind of thing I > would expect to be backpatched. > OK, just asking. > > I'm not inclined to throw it in at the last minute, as it's not been > > through any testing and I'm not sure the behavior has really been agreed > > on anyway. (The diff you cite starts from code that's not in 8.0.* either.) > > Regarding the behavior I pretty much thought it was agreed upon. > I saw people proposing reasons advocating both the log file and > the client getting the message. Simon's "Can we have both?" > comment got one positive response (Bruce's with the patch) and > no negative ones, I thought that indicated general agreement. > > If we did want to re-open the behavior question, I might mention > that this message is only printed on a database-wide VACUUM; and > with autovacuum targeting specific tables such database-wide > VACUUMs might become more and more rare. But I think that's a > separate issue. Yea, I think we are agreed. I was not asking for 8.0.2 but just 8.0.X in general. I ask only to see if someone jumps up and wants to explain why it should be in 8.0.X. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [GENERAL] [PATCHES] A way to let Vacuum warn if FSM settings
Tom Lane wrote: Bruce Momjian writes: I never heard any discussion on whether this should be backpatched to 8.0.X. Should it? I personally think it should _not_ be backpatched. Since it doesn't fix any bugs, it's not really the kind of thing I would expect to be backpatched. I'm not inclined to throw it in at the last minute, as it's not been through any testing and I'm not sure the behavior has really been agreed on anyway. (The diff you cite starts from code that's not in 8.0.* either.) Regarding the behavior I pretty much thought it was agreed upon. I saw people proposing reasons advocating both the log file and the client getting the message. Simon's "Can we have both?" comment got one positive response (Bruce's with the patch) and no negative ones, I thought that indicated general agreement. If we did want to re-open the behavior question, I might mention that this message is only printed on a database-wide VACUUM; and with autovacuum targeting specific tables such database-wide VACUUMs might become more and more rare. But I think that's a separate issue. Ron ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [GENERAL] [PATCHES] A way to let Vacuum warn if FSM settings are low. [final?]
Bruce Momjian writes: > I never heard any discussion on whether this should be backpatched to > 8.0.X. Should it? I'm not inclined to throw it in at the last minute, as it's not been through any testing and I'm not sure the behavior has really been agreed on anyway. (The diff you cite starts from code that's not in 8.0.* either.) regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [GENERAL] [PATCHES] A way to let Vacuum warn if FSM settings are
I never heard any discussion on whether this should be backpatched to
8.0.X. Should it?
---
pgman wrote:
> Simon Riggs wrote:
> > On Mon, 2005-03-14 at 01:40 -0500, Tom Lane wrote:
> > > Bruce Momjian writes:
> > > > Ron Mayer wrote:
> > > >> My reasoning why I thought the log file was more useful was
> > > >> that only an admin with access to the log files could really
> > > >> do anything about the message anyway.
> > >
> > > > The log file is useful, but I think showing the VACUUM user is _more_
> > > > useful than the log file.
> > >
> > > I think that reasoning is fundamentally unsound, because (a) a lot of
> > > people already do vacuuming via a cron job or autovacuum, and (b)
> > > autovacuum is definitely the wave of the future. So it's foolish
> > > to design this messaging around the assumption that there will be
> > > a human attentive to the on-line output from VACUUM. We should be
> > > ensuring that the message gets into the postmaster log --- whether
> > > it gets sent to the client is secondary.
> >
> > Personally, I prefer the postmaster log as the place for this.
> >
> > However, whilst vacuum exists as a separate command, there will be an
> > argument to return a message back to the person running it; we cannot
> > assume that people would be inattentive.
> >
> > Possibly the deciding factor should be whether autovacuum makes it fully
> > into becoming a special backend anytime soon, since in that case only
> > the log would remain as an option for reporting this message, in that
> > case.
> >
> > Can we have both?
>
> Sure. It is very easy and in fact looks even cleaner than the original
> code because now the optional stuff is in its own function.
>
> Patch attached and applied.
>
> --
> Bruce Momjian| http://candle.pha.pa.us
> pgman@candle.pha.pa.us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup.| Newtown Square, Pennsylvania 19073
> Index: src/backend/storage/freespace/freespace.c
> ===
> RCS file: /cvsroot/pgsql/src/backend/storage/freespace/freespace.c,v
> retrieving revision 1.38
> diff -c -c -r1.38 freespace.c
> *** src/backend/storage/freespace/freespace.c 12 Mar 2005 05:21:52 -
> 1.38
> --- src/backend/storage/freespace/freespace.c 14 Mar 2005 20:04:00 -
> ***
> *** 221,226
> --- 221,228
>
> * FSMHeader->relHash */
>
>
> + static void CheckFreeSpaceMapStatistics(int elevel, int numRels,
> + double needed);
> static FSMRelation *lookup_fsm_rel(RelFileNode *rel);
> static FSMRelation *create_fsm_rel(RelFileNode *rel);
> static void delete_fsm_rel(FSMRelation *fsmrel);
> ***
> *** 711,726
>errdetail("FSM size: %d relations + %d pages = %.0f kB
> shared memory.",
> MaxFSMRelations, MaxFSMPages,
> (double) FreeSpaceShmemSize() /
> 1024.0)));
> !
> ! if (numRels == MaxFSMRelations)
> ! ereport(NOTICE,
> (errmsg("max_fsm_relations(%d) equals the number of
> relations checked",
>MaxFSMRelations),
>errhint("You have >= %d relations.\n"
>"Consider increasing the configuration
> parameter \"max_fsm_relations\".",
>numRels)));
> else if (needed > MaxFSMPages)
> ! ereport(NOTICE,
> (errmsg("the number of page slots needed (%.0f) exceeds
> max_fsm_pages (%d)",
>needed,MaxFSMPages),
>errhint("Consider increasing the configuration
> parameter \"max_fsm_relations\"\n"
> --- 713,736
>errdetail("FSM size: %d relations + %d pages = %.0f kB
> shared memory.",
> MaxFSMRelations, MaxFSMPages,
> (double) FreeSpaceShmemSize() /
> 1024.0)));
> !
> ! CheckFreeSpaceMapStatistics(NOTICE, numRels, needed);
> ! /* Print to server logs too because is deals with a config variable. */
> ! CheckFreeSpaceMapStatistics(LOG, numRels, needed);
> ! }
> !
> ! static void
> ! CheckFreeSpaceMapStatistics(int elevel, int numRels, double needed)
> ! {
> ! if (numRels == MaxFSMRelations)
> ! ereport(elevel,
> (errmsg("max_fsm_relations(%d) equals the number of
> relations checked",
>MaxFSMRelations),
>errhint("You have >= %d relations.\n"
>"Consider incre
[PATCHES] fork_process() for pgstat
Much to my chagrin, I recently noticed that I forgot to convert the
pgstat fork() code to use fork_process() when I applied the original
fork_process() patch a few months ago. This patch does that.
Barring any objections, I'll apply this tomorrow.
-Neil
Index: src/backend/postmaster/pgstat.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.89
diff -c -r1.89 pgstat.c
*** src/backend/postmaster/pgstat.c 31 Mar 2005 23:20:49 - 1.89
--- src/backend/postmaster/pgstat.c 7 Apr 2005 07:51:23 -
***
*** 40,45
--- 40,46
#include "libpq/pqsignal.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+ #include "postmaster/fork_process.h"
#include "postmaster/postmaster.h"
#include "storage/backendid.h"
#include "storage/fd.h"
***
*** 576,601
/*
* Okay, fork off the collector.
*/
-
- fflush(stdout);
- fflush(stderr);
-
- #ifdef __BEOS__
- /* Specific beos actions before backend startup */
- beos_before_backend_startup();
- #endif
-
#ifdef EXEC_BACKEND
switch ((pgStatPid = pgstat_forkexec(STAT_PROC_BUFFER)))
#else
! switch ((pgStatPid = fork()))
#endif
{
case -1:
- #ifdef __BEOS__
- /* Specific beos actions */
- beos_backend_startup_failed();
- #endif
ereport(LOG,
(errmsg("could not fork statistics buffer: %m")));
return 0;
--- 577,589
/*
* Okay, fork off the collector.
*/
#ifdef EXEC_BACKEND
switch ((pgStatPid = pgstat_forkexec(STAT_PROC_BUFFER)))
#else
! switch ((pgStatPid = fork_process()))
#endif
{
case -1:
ereport(LOG,
(errmsg("could not fork statistics buffer: %m")));
return 0;
***
*** 603,612
#ifndef EXEC_BACKEND
case 0:
/* in postmaster child ... */
- #ifdef __BEOS__
- /* Specific beos actions after backend startup */
- beos_backend_startup();
- #endif
/* Close the postmaster's sockets */
ClosePostmasterPorts(false);
--- 591,596
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
[PATCHES] add_missing_from = false
Now that DELETE has a USING clause, we should be able to make add_missing_from=false the default in 8.1; the attached patch implements this and updates the documentation. Barring any objections, I'll apply this tomorrow. -Neil Index: doc/src/sgml/runtime.sgml === RCS file: /var/lib/cvs/pgsql/doc/src/sgml/runtime.sgml,v retrieving revision 1.312 diff -c -r1.312 runtime.sgml *** doc/src/sgml/runtime.sgml 29 Mar 2005 03:01:29 - 1.312 --- doc/src/sgml/runtime.sgml 7 Apr 2005 07:02:30 - *** *** 3553,3567 ! When true, tables that are referenced by a query will be ! automatically added to the FROM clause if not already ! present. The default is true for compatibility with ! previous releases of PostgreSQL. However, this ! behavior is not SQL-standard, and many people dislike it because it ! can mask mistakes (such as referencing a table where you should have ! referenced its alias). Set to false for the SQL-standard ! behavior of rejecting references to tables that are not listed in ! FROM. --- 3553,3577 ! When true, tables that are referenced by a query ! will be automatically added to the FROM clause if ! not already present. This behavior does not comply with the ! SQL standard and many people dislike it because it can mask ! mistakes (such as referencing a table where you should have ! referenced its alias). The default is false. This ! variable can be enabled for compatibility with releases of ! PostgreSQL prior to 8.1, where this behavior ! was allowed by default. ! ! ! ! Note that even when this variable is enabled, a warning ! message will be emitted for each implicit FROM ! entry referenced by a query. Users are encouraged to update ! their applications to not rely on this behavior, by adding all ! tables referenced by a query to the query's FROM ! clause (or its USING clause in the case of ! DELETE). Index: doc/src/sgml/ref/delete.sgml === RCS file: /var/lib/cvs/pgsql/doc/src/sgml/ref/delete.sgml,v retrieving revision 1.23 diff -c -r1.23 delete.sgml *** doc/src/sgml/ref/delete.sgml 7 Apr 2005 01:51:37 - 1.23 --- doc/src/sgml/ref/delete.sgml 7 Apr 2005 06:30:31 - *** *** 148,159 In some cases the join style is easier to write or faster to execute than the sub-select style. - - -If add_missing_from is enabled, any relations -mentioned in the WHERE condition will be -implicitly added to the USING clause. - --- 148,153 Index: doc/src/sgml/ref/select.sgml === RCS file: /var/lib/cvs/pgsql/doc/src/sgml/ref/select.sgml,v retrieving revision 1.82 diff -c -r1.82 select.sgml *** doc/src/sgml/ref/select.sgml 10 Mar 2005 23:21:20 - 1.82 --- doc/src/sgml/ref/select.sgml 7 Apr 2005 06:12:25 - *** *** 1011,1052 ! A less obvious use is to abbreviate a normal ! SELECT from tables: SELECT distributors.* WHERE distributors.name = 'Westward'; - - did | name - -+-- - 108 | Westward - - This works because an implicit FROM item is - added for each table that is referenced in other parts of the - SELECT statement but not mentioned in - FROM. - - - - While this is a convenient shorthand, it's easy to misuse. For - example, the command - - SELECT distributors.* FROM distributors d; - - is probably a mistake; most likely the user meant - - SELECT d.* FROM distributors d; - - rather than the unconstrained join - - SELECT distributors.* FROM distributors d, distributors distributors; ! that he will actually get. To help detect this sort of mistake, ! PostgreSQL will warn if the ! implicit-FROM feature is used in a ! SELECT statement that also contains an explicit ! FROM clause. Also, it is possible to disable ! the implicit-FROM feature by setting the ! parameter to false. --- 1011,1031 ! Note that if a FROM clause is not specified, ! the query cannot reference any database tables. For example, the ! following query is invalid: SELECT distributors.* WHERE distributors.name = 'Westward'; ! PostgreSQL releases prior to ! 8.1 would accept queries of this form, and add an implicit entry ! to the query's FROM clause for each table ! referenced by the query. This is no longer the default behavior, ! because it does not comply with the SQL standard, and is ! considered by many to be error-prone.
