Re: [PATCHES] DTrace probe patch for OS X Leopard
Robert Lor wrote: > Please find the patch attached per this thread > http://archives.postgresql.org/pgsql-hackers/2008-02/msg00912.php Another thing that is concerning me about this new approach is the way the probes are named. For example, we'd now have a call POSTGRESQL_LWLOCK_ACQUIRE() in the code. This does not say we are *tracing* lock aquisition, but it looks like a macro that actually acquires a lock. I understand that these probe names follow some global naming scheme. Is it hard to change that? I'd feel more comfortable with, say, (D)TRACE_POSTGRESQL_LWLOCK_ACQUIRE(). Comments? ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] tuplestore_putvalues()
Neil Conway <[EMAIL PROTECTED]> writes: > Attached is a patch that allows an array of Datums + nulls to be > inserted into a tuplestore without first creating a HeapTuple, per > recent suggestion on -hackers. This avoids making an unnecessary copy. A small thought here: we were jousting recently over a point that came down to whether or not tuplestore kept track of the tupdesc for the tuples it was storing. I can hardly imagine a use-case for a tuplestore in which the tuples don't all have the same tupdesc. I think I dropped tupdesc from tuplestore's original API on the grounds that it wasn't doing anything much with the tupdesc. But now this patch adds back a tuplestore API call that needs the tupdesc. Would it be saner to supply the tupdesc to tuplestore_begin_heap instead, as tuplesort does? I haven't looked at all into what the implications of this would be, either from a performance or number-of-places-to-change standpoint. But it seems worth a bit of investigation while we're touching the code. Other than that issue, the patch seems OK in a quick once-over. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] DTrace probe patch for OS X Leopard
Robert Lor <[EMAIL PROTECTED]> writes: > Peter Eisentraut wrote: >> Is c.h the right place to include this? The probes are only in the backend >> code, so perhaps postgres.h would be more appropriate. Or just include it >> in >> the .c files that need it, of which there are only 3. >> > I think putting probes.h in c.h is the right place. It's true that the > probes are only in the backend now and only in a few files, but in the > future I can foresee probes added to more files in the backend, the > procedural language code or any of the commands (initdb, psql, etc). I agree with Peter. There are a whole lot of include files that are needed by way more than 3 .c files, and yet are not folded into postgres.h. c.h is right out. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Fix for initdb failures on Vista
Dave Page wrote: The attached patch fixes problems reported primarily on Vista, but also on some Windows 2003 and XP installations in which initdb reports that it cannot find postgres.exe. This occurs because of security-related changes implemented in Windows Vista and recent patches on older OS's. When running initdb or pg_ctl we currently create a restricted security token with the Administrators and Power Users groups (and thus their privileges) removed and re-execute the same program using the restricted token. This ensures that the process is run without potentially dangerous privileges no matter what user account it was started from. On Vista and friends however, the default DACL (list of Access Control Entries) used in the restricted token contains Administrators (the group) & System when we run as Administrator, vs. User + System when run as other users. Because we then drop Administrators, we are left with only the System ACE in the DACL, which does not allow us to use CreatePipe()/CreateProcess(). To fix this, when we create the restricted process, we initially start it in suspended mode. We modify it's DACL to explicitly add an ACE for the current user, and then resume the child process. This remains secure because administrative privileges are granted to the groups that we've dropped, not the user itself. I've tested on Vista and XP, but additional testing would be useful (Andrew, Magnus?). Please apply to head, 8.3 and 8.2 This appears to work for initdb. But "make check" fails after the initdb stage, I think because pg_regress doesn't use pg_ctl to start the postmaster. The log just reads "Access is denied'" I don't have too much difficulty with that as long as we stipulate that postgres has to be built, or at least checked, as a non-privileged user (c.f. recent discussion of building RPMs as root). Alternatively, we should also patch pg_regress.c cheers andrew ---(end of broadcast)--- TIP 1: 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
[PATCHES] Logging conflicted queries on deadlocks
Here is a patch to log conflicted queries on deadlocks. Queries are dumped at CONTEXT in the same sorting order as DETAIL messages. Those queries are picked from pg_stat_get_backend_activity, as same as pg_stat_activity, so that users cannot see other user's queries. (It might be better to log all queries in the server log and mask them in the client response, but I'm not sure how to do it...) | ERROR: deadlock detected | DETAIL: Process 3088 waits for ShareLock on transaction 608; blocked by process 2928. | Process 2928 waits for ShareLock on transaction 609; blocked by process 2824. | Process 2824 waits for ShareLock on transaction 610; blocked by process 3088. | CONTEXT: Process 3088: UPDATE test SET i = i WHERE i = 1; | Process 2928: | Process 2824: UPDATE test SET i = i WHERE i = 3; | STATEMENT: UPDATE test SET i = i WHERE i = 1; Alvaro Herrera <[EMAIL PROTECTED]> wrote: > Perhaps it could be shown in CONTEXT, like so: > > I think it's useful to show the PID of each statement, for the case > where there are more than two processes deadlocked. Thanks for response. I bought your suggestion :-) Regards, --- ITAGAKI Takahiro NTT Open Source Software Center deadlock.patch Description: Binary data ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] SRF memory leaks
On Wed, 2008-02-27 at 10:54 -0800, Neil Conway wrote: > Barring any objections, I'll apply this to HEAD and back branches > tonight or tomorrow. Applied to HEAD, REL8_3_STABLE, and REL8_2_STABLE. (I can backpatch further if anyone feels strongly about it.) -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] tuplestore_putvalues()
Attached is a patch that allows an array of Datums + nulls to be
inserted into a tuplestore without first creating a HeapTuple, per
recent suggestion on -hackers. This avoids making an unnecessary copy.
There isn't a really analogous optimization to be applied to tuplesort:
it takes either a TTS, an IndexTuple or a basic Datum, none of which
involve an extra copy.
BTW, I notice that almost all of the callers of the various
tuplestore_put methods switch into the tuplestore's context first. We
could simplify their lives a bit by having the tuplestore remember the
context in which it is allocated and do the switch itself. Perhaps it's
not worth bothering with at this point, though.
-Neil
Index: src/backend/commands/prepare.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/commands/prepare.c,v
retrieving revision 1.80
diff -p -c -r1.80 prepare.c
*** src/backend/commands/prepare.c 1 Jan 2008 19:45:49 - 1.80
--- src/backend/commands/prepare.c 27 Feb 2008 21:55:21 -
*** pg_prepared_statement(PG_FUNCTION_ARGS)
*** 764,770
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- HeapTuple tuple;
Datum values[5];
bool nulls[5];
--- 764,769
*** pg_prepared_statement(PG_FUNCTION_ARGS)
*** 787,797
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
- tuple = heap_form_tuple(tupdesc, values, nulls);
-
/* switch to appropriate context while storing the tuple */
MemoryContextSwitchTo(per_query_ctx);
! tuplestore_puttuple(tupstore, tuple);
}
}
--- 786,794
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
/* switch to appropriate context while storing the tuple */
MemoryContextSwitchTo(per_query_ctx);
! tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
}
Index: src/backend/executor/execQual.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/execQual.c,v
retrieving revision 1.226
diff -p -c -r1.226 execQual.c
*** src/backend/executor/execQual.c 1 Jan 2008 19:45:49 - 1.226
--- src/backend/executor/execQual.c 27 Feb 2008 22:13:10 -
*** ExecMakeTableFunctionResult(ExprState *f
*** 1547,1553
for (;;)
{
Datum result;
- HeapTuple tuple;
CHECK_FOR_INTERRUPTS();
--- 1547,1552
*** ExecMakeTableFunctionResult(ExprState *f
*** 1649,1663
*/
tmptup.t_len = HeapTupleHeaderGetDatumLength(td);
tmptup.t_data = td;
! tuple = &tmptup;
}
else
{
! tuple = heap_form_tuple(tupdesc, &result, &fcinfo.isnull);
}
-
- oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
- tuplestore_puttuple(tupstore, tuple);
MemoryContextSwitchTo(oldcontext);
/*
--- 1648,1662
*/
tmptup.t_len = HeapTupleHeaderGetDatumLength(td);
tmptup.t_data = td;
!
! oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
! tuplestore_puttuple(tupstore, &tmptup);
}
else
{
! oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
! tuplestore_putvalues(tupstore, tupdesc, &result, &fcinfo.isnull);
}
MemoryContextSwitchTo(oldcontext);
/*
*** no_function_result:
*** 1702,1716
int natts = expectedDesc->natts;
Datum *nulldatums;
bool *nullflags;
- HeapTuple tuple;
MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
nulldatums = (Datum *) palloc0(natts * sizeof(Datum));
nullflags = (bool *) palloc(natts * sizeof(bool));
memset(nullflags, true, natts * sizeof(bool));
- tuple = heap_form_tuple(expectedDesc, nulldatums, nullflags);
MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
! tuplestore_puttuple(tupstore, tuple);
}
}
--- 1701,1713
int natts = expectedDesc->natts;
Datum *nulldatums;
bool *nullflags;
MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
nulldatums = (Datum *) palloc0(natts * sizeof(Datum));
nullflags = (bool *) palloc(natts * sizeof(bool));
memset(nullflags, true, natts * sizeof(bool));
MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
! tuplestore_putvalues(tupstore, expectedDesc, nulldatums, nullflags);
}
}
Index: src/backend/utils/mmgr/portalmem.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/mmgr/portalmem.c,v
retrieving revision 1.106
diff -p -c -r1.106 portalmem.c
*** src/backend/utils/mmgr/portalmem.c 1 Jan 2008 19:45:55 - 1.106
--- src/backend/utils/mmgr/portalmem.c 27 Feb 2008 22:03:33 -
**
Re: [PATCHES] DTrace probe patch for OS X Leopard
Peter Eisentraut wrote: Is c.h the right place to include this? The probes are only in the backend code, so perhaps postgres.h would be more appropriate. Or just include it in the .c files that need it, of which there are only 3. I think putting probes.h in c.h is the right place. It's true that the probes are only in the backend now and only in a few files, but in the future I can foresee probes added to more files in the backend, the procedural language code or any of the commands (initdb, psql, etc). Regards, -Robert ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] DTrace probe patch for OS X Leopard
Robert Lor wrote: > dtrace call in src/Makefile is to generate probes.h before any file is > compiled so it can be used in c.h to avoid "probes.h not found" error. > The dtrace call in src/backend/Makefile is only needed for Solaris. Is c.h the right place to include this? The probes are only in the backend code, so perhaps postgres.h would be more appropriate. Or just include it in the .c files that need it, of which there are only 3. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] DTrace probe patch for OS X Leopard
Peter, Robert Lor wrote: Peter Eisentraut wrote: I have reworked your build rules so they look more like the idioms that we already use for other similar cases. This should fix the troubles you describe and others. There are a couple of problems with your updated patch: Based on your patch, I made a few changes and everything works now. Patch attached! Thanks for your help! Regards, -Robert Index: src/Makefile === RCS file: /projects/cvsroot/pgsql/src/Makefile,v retrieving revision 1.42 diff -u -3 -p -r1.42 Makefile --- src/Makefile 21 Aug 2007 01:11:12 - 1.42 +++ src/Makefile 28 Feb 2008 21:41:49 - @@ -14,6 +14,9 @@ include Makefile.global all install installdirs uninstall distprep: +ifeq ($(enable_dtrace), yes) + $(MAKE) -C backend ../../src/include/utils/probes.h +endif $(MAKE) -C port $@ $(MAKE) -C timezone $@ $(MAKE) -C backend $@ Index: src/backend/Makefile === RCS file: /projects/cvsroot/pgsql/src/backend/Makefile,v retrieving revision 1.127 diff -u -3 -p -r1.127 Makefile --- src/backend/Makefile 26 Feb 2008 14:42:27 - 1.127 +++ src/backend/Makefile 28 Feb 2008 21:41:49 - @@ -20,9 +20,11 @@ SUBDIRS = access bootstrap catalog parse include $(srcdir)/common.mk +ifeq ($(PORTNAME), solaris) ifeq ($(enable_dtrace), yes) LOCALOBJS += utils/probes.o endif +endif OBJS = $(SUBDIROBJS) $(LOCALOBJS) $(top_builddir)/src/port/libpgport_srv.a @@ -122,6 +124,9 @@ $(srcdir)/parser/parse.h: parser/gram.y utils/fmgroids.h: utils/Gen_fmgrtab.sh $(top_srcdir)/src/include/catalog/pg_proc.h $(MAKE) -C utils fmgroids.h +utils/probes.h: utils/probes.d + $(MAKE) -C utils probes.h + # Make symlinks for these headers in the include directory. That way # we can cut down on the -I options. Also, a symlink is automatically # up to date when we update the base file. @@ -135,9 +140,15 @@ $(top_builddir)/src/include/utils/fmgroi cd $(dir $@) && rm -f $(notdir $@) && \ $(LN_S) ../../../$(subdir)/utils/fmgroids.h . +$(top_builddir)/src/include/utils/probes.h: utils/probes.h + cd $(dir $@) && rm -f $(notdir $@) && \ + $(LN_S) ../../../$(subdir)/utils/probes.h . + +ifeq ($(PORTNAME), solaris) utils/probes.o: utils/probes.d $(SUBDIROBJS) $(DTRACE) $(DTRACEFLAGS) -G -s $(call expand_subsys,$^) -o $@ +endif ## Index: src/backend/access/transam/xact.c === RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v retrieving revision 1.257 diff -u -3 -p -r1.257 xact.c --- src/backend/access/transam/xact.c 15 Jan 2008 18:56:59 - 1.257 +++ src/backend/access/transam/xact.c 28 Feb 2008 21:41:49 - @@ -1479,7 +1479,7 @@ StartTransaction(void) Assert(MyProc->backendId == vxid.backendId); MyProc->lxid = vxid.localTransactionId; - PG_TRACE1(transaction__start, vxid.localTransactionId); + POSTGRESQL_TRANSACTION_START(vxid.localTransactionId); /* * set transaction_timestamp() (a/k/a now()). We want this to be the same @@ -1604,7 +1604,7 @@ CommitTransaction(void) */ latestXid = RecordTransactionCommit(); - PG_TRACE1(transaction__commit, MyProc->lxid); + POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid); /* * Let others know about no transaction in progress by me. Note that this @@ -1990,7 +1990,7 @@ AbortTransaction(void) */ latestXid = RecordTransactionAbort(false); - PG_TRACE1(transaction__abort, MyProc->lxid); + POSTGRESQL_TRANSACTION_ABORT(MyProc->lxid); /* * Let others know about no transaction in progress by me. Note that this Index: src/backend/storage/lmgr/lock.c === RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v retrieving revision 1.181 diff -u -3 -p -r1.181 lock.c --- src/backend/storage/lmgr/lock.c 2 Feb 2008 22:26:17 - 1.181 +++ src/backend/storage/lmgr/lock.c 28 Feb 2008 21:41:50 - @@ -787,11 +787,11 @@ LockAcquire(const LOCKTAG *locktag, * Sleep till someone wakes me up. */ - PG_TRACE2(lock__startwait, locktag->locktag_field2, lockmode); + POSTGRESQL_LOCK_STARTWAIT(locktag->locktag_field2, lockmode); WaitOnLock(locallock, owner); - PG_TRACE2(lock__endwait, locktag->locktag_field2, lockmode); + POSTGRESQL_LOCK_ENDWAIT(locktag->locktag_field2, lockmode); /* * NOTE: do not do any material change of state between here and Index: src/backend/storage/lmgr/lwlock.c === RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v retrieving revision 1.50 diff -u -3 -p -r1.50 lwlock.c --- src/backend/storage/lmgr/lwlock.c 1 Jan 2008 19:45:52 - 1.50 +++ src/backend/storage/lmgr/lwlock.c 28 Feb 2008 21:41:50 - @@ -447,7 +447,7 @@ LWLockAcquire(LWLo
Re: [PATCHES] lc_time and localized dates
Peter Eisentraut napsal(a): Am Dienstag, 26. Februar 2008 schrieb Zdenek Kotala: But how you handle situation when you have multi language application which needs correct output for all languages? You cannot use any of the pattern because it will be wrong for some group of languages. This is what you get when you copy a proprietary, poorly specified interface. The to_char functions are supposed to be Oracle-compatible, so we need to check what Oracle does. Whether that is useful in practice is a bit secondary. I see. It is ok from this point of view. I'm beginning to think, if you want formatting functions that are more sane, stable, and standardized, export sprintf and strftime to PostgreSQL Yes, it should solve a problem. Zdenek ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
