Charles Duffy [EMAIL PROTECTED] wrote
We came up with this patch in response to a problem reported to us by a
client. They had a query which took an unacceptably long time to respond
to a cancel request (SIGINT). The client uses 8.1.4, so the patch is
against that.
How long is that
On Mon, 12 Jun 2006, Tom Lane wrote:
Qingqing Zhou [EMAIL PROTECTED] writes:
AllocateFile() and AllocateDir() should return the control to the caller
since we might want to upgrade the elevel.
That is not what we do for upgrading errors. Use a critical section in
a caller that doesn't
Tom Lane [EMAIL PROTECTED] wrote
True, but current code just check the return value of AllocateABC() then
decide to upgrade elevel to FATAL.
Where?
In ValidatePgVersion(), FindMyDatabase(), etc -- though in practice this
error can hardly happen anyway.
Regards,
Qingqing
AllocateFile() and AllocateDir() should return the control to the caller
since we might want to upgrade the elevel.
BTW: Seems this premature-elog-error problem (we talked about it in an old
thread and fix the dynahash code) also exists in some other places (e.g. the
assign_hook functions), but
Jim C. Nasby [EMAIL PROTECTED] wrote
Maybe a comment patch would be in order to prevent future confusion?
Not really needed because the buffer/README access rule#1 has already said
that ...
Regards,
Qingqing
---(end of broadcast)---
TIP 5:
Qingqing Zhou [EMAIL PROTECTED] wrote
The overall performance improvement might be marginal but why not if it is
right. What I cares is the correctness. As I understand, the orginal code
puts a shared lock (1) to prevent the vacuum process to move tuples around
so the hint bits change may
Attached is a patch to remove the lock protection for
HeapTupleSatisfiesVacuum() in index code. The basic idea is: if we can do
a pin/lock/unlock sequence on a page, then without locking again, we are
gauranteed that there is no vacuum process acting on the same page.
According to buffer pool
Tom Lane [EMAIL PROTECTED] wrote
This patch scares the heck out of me. You need to offer some pretty
compelling performance reasons before I'd accept any part of it,
Changing a buffer you hold no lock on is a recipe for disaster.
Me too, in fact :-(.
The overall performance improvement
In spirit of incremental improvement, here is a trivial fix to use %p
instead %d to print pointer. -- maybe we should use context name better.
Regards,
Qingqing? printf_patch.diff
Index: nodeAppend.c
===
RCS file:
Fix a format warning in fd.c when FDDEBUG is on.
By the way (to save a thread): What's the rationale of designing resowner
APIs like this:
ResourceOwnerEnlargeABC();
ResourceOwnerRememberABC();
Is that because sometimes we don't allow any elog in
ResourceOwnerRememberABC()?
Regards,
Martijn van Oosterhout kleptog@svana.org wrote
What it does behave normally for the first 50 tuples of any node, but
after that it starts sampling at ever increasing intervals, the
intervals controlled by an exponential function.
I got two questions after scanning the patch:
(1) For a node
On Sun, 7 May 2006, Bruce Momjian wrote:
Leave 'em alone. That code has zero field validation, and should
certainly not get shipped until it's survived a beta-test cycle.
Uh, this is a bug fix, and the patch I am asking about is not the Win32
semaphore reimplementation but a more
Bruce Momjian pgman@candle.pha.pa.us wrote
Sorry for the late reply. Maybe more intensive tests are needed? Since
this bug seems could not lead data corruption, we can wait till next bug
report and let the user test it then decide to apply?
Well we did have a bug report by Peter Brant,
Magnus Hagander [EMAIL PROTECTED] wrote
I've been working on getting the full backend to compile and run using
Visual C++ instead of mingw/gcc, and have made some good progress.
This is great!
*) Add s_lock implementation based on InterlockedCompareExchange instead
of assembly code (gcc
Tom Lane [EMAIL PROTECTED] wrote
Applied with minor corrections --- mostly, being careful to pfree the
temporary StringInfos so we don't have an indefinite memory leak during
xlog replay.
Right. By the way, your worry about buffer overflow is proved in
xact_desc_commit()/xact_desc_abort().
Neil Conway [EMAIL PROTECTED] wrote
Does it make sense to implementation or for the tid type?
I can't get too excited about it, but I wouldn't object if someone can
see a use for it.
There might be another usage of tid or . Consider a heap with one middle
page is broken, I could save
Neil Conway [EMAIL PROTECTED] wrote
Attached is a patch that implements for the tid type. This is based
on a patch submitted by Mark Kirkwood in October of 2005. I added some
regression tests, avoided unnecessarily renumbering a few OIDs in the
system catalogs, and bumped the catversion.
Tom Lane [EMAIL PROTECTED] wrote
Yes, the patch is wrong as-is because it may lose uncompleted fsyncs.
But I think that we could just add the AbsorbFsyncRequests call in the
fsync loop and not worry about trying to avoid doing extra fsyncs.
Another possibility is to make the copied list as
ITAGAKI Takahiro [EMAIL PROTECTED] wrote
Attached is a patch that fixes overflow of bgwriter's file-fsync request
queue.
while ((entry = (PendingOperationEntry *) hash_seq_search(hstat)) !=
NULL)
{
+ if (i = count)
+elog(ERROR, pendingOpsTable corrupted);
+
+
ITAGAKI Takahiro [EMAIL PROTECTED] wrote
AbsorbFsyncRequests will be called during the fsync loop in my patch,
so new files might be added to pendingOpsTable and they will be removed
from the table *before* writing the pages belonging to them.
So I changed it to copy the contents of
The following patch changes BgWriterCommLock to a spinlock. To confirm to
the spinlock coding rules(though not needed since we are in critical
section), rewrite the requests array into a circular one, which will
reduce the lock time when the bgwriter absorbs the requests. A
byproduct-benefit is
On Sun, 8 Jan 2006, Tom Lane wrote:
Why is this a good idea?
In spirit of incremental improvement:
(1) The spinlock itself are light weight than the LWLock here and we can
reduce the lock contention a little bit in AbsorbFsyncRequests();
(2) Don't need the CRITICAL SECTION in
On Sun, 8 Jan 2006, Tom Lane wrote:
(1) The spinlock itself are light weight than the LWLock here and we
can reduce the lock contention a little bit in AbsorbFsyncRequests();
Spinlock-based coding is inherently much more fragile than LWLock-based
coding. I'm against changing things in
On Sun, 8 Jan 2006, Tom Lane wrote:
If you want the bgwriter to keep working in the face of an out-of-memory
condition in the hashtable, I think you'd have to change the coding so
that it takes requests one at a time from the queue.
Patched version will issue ERROR instead of PANIC at this
On Sun, 8 Jan 2006, Tom Lane wrote:
I don't see any problem here that urgently needs solving. If we ever
see any reports of out-of-memory failures in the bgwriter, then it'll
be time to worry about this, but I think it quite unlikely that we
ever will. (Even if we do, a simpler answer
Tom Lane [EMAIL PROTECTED] wrote
I noticed that shmem.c holds ShmemIndexLock considerably longer than any
other spinlock is held, and across operations that could theoretically
fail (hashtable manipulations). This doesn't matter a lot in the Unix
code because only the postmaster ever
There is no LWLock protecting the spinlock in UnpinBuffer(), so we need do
so ourselves. I also checked other NoHoldOff spinlock, seems they are ok.
Regards,
Qingqing
Index: bufmgr.c
===
RCS file:
On Wed, 28 Dec 2005, Tom Lane wrote:
Because the code uses _NoHoldoff, there won't be any check of
InterruptPending in that segment of code.
I guess the danger I claimed may not really happen because of the
ImmediateInterruptOK variable. Since it is almost always false (except
reading inputs
Tom Lane [EMAIL PROTECTED] wrote
So I'm thinking the right answer is to make all the spinlock macros be
the equivalent of the NoHoldoff case. It's reasonable for LWLockAcquire
to do a HOLD_INTERRUPTS, but I don't see the justification for doing it
at the spinlock level.
I agree on this.
On Wed, 28 Dec 2005, Tom Lane wrote:
Qingqing Zhou [EMAIL PROTECTED] writes:
I agree on this. But before changing it, we need to inspect those spinlocks
one by one to making sure two things (1) if there is out-of-line-call, make
sure no CHECK_FOR_INTERRUPTS(); (2) ImmediateInterruptsOK
On Sat, 24 Dec 2005, Tom Lane wrote:
Removing these comments entirely, without changing the code they explain,
doesn't strike me as an improvement.
I just checked if we can remove XLOG_NO_TRAN happily, and the conclusion
is that it could bring some benefits (though not much) to our system.
The following patch improves XLOG_NO_TRAN related comments per discussion.
Regards,
Qingqing
---
Index: backend/access/transam/clog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/clog.c,v
retrieving revision
On Sat, 24 Dec 2005, Tom Lane wrote:
Removing these comments entirely, without changing the code they explain,
doesn't strike me as an improvement.
Well, I still kept the XLOG_NO_TRAN and collect repeated comments to
xlog.h where it is defined.
Regards,
Qingqing
Add a note to Win32 gettimeofday() emulation.
Regards,
Qingqing
---
Index: gettimeofday.c
===
RCS file: /projects/cvsroot/pgsql/src/port/gettimeofday.c,v
retrieving revision 1.6
diff -c -r1.6 gettimeofday.c
*** gettimeofday.c
This is half of an item in TODO list. I patch this because now pgbench is
threaded in Win32, so it is better to check thread safety of libpq.dll.
Patch ecpg could be done in a similar way, but I am not sure how we will
use this function there ...
Regards,
Qingqing
---
Index: fe-connect.c
Tom Lane [EMAIL PROTECTED] wrote
Is there a point in checking the thread-safety of
libpq when you can't check the thread-safety of libc?
Exactly :-(
Regards,
Qingqing
---(end of broadcast)---
TIP 5: don't forget to increase your free space
On Thu, 1 Dec 2005, Tom Lane wrote:
Argh, I'm an idiot ... a big part of the problem with the original
fork-based pgbench is that I'd rearranged the startup code without
noticing a data dependency. You can't initialize the default scripts
until you've gotten the correct value of tps by
Qingqing Zhou [EMAIL PROTECTED] wrote
I've threaded it in Win32 ...
Another thing is that if unix does fork() and windows does _beginthread(),
then there will be some potential problem. For example, you have to be
careful to add global variables ...
Regards,
Qingqing
On Sun, 13 Nov 2005, Peter Eisentraut wrote:
! (*stmt)-command = (char *)query;
(*stmt)-connection = connection;
(*stmt)-lineno = lineno;
(*stmt)-compat = compat;
This sort of cheating should be avoided.
According to Peter's comment, revised patch is attached. It
Add missing const qualifier to (char *) parameters in ECPG. Per reported
by Tomasz Ostrowski.
Regards,
Qingqing
---
Index: ecpglib/descriptor.c
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/ecpg/ecpglib/descriptor.c,v
On Sat, 12 Nov 2005, Peter Eisentraut wrote:
Qingqing Zhou wrote:
***
*** 149,155
if (!(*stmt = (struct statement *) ECPGalloc(sizeof(struct
statement), lineno))) return false;
! (*stmt)-command = query;
(*stmt)-connection = connection;
(*stmt
Tom Lane [EMAIL PROTECTED] wrote
If we were willing to invent the varlena2 datum format then we could
save four bytes per numeric, plus reduce numeric's alignment requirement
from int to short which would probably save another byte per value on
average. I'm not sure that that's worth doing
I wrote a small script to make PostgreSQL bug report easier. It prints the
uname, pg_config and try to backtrace all core dumps in data directory.
Also, it provides a mode which you can interact with gdb. Put this shell
script together in PG bin directory with other executables.
It does not cover
On Thu, 27 Oct 2005, Tom Lane wrote:
Does that actually work? On most of the platforms I use, gdb doesn't
get far with only a core file, you have to tell it which executable file
to reference.
Yeah, it works. There is a command in the script like:
file ./postgres
Worse, many
This patch improves the win32 CHECK_FOR_INTERRUPTS() performance by
testing if any unblocked signals are queued before check
pgwin32_signal_event. This avoids an unnecessary system call.
Regards,
Qingqing
---
Index: backend/port/win32/signal.c
Martijn van Oosterhout kleptog@svana.org wrote
Below is a patch that tries to determine (for linux) if the pread is
emulated or not (using an autoconf test), and if so use it instead of
lseek.
Just FYI, this was proposed 2 years ago:
Neil Conway [EMAIL PROTECTED] wrote
Other inconsistencies:
- the patch prints a trailing comma when remote-port is the empty
string
- the patch separates the elements of the ereport() line with commas,
whereas the original behavior was to use a single space as a separator
- the original
On Mon, 3 Oct 2005, Neil Conway wrote:
Why does the patch change behavior when
- port-remote_port is the empty string
Ooops, this wording change is inconsistent with Log_connections, so I
reverted it. Updated patch is attached.
- or, when end.tv_sec is negative
I can only imagine
This patches checks MAXIMUM_ALIGNOF and endian to make sure that the
database file used is compatible with the server version. We use
SHORT_ALIGNOF, INT_ALIGNOF, DOUBLE_ALIGNOF and MAXIMUM_ALIGNOF (which is
just the largest of these) to align columns within a row (see att_align())
or rows within
Tom Lane [EMAIL PROTECTED] wrote
There's not much need to check endianness explicitly, since the
pg_control_version check will surely fail if there's an endianness
discrepancy (not to mention the other checks on pg_control fields).
Oh, right. So for the same reason, is it safe to remove
Trivial patch to cleanup log_disconnections() function.
Regards,
Qingqing
---
Index: postgres.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.463
diff -u -r1.463 postgres.c
---
Tom Lane [EMAIL PROTECTED] writes:
This is definitely going to tell us a lot more about the compiler's loop
strength reduction algorithms than it is going to tell about the time to
evaluate any one of these expressions in isolation. What's worse, the
compiler would be quite within its
Tom Lane [EMAIL PROTECTED] writes:
That's a contrived example (and if I believed it, I would think that the
right answer is to emit no errcontext if the elevel is less than ERROR).
Yes, I've thought about ignore errcontext by considering elevel. But I
scratched the source code for other uses
Tom Lane [EMAIL PROTECTED] writes
Please exhibit a case in which you feel this is needed.
Suppose I want to print a debug info in parseTypeString() like this:
+ elog(DEBUG1, parse type %s, buf.data);
raw_parsetree_list = raw_parser(buf.data);
Rebuild the server, psql it:
Since we will invoke callback functions unconditionally in errfinish(), so
pts_error_callback() should not report invalid type name without checking
current error status.
Regards,
Qingqing
Index: src/backend/parser/parse_type.c
===
As this thread reports (some sever messages are in Chinese):
http://archives.postgresql.org/pgsql-bugs/2005-07/msg00247.php
a SQL grammar error could crash the error stack.
My explaination is that select; incurs a parse error and this error
message is
supposed to be translated into your
Tom Lane [EMAIL PROTECTED] writes
This is a really ugly solution ... and I don't think it solves the
general problem anyway, since this isn't the only possible error message.
Yeah, it is not a very clean solution. Do you mean the general problem is
prevent recursive error reporting because
Revised patch to avoid lost signals before signaling mechanism is set up
in Win32. This was tested by plus a line:
Sleep(10*1000);
in the front of pgwin32_signal_initialize().
Regards,
Qingqing
Index: src/port/kill.c
===
Neil Conway [EMAIL PROTECTED] writes
On Thu, 2005-06-02 at 22:15 -0400, Tom Lane wrote:
Does that look better or worse to you?
I agree the patch's format is a bit off. What about
mi btree (i), tablespace testspace
PRIMARY KEY is currently separated from the rest of the index
This patch simplified Win32 signaling code per discussion in hackers. In
this implementation, each process will have a named (by its pid) mutex,
named shared memory area and named event in global namespace. The process
is
sending/receiving signals as the following:
(*) the process who kill the
Magnus Hagander [EMAIL PROTECTED] writes
Looking at this patch reminds me of another discussion we had:
Signals sent by the postmaster *before the signaling code is running in
the child* has to be handled.
This is handled in the curernt code by creating the pipe in the
postmaster and then
In thread:
http://archives.postgresql.org/pgsql-hackers-win32/2004-11/msg00010.php
---
Do we actually need to pass the handle, or could the subprocess reopen
the pipe for itself?
Nope, we need to pass the handle. Only one process can be the
server-side of the pipe, and once the postmaster has
Change elog(ERROR) to Assert(false) for two reasons:
(1) unrecognized hash action code could hardly really happen;
(2) even if it could happen, elog(ERROR) won't save us since in many places
we have to check the return code of hash_search() and decide the error
level.
On a separate matter, can
Tom Lane [EMAIL PROTECTED] writes
Qingqing Zhou [EMAIL PROTECTED] writes:
No. Remember that in most installations Assert() is a no-op.
Well, I still suggest to change it :(
The only chance elog(ERROR, unrecognized hash action code) could be
triggered is the *unbelievable* programming typo
Fix a comment typo in getrusage.c and add a comment in gettimeofday.c.
Index: getrusage.c
===
RCS file: /projects/cvsroot/pgsql/src/port/getrusage.c,v
retrieving revision 1.8
diff -c -r1.8 getrusage.c
*** getrusage.c 31 Dec 2004
Tom Lane [EMAIL PROTECTED] writes
I thought your version was even more so :-(. I've applied the attached
patch instead.
That's too bad :-( but the happy part is that it is fixed now.
Regards,
Qingqing
---(end of broadcast)---
TIP 8:
The original comment of why we are safe without protection of critical
section is confusing.
Index: xlog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.187
diff -c -r1.187 xlog.c
Remove an unused variable waitingForSignal in /storage/lmgr/proc.c.
I guess it was there because orignally PG uses singals to implement
ProcWaitForSignal() stuff, so we may need to remember the process status.
But now we don't need it.
Index: proc.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)---
Tom Lane [EMAIL PROTECTED] writes
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
Tom Lane [EMAIL PROTECTED] writes
Actually, on further thought, there's a really simple solution that
we've used elsewhere: make sure you have the resource you need *before*
you get into the critical section of code. I've applied the attached
revised patch.
Oh, that's the one - why didn't
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
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);
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
!gettext_noop(Create new tables with OIDs by default.),
without?
Neil Conway [EMAIL PROTECTED]
This patch makes default_with_oids disabled by default, per earlier
discussion, and updates the documentation accordingly. I might have
missed a few spots in the documentation that implicitly
(new_status);
awaitedLock = locallock;
On Wed, 9 Mar 2005 06:42 pm, Qingqing Zhou wrote:
off-by-one is true, but I am not sure if the revised code is faster.
sprintf() need the extra job to parse the format. In win32, I am sure it
is
much slower.
Neil Conway [EMAIL PROTECTED
off-by-one is true, but I am not sure if the revised code is faster.
sprintf() need the extra job to parse the format. In win32, I am sure it is
much slower.
Neil Conway [EMAIL PROTECTED] news:[EMAIL PROTECTED]
This patch refactors some code in WaitOnLock slightly. The old code was
slow,
77 matches
Mail list logo
