Re: [HACKERS] [PATCHES] GIN improvements
On Tue, 2008-07-08 at 14:51 -0400, Tom Lane wrote: > I'd still like to take a look. I was tasked with reviewing this for the current commit fest, although so far I've just been working on grokking the rest of the GIN code. But if you'd like to review it instead, that's fine with me. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] A GUC variable to replace PGBE_ACTIVITY_SIZE
On Mon, 2008-06-23 at 03:52 +1000, Thomas Lee wrote: > * Should it be possible to set this new variable via a command-line > option ala shared_buffers? I would say not: most parameters cannot be set by special command-line parameters, and this one is not important enough to warrant special treatment. Instead, "--track_activity_query_size=x" can be used as-is. > * I'm not exactly sure about the best way to add unit/regr tests for > this change. AFAICS there isn't an easy way. > * I'm also not sure what's required in the way of updates to the > documentation. postgresql.conf.sample and doc/src/sgml/config.sgml should be updated at a minimum. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] small typo in DTrace docs
On Fri, 2008-06-13 at 01:49 -0300, Euler Taveira de Oliveira wrote: > Attached is a small patch to fix some typos in probes.d path. Applied, thanks. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Minor pedantry for "help" text
On Mon, 2008-06-09 at 00:25 -0700, Neil Conway wrote: > Attached is a patch that makes some minor changes to the text emitted by > the new "help" command. Applied to HEAD. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] SQL: table function support
On Tue, 2008-06-10 at 06:42 +0200, Pavel Stehule wrote: > internally is table functions implemenation identical with SRF. It's not the internals that I'm concerned about. > Semantically is far - user's doesn't specify return type (what is from > PostgreSQL), but specifies return table, what is more natural. What > more - for users is transparent chaotic joice betwen "SETOF RECORD" > for multicolumns sets and "SETOF type". Well, I'd just like to see some thought about how this *entire* feature ought to work, rather than just adding new knobs and syntax variants incrementally and seemingly at random. Just because it happens to be in the standard isn't really a compelling reason to make an overly-complex part of the system even more complicated, IMHO... -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] SQL: table function support
On Tue, 2008-06-03 at 13:03 +0200, Pavel Stehule wrote: > this patch add support of table functions syntax like ANSI SQL 2003. I'm not necessarily opposed to this, but I wonder if we really need *more* syntax variants for declaring set-returning functions. The existing patchwork of features is confusing enough as it is... -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Minor pedantry for "help" text
Attached is a patch that makes some minor changes to the text emitted by the new "help" command. Previous output: postgres=# help You are using psql, the command-line interface to PostgreSQL. \? for psql help \h or \help for SQL help \g or ";" to execute a query \q to quit psql \copyright to view the copyright postgres=# New output: postgres=# help You are using psql, the command-line interface to PostgreSQL. Type: \copyright for distribution terms \h for help with SQL commands \? for help with psql commands \g or terminate with semicolon to execute query \q to quit postgres=# The newlines in the previous text were inconsistent with psql error message style elsewhere, aside from being distracting. The advice on commands to enter next was also just emitted, without actually telling the user that these are possible inputs. Essentially the text was a regression from the text we've always used in the startup banner, so I just re-instituted the old text. -Neil, doing his best to suppress his aesthetic objection to having "help" be valid psql input in the first place *** a/src/bin/psql/mainloop.c --- b/src/bin/psql/mainloop.c *** *** 177,188 MainLoop(FILE *source) (line[4] == '\0' || line[4] == ';' || isspace((unsigned char) line[4]))) { free(line); ! puts(_("\nYou are using psql, the command-line interface to PostgreSQL.")); ! puts(_("\t\\? for psql help")); ! puts(_("\t\\h or \\help for SQL help\n")); ! puts(_("\t\\g or \";\" to execute a query")); ! puts(_("\t\\q to quit psql\n")); ! puts(_("\t\\copyright to view the copyright\n")); fflush(stdout); continue; --- 177,188 (line[4] == '\0' || line[4] == ';' || isspace((unsigned char) line[4]))) { free(line); ! puts(_("You are using psql, the command-line interface to PostgreSQL.")); ! printf(_("Type: \\copyright for distribution terms\n" ! " \\h for help with SQL commands\n" ! " \\? for help with psql commands\n" ! " \\g or terminate with semicolon to execute query\n" ! " \\q to quit\n")); fflush(stdout); continue; -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] TRUNCATE TABLE with IDENTITY
On Fri, 2008-05-16 at 19:41 -0400, Tom Lane wrote: > Applied with corrections. Most notably, since ALTER SEQUENCE RESTART > is nontransactional like most other ALTER SEQUENCE operations, I > rearranged things to try to ensure that foreseeable failures like > deadlock and lack of permissions would be detected before TRUNCATE > starts to issue any RESTART commands. Ugh. The fact that the RESTART IDENTITY part of TRUNCATE is non-transactional is a pretty unsightly wort. I would also quarrel with your addition to the docs that suggests this is only an issue "in practice" if TRUNCATE RESTART IDENTITY is used in a transaction block: unpredictable failures (such as OOM or query cancellation) can certainly occur in practice, and would be very disruptive (e.g. if the sequence values are stored into a column with a UNIQUE constraint, it would break all inserting transactions until the DBA intervenes). I wonder if it would be possible to make the sequence operations performed by TRUNCATE transactional: while the TRUNCATE remains uncommitted, it should be okay to block concurrent access to the sequence. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Integer datetime by default
On Tue, 2008-03-25 at 12:54 -0700, Neil Conway wrote: > Barring any objections, I'll apply this to HEAD tomorrow. Applied to HEAD. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Doc patch for DTrace
On Tue, 2008-03-25 at 15:45 -0500, Robert Lor wrote: > Attached is the doc patch for the recent DTrace changes Applied, thanks. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
On Tue, 2008-03-18 at 16:19 -0300, Alvaro Herrera wrote: > The other approach, of course, is to just keep all the code in tqual.c > and not create a separate module at all. Opinions? I prefer to keep > them separate, but I'm not wedded to it if there's any strong reason not > to do it. Also, the line is currently blurred because some users of > snapshots mess with the internals directly (setting snapshot->curcid), > but we could clean that up and make it so that those become external > interface users too. Sounds like a good idea to me -- +1 on keeping the code in two separate files, and moving snapshot users toward using the external interface. Given that there's no functional change here, I don't see anything to stop this patch being applied sooner rather than later... -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] tuplestore_putvalues()
On Sat, 2008-03-22 at 21:35 -0400, Tom Lane wrote: > After a quick read, looks sane except for one stylistic gripe: > in exec_stmt_return_next, you added an initialization of tuple = NULL > in order to remove a couple of lines like > > tuple = NULL; /* keep compiler quiet */ > > I think this is not good coding style. The point of not having the > initialization is so that the compiler will warn us if there are > any paths through the function in which we fail to set "tuple". > You've just disabled that bit of early-warning error detection. > It's better if each switch arm has to set tuple for itself. > (If only a minority of them needed to do it, I might think > differently, but in this case I'd vote for sticking with the > way it's being done.) I agree with your general reasoning, but in this case, it seemed cleaner to initialize "tuple" when it is declared: only 2 of the 6 branches in the function actually initialize tuple to a non-NULL value. I think the warn-about-uninitialized-value argument doesn't hold much water in this particular case, either: each branch of the function either makes use of "tuple", or does not. When "tuple" is utilized, we want to push it into the tuplestore; otherwise, we want to do nothing. Since "tuple" is only used in two cases, it seems cleaner to me to have the default be "do nothing", and only deviate from it where needed. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] tuplestore_putvalues()
On Thu, 2008-02-28 at 16:37 -0800, Neil Conway wrote: > 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. Applied to HEAD. -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
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] SRF memory leaks
On Wed, 2008-02-27 at 15:07 -0500, Tom Lane wrote: > Negative refcount does not prove that the SRF itself hasn't > still got a pointer to the tupdesc. That sounds quite bizarre. The SRF has already finished execution at this point, so keeping a pointer to the tupledesc around would only make sense if you wanted to use that tupledesc on a *subsequent* invocation of the SRF. The SRF would need to store the pointer in a static variable, too, and it wouldn't have an easy way to distinguish between repeated calls to the SRF within the same query and in different queries (since in the latter case the TupleDesc will be long gone anyway). I can't see why anyone would want to do this. > Can't we fix it so that the tupdesc is allocated in the new special > context (at least in the primary code paths), and then no explicit > free is needed? As I said earlier, the tupdesc is explicitly allocated in the per-query context by the SRFs themselves. If by "primary code paths", you mean "SRFs in the main source tree", then sure, we can make arbitrary changes to those. That won't help out-of-tree SRFs though. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] SRF memory leaks
On Tue, 2008-02-26 at 12:09 -0800, Neil Conway wrote: > I'd like to apply this change to back branches reasonably soon, so if > you have a better way to do the FreeTupleDesc() hack, let me know. Barring any objections, I'll apply this to HEAD and back branches tonight or tomorrow. -Neil ---(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] SRF memory leaks
On Tue, 2008-02-26 at 00:17 -0800, Neil Conway wrote: > You didn't comment on my proposed solution (FreeTupleDesc() iff refcount > == -1). Attached is a revised version of this patch. It makes the FreeTupleDesc() change described above, and fixes a bug: in SRF_RETURN_DONE(), we need to be sure to switch back to the caller's context before deleting the multi_call_ctx, since some SRFs (e.g. dblink) call SRF_RETURN_DONE() while still inside the multi_call_ctx. I'd like to apply this change to back branches reasonably soon, so if you have a better way to do the FreeTupleDesc() hack, let me know. -Neil Index: src/backend/executor/nodeFunctionscan.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeFunctionscan.c,v retrieving revision 1.45 diff -p -c -r1.45 nodeFunctionscan.c *** src/backend/executor/nodeFunctionscan.c 1 Jan 2008 19:45:49 - 1.45 --- src/backend/executor/nodeFunctionscan.c 26 Feb 2008 19:04:49 - *** FunctionNext(FunctionScanState *node) *** 77,83 --- 77,93 * do it always. */ if (funcTupdesc) + { tupledesc_match(node->tupdesc, funcTupdesc); + + /* + * If it is a dynamically-allocated TupleDesc, free it: it is + * typically allocated in the EState's per-query context, so we + * must avoid leaking it on rescan. + */ + if (funcTupdesc->tdrefcount == -1) + FreeTupleDesc(funcTupdesc); + } } /* Index: src/backend/utils/fmgr/funcapi.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/fmgr/funcapi.c,v retrieving revision 1.37 diff -p -c -r1.37 funcapi.c *** src/backend/utils/fmgr/funcapi.c 1 Jan 2008 19:45:53 - 1.37 --- src/backend/utils/fmgr/funcapi.c 26 Feb 2008 19:32:47 - *** *** 23,28 --- 23,29 #include "utils/array.h" #include "utils/builtins.h" #include "utils/lsyscache.h" + #include "utils/memutils.h" #include "utils/syscache.h" #include "utils/typcache.h" *** init_MultiFuncCall(PG_FUNCTION_ARGS) *** 63,75 /* * First call */ ! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo->resultinfo; /* * Allocate suitably long-lived space and zero it */ retval = (FuncCallContext *) ! MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt, sizeof(FuncCallContext)); /* --- 64,86 /* * First call */ ! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo->resultinfo; ! MemoryContext multi_call_ctx; ! ! /* ! * Create a suitably long-lived context to hold cross-call data ! */ ! multi_call_ctx = AllocSetContextCreate(fcinfo->flinfo->fn_mcxt, ! "SRF multi-call context", ! ALLOCSET_SMALL_MINSIZE, ! ALLOCSET_SMALL_INITSIZE, ! ALLOCSET_SMALL_MAXSIZE); /* * Allocate suitably long-lived space and zero it */ retval = (FuncCallContext *) ! MemoryContextAllocZero(multi_call_ctx, sizeof(FuncCallContext)); /* *** init_MultiFuncCall(PG_FUNCTION_ARGS) *** 81,87 retval->user_fctx = NULL; retval->attinmeta = NULL; retval->tuple_desc = NULL; ! retval->multi_call_memory_ctx = fcinfo->flinfo->fn_mcxt; /* * save the pointer for cross-call use --- 92,98 retval->user_fctx = NULL; retval->attinmeta = NULL; retval->tuple_desc = NULL; ! retval->multi_call_memory_ctx = multi_call_ctx; /* * save the pointer for cross-call use *** shutdown_MultiFuncCall(Datum arg) *** 168,180 flinfo->fn_extra = NULL; /* ! * Caller is responsible to free up memory for individual struct elements ! * other than att_in_funcinfo and elements. */ ! if (funcctx->attinmeta != NULL) ! pfree(funcctx->attinmeta); ! ! pfree(funcctx); } --- 179,189 flinfo->fn_extra = NULL; /* ! * Delete context that holds all multi-call data, including the ! * FuncCallContext itself */ ! MemoryContextSwitchTo(flinfo->fn_mcxt); ! MemoryContextDelete(funcctx->multi_call_memory_ctx); } ---(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
Re: [PATCHES] SRF memory leaks
On Tue, 2008-02-26 at 03:13 -0500, Tom Lane wrote: > "It's OK in the built-in SRFs" is disastrously different from "It's OK". Right, I never said that, I was just commenting on your view that the predominant use-case for SRFs is returning refcounted tupdescs. You didn't comment on my proposed solution (FreeTupleDesc() iff refcount == -1). ISTM that we *need* to free the TupleDesc in at least some cases, in order to defend against the practice of explicitly allocating the TupleDesc in the per-query context. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] SRF memory leaks
On Mon, 2008-02-25 at 21:00 -0500, Tom Lane wrote: > I find this part of the patch to be a seriously bad idea. > nodeFunctionscan has no right to assume that the function has returned > an expendable tupdesc; indeed, I would think that the other case is > more nearly what's expected by the API for SRFs. AFAICS this is not true of any of the SRFs in the backend, which always return expendable tupdescs. > A safer fix would be to try to make the tupdesc be in the new > multi_call_ctx when it's being created by the funcapi.c code. funcapi.c doesn't create the tupdesc, though: it is created by user code, and merely assigned to a field of the RSI (note that the TupleDesc in question is the one in the ReturnSetInfo, not in FuncCallContext.) A minor detail is that the lifetime of the tupdesc also needs to exceed the lifetime of the multi_call_ctx, at least as currently implemented. >From looking at the existing SRFs in the backend, the TupleDesc is always *explicitly* allocated in the estate's per-query context, so it will leak unless freed. Perhaps it would be sufficient to FreeTupleDesc iff tupdesc->refcount == -1? BTW, I'm thinking of changing the various SRFs that make allocations in the EState's per-query context to instead use the SRF's multi_call_ctx. This means the memory will be reclaimed sooner and avoids possible memory leaks. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
[PATCHES] SRF memory leaks
Attached is a patch that fixes the SRF memory leaks I recently reported on -hackers[1]. The patch creates a distinct memory context for the SRF's "multi_call_memory_ctx", and then deletes that context when the SRF finishes. This ensures that any user allocations made in this context are reclaimed. The patch also frees a TupleDesc that was leaked in the per-query context when nodeFunctionscan was rescanned. Along the way, it also fixes a leak in shutdown_MultiFuncCall() ("attinmeta" was freed, but its palloc'd fields were not.) It would be possible to allocate the TupleDesc in the multi-call memory context, but it doesn't seem worth bothering about to me (since it would require passing the context from the RSI down to the FuncCallContext). I also didn't try to have multiple SRFs in the same subquery block use the same context -- that seems like a lot of pain for little gain. Comments welcome -- I think this fix should be applied to back branches. -Neil [1] http://markmail.org/message/45hekjinzl3e5i6q Index: source/src/backend/executor/nodeFunctionscan.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeFunctionscan.c,v retrieving revision 1.45 diff -p -c -r1.45 nodeFunctionscan.c *** source/src/backend/executor/nodeFunctionscan.c 1 Jan 2008 19:45:49 - 1.45 --- source/src/backend/executor/nodeFunctionscan.c 25 Feb 2008 19:45:40 - *** FunctionNext(FunctionScanState *node) *** 77,83 --- 77,86 * do it always. */ if (funcTupdesc) + { tupledesc_match(node->tupdesc, funcTupdesc); + FreeTupleDesc(funcTupdesc); + } } /* Index: source/src/backend/utils/fmgr/funcapi.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/fmgr/funcapi.c,v retrieving revision 1.37 diff -p -c -r1.37 funcapi.c *** source/src/backend/utils/fmgr/funcapi.c 1 Jan 2008 19:45:53 - 1.37 --- source/src/backend/utils/fmgr/funcapi.c 25 Feb 2008 20:09:34 - *** *** 23,28 --- 23,29 #include "utils/array.h" #include "utils/builtins.h" #include "utils/lsyscache.h" + #include "utils/memutils.h" #include "utils/syscache.h" #include "utils/typcache.h" *** init_MultiFuncCall(PG_FUNCTION_ARGS) *** 63,75 /* * First call */ ! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo->resultinfo; /* * Allocate suitably long-lived space and zero it */ retval = (FuncCallContext *) ! MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt, sizeof(FuncCallContext)); /* --- 64,86 /* * First call */ ! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo->resultinfo; ! MemoryContext multi_call_ctx; ! ! /* ! * Create a suitably long-lived context to hold cross-call data ! */ ! multi_call_ctx = AllocSetContextCreate(fcinfo->flinfo->fn_mcxt, ! "SRF multi-call context", ! ALLOCSET_SMALL_MINSIZE, ! ALLOCSET_SMALL_INITSIZE, ! ALLOCSET_SMALL_MAXSIZE); /* * Allocate suitably long-lived space and zero it */ retval = (FuncCallContext *) ! MemoryContextAllocZero(multi_call_ctx, sizeof(FuncCallContext)); /* *** init_MultiFuncCall(PG_FUNCTION_ARGS) *** 81,87 retval->user_fctx = NULL; retval->attinmeta = NULL; retval->tuple_desc = NULL; ! retval->multi_call_memory_ctx = fcinfo->flinfo->fn_mcxt; /* * save the pointer for cross-call use --- 92,98 retval->user_fctx = NULL; retval->attinmeta = NULL; retval->tuple_desc = NULL; ! retval->multi_call_memory_ctx = multi_call_ctx; /* * save the pointer for cross-call use *** shutdown_MultiFuncCall(Datum arg) *** 168,180 flinfo->fn_extra = NULL; /* ! * Caller is responsible to free up memory for individual struct elements ! * other than att_in_funcinfo and elements. */ ! if (funcctx->attinmeta != NULL) ! pfree(funcctx->attinmeta); ! ! pfree(funcctx); } --- 179,188 flinfo->fn_extra = NULL; /* ! * Delete context that holds all multi-call data, including the ! * FuncCallContext itself */ ! MemoryContextDelete(funcctx->multi_call_memory_ctx); } ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] NUMERIC key word
On Tue, 2008-01-29 at 13:20 -0500, Tom Lane wrote: > The reason it was kept was to override the search path --- unqualified > NUMERIC will always be taken as pg_catalog.numeric even if you have some > other type "numeric" in front of it. It should be possible to implement this behavior without requiring NUMERIC to be a keyword, though. -Neil ---(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] Proposed patch: synchronized_scanning GUC variable
On Mon, 2008-01-28 at 17:27 +1100, Russell Smith wrote: > Can somebody explain why it's important to load with > synchronized_scanning off? *Loading* with synchronized scanning off is not important (and is not implemented by the patch). *Dumping* with synchronized scanning off is necessary to ensure that the order of the rows in the pg_dump matches the on-disk order of the rows in the table, which is important if you want to preserve the clustering of the table data on restore. See the -hackers thread: http://markmail.org/message/qbytsco6oj2qkxsa -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Proposed patch: synchronized_scanning GUC variable
On Sun, 2008-01-27 at 21:54 +, Gregory Stark wrote: > I liked the "synchronized_sequential_scans" idea myself. I think that's a bit too long. How about "synchronized_scans", or "synchronized_seqscans"? -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
On Sun, 2008-01-27 at 12:36 -0500, Tom Lane wrote: > Both of the above arguments hold water only if we implement compatible > *semantics*, not merely syntax, so I find them unconvincing at this > stage. How are the semantics of the proposed patch incompatible with the SQL spec or the implementations in other systems? The proposed patch is a *subset* of the functionality in the SQL spec, but it isn't incompatible with it as far as I know (recursive and non-recursive WITH are distinct features). > > (3) It avoids the need to repeat subqueries multiple times in the main > > query, which can make queries more concise. Defining subqueries outside > > the main SELECT body can also have readability advantages. > > Views fix that too. Sure, if you're willing to resort to DDL, and lose most of the conciseness / readability gain. > The point is that when you break people's apps you'll be able to point > to some real increment in functionality to justify it. If your application uses an identifier that is a reserved word in SQL-92 and in pretty much all major databases, I don't think you have much cause for grievance when it becomes a reserved word in Postgres -- the writing has been on the wall for a while. Do you have any reason to think that "WITH" is a particularly common table or column name, by the way? Note also the keywords.c hack in 8.3 for the WITH keyword means that pg_dump already treats WITH as a reserved word, so most dumps should load without changes. > With the patch as it stands you'd essentially be saying "we're going > to cause you pain now for benefit later", which is a hard selling > proposition. Again, the readability + compatibility arguments are non-zero benefits, IMHO. But your argument is essentially a public-relations one ("it will look bad if..."), which I don't find very convincing. If we explain that WITH is a reserved word per SQL spec and is part of the planned support for recursive queries (whether in 8.4 or later), I can't see very many users being annoyed. (Compare that with the irritation we may well see from the removal of implicit casts in 8.3, which will break *far* more applications, for a benefit that many users will no doubt find rather hard to observe.) -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
On Sun, 2008-01-27 at 09:17 +, Gregory Stark wrote: > Tom's feeling at the time was that even though it was providing something from > the standard, it wasn't actually allowing the user to do anything he couldn't > before. I think this feature has value: (1) This is SQL-standard syntax (and not even wacko syntax, at that!), and there is merit in implementing it on those grounds alone. (2) It is supported by DB2, MS SQL and Oracle, so there is a further compatibility argument to be made. (3) It avoids the need to repeat subqueries multiple times in the main query, which can make queries more concise. Defining subqueries outside the main SELECT body can also have readability advantages. > If it doesn't provide any additional expressive capabilities then I > think he didn't like taking "with" as a reserved word. Note that we can make WITH a type_func_name_keyword, rather than a full-on reserved_keyword, which reduces the force of this argument slightly. If we're going to implement recursive queries eventually (which we almost surely will, whether in 8.4 or a future release), we'll need to make WITH more reserved at some point anyway, so I don't see much to be gained in the long run by delaying it. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Attached is an updated version of Greg Stark's patch to add support for the non-recursive variant of the SQL99 WITH clause[1]. I haven't looked at the actual functionality of the patch yet (which is quite trivial) -- I just fixed up bitrot and the like. I also removed support for RECURSIVE and the search/cycle clause, along with their associated keywords -- the current patch doesn't approach anything close to adding support for the non-recursive case, so it seems like a net loss to add additional keywords for no gain in functionality. Remaining work is to review the guts of the patch (which shouldn't take long), and write documentation and regression tests. I'm personally hoping to see this get into the tree fairly early in the 8.4 cycle, pending discussion of course. -Neil [1] http://archives.postgresql.org/pgsql-patches/2007-03/msg00139.php http://archives.postgresql.org/pgsql-patches/2007-04/msg00055.php Index: src/backend/nodes/copyfuncs.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/nodes/copyfuncs.c,v retrieving revision 1.387 diff -p -c -r1.387 copyfuncs.c *** src/backend/nodes/copyfuncs.c 1 Jan 2008 19:45:50 - 1.387 --- src/backend/nodes/copyfuncs.c 27 Jan 2008 07:05:29 - *** _copySelectStmt(SelectStmt *from) *** 1930,1935 --- 1930,1936 COPY_NODE_FIELD(limitOffset); COPY_NODE_FIELD(limitCount); COPY_NODE_FIELD(lockingClause); + COPY_NODE_FIELD(with_cte_list); COPY_SCALAR_FIELD(op); COPY_SCALAR_FIELD(all); COPY_NODE_FIELD(larg); Index: src/backend/nodes/equalfuncs.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/nodes/equalfuncs.c,v retrieving revision 1.317 diff -p -c -r1.317 equalfuncs.c *** src/backend/nodes/equalfuncs.c 1 Jan 2008 19:45:50 - 1.317 --- src/backend/nodes/equalfuncs.c 27 Jan 2008 07:05:29 - *** _equalSelectStmt(SelectStmt *a, SelectSt *** 821,826 --- 821,827 COMPARE_NODE_FIELD(limitOffset); COMPARE_NODE_FIELD(limitCount); COMPARE_NODE_FIELD(lockingClause); + COMPARE_NODE_FIELD(with_cte_list); COMPARE_SCALAR_FIELD(op); COMPARE_SCALAR_FIELD(all); COMPARE_NODE_FIELD(larg); Index: src/backend/nodes/outfuncs.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/nodes/outfuncs.c,v retrieving revision 1.322 diff -p -c -r1.322 outfuncs.c *** src/backend/nodes/outfuncs.c 9 Jan 2008 08:46:44 - 1.322 --- src/backend/nodes/outfuncs.c 27 Jan 2008 07:05:29 - *** _outSelectStmt(StringInfo str, SelectStm *** 1599,1604 --- 1599,1605 WRITE_NODE_FIELD(limitOffset); WRITE_NODE_FIELD(limitCount); WRITE_NODE_FIELD(lockingClause); + WRITE_NODE_FIELD(with_cte_list); WRITE_ENUM_FIELD(op, SetOperation); WRITE_BOOL_FIELD(all); WRITE_NODE_FIELD(larg); Index: src/backend/parser/analyze.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/parser/analyze.c,v retrieving revision 1.371 diff -p -c -r1.371 analyze.c *** src/backend/parser/analyze.c 1 Jan 2008 19:45:50 - 1.371 --- src/backend/parser/analyze.c 27 Jan 2008 07:23:39 - *** transformSelectStmt(ParseState *pstate, *** 688,693 --- 688,696 /* make FOR UPDATE/FOR SHARE info available to addRangeTableEntry */ pstate->p_locking_clause = stmt->lockingClause; + /* process the WITH clause (pull CTEs into the pstate's ctenamespace) */ + transformWithClause(pstate, stmt->with_cte_list); + /* process the FROM clause */ transformFromClause(pstate, stmt->fromClause); Index: src/backend/parser/gram.y === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/parser/gram.y,v retrieving revision 2.605 diff -p -c -r2.605 gram.y *** src/backend/parser/gram.y 1 Jan 2008 19:45:50 - 2.605 --- src/backend/parser/gram.y 27 Jan 2008 07:21:28 - *** static List *extractArgTypes(List *param *** 103,109 static SelectStmt *findLeftmostSelect(SelectStmt *node); static void insertSelectOptions(SelectStmt *stmt, List *sortClause, List *lockingClause, ! Node *limitOffset, Node *limitCount); static Node *makeSetOp(SetOperation op, bool all, Node *larg, Node *rarg); static Node *doNegate(Node *n, int location); static void doNegateFloat(Value *v); --- 103,110 static SelectStmt *findLeftmostSelect(SelectStmt *node); static void insertSelectOptions(SelectStmt *stmt, List *sortClause, List *lockingClause, ! Node *limitOffset, Node *limitCount, ! List *with_cte_list); static Node *makeSetOp(SetOperation op, bool all, Node *larg, Node *rarg); static Node *doNegate(Node *n, int location); static void doNegateFloat(Value *v);
Re: [PATCHES] Revised xml memory allocation patch
On Mon, 2008-01-14 at 20:55 -0500, Tom Lane wrote: > Any thoughts whether to apply or not? Seems like a much more sane approach to me -- +1. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Fix for _outAgg()
On Tue, 2008-01-08 at 23:08 -0500, Tom Lane wrote: > Hmm, I think that must be my fault, but I'm not sure how it got by me > ... I'm usually pretty careful about adding outfuncs support when I add > a node field. Patch looks good, please apply. Applied to HEAD. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] Fix for _outAgg()
Attached is a patch which fixes an oversight in _outAgg(): the "grpColIdx" and "grpOperators" fields of the Agg struct were not emitted by _outAgg(). I don't see any good reason to omit this information. Note that while the "grpOperators" field was added during the 8.3 devel cycle, the "grpColIdx" field has been around since '02 (without outfuncs support). So I can backpatch that portion of the patch to back branches if anyone feels strongly. -Neil Index: src/backend/nodes/outfuncs.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/nodes/outfuncs.c,v retrieving revision 1.321 diff -p -c -r1.321 outfuncs.c *** src/backend/nodes/outfuncs.c 7 Jan 2008 21:33:10 - 1.321 --- src/backend/nodes/outfuncs.c 9 Jan 2008 03:01:33 - *** _outHashJoin(StringInfo str, HashJoin *n *** 501,512 --- 501,523 static void _outAgg(StringInfo str, Agg *node) { + int i; + WRITE_NODE_TYPE("AGG"); _outPlanInfo(str, (Plan *) node); WRITE_ENUM_FIELD(aggstrategy, AggStrategy); WRITE_INT_FIELD(numCols); + + appendStringInfo(str, " :grpColIdx"); + for (i = 0; i < node->numCols; i++) + appendStringInfo(str, " %d", node->grpColIdx[i]); + + appendStringInfo(str, " :grpOperators"); + for (i = 0; i < node->numCols; i++) + appendStringInfo(str, " %u", node->grpOperators[i]); + WRITE_LONG_FIELD(numGroups); } ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] pgbench - startup delay
On Mon, 2007-12-10 at 19:12 -0500, Greg Smith wrote: > I just poked around the > documentation a bit and I didn't find anything that cleared up which > options you can pass from a client http://www.postgresql.org/docs/8.2/static/libpq-envars.html Which says only "PGOPTIONS sets additional run-time options for the PostgreSQL server." This could probably be elaborated upon -- for the list of options accepted, see PostgresMain() in tcop/postgres.c Perhaps one of the slightly unfortunate consequences of the postmaster => postgres merge is that there is less of a clear distinction between "postmaster options" and "postgres" options... -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pgbench - startup delay
On Mon, 2007-12-10 at 19:27 +, Dave Page wrote: > Whilst doing some profiling of the server I found it useful to add an > option to pgbench to introduce a delay between client connection setup > and the start of the benchmark itself to allow me time to attach the > profiler to one of the backends. "postgres -W n" already does this. It is more flexible to put this functionality in the backend that in individual client apps anyway. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] A minor typo fix on pg_standby docs
Applied, thanks. -Neil On Fri, 2007-12-07 at 18:48 +1100, FAST PostgreSQL wrote: > Rgds, > Arul Shaji > ---(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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Avoid needless copy in nodeMaterial
On Tue, 2007-10-16 at 00:34 -0400, Tom Lane wrote: > Seems like this needs more comments about what's happening, rather > than less ... Fair point. > Also, it looks to me like the plan node's own resultslot might never be > assigned to at all, when the subplan returns zero rows. Does this > corner case still work correctly? ISTM the node's own result slot wouldn't be assigned to in this case regardless: (nodeMaterial.c, circa 116) outerslot = ExecProcNode(outerNode); if (TupIsNull(outerslot)) { node->eof_underlying = true; return NULL; } There's no requirement that we must assign to the result slot, AFAICS. -Neil ---(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
[PATCHES] Avoid needless copy in nodeMaterial
Attached is a patch that avoids a needless copy of the result tuple in nodeMaterial, in the case that we don't have a previously-materialized tuple to return. We can just return the TTS produced by executing our child node, rather than returning a copy of it. I didn't bother pulling the MinimalTuple out of "outerslot" and stuffing it back into the nodeMaterial's result slot, as AFAICS that is not necessary. Although I suppose you could make a cleanliness argument that that would be worth doing instead. (This is 8.4 material...) -Neil Index: source/src/backend/executor/nodeMaterial.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeMaterial.c,v retrieving revision 1.59 diff -p -c -r1.59 nodeMaterial.c *** source/src/backend/executor/nodeMaterial.c 21 May 2007 17:57:33 - 1.59 --- source/src/backend/executor/nodeMaterial.c 16 Oct 2007 04:00:46 - *** ExecMaterial(MaterialState *node) *** 98,103 --- 98,105 eof_tuplestore = true; } + ExecClearTuple(slot); + /* * If necessary, try to fetch another row from the subplan. * *** ExecMaterial(MaterialState *node) *** 124,147 } /* ! * Append returned tuple to tuplestore. NOTE: because the tuplestore ! * is certainly in EOF state, its read position will move forward over ! * the added tuple. This is what we want. */ if (tuplestorestate) tuplestore_puttupleslot(tuplestorestate, outerslot); ! /* ! * And return a copy of the tuple. (XXX couldn't we just return the ! * outerslot?) ! */ ! return ExecCopySlot(slot, outerslot); } ! /* ! * Nothing left ... ! */ ! return ExecClearTuple(slot); } /* --- 126,143 } /* ! * Append a copy of the returned tuple to tuplestore. NOTE: because ! * the tuplestore is certainly in EOF state, its read position will ! * move forward over the added tuple. This is what we want. */ if (tuplestorestate) tuplestore_puttupleslot(tuplestorestate, outerslot); ! return outerslot; } ! /* Nothing left, return empty slot */ ! return slot; } /* ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] tsearch limitations doc correction
On Tue, 2007-10-09 at 19:31 +0100, Heikki Linnakangas wrote: [...] Applied, thanks. -Neil ---(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] minor compiler warning in libpg/fe-secure.c on win32 (msvc)
On Tue, 2007-10-02 at 23:45 +0200, Hannes Eder wrote: > while rebuilding the entire project I ran across following warning: > > .\src\interfaces\libpq\fe-secure.c(593): warning C4101: 'fp' : > unreferenced local variable Applied, thanks. BTW, CC'ing hackers is not necessary for trivial patches like this. -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] too many variants of relation_open
On Fri, 2007-09-28 at 10:02 +0100, Heikki Linnakangas wrote: > Well, yes it could, but why? Keeping them separate looks slightly more > readable to me, and the change could break a lot of external modules for > no good reason. I agree: it also complicates the common case (calling relation_open() and waiting to acquire for a lock). If anything, you could perhaps refactor relation_open_nowait() to be implemented in terms of relation_open() (first try to get the lock, then do a relation_open(rel, NoLock)), but since you'd only be saving a handful of duplicated lines, it hardly seems worth it. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Implementation of aggregate functions
The pgsql-patches mailing list is for submitting modifications to Postgres, not for asking these sorts of questions. On Thu, 2007-20-09 at 06:49 +0100, sayali k wrote: > I am keen in implementing certain additional aggregate > functions like percentage, standard deviation etc. I > would like to know the complexity of this and also the > parts of the code which would have to be modified for > the same, stddev() already exists. Take a look at the implementation of the existing aggregate functions, which can be found in src/backend/utils/adt (float.c, numeric.c, etc.) There is no need to modify the core Postgres backend code to add new aggregate functions, of course. I suggest doing a modicum of research first, and then asking on pgsql-hackers if you have more specific questions about Postgres internals. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pg_next_dst_boundary optimization
On Wed, 2007-09-19 at 12:25 +0100, Heikki Linnakangas wrote: > It would be nice to slip this into 8.3... +1 from me: the tzcode merge is pretty obviously 8.4 material, but it would be nice to get this perf tweak in for 8.3. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Incorrect results from corr()
On Tue, 2007-09-18 at 18:27 -0700, Neil Conway wrote: > The builtin corr() aggregate doesn't produce the correct results in some > circumstances. Applied to HEAD and REL8_2_STABLE. -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
[PATCHES] Incorrect results from corr()
The builtin corr() aggregate doesn't produce the correct results in some circumstances. Per the SQL spec, corr(x, y) is defined as equivalent to covar_pop(x, y) / (stddev_pop(x) * stddev_pop(y)). postgres=# create table t1 (x float8, y float8); CREATE TABLE postgres=# copy t1 from stdin with csv; 0.940839937888086,0.539830380585045 0.84795232815668,0.396385048050433 0.601479615084827,0.85123967528 0.785623408854008,0.302559469360858 0.829138438683003,0.0211085784249008 0.926528611686081,0.315794581547379 0.25934984581545,0.609216409735382 0.976522764191031,0.877208305988461 \. postgres=# select corr(x, y) from t1; corr --- 0.214150892978763 (1 row) postgres=# select covar_pop(x, y) / (stddev_pop(x) * stddev_pop(y)) from t1; ?column? -0.214150892978763 (1 row) With the attached patch, we get the expected results: postgres=# select corr(x, y) from t1; corr -0.214150892978763 (1 row) Credit: Jie Zhang at Greenplum and Gavin Sherry for reporting the issue. Barring any objections, I'll apply this to HEAD and 8.2 later tonight or tomorrow. -Neil Index: src/backend/utils/adt/float.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/float.c,v retrieving revision 1.150 diff -p -c -r1.150 float.c *** src/backend/utils/adt/float.c 5 Jun 2007 21:31:06 - 1.150 --- src/backend/utils/adt/float.c 19 Sep 2007 00:51:27 - *** float8_corr(PG_FUNCTION_ARGS) *** 2274,2281 if (numeratorX <= 0 || numeratorY <= 0) PG_RETURN_NULL(); ! PG_RETURN_FLOAT8(sqrt((numeratorXY * numeratorXY) / ! (numeratorX * numeratorY))); } Datum --- 2274,2280 if (numeratorX <= 0 || numeratorY <= 0) PG_RETURN_NULL(); ! PG_RETURN_FLOAT8(numeratorXY / sqrt(numeratorX * numeratorY)); } Datum ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Default location for docbook stylesheets in Gentoo
On Thu, 2007-08-09 at 04:08 +1000, Brendan Jurd wrote: > This patch adds the default location for the DocBook DSSSL stylesheets > in gentoo's package system to the configure script. FYI, patches should typically be submitted as context diffs. For the specific case of configure, you should submit patches against the source file (configure.in), not the generated file (configure). -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Memory leak in nodeAgg
On Mon, 2007-08-06 at 14:21 -0700, Neil Conway wrote: > Attached is a patch that fixes a gradual memory leak in ExecReScanAgg(), > when the AGG_HASHED strategy is used Applied to HEAD, and backpatched back to 7.4. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] MemoryContextStats tweak: show tree structure
On Mon, 2007-06-08 at 17:48 -0700, Neil Conway wrote: > Obviously this is just for debugging, but I've found it useful while > looking at some memory-related issues. Any comments or objections to > including this in HEAD? Applied, with an indentation of two spaces per level. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] MemoryContextStats tweak: show tree structure
On Mon, 2007-08-06 at 20:56 -0400, Tom Lane wrote: > Seems reasonable except I'd vote for less indentation --- the lines are > too long already. How do you feel about 1 or 2 spaces per level? Sure, 2 spaces per level is fine with me (1 makes it a bit hard to see the tree structure, IMHO). -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] MemoryContextStats tweak: show tree structure
Previously, MemoryContextStats() simply emitted a line of output for each MemoryContext. This is fine, but it makes it difficult to see the shape of the MemoryContext hierarchy. Attached is a trivial patch to indent each context by "4 * level" spaces, where "level" is the depth of the node within the subtree printed by MemoryContextStats(). For example, suppose we have three contexts beneath TopMemoryContext: TopMemoryContext (...) FooContext (...) BarContext (...) BazContext (...) With the patch, these might be printed as: TopMemoryContext (...) FooContext (...) BarContext (...) BazContext (...) Assuming that's the parent/child relationship between them, of course. Obviously this is just for debugging, but I've found it useful while looking at some memory-related issues. Any comments or objections to including this in HEAD? -Neil Index: source/src/backend/utils/mmgr/aset.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/mmgr/aset.c,v retrieving revision 1.72 diff -p -c -r1.72 aset.c *** source/src/backend/utils/mmgr/aset.c 30 Apr 2007 00:12:08 - 1.72 --- source/src/backend/utils/mmgr/aset.c 7 Aug 2007 00:29:41 - *** static void AllocSetReset(MemoryContext *** 214,220 static void AllocSetDelete(MemoryContext context); static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer); static bool AllocSetIsEmpty(MemoryContext context); ! static void AllocSetStats(MemoryContext context); #ifdef MEMORY_CONTEXT_CHECKING static void AllocSetCheck(MemoryContext context); --- 214,220 static void AllocSetDelete(MemoryContext context); static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer); static bool AllocSetIsEmpty(MemoryContext context); ! static void AllocSetStats(MemoryContext context, int level); #ifdef MEMORY_CONTEXT_CHECKING static void AllocSetCheck(MemoryContext context); *** AllocSetIsEmpty(MemoryContext context) *** 1034,1040 * Displays stats about memory consumption of an allocset. */ static void ! AllocSetStats(MemoryContext context) { AllocSet set = (AllocSet) context; long nblocks = 0; --- 1034,1040 * Displays stats about memory consumption of an allocset. */ static void ! AllocSetStats(MemoryContext context, int level) { AllocSet set = (AllocSet) context; long nblocks = 0; *** AllocSetStats(MemoryContext context) *** 1044,1049 --- 1044,1050 AllocBlock block; AllocChunk chunk; int fidx; + int i; for (block = set->blocks; block != NULL; block = block->next) { *** AllocSetStats(MemoryContext context) *** 1060,1065 --- 1061,1070 freespace += chunk->size + ALLOC_CHUNKHDRSZ; } } + + for (i = 0; i < level; i++) + fprintf(stderr, ""); + fprintf(stderr, "%s: %lu total in %ld blocks; %lu free (%ld chunks); %lu used\n", set->header.name, totalspace, nblocks, freespace, nchunks, Index: source/src/backend/utils/mmgr/mcxt.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/mmgr/mcxt.c,v retrieving revision 1.61 diff -p -c -r1.61 mcxt.c *** source/src/backend/utils/mmgr/mcxt.c 25 Jul 2007 12:22:52 - 1.61 --- source/src/backend/utils/mmgr/mcxt.c 7 Aug 2007 00:30:26 - *** MemoryContext CurTransactionContext = NU *** 49,54 --- 49,56 /* This is a transient link to the active portal's memory context: */ MemoryContext PortalContext = NULL; + static void MemoryContextStatsInternal(MemoryContext context, int level); + /* * EXPORTED ROUTINES * *** MemoryContextIsEmpty(MemoryContext conte *** 321,336 void MemoryContextStats(MemoryContext context) { MemoryContext child; AssertArg(MemoryContextIsValid(context)); ! (*context->methods->stats) (context); for (child = context->firstchild; child != NULL; child = child->nextchild) ! MemoryContextStats(child); } - /* * MemoryContextCheck * Check all chunks in the named context. --- 323,343 void MemoryContextStats(MemoryContext context) { + MemoryContextStatsInternal(context, 0); + } + + static void + MemoryContextStatsInternal(MemoryContext context, int level) + { MemoryContext child; AssertArg(MemoryContextIsValid(context)); ! (*context->methods->stats) (context, level); for (child = context->firstchild; child != NULL; child = child->nextchild) ! MemoryContextStatsInternal(child, level + 1); } /* * MemoryContextCheck * Check all chunks in the named context. Index: source/src/include/nodes/memnodes.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/n
Re: [PATCHES] Memory leak in nodeAgg
On Mon, 2007-08-06 at 18:52 -0400, Tom Lane wrote: > Hmm. Good catch, but I can't help wondering if this is just the tip > of the iceberg. Should *every* MemoryContextReset be > MemoryContextResetAndDeleteChildren? Yeah, the same thought occurred to me. Certainly having the current behavior as the default is error-prone: it's quite easy to leak child contexts on Reset. Perhaps we could redefine Reset to mean ResetAndDeleteChildren, and add another name for the current Reset functionality. ResetAndPreserveChildren, maybe? > If we redefined MemoryContextReset to be the same as > MemoryContextResetAndDeleteChildren, it'd be possible to keep the > headers for child contexts in their parent context, thus easing > traffic in TopMemoryContext, and perhaps saving a few pfree cycles > when resetting the parent The fact that MemoryContextCreate allocates the context header in TopMemoryContext has always made me uneasy, so getting rid of that would be nice. I wonder if there's not at least *one* place that depends on the current Reset behavior, though... > Anyone want to investigate what happens if we make MemoryContextReset > the same as MemoryContextResetAndDeleteChildren? Sure, I'll take a look, but I'll apply the attached patch in the mean time (above cleanup is probably 8.4 material anyway). -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
[PATCHES] Memory leak in nodeAgg
Attached is a patch that fixes a gradual memory leak in ExecReScanAgg(), when the AGG_HASHED strategy is used: * the aggregation hash table is allocated in a newly-created sub-context of the agg's aggcontext * MemoryContextReset() resets the memory allocated in child contexts, but not the child contexts themselves * ExecReScanAgg() builds a brand-new hash table, which allocates a brand-new sub-context, thus leaking the header for the previous hashtable sub-context The patch fixes this by using MemoryContextDeleteAndResetChildren(). (I briefly looked at other call-sites of hash_create() to see if this problem exists elsewhere, but I didn't see anything obvious.) We run into the leak quite easily at Truviso; with a sufficiently long-lived query in vanilla Postgres, you should be able to reproduce the same problem. Credit: Sailesh Krishnamurthy at Truviso for diagnosing the cause of the leak. -Neil Index: src/backend/executor/nodeAgg.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeAgg.c,v retrieving revision 1.152 diff -p -c -r1.152 nodeAgg.c *** src/backend/executor/nodeAgg.c 2 Apr 2007 03:49:38 - 1.152 --- src/backend/executor/nodeAgg.c 6 Aug 2007 19:29:11 - *** ExecReScanAgg(AggState *node, ExprContex *** 1646,1653 MemSet(econtext->ecxt_aggvalues, 0, sizeof(Datum) * node->numaggs); MemSet(econtext->ecxt_aggnulls, 0, sizeof(bool) * node->numaggs); ! /* Release all temp storage */ ! MemoryContextReset(node->aggcontext); if (((Agg *) node->ss.ps.plan)->aggstrategy == AGG_HASHED) { --- 1646,1657 MemSet(econtext->ecxt_aggvalues, 0, sizeof(Datum) * node->numaggs); MemSet(econtext->ecxt_aggnulls, 0, sizeof(bool) * node->numaggs); ! /* ! * Release all temp storage. Note that in the AGG_HASHED case the agg ! * hash table is allocated in a sub-context, so we need to use ! * MemoryContextResetAndDeleteChildren() to reclaim that storage. ! */ ! MemoryContextResetAndDeleteChildren(node->aggcontext); if (((Agg *) node->ss.ps.plan)->aggstrategy == AGG_HASHED) { ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Memory leak in tuplestore_end()
On Wed, 2007-08-01 at 16:49 -0700, Neil Conway wrote: > Attached is a patch which fixes a memory leak in tuplestore_end(). Applied to HEAD, and to back branches as far back as 7.4. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] Memory leak in tuplestore_end()
Attached is a patch which fixes a memory leak in tuplestore_end(). Barring any objections, I'll apply this to HEAD and back branches tomorrow. -Neil Index: source/src/backend/utils/sort/tuplestore.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/sort/tuplestore.c,v retrieving revision 1.33 diff -p -c -r1.33 tuplestore.c *** source/src/backend/utils/sort/tuplestore.c 7 Jun 2007 19:19:57 - 1.33 --- source/src/backend/utils/sort/tuplestore.c 1 Aug 2007 23:45:24 - *** tuplestore_end(Tuplestorestate *state) *** 322,327 --- 322,328 pfree(state->memtuples[i]); pfree(state->memtuples); } + pfree(state); } /* ---(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
Re: [PATCHES] RETURN QUERY
On Mon, 2007-23-07 at 23:57 -0700, Neil Conway wrote: > Attached is a patch implementing RETURN QUERY, per earlier discussion Applied to HEAD. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] RETURN QUERY
Attached is a patch implementing RETURN QUERY, per earlier discussion, and based on a patch from Pavel Stehule. Like RETURN NEXT, RETURN QUERY doesn't immediately return from the function, allowing RETURN NEXT and RETURN QUERY to be intermixed in a single function. Barring any objections, I'll apply this tomorrow. -Neil Index: doc/src/sgml/plpgsql.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/plpgsql.sgml,v retrieving revision 1.115 diff -p -c -r1.115 plpgsql.sgml *** doc/src/sgml/plpgsql.sgml 16 Jul 2007 17:01:10 - 1.115 --- doc/src/sgml/plpgsql.sgml 24 Jul 2007 06:40:47 - *** *** 135,141 PL/pgSQL functions can also be declared to return a set, or table, of any data type they can return a single instance of. Such a function generates its output by executing ! RETURN NEXT for each desired element of the result set. --- 135,143 PL/pgSQL functions can also be declared to return a set, or table, of any data type they can return a single instance of. Such a function generates its output by executing ! RETURN NEXT for each desired element of the result ! set, or by using RETURN QUERY to output the result of ! executing a query. *** RETURN expression ! RETURN NEXT RETURN NEXT expression; When a PL/pgSQL function is declared to return SETOF sometype, the procedure to follow is slightly different. In that case, the individual ! items to return are specified in RETURN NEXT ! commands, and then a final RETURN command ! with no argument is used to indicate that the function has ! finished executing. RETURN NEXT can be used ! with both scalar and composite data types; with a composite result ! type, an entire table of results will be returned. ! RETURN NEXT does not actually return from the ! function — it simply saves away the value of the expression. ! Execution then continues with the next statement in ! the PL/pgSQL function. As successive ! RETURN NEXT commands are executed, the result ! set is built up. A final RETURN, which should ! have no argument, causes control to exit the function (or you can ! just let control reach the end of the function). If you declared the function with output parameters, write just RETURN NEXT with no expression. On each ! execution, the current values ! of the output parameter variable(s) will be saved for eventual return ! as a row of the result. ! Note that you must declare the function as returning ! SETOF record when there are ! multiple output parameters, or ! SETOF sometype when there is ! just one output parameter of type sometype, in ! order to create a set-returning function with output parameters. ! Functions that use RETURN NEXT should be ! called in the following fashion: SELECT * FROM some_func(); --- 1351,1419 ! RETURN NEXT and RETURN QUERY ! ! RETURN NEXT ! in PL/PgSQL ! ! ! RETURN QUERY ! in PL/PgSQL ! RETURN NEXT expression; + RETURN QUERY query; When a PL/pgSQL function is declared to return SETOF sometype, the procedure to follow is slightly different. In that case, the individual ! items to return are specified by a sequence of RETURN ! NEXT or RETURN QUERY commands, and ! then a final RETURN command with no argument ! is used to indicate that the function has finished executing. ! RETURN NEXT can be used with both scalar and ! composite data types; with a composite result type, an entire ! table of results will be returned. ! RETURN QUERY appends the results of executing ! a query to the function's result set. RETURN ! NEXT and RETURN QUERY can be freely ! intermixed in a single set-returning function, in which case ! their results will be concatenated. ! RETURN NEXT and RETURN ! QUERY do not actually return from the function — ! they simply append zero or more rows to the function's result ! set. Execution then continues with the next statement in the ! PL/pgSQL function. As successive ! RETURN NEXT or RETURN ! QUERY commands are executed, the result set is built ! up. A final RETURN, which should have no ! argument, causes control to exit the function (or you can just ! let control reach the end of the function). If you declared the function with output parameters, write just RETURN NEXT with no expression. On each ! execution, the current
Re: [PATCHES] execl() sentinel
On Wed, 2007-07-18 at 17:22 -0400, Alvaro Herrera wrote: > I wouldn't know how to look for other variadic functions using NULL > sentinels though. You would need something with more knowledge of C than "grep" has, at any rate. Perhaps you could teach sparse to do this analysis, if it can't do it already... -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] execl() sentinel
On Wed, 2007-07-18 at 16:59 +0200, Peter Eisentraut wrote: > It might be good to check the actual definition of NULL in this case, > however, > before wondering further. Well, the existing coding is plainly wrong, regardless of the NULL implementation used on any given machine (although it will usually work). The simple rule is "you need to cast NULL to a pointer type when passing arguments to a variadic function, or to a function whose prototype is not in scope". So +1 on this patch from me. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
I've applied the latest version of the patch to HEAD. Thanks for the patches, Nikhil and Trevor -- we can take a look at improving INCLUDING CONSTRAINTS for 8.4. On Mon, 2007-16-07 at 15:50 +0530, NikhilS wrote: > I agree, since its a LIKE operation and not inheritance as such, how about > "src_idxoptions", just to make the reference to the source index > clearer? I decided to keep it as "src_options", but we can always change it later. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] bogus unixware compiler warnings
On Mon, 2007-07-16 at 13:11 +0200, Stefan Kaltenbrunner wrote: > I'm slowly working on submitting patches to reduce the amount of (bogus) > compiler warnings generated by various buildfarm members. > > Warthog(the unixware box) generates some 1140 lines of: > > UX:cc: WARNING: debugging and optimization mutually exclusive; -O disabled > UX:cc: WARNING: debugging and optimization mutually exclusive; -O disabled Applied, thanks for the patch. -Neil ---(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] CREATE TABLE LIKE INCLUDING INDEXES support
On Mon, 2007-16-07 at 14:28 +0530, NikhilS wrote: > Guess you want to use "src_options" here to be uniform with usages > elsewhere that you have replaced. Thanks, good catch. > You suppose "src_options" is much more readable than "inhreloptions" > or "inh_idxoptions"? Your call :). Yeah, I'm not necessarily sure that it is. "inhreloptions" made me think of table inheritance, whereas LIKE in general is more of a "source" => "target" copying operation. But I'm not convinced about src_options, either ... suggestions/comments welcome. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Tue, 2007-10-07 at 12:53 +0530, NikhilS wrote: > Yes, in the CREATE..LIKE case, options will be NULL and in the normal > CREATE..INDEX case inhreloptions will be NULL. Note that options is a > List of DefElem entries, whereas inhreloptions is a char pointer. Hmm, right -- ugly. I'll just stick with your approach. BTW, I notice a problem with the patch as implemented: # create table abc (a int, b int); CREATE TABLE # create index abc_a_idx on abc using hash (a); CREATE INDEX # create index abc_a_idx2 on abc (a); CREATE INDEX # create table abc2 (like abc including indexes); CREATE TABLE # \d abc2 Table "public.abc2" Column | Type | Modifiers +-+--- a | integer | b | integer | Indexes: "abc2_a_key" hash (a) This occurs because transformIndexConstraints() eliminates "duplicate" indexes from the index list by looking for two indexes with equal() column lists. This makes some sense for the normal CREATE TABLE case, but the above behavior is certainly objectionable -- when the access method differs it is merely surprising, but when partial indexes are involved it is surely a bug. One way to fix this would be to check for duplicates by comparing all the fields of the two IndexStmts, *except* the index name and "is PK?" status. But that's ugly -- it seems much cleaner to keep index definitions arising from LIKE ... INCLUDING INDEXES in a separate list from the indexes derived from constraints. Attached is a revised patch that does that. Barring any objections, I'll apply this to HEAD tomorrow. -Neil Index: doc/src/sgml/ref/create_table.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/ref/create_table.sgml,v retrieving revision 1.108 diff -p -c -r1.108 create_table.sgml *** doc/src/sgml/ref/create_table.sgml 3 Jun 2007 17:06:03 - 1.108 --- doc/src/sgml/ref/create_table.sgml 16 Jul 2007 05:19:43 - *** PostgreSQL documentation *** 23,29 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE table_name ( [ { column_name data_type [ DEFAULT default_expr ] [ column_constraint [ ... ] ] | table_constraint ! | LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... } [, ... ] ] ) [ INHERITS ( parent_table [, ... ] ) ] --- 23,29 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE table_name ( [ { column_name data_type [ DEFAULT default_expr ] [ column_constraint [ ... ] ] | table_constraint ! | LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ] ... } [, ... ] ] ) [ INHERITS ( parent_table [, ... ] ) ] *** and table *** 237,243 ! LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] The LIKE clause specifies a table from which --- 237,243 ! LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ] The LIKE clause specifies a table from which *** and table *** 266,275 requested, all check constraints are copied. Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause an error is signalled. --- 266,280 requested, all check constraints are copied. + Any indexes on the original table will not be created on the new + table, unless the INCLUDING INDEXES clause is + specified. + + Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause, an error is signalled. Index: src/backend/bootstrap/bootparse.y === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/bootstrap/bootparse.y,v retrieving revision 1.88 diff -p -c -r1.88 bootparse.y *** src/backend/bootstrap/bootparse.y 13 Mar 2007 00:33:39 - 1.88 --- src/backend/bootstrap/bootparse.y 16 Jul 2007 05:19:43 - *** Boot_DeclareIndexStmt: *** 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, false, false, false, false, false, true, false, false); do_end(); --- 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, NULL, false, false, false, false, false, true, false, false); do_end(); *** Boot_DeclareUniqueIndexStmt: *** 270,276 LexIDStr($9), NULL, $11, ! NULL, NIL, true, false, false, false, fa
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Tue, 2007-05-06 at 14:50 +0530, NikhilS wrote: > PFA, a patch which provides the "CREATE .. INCLUDING INDEXES" > functionality. This patch uses the same functionality introduced by > the earlier patches in this thread albeit for the "INCLUDING INDEXES" > case. Attached is a revised version of this patch. Sorry for taking so long to make progress on this: my new job been keeping my busy, and I've recently been ill. This version updates the patch to CVS HEAD and has various fixes and refactoring, including proper docs. I refactored get_opclass_name() into lsyscache.c, but then realized that this means that lsyscache.c will depend on commands/indexcmds.c (for GetDefaultOpClass()), which is arguably improper, so I'm tempted to revert and just duplicate the syscache lookups in both ruleutils.c and parse_utilcmd.c Nikhil: why are both "options" and "inhreloptions" necessary in IndexStmt? Won't at least one be NULL? BTW, I notice that include/defrem.h contains declarations for several different, marginally-related .c files (indexcmds.c, functioncmds.c, operatorcmds.c, aggregatecmds.c, opclasscmds.c, define.c). I'm inclined to separate these declarations into separate header files; any objections to doing that? -Neil Index: doc/src/sgml/ref/create_table.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/ref/create_table.sgml,v retrieving revision 1.108 diff -p -c -r1.108 create_table.sgml *** doc/src/sgml/ref/create_table.sgml 3 Jun 2007 17:06:03 - 1.108 --- doc/src/sgml/ref/create_table.sgml 9 Jul 2007 04:34:40 - *** PostgreSQL documentation *** 23,29 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE table_name ( [ { column_name data_type [ DEFAULT default_expr ] [ column_constraint [ ... ] ] | table_constraint ! | LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... } [, ... ] ] ) [ INHERITS ( parent_table [, ... ] ) ] --- 23,29 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE table_name ( [ { column_name data_type [ DEFAULT default_expr ] [ column_constraint [ ... ] ] | table_constraint ! | LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ] ... } [, ... ] ] ) [ INHERITS ( parent_table [, ... ] ) ] *** and table *** 237,243 ! LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] The LIKE clause specifies a table from which --- 237,243 ! LIKE parent_table [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ] The LIKE clause specifies a table from which *** and table *** 266,275 requested, all check constraints are copied. Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause an error is signalled. --- 266,280 requested, all check constraints are copied. + Any indexes on the original table will not be created on the new + table, unless the INCLUDING INDEXES clause is + specified. + + Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause, an error is signalled. Index: src/backend/bootstrap/bootparse.y === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/bootstrap/bootparse.y,v retrieving revision 1.88 diff -p -c -r1.88 bootparse.y *** src/backend/bootstrap/bootparse.y 13 Mar 2007 00:33:39 - 1.88 --- src/backend/bootstrap/bootparse.y 8 Jul 2007 00:49:29 - *** Boot_DeclareIndexStmt: *** 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, false, false, false, false, false, true, false, false); do_end(); --- 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, NULL, false, false, false, false, false, true, false, false); do_end(); *** Boot_DeclareUniqueIndexStmt: *** 270,276 LexIDStr($9), NULL, $11, ! NULL, NIL, true, false, false, false, false, true, false, false); do_end(); --- 270,276 LexIDStr($9), NULL, $11, ! NULL, NIL, NULL, true, false, false, false, false, true, false, false); do_end(); Index: src/backend/commands/indexcmds.c === RC
Re: [PATCHES] [DOCS] rename of a view
On Tue, 2007-07-03 at 09:56 -0400, Tom Lane wrote: > Neil, according to > http://developer.postgresql.org/index.php/Todo:PatchStatus > you have accepted two patches to review --- what is happening with > those? Yeah, sorry, forgot about those -- I'll take a look at them tonight. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [DOCS] rename of a view
On Mon, 2007-02-07 at 23:13 -0400, Tom Lane wrote: > Er, was this on the agenda for 8.3? Well, it seemed fairly harmless to me (no behavioral changes and very little new code, just syntax), so I didn't see a compelling reason to delay applying it for a few months. But I can revert it if you'd prefer. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [DOCS] rename of a view
On Sun, 2007-07-01 at 12:55 -0700, David Fetter wrote: > Here's a new patch + file. This one allows ALTER [SEQUENCE | VIEW] to > work only on the respective database objects, but permits the old > ALTER TABLE syntax. Applied with some fixes. Thanks for the patch. ALTER VIEW ... SET SCHEMA might be another worthwhile thing to add, for consistency. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [DOCS] rename of a view
On Sun, 2007-01-07 at 12:55 -0700, David Fetter wrote: > Here's a new patch + file. This one allows ALTER [SEQUENCE | VIEW] to > work only on the respective database objects, but permits the old > ALTER TABLE syntax. How about taking a look at the more thorough documentation updates Tom suggested? -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] SPI-header-files safe for C++-compiler
On Thu, 2007-06-28 at 12:08 +0200, Jacob Rief wrote: > If there is any chance to get this patch applied, I will of course regenerate > it out of the HEAD-revision of the CVS repository. I don't see a reason to reject the patch. All the arguments about why using C++ in the backend is ill-advised are well-taken, but the patch does *not* require "making a real commitment to making C++ usable as a backend extension language", it just obviates the need for some people to patch the source. So +1 from me. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] psql: add volatility to \df+
On Tue, 2007-26-06 at 15:10 -0700, Neil Conway wrote: > Attached is a patch that adds information about function volatility to > the output of psql's "\df+" slash command. I'll apply this to HEAD > tomorrow, barring any objections. Applied. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] SPI-header-files safe for C++-compiler
On Thu, 2007-28-06 at 02:07 -0400, Tom Lane wrote: > It was already pointed out upthread that wrapping the inclusions in > extern "C" {...} would fix the identifier part of the problem from > the user side No, my point about extern "C" was that I don't think we need to add it to the Postgres headers. As far as I can see, it doesn't help with the identifier part of the problem: % cat foo.cpp extern "C" { #include "postgres.h" #include "commands/trigger.h" } int main(void) { return 0; } % g++ -I$HOME/postgres/head/prefix/include/server foo.cpp /home/neilc/postgres/head/prefix/include/server/nodes/primnodes.h:1078: error: expected unqualified-id before 'using' /home/neilc/postgres/head/prefix/include/server/nodes/primnodes.h:1078: error: abstract declarator 'List*' used as declaration /home/neilc/postgres/head/prefix/include/server/nodes/primnodes.h:1078: error: expected ';' before 'using' /home/neilc/postgres/head/prefix/include/server/nodes/parsenodes.h:167: error: expected unqualified-id before 'typeid' /home/neilc/postgres/head/prefix/include/server/nodes/parsenodes.h:238: error: expected unqualified-id before 'typename' [ ... and so on ... ] -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] SPI-header-files safe for C++-compiler
On Thu, 2007-28-06 at 01:15 -0400, Tom Lane wrote: > The patch as given merely renames some random identifiers that happen to > be keywords in some non-C language (thereby breaking not only a lot of > core backend code, which we can fix, but who knows what other > user-written extensions that we can't fix so easily). The fact is, any user-written extensions that depend on types defined in parsenodes.h and primnodes.h are going to get broken all the time *anyway*, so I don't see this as a major disadvantage. Doing "s/typename/type_name/g" is likely to be the least of your concerns if your extension integrates with Postgres that closely. It would be one thing if we made a significant effort to preserve internal API stability -- but we plainly do not. (See the varlena API changes made in 8.3 for an example of an API change that will break far more user-written extensions.) > The *real* problem here is that to make this useful, we have to buy into > the assumption that C++ should work in the backend. I agree that C++ in the backend is always going to be a little hokey, but (a) I don't agree that it is completely impossible (b) if we can make people's lives a bit easier, I don't see a good reason not to. Like it or not, people have called into C++ libraries from C UDFs in the past, and likely will do so in the future. > The error handling assumptions are completely incompatible (setjmp and > throw do not usually interoperate) AFAIK this is resolvable with some degree of pain: before entering C++ land, wrap the call site in a C++ exception handler, and before calling back into Postgres, use a PG_TRY() block to rethrow elog(ERROR) as a C++ exception (and then rethrow as an elog(ERROR) once you've unwound the C++ portion of the stack) ... hey, I didn't say it was clean ;-) It's also worth noting that some people use C++ as "C with classes", and disallow the use of exceptions, RTTI, and that sort of stuff. Calling into such code from the backend is marginally more sane. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] SPI-header-files safe for C++-compiler
On Wed, 2007-06-27 at 20:51 -0400, Andrew Dunstan wrote: > He's used the usual "#ifdef __cplusplus" wrapper - isn't that good enough? Well, there's nothing wrong with it per se, I'm just wondering (1) what the proper set of headers to add it to is -- the patch just does a handful (2) whether we need to bother doing it at all -- can't clients do the extern "C" trick before including any Postgres headers? -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] SPI-header-files safe for C++-compiler
On Thu, 2007-06-28 at 00:43 +0200, Jacob Rief wrote: > But the C++-compiler rejects to compile legal C code, > because some of the included Postgres-headers, ie. postgres.h, > executor/spi.h, commands/trigger.h, fmgr.h use a few C++ keywords to > defined a some struct members and function arguments. The incriminating > C++-keywords used in the Postgres-headers are: 'typeid', 'typename' and > 'using'. > > It would do no harm to the Postgres-sources if these keywords would be > replaced with a similar identifier. No objection, although it would be nice if we could find something nicer to rename "using" to than "using_". What about "using_clause" or "using_list"? You also changed "namespace" => "name_space" in builtins.h; is that necessary? > I wrote a patch which applies cleanly onto version 8.2.4 Patches should be submitted against the CVS HEAD code: we wouldn't want to apply a patch like this to the REL8_2_STABLE branch, in any case. BTW, I notice the patch also adds 'extern "C" { ... }' statements to a few random header files. Can't client programs do this before including the headers, if necessary? -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] psql: add volatility to \df+
On Wed, 2007-27-06 at 00:16 -0400, Alvaro Herrera wrote: > +1 as well but I'm wondering whether the output is too wide. Well, the output of \df+ is already likely to be wider than 72 characters, so I'm not sure the patch would really make the situation materially worse ("\df+ nextval" is 118 characters wide without the patch, for example). You could also argue that if the user specifies "+", presumably they are less worried about a concise output format. > Is it possible to find some shorter string to convey the same meaning? Nothing obvious comes to mind, but I'm not opposed in principle. Any suggestions? -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] psql: add volatility to \df+
On Tue, 2007-26-06 at 20:29 -0700, Joshua D. Drake wrote: > Tom Lane wrote: > > +1, but are there not any documentation changes to make? Sure, I'll update the psql ref page. > Well here is a question (just because I haven't seen it) is there a list > of functions and their volatility in the docs? Not relevant to this thread, but AFAICS there is no explicit list of the volatilities of the builtin functions. -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
[PATCHES] psql: add volatility to \df+
Attached is a patch that adds information about function volatility to the output of psql's "\df+" slash command. I'll apply this to HEAD tomorrow, barring any objections. -Neil Index: src/bin/psql/describe.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/bin/psql/describe.c,v retrieving revision 1.155 diff -p -c -r1.155 describe.c *** src/bin/psql/describe.c 19 Mar 2007 23:38:31 - 1.155 --- src/bin/psql/describe.c 26 Jun 2007 22:01:55 - *** describeFunctions(const char *pattern, b *** 205,215 if (verbose) appendPQExpBuffer(&buf, ",\n r.rolname as \"%s\",\n" " l.lanname as \"%s\",\n" " p.prosrc as \"%s\",\n" " pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"", ! _("Owner"), _("Language"), _("Source code"), _("Description")); if (!verbose) --- 205,220 if (verbose) appendPQExpBuffer(&buf, + ",\n CASE\n" + " WHEN p.provolatile = 'i' THEN 'immutable'\n" + " WHEN p.provolatile = 's' THEN 'stable'\n" + " WHEN p.provolatile = 'v' THEN 'volatile'\n" + "END as \"%s\"" ",\n r.rolname as \"%s\",\n" " l.lanname as \"%s\",\n" " p.prosrc as \"%s\",\n" " pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"", ! _("Volatility"), _("Owner"), _("Language"), _("Source code"), _("Description")); if (!verbose) ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] psql: flush output in cursor-fetch mode
On Thu, 2007-21-06 at 22:09 -0400, Tom Lane wrote: > Seems reasonable to back-patch into 8.2 too... Okay, done. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] psql: flush output in cursor-fetch mode
On Wed, 2007-06-20 at 15:51 -0700, Neil Conway wrote: > Attached is a patch that fixes this, by calling fflush() on the psql > output stream after each call to printQuery() in ExecQueryUsingCursor(). Applied to HEAD. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] psql: flush output in cursor-fetch mode
psql's "FETCH_COUNT" feature is useful for incrementally displaying the results of a long-running query. However, psql fails to flush its output stream as new rows from the partial result set are produced, which means that partial query results may not be visible to the client until the stdio buffer is eventually flushed or the query produces its complete result set. Example: $ cat ~/test.sql -- a contrived function to get a query that slowly produces -- more rows create function slow_func() returns boolean as $$ begin perform pg_sleep(2); return true; end; $$ language plpgsql; neilc=# \i ~/test.sql CREATE FUNCTION neilc=# create table t1 (a int, b int); CREATE TABLE neilc=# insert into t1 values (5, 10), (10, 15), (20, 25), (30, 35); INSERT 0 4 neilc=# \set FETCH_COUNT 1 neilc=# select * from t1 where slow_func() is true; With CVS HEAD, no output is visible until the complete result set has been produced, at which point all 4 rows are printed. This is undesirable: since the client has gone to the trouble of FETCH'ing the rows one-at-a-time, it should display the partial result set before issuing another FETCH. Attached is a patch that fixes this, by calling fflush() on the psql output stream after each call to printQuery() in ExecQueryUsingCursor(). -Neil Index: src/bin/psql/common.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/bin/psql/common.c,v retrieving revision 1.134 diff -p -c -r1.134 common.c *** src/bin/psql/common.c 16 Apr 2007 20:15:38 - 1.134 --- src/bin/psql/common.c 20 Jun 2007 22:34:04 - *** ExecQueryUsingCursor(const char *query, *** 1076,1081 --- 1076,1087 printQuery(results, &my_popt, pset.queryFout, pset.logfile); + /* + * Make sure to flush the output stream, so intermediate + * results are visible to the client immediately. + */ + fflush(pset.queryFout); + /* after the first result set, disallow header decoration */ my_popt.topt.start_table = false; my_popt.topt.prior_records += ntuples; ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Mon, 2007-21-05 at 12:23 +0530, NikhilS wrote: > I had spent some time on this earlier so decided to complete and send > the patch to you for review. This patch supports copying of > expressions, predicates, opclass, amorder, reloptions etc. The test > case also contains some more additions with this patch. Please let me > know if there are any issues. Attached is a revised version of this patch. Note that this pattern is always unsafe: ht_am = SearchSysCache(AMOID, ...); if (!HeapTupleIsValid(ht_am)) elog(ERROR, "..."); amrec = (Form_pg_am) GETSTRUCT(ht_am); index->accessMethod = NameStr(amrec->amname); /* ... */ ReleaseSysCache(ht_am); return index; Before calling ReleaseSysCache(), all the data you need from the syscache entry needs to be deep-copied to allow subsequent access, but NameStr() doesn't do a deep-copy. Adding "-DFORCE_CATCACHE_RELEASE" is a useful way to catch these kinds of problems (I wonder if this is worth adding to the default CFLAGS when assertions are enabled?) I also made a bunch of editorial changes, including moving the varattnos_map_schema() call out of the loop in transformInhRelation(). BTW, comments like "This function is based on code from ruleutils.c" would be helpful for reviewers (whether in the patch itself or just in the email containing the patch). There's still a few things I need to fix in the patch, but I'll apply a revised version of the attached patch to HEAD tomorrow, barring any objections. -Neil Index: doc/src/sgml/ref/create_table.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/ref/create_table.sgml,v retrieving revision 1.107 diff -p -c -r1.107 create_table.sgml *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 - 1.107 --- doc/src/sgml/ref/create_table.sgml 2 Jun 2007 19:52:22 - *** and table *** 259,275 Not-null constraints are always copied to the new table. ! CHECK constraints will only be copied if ! INCLUDING CONSTRAINTS is specified; other types of ! constraints will never be copied. Also, no distinction is made between ! column constraints and table constraints — when constraints are ! requested, all check constraints are copied. Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause an error is signalled. --- 259,276 Not-null constraints are always copied to the new table. ! CHECK, UNIQUE, and ! PRIMARY KEY constraints will only be copied ! if INCLUDING CONSTRAINTS is specified. Also, ! no distinction is made between column constraints and table ! constraints — when constraints are requested, all eligible ! constraints are copied. Note also that unlike INHERITS, copied columns and constraints are not merged with similarly named columns and constraints. If the same name is specified explicitly or in another ! LIKE clause, an error is signalled. Index: src/backend/bootstrap/bootparse.y === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/bootstrap/bootparse.y,v retrieving revision 1.88 diff -p -c -r1.88 bootparse.y *** src/backend/bootstrap/bootparse.y 13 Mar 2007 00:33:39 - 1.88 --- src/backend/bootstrap/bootparse.y 2 Jun 2007 19:46:07 - *** Boot_DeclareIndexStmt: *** 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, false, false, false, false, false, true, false, false); do_end(); --- 252,258 LexIDStr($8), NULL, $10, ! NULL, NIL, NULL, false, false, false, false, false, true, false, false); do_end(); *** Boot_DeclareUniqueIndexStmt: *** 270,276 LexIDStr($9), NULL, $11, ! NULL, NIL, true, false, false, false, false, true, false, false); do_end(); --- 270,276 LexIDStr($9), NULL, $11, ! NULL, NIL, NULL, true, false, false, false, false, true, false, false); do_end(); Index: src/backend/commands/indexcmds.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/commands/indexcmds.c,v retrieving revision 1.158 diff -p -c -r1.158 indexcmds.c *** src/backend/commands/indexcmds.c 2 May 2007 21:08:45 - 1.158 --- src/backend/commands/indexcmds.c 2 Jun 2007 19:46:07 - *** DefineIndex(RangeVar *heapRelation, *** 100,1
Re: [PATCHES] boolean <=> text explicit casts
On Wed, 2007-30-05 at 21:23 +0200, Peter Eisentraut wrote: > I'm not sure what your rationale was for creating lower-case words > instead of upper case, except for it looks nicer. Is there a technical > reason? There's no real technical reason: the standard says upper-case, but PG's general philosophy of case folding would suggest folding to lower-case. If we were compliant with the spec's case folding requirements then emitting uppercase would be the clear choice, but since we aren't, I don't have strong feelings either way. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] boolean <=> text explicit casts
On Mon, 2007-28-05 at 15:38 -0400, Tom Lane wrote: > More generally, I'm really hoping to get rid of bespoke text<->whatever > cast functions in favor of using datatypes' I/O functions. To what > extent can we make the boolean I/O functions serve for this? It seems > relatively painless on the input side --- just allow whitespace --- but > I suppose we can't change boolout's historical result of "t"/"f" without > causing problems. Attached is a revised version of this patch that modifies boolin() to ignore leading and trailing whitespace. This makes text => boolean trivial, but boolean => text is still distinct from boolout(). Barring any objections, I'll apply this later today or tomorrow. -Neil Index: src/backend/utils/adt/bool.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/bool.c,v retrieving revision 1.38 diff -p -c -r1.38 bool.c *** src/backend/utils/adt/bool.c 5 Jan 2007 22:19:40 - 1.38 --- src/backend/utils/adt/bool.c 30 May 2007 18:55:26 - *** *** 15,20 --- 15,22 #include "postgres.h" + #include + #include "libpq/pqformat.h" #include "utils/builtins.h" *** *** 33,73 Datum boolin(PG_FUNCTION_ARGS) { ! char *b = PG_GETARG_CSTRING(0); ! switch (*b) { case 't': case 'T': ! if (pg_strncasecmp(b, "true", strlen(b)) == 0) PG_RETURN_BOOL(true); break; case 'f': case 'F': ! if (pg_strncasecmp(b, "false", strlen(b)) == 0) PG_RETURN_BOOL(false); break; case 'y': case 'Y': ! if (pg_strncasecmp(b, "yes", strlen(b)) == 0) PG_RETURN_BOOL(true); break; case '1': ! if (pg_strncasecmp(b, "1", strlen(b)) == 0) PG_RETURN_BOOL(true); break; case 'n': case 'N': ! if (pg_strncasecmp(b, "no", strlen(b)) == 0) PG_RETURN_BOOL(false); break; case '0': ! if (pg_strncasecmp(b, "0", strlen(b)) == 0) PG_RETURN_BOOL(false); break; --- 35,88 Datum boolin(PG_FUNCTION_ARGS) { ! const char *in_str = PG_GETARG_CSTRING(0); ! const char *str; ! size_t len; ! ! /* ! * Skip leading and trailing whitespace ! */ ! str = in_str; ! while (isspace((unsigned char) *str)) ! str++; ! ! len = strlen(str); ! while (len > 0 && isspace((unsigned char) str[len - 1])) ! len--; ! switch (*str) { case 't': case 'T': ! if (pg_strncasecmp(str, "true", len) == 0) PG_RETURN_BOOL(true); break; case 'f': case 'F': ! if (pg_strncasecmp(str, "false", len) == 0) PG_RETURN_BOOL(false); break; case 'y': case 'Y': ! if (pg_strncasecmp(str, "yes", len) == 0) PG_RETURN_BOOL(true); break; case '1': ! if (pg_strncasecmp(str, "1", len) == 0) PG_RETURN_BOOL(true); break; case 'n': case 'N': ! if (pg_strncasecmp(str, "no", len) == 0) PG_RETURN_BOOL(false); break; case '0': ! if (pg_strncasecmp(str, "0", len) == 0) PG_RETURN_BOOL(false); break; *** boolin(PG_FUNCTION_ARGS) *** 77,83 ereport(ERROR, (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), ! errmsg("invalid input syntax for type boolean: \"%s\"", b))); /* not reached */ PG_RETURN_BOOL(false); --- 92,98 ereport(ERROR, (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), ! errmsg("invalid input syntax for type boolean: \"%s\"", in_str))); /* not reached */ PG_RETURN_BOOL(false); *** boolsend(PG_FUNCTION_ARGS) *** 127,132 --- 142,178 PG_RETURN_BYTEA_P(pq_endtypsend(&buf)); } + /* + * textbool - cast function for text => bool + */ + Datum + textbool(PG_FUNCTION_ARGS) + { + Datum in_text = PG_GETARG_DATUM(0); + char *str; + + str = DatumGetCString(DirectFunctionCall1(textout, in_text)); + + PG_RETURN_DATUM(DirectFunctionCall1(boolin, CStringGetDatum(str))); + } + + /* + * booltext - cast function for bool => text + */ + Datum + booltext(PG_FUNCTION_ARGS) + { + bool arg1 = PG_GETARG_BOOL(0); + char *str; + + if (arg1) + str = "true"; + else + str = "false"; + + PG_RETURN_DATUM(DirectFunctionCall1(textin, CStringGetDatum(str))); + } + /* * PUBLIC ROUTINES * Index: src/include/catalog/pg_cast.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_cast.h,v retrieving revision 1.32 diff -p -c -r1.32 pg_cast.h *** src/include/catalog/pg_cast.h 2 Apr 2007 03:49:40 - 1.32 --- src/include/catalog/pg_cast.h 30 May 2007 17:59:20 - *** DATA(insert ( 1700 25 1688 i )); *** 302,307 --- 302,309 DATA(insert ( 25 1700 1686 e )); DATA(insert ( 142 25 2922 e )); DATA(insert ( 25 142 2896 e ));
Re: [PATCHES] Interval input: usec, msec
On Mon, 2007-28-05 at 13:54 -0400, Neil Conway wrote: > Okay, attached is a patch that does this. To summarize, the changes are: > >* add tmask bits for msec, usec (I reordered the #defines to keep > them logically contiguous, but AFAICS this isn't necessary) >* if the user specifies multiple instances of usec, msec, or sec, > reject as invalid input >* if the user specifies a fractional second ("5.5 seconds"), also > consider that to be millisecond and microsecond input from the > POV of rejecting duplicate units. So "5.5 seconds 1 millisecond" > will be rejected. Applied to HEAD, backported to 8.2 and 8.1 After I applied the patches, it occurred to me that this patch actually slightly changes previous behavior: the previous coding accepted inputs like "1 microsecond 1 microsecond 1 second", whereas the patch rejects this input. I don't think this is a major problem (input of this kind is likely a client bug), but I figured I'd mention it... -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Interval input: usec, msec
On Mon, 2007-28-05 at 15:05 -0400, Tom Lane wrote: > Right. I guess you misunderstood me: I was arguing for rejecting double > occurrences of the same unit name, but not occurrences of different unit > names that we happen to map into the same interval field internally. Makes sense to me. I'll send a patch for this shortly (but I'll commit the patch for the usec/msec issue first -- this second change shouldn't be backpatched). > I forget --- are the tmask bits used in stored typmod values for > intervals? It'd probably be best not to change the meanings of typmod > bits, since those are visible to client code if it wants to look. According to datetime.h, only some of the tmask bits are used in stored typmods: YEAR, MONTH, DAY, HOUR, MINUTE, and SECOND, and the patch doesn't modify any of those. So I think we're okay. > Hmmm ... if you check the cvs history for those tests you might find > some evidence. "Modify regression tests to allow GEQ optimizer (order results).", according to the 1997 CVS commit from Thomas Lockhart that added the lines in question. Presumably that is no longer relevant. Nothing unexpected happens if the disabling of GEQO is removed, so I'll apply the attached patch shortly barring any objections. -Neil Index: src/test/regress/expected/geometry.out === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/geometry.out,v retrieving revision 1.25 diff -p -c -r1.25 geometry.out *** src/test/regress/expected/geometry.out 1 Feb 2007 19:10:30 - 1.25 --- src/test/regress/expected/geometry.out 29 May 2007 02:16:43 - *** SELECT '' AS twenty, b.f1 / p.f1 AS rota *** 284,290 -- -- Paths -- - SET geqo TO 'off'; SELECT '' AS eight, npoints(f1) AS npoints, f1 AS path FROM PATH_TBL; eight | npoints | path ---+-+--- --- 284,289 *** SELECT '' AS eight, p1.f1 * point '(2,-1 *** 337,343 | ((34,13),(40,15)) (8 rows) - RESET geqo; -- -- Polygons -- --- 336,341 Index: src/test/regress/expected/geometry_1.out === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/geometry_1.out,v retrieving revision 1.10 diff -p -c -r1.10 geometry_1.out *** src/test/regress/expected/geometry_1.out 1 Feb 2007 20:11:18 - 1.10 --- src/test/regress/expected/geometry_1.out 29 May 2007 02:16:50 - *** SELECT '' AS twenty, b.f1 / p.f1 AS rota *** 284,290 -- -- Paths -- - SET geqo TO 'off'; SELECT '' AS eight, npoints(f1) AS npoints, f1 AS path FROM PATH_TBL; eight | npoints | path ---+-+--- --- 284,289 *** SELECT '' AS eight, p1.f1 * point '(2,-1 *** 337,343 | ((34,13),(40,15)) (8 rows) - RESET geqo; -- -- Polygons -- --- 336,341 Index: src/test/regress/expected/geometry_2.out === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/geometry_2.out,v retrieving revision 1.7 diff -p -c -r1.7 geometry_2.out *** src/test/regress/expected/geometry_2.out 1 Feb 2007 20:11:18 - 1.7 --- src/test/regress/expected/geometry_2.out 29 May 2007 02:16:56 - *** SELECT '' AS twenty, b.f1 / p.f1 AS rota *** 284,290 -- -- Paths -- - SET geqo TO 'off'; SELECT '' AS eight, npoints(f1) AS npoints, f1 AS path FROM PATH_TBL; eight | npoints | path ---+-+--- --- 284,289 *** SELECT '' AS eight, p1.f1 * point '(2,-1 *** 337,343 | ((34,13),(40,15)) (8 rows) - RESET geqo; -- -- Polygons -- --- 336,341 Index: src/test/regress/expected/point.out === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/point.out,v retrieving revision 1.16 diff -p -c -r1.16 point.out *** src/test/regress/expected/point.out 9 Jan 2007 02:14:16 - 1.16 --- src/test/regress/expected/point.out 29 May 2007 02:16:00 - *** SELECT '' AS six, p.f1, p.f1 <-> point ' *** 105,111 | (5.1,34.5) | 34.8749193547455 (6 rows) - SET geqo TO 'off'; SELECT '' AS thirtysix, p1.f1 AS point1, p2.f1 AS point2, p1.f1 <-> p2.f1 AS dist FROM POINT_TBL p1, POINT_TBL p2 ORDER BY dist, p1.f1[0], p2.f1[0]; --- 105,110 *** SELECT '' AS three, p1.f1 AS point1, p2. *** 222,225 | (5.1,34.5) | (10,10) | 24.9851956166046 (3 rows) - RESET geqo; --- 221,223 Index: src/test/regress/expected/tinterval.out === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/tinterval.out,v retrieving revisio
Re: [PATCHES] boolean <=> text explicit casts
On Mon, 2007-28-05 at 15:38 -0400, Tom Lane wrote: > More generally, I'm really hoping to get rid of bespoke text<->whatever > cast functions in favor of using datatypes' I/O functions. I don't object, but I'm curious: is there a benefit to this other than brevity of implementation? ISTM the spec has the idea that the input to a type's constructor is often distinct from the type's text => type casting behavior. -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
[PATCHES] boolean <=> text explicit casts
I noticed that SQL:2003 specifies explicit casts between "boolean" and the character string types. Attached is a patch that implements them, and adds some simple regression tests. A few points worth noting: (1) The SQL spec requires that text::boolean trim leading and trailing whitespace from the input (2) The spec also requires that boolean::varchar(n) should raise an error if "n" is not large enough to accomodate the textual representation of the boolean value. We currently truncate: => select true::boolean::varchar(3); varchar - TRU Not sure offhand if there's an easy way to satisfy the spec's requirement... (3) The spec suggests that true/false should be upper-cased when converted to text, so that's what I've implemented, but one could argue that converting to lower-case would be more consistent with PG's general approach to case folding. -Neil Index: src/backend/utils/adt/bool.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/bool.c,v retrieving revision 1.38 diff -p -c -r1.38 bool.c *** src/backend/utils/adt/bool.c 5 Jan 2007 22:19:40 - 1.38 --- src/backend/utils/adt/bool.c 28 May 2007 18:47:59 - *** boolsend(PG_FUNCTION_ARGS) *** 127,132 --- 127,177 PG_RETURN_BYTEA_P(pq_endtypsend(&buf)); } + /* + * textbool - cast function for text => bool + */ + Datum + textbool(PG_FUNCTION_ARGS) + { + text *arg1 = PG_GETARG_TEXT_P(0); + text *trim; + char *trim_str; + + trim = DatumGetTextP(DirectFunctionCall1(btrim1, + PointerGetDatum(arg1))); + trim_str = DatumGetCString(DirectFunctionCall1(textout, + PointerGetDatum(trim))); + + if (pg_strcasecmp(trim_str, "true") == 0) + PG_RETURN_BOOL(true); + else if (pg_strcasecmp(trim_str, "false") == 0) + PG_RETURN_BOOL(false); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_CHARACTER_VALUE_FOR_CAST), + errmsg("invalid input syntax for type boolean: \"%s\"", + trim_str))); + + PG_RETURN_BOOL(false); /* keep the compiler quiet */ + } + + /* + * booltext - cast function for bool => text + */ + Datum + booltext(PG_FUNCTION_ARGS) + { + bool arg1 = PG_GETARG_BOOL(0); + char *str; + + if (arg1) + str = "TRUE"; + else + str = "FALSE"; + + PG_RETURN_DATUM(DirectFunctionCall1(textin, CStringGetDatum(str))); + } + /* * PUBLIC ROUTINES * Index: src/include/catalog/pg_cast.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_cast.h,v retrieving revision 1.32 diff -p -c -r1.32 pg_cast.h *** src/include/catalog/pg_cast.h 2 Apr 2007 03:49:40 - 1.32 --- src/include/catalog/pg_cast.h 28 May 2007 18:41:38 - *** DATA(insert ( 1700 25 1688 i )); *** 302,307 --- 302,309 DATA(insert ( 25 1700 1686 e )); DATA(insert ( 142 25 2922 e )); DATA(insert ( 25 142 2896 e )); + DATA(insert ( 16 25 2971 e )); + DATA(insert ( 25 16 2970 e )); /* * Cross-category casts to and from VARCHAR *** DATA(insert ( 1700 1043 1688 a )); *** 342,347 --- 344,351 DATA(insert ( 1043 1700 1686 e )); DATA(insert ( 142 1043 2922 e )); DATA(insert ( 1043 142 2896 e )); + DATA(insert ( 16 1043 2971 e )); + DATA(insert ( 1043 16 2970 e )); /* * Cross-category casts to and from BPCHAR Index: src/include/catalog/pg_proc.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_proc.h,v retrieving revision 1.456 diff -p -c -r1.456 pg_proc.h *** src/include/catalog/pg_proc.h 21 May 2007 17:10:29 - 1.456 --- src/include/catalog/pg_proc.h 28 May 2007 18:40:53 - *** DESCR("List all files in a directory"); *** 3221,3226 --- 3221,3230 DATA(insert OID = 2626 ( pg_sleep PGNSP PGUID 12 1 0 f f t f v 1 2278 "701" _null_ _null_ _null_ pg_sleep - _null_ )); DESCR("Sleep for the specified time in seconds"); + DATA(insert OID = 2970 ( boolean PGNSP PGUID 12 1 0 f f t f i 1 16 "25" _null_ _null_ _null_ textbool - _null_ )); + DESCR("text to boolean"); + DATA(insert OID = 2971 ( textPGNSP PGUID 12 1 0 f f t f i 1 25 "16" _null_ _null_ _null_ booltext - _null_ )); + DESCR("boolean to text"); /* Aggregates (moved here from pg_aggregate for 7.3) */ Index: src/include/utils/builtins.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/utils/builtins.h,v retrieving revision 1.293 diff -p -c -r1.293 builtins.h *** src/include/utils/builtins.h 17 May 2007 23:31:49 - 1.293 --- src/include/utils/builtins.h 28 May 2007 18:13:15 - *** extern Datum boolin(PG_FUNCTION_ARGS); *** 70,75 --- 70,77
Re: [PATCHES] Interval input: usec, msec
On Mon, 2007-28-05 at 10:50 -0400, Tom Lane wrote: > I'd argue that it's an oversight. I don't have a problem with adding up > the values of units that really translate to the same thing (eg, > '1 week 1 day' -> '8 days'), but I think '1 second 2 second' should > be rejected because it's almost certainly user error. I don't see why "1 week 1 week" is any less likely to be user error than "1 second 1 second". > Does your patch allow '1 millisec 2 microsec', which should be allowed > by this argument? Yes: in order to allow the above input, the straightforward coding also allows "1 second 2 second". > I suspect that to make it work unsurprisingly, we'd > need to allocate a distinct tmask bit to each logically distinct unit. Okay, attached is a patch that does this. To summarize, the changes are: * add tmask bits for msec, usec (I reordered the #defines to keep them logically contiguous, but AFAICS this isn't necessary) * if the user specifies multiple instances of usec, msec, or sec, reject as invalid input * if the user specifies a fractional second ("5.5 seconds"), also consider that to be millisecond and microsecond input from the POV of rejecting duplicate units. So "5.5 seconds 1 millisecond" will be rejected. Docs and regression tests updated. If people are happy with the above behavior, I'll commit this to HEAD shortly (today/tomorrow), and perhaps backport it: not accepting "1 msec" and similar inputs is a clear bug, IMHO. BTW, does anyone know why a few of the regression tests (tinterval, point, geometry, etc.) explicitly disable and then re-enable GEQO? AFAICS the regression tests are just doing fairly routine two-table joins, so GEQO will not be invoked with a sane configuration anyway. -Neil Index: doc/src/sgml/datatype.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/datatype.sgml,v retrieving revision 1.201 diff -p -c -r1.201 datatype.sgml *** doc/src/sgml/datatype.sgml 21 May 2007 17:10:28 - 1.201 --- doc/src/sgml/datatype.sgml 28 May 2007 17:11:14 - *** January 8 04:05:06 1999 PST *** 1880,1886 Where: quantity is a number (possibly signed); ! unit is second, minute, hour, day, week, month, year, decade, century, millennium, --- 1880,1887 Where: quantity is a number (possibly signed); ! unit is microsecond, ! millisecond, second, minute, hour, day, week, month, year, decade, century, millennium, Index: src/backend/utils/adt/datetime.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/datetime.c,v retrieving revision 1.179 diff -p -c -r1.179 datetime.c *** src/backend/utils/adt/datetime.c 27 May 2007 20:32:16 - 1.179 --- src/backend/utils/adt/datetime.c 28 May 2007 17:25:09 - *** DecodeDateTime(char **field, int *ftype, *** 924,929 --- 924,930 #else *fsec = frac; #endif + tmask = DTK_ALL_SECS_M; } break; *** DecodeTimeOnly(char **field, int *ftype, *** 1699,1704 --- 1700,1706 #else *fsec = frac; #endif + tmask = DTK_ALL_SECS_M; } break; *** DecodeInterval(char **field, int *ftype, *** 2805,2810 --- 2807,2813 #else *fsec += (val + fval) * 1e-6; #endif + tmask = DTK_M(MICROSECOND); break; case DTK_MILLISEC: *** DecodeInterval(char **field, int *ftype, *** 2813,2818 --- 2816,2822 #else *fsec += (val + fval) * 1e-3; #endif + tmask = DTK_M(MILLISECOND); break; case DTK_SECOND: *** DecodeInterval(char **field, int *ftype, *** 2822,2828 #else *fsec += fval; #endif ! tmask = DTK_M(SECOND); break; case DTK_MINUTE: --- 2826,2840 #else *fsec += fval; #endif ! /* ! * If any subseconds were specified, consider ! * this microsecond and millisecond input as ! * well. ! */ ! if (fval == 0) ! tmask = DTK_M(SECOND); ! else ! tmask = DTK_ALL_SECS_M; break; case DTK_MINUTE: Index: src/include/utils/datetime.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/utils/datetime.h,v retrieving revision 1.65 diff -p -c -r1.65 datetime.h *** src/include/utils/datetime.h 19 Feb 2007 17:41:39 - 1.65 --- src/include/utils/datetime.h 28 May 2007 17:18:47 - *** *** 100,116 #define HOUR 10 #define MINUTE 11 #define SECOND 12 ! #define DOY 13 ! #define DOW 14 ! #define UNITS 15 ! #define ADBC 16 /* these are only for relative dates */ ! #define AGO 17 ! #define ABS_BEFORE 18 ! #define ABS_AFT
[PATCHES] Interval input: usec, msec
neilc=# select '1 day 1 millisecond'::interval; interval 1 day 00:00:00.001 (1 row) neilc=# select '1 millisecond'::interval; ERROR: invalid input syntax for type interval: "1 millisecond" neilc=# select '0.001 seconds'::interval; interval -- 00:00:00.001 (1 row) Similarly for "microsecond". I think allowing the first input but not the second is pretty inconsistent. Attached is a patch that fixes this for both milliseconds and microseconds. The previous coding allowed "x seconds y milliseconds" to be specified, so this patch preserves that behavior. It *also* allows "x seconds y seconds" as valid input, although you could make a consistency argument for that anyway: (with CVS HEAD, unpatched) neilc=# select '5 days 10 days'::interval; interval -- 15 days neilc=# select '1 week 2 week 3 weeks'::interval; interval -- 42 days neilc=# select '1 second 2 second'::interval; ERROR: invalid input syntax for type interval: "1 second 2 second" Is there any reason to why DecodeInterval() is willing to accept multiple specifications for some time units but not others? -Neil Index: src/backend/utils/adt/datetime.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/datetime.c,v retrieving revision 1.179 diff -p -c -r1.179 datetime.c *** src/backend/utils/adt/datetime.c 27 May 2007 20:32:16 - 1.179 --- src/backend/utils/adt/datetime.c 28 May 2007 04:37:15 - *** DecodeInterval(char **field, int *ftype, *** 2805,2810 --- 2805,2811 #else *fsec += (val + fval) * 1e-6; #endif + tmask = (fmask & DTK_M(SECOND)) ? 0 : DTK_M(SECOND); break; case DTK_MILLISEC: *** DecodeInterval(char **field, int *ftype, *** 2813,2818 --- 2814,2820 #else *fsec += (val + fval) * 1e-3; #endif + tmask = (fmask & DTK_M(SECOND)) ? 0 : DTK_M(SECOND); break; case DTK_SECOND: *** DecodeInterval(char **field, int *ftype, *** 2822,2828 #else *fsec += fval; #endif ! tmask = DTK_M(SECOND); break; case DTK_MINUTE: --- 2824,2830 #else *fsec += fval; #endif ! tmask = (fmask & DTK_M(SECOND)) ? 0 : DTK_M(SECOND); break; case DTK_MINUTE: ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote: > Has a revised version of this patch been submitted? In the absence of a revised patch, I can finish the feature myself, but I won't get the free cycles until after PGCon. I can commit to getting it done before the end of May, or else we can just push this to 8.4. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [DOCS] Autovacuum and XID wraparound
On Tue, 2007-15-05 at 09:07 -0400, Alvaro Herrera wrote: > I agree, the note should be added there (but it should be a short one > and refer the reader someplace else for more complete details). I've applied the attached patch to HEAD and REL8_2_STABLE. -Neil Index: doc/src/sgml/config.sgml === RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/config.sgml,v retrieving revision 1.122 diff -c -p -r1.122 config.sgml *** doc/src/sgml/config.sgml 20 Apr 2007 02:37:37 - 1.122 --- doc/src/sgml/config.sgml 15 May 2007 15:04:35 - *** SELECT * FROM parent WHERE key = 2400; *** 3172,3177 --- 3172,3183 This parameter can only be set in the postgresql.conf file or on the server command line. + + Note that even when this parameter is disabled, the system + will periodically launch autovacuum processes in order to + prevent transaction ID wraparound. See for more information. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Fri, 2007-27-04 at 18:59 -0400, Neil Conway wrote: > This patch needs more work. Has a revised version of this patch been submitted? -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [DOCS] Autovacuum and XID wraparound
On Mon, 2007-14-05 at 16:22 -0400, Bruce Momjian wrote: > I agree with Tom. I don't think the current behavior is a major issue > for users for it to be mentioned more than it already is Are you really suggesting that we shouldn't modify config.sgml to note that "autovacuum = off" does not actually imply that "the autovacuum daemon is disabled"? ISTM that plainly violates the principle of least surprise -- it is almost the definition of what an entry in config.sgml *should* include. > though if you want to move one of those, we can do that. So the change would be okay if we also removed one of the other mentions in an unrelated section of the manual? I don't see the logic. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [DOCS] OS/X startup scripts
On Sun, 2007-13-05 at 18:58 -0700, David Fetter wrote: > cvs diff works just great until you want to add or remove a file > without write permissions to the CVS repository, i.e. when you've > checked out as anonymous. Personally, I usually work against a checkout from a local mirror of the CVS repository (which you can create via cvsup or rsync). With that setup, "cvs add" and "cvs diff -N" work fine, since you can arrange for write access to the local mirror. (I'm always surprised to hear that anyone does a non-trivial amount of work on Postgres without setting up a CVS mirror...) -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [DOCS] Autovacuum and XID wraparound
On Sun, 2007-13-05 at 22:06 -0400, Tom Lane wrote: > This fact is already documented in at least three places; do we really > need two more? I think we need to at least modify the documentation for the autovacuum GUC parameter, which currently states only that it "controls whether the server should run the autovacuum launcher daemon" -- this is not strictly true, and in any case, it isn't the whole story. > The proposed addition to postgresql.conf seems particularly > over-the-top I agree that this information doesn't really belong in postgresql.conf. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Hash function for numeric (WIP)
On Sun, 2007-06-05 at 21:30 -0400, Tom Lane wrote: > It'd be a good idea if you repeat the previous number-of-collisions > experiment on this code. I repeated the same experiment, and got essentially the same number of collisions (829 collisions on ~2 million randomly generated numerics, with 273 duplicates). Since the modified hash still uses hash_any() and really only differs when there are leading/trailing zeros, this is consistent with what I'd expect. Patch applied to HEAD. -Neil ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Implemented current_query
On Mon, 2007-07-05 at 19:48 +0100, Tomas Doran wrote: > As suggested in the TODO list (and as I need the functionality > myself), I have implemented the current_query interface to > debug_query_string. Comments: * docs need a bit more detail (they should emphasize that it is the current query string submitted by the client, as opposed to the currently executing SPI command or the like). Also, the docs currently claim current_query() returns "name". * use textin() to convert C-style strings to text, rather than constructing a text datum by hand * perhaps we can get away with marking current_query() stable? * AFAIK debug_query_string() still does the wrong thing when the user submits multiple queries in a single protocol message (separated by semi-colons). Not sure there's a way to fix that that is both easy and efficient, though... -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Hash function for numeric (WIP)
On Sun, 2007-06-05 at 21:30 -0400, Tom Lane wrote: > You can do it always if you simply decrement the weight for each leading > zero removed. On reflection, the patch as given was wrong anyway: if two Numerics are identical except for the presence of leading zeroes, we can't consider the weight when hashing one of them but not the other. > The patch as given is wrong since it makes the test for no-digits > before instead of after removing zeroes.) Okay, how about the attached revision? > It'd be a good idea if you repeat the previous number-of-collisions > experiment on this code. I'll do this shortly. -Neil Index: src/backend/utils/adt/numeric.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/numeric.c,v retrieving revision 1.101 diff -c -p -r1.101 numeric.c *** src/backend/utils/adt/numeric.c 27 Feb 2007 23:48:08 - 1.101 --- src/backend/utils/adt/numeric.c 7 May 2007 01:49:18 - *** *** 26,31 --- 26,32 #include #include + #include "access/hash.h" #include "catalog/pg_type.h" #include "libpq/pqformat.h" #include "utils/array.h" *** cmp_numerics(Numeric num1, Numeric num2) *** 1149,1154 --- 1150,1230 return result; } + Datum + hash_numeric(PG_FUNCTION_ARGS) + { + Numeric key = PG_GETARG_NUMERIC(0); + Datum digit_hash; + Datum result; + int weight; + int start_offset; + int end_offset; + int i; + int hash_len; + + /* If it's NaN, don't try to hash the rest of the fields */ + if (NUMERIC_IS_NAN(key)) + PG_RETURN_UINT32(0); + + weight = key->n_weight; + start_offset = 0; + end_offset = 0; + + /* + * Omit any leading or trailing zeros from the input to the + * hash. The numeric implementation *should* guarantee that + * leading and trailing zeros are suppressed, but we're + * paranoid. Note that we measure the starting and ending offsets + * in units of NumericDigits, not bytes. + */ + for (i = 0; i < NUMERIC_NDIGITS(key); i++) + { + if (NUMERIC_DIGITS(key)[i] != (NumericDigit) 0) + break; + + start_offset++; + /* + * The weight is effectively the # of digits before the + * decimal point, so decrement it for each leading zero we + * skip. + */ + weight--; + } + + /* + * If there are no non-zero digits, then the value of the number + * is zero, regardless of any other fields. + */ + if (NUMERIC_NDIGITS(key) == start_offset) + PG_RETURN_UINT32(-1); + + for (i = NUMERIC_NDIGITS(key) - 1; i >= 0; i--) + { + if (NUMERIC_DIGITS(key)[i] != (NumericDigit) 0) + break; + + end_offset++; + } + + /* If we get here, there should be at least one non-zero digit */ + Assert(start_offset + end_offset < NUMERIC_NDIGITS(key)); + + /* + * Note that we don't hash on the Numeric's scale, since two + * numerics can compare equal but have different scales. We also + * don't hash on the sign, although we could: since a sign + * difference implies inequality, this shouldn't affect correctness. + */ + hash_len = NUMERIC_NDIGITS(key) - start_offset - end_offset; + digit_hash = hash_any((unsigned char *) (NUMERIC_DIGITS(key) + start_offset), + hash_len * sizeof(NumericDigit)); + + /* Mix in the weight, via XOR */ + result = digit_hash ^ weight; + + PG_RETURN_DATUM(result); + } + /* -- * Index: src/include/catalog/pg_amop.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amop.h,v retrieving revision 1.80 diff -c -p -r1.80 pg_amop.h *** src/include/catalog/pg_amop.h 2 Apr 2007 03:49:40 - 1.80 --- src/include/catalog/pg_amop.h 7 May 2007 00:48:32 - *** DATA(insert ( 2232 19 19 1 f 2334 405 *** 568,573 --- 568,575 DATA(insert ( 2235 1033 1033 1 f 974 405 )); /* uuid_ops */ DATA(insert ( 2969 2950 2950 1 f 2972 405 )); + /* numeric_ops */ + DATA(insert ( 1998 1700 1700 1 f 1752 405 )); /* Index: src/include/catalog/pg_amproc.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amproc.h,v retrieving revision 1.64 diff -c -p -r1.64 pg_amproc.h *** src/include/catalog/pg_amproc.h 2 Apr 2007 03:49:40 - 1.64 --- src/include/catalog/pg_amproc.h 7 May 2007 00:48:32 - *** DATA(insert ( 1990 26 26 1 453 )); *** 148,153 --- 148,154 DATA(insert ( 1992 30 30 1 457 )); DATA(insert ( 1995 25 25 1 400 )); DATA(insert ( 1997 1083 1083 1 452 )); + DATA(insert ( 1998 1700 1700 1 432 )); DATA(insert ( 1999 1184 1184 1 452 )); DATA(insert ( 2001 1266 1266 1 1696 )); DATA(insert ( 2040 1114 1114 1 452 )); Index: src/include/catalog/pg_opclass.h ==
Re: [PATCHES] Hash function for numeric (WIP)
On Thu, 2007-03-05 at 23:57 -0400, Tom Lane wrote: > Hm, but apply hash_any() to the remaining digits? That might work, if > you are careful about how you factor the weight into it (or just not try > to use the weight in the hash). Attached is a patch that implements this idea. Since leading or trailing zeroes are far from the common case, I think we should still include the weight in the hash when possible: the patch does so when it doesn't find a leading zero in the Numeric. I did some testing by constructing an in-memory Numeric with leading and trailing zeroes, and this approach seems to work. Unless you see anything else that needs fixing, I'll apply this patch to HEAD in a day or two. -Neil Index: src/backend/utils/adt/numeric.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/numeric.c,v retrieving revision 1.101 diff -c -p -r1.101 numeric.c *** src/backend/utils/adt/numeric.c 27 Feb 2007 23:48:08 - 1.101 --- src/backend/utils/adt/numeric.c 7 May 2007 01:16:30 - *** *** 26,31 --- 26,32 #include #include + #include "access/hash.h" #include "catalog/pg_type.h" #include "libpq/pqformat.h" #include "utils/array.h" *** cmp_numerics(Numeric num1, Numeric num2) *** 1149,1154 --- 1150,1224 return result; } + Datum + hash_numeric(PG_FUNCTION_ARGS) + { + Numeric key = PG_GETARG_NUMERIC(0); + Datum digit_hash; + Datum result; + int start_offset; + int end_offset; + int i; + int hash_len; + + /* If it's NaN, don't try to hash the rest of the fields */ + if (NUMERIC_IS_NAN(key)) + PG_RETURN_UINT32(0); + + /* + * If ndigits is zero, the value of the number is zero, regardless + * of any other fields + */ + if (NUMERIC_NDIGITS(key) == 0) + PG_RETURN_UINT32(-1); + + /* + * Omit any leading or trailing zeros from the input to the + * hash. The numeric implementation *should* guarantee that + * leading and trailing zeros are suppressed, but we're + * paranoid. Note that we measure the start and end offsets in + * units of NumericDigits, not bytes. + */ + start_offset = 0; + end_offset = 0; + + for (i = 0; i < NUMERIC_NDIGITS(key); i++) + { + if (NUMERIC_DIGITS(key)[i] != (NumericDigit) 0) + break; + + start_offset++; + } + + for (i = NUMERIC_NDIGITS(key) - 1; i >= 0; i--) + { + if (NUMERIC_DIGITS(key)[i] != (NumericDigit) 0) + break; + + end_offset++; + } + + /* + * Note that we don't hash on the Numeric's scale, since two + * numerics can compare equal but have different scales. We also + * don't hash on the sign, although we could: since a sign + * difference implies inequality, this shouldn't affect correctness. + */ + hash_len = NUMERIC_NDIGITS(key) - start_offset - end_offset; + digit_hash = hash_any((unsigned char *) (NUMERIC_DIGITS(key) + start_offset), + hash_len * sizeof(NumericDigit)); + + /* + * Mix in the weight, via XOR. Since the weight is the exponent to + * which the first digit is implicitly raised, if the first digit + * was a zero we shouldn't include the weight in the hash. + */ + if (start_offset == 0) + result = digit_hash ^ key->n_weight; + + PG_RETURN_DATUM(result); + } + /* -- * Index: src/include/catalog/pg_amop.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amop.h,v retrieving revision 1.80 diff -c -p -r1.80 pg_amop.h *** src/include/catalog/pg_amop.h 2 Apr 2007 03:49:40 - 1.80 --- src/include/catalog/pg_amop.h 7 May 2007 00:48:32 - *** DATA(insert ( 2232 19 19 1 f 2334 405 *** 568,573 --- 568,575 DATA(insert ( 2235 1033 1033 1 f 974 405 )); /* uuid_ops */ DATA(insert ( 2969 2950 2950 1 f 2972 405 )); + /* numeric_ops */ + DATA(insert ( 1998 1700 1700 1 f 1752 405 )); /* Index: src/include/catalog/pg_amproc.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amproc.h,v retrieving revision 1.64 diff -c -p -r1.64 pg_amproc.h *** src/include/catalog/pg_amproc.h 2 Apr 2007 03:49:40 - 1.64 --- src/include/catalog/pg_amproc.h 7 May 2007 00:48:32 - *** DATA(insert ( 1990 26 26 1 453 )); *** 148,153 --- 148,154 DATA(insert ( 1992 30 30 1 457 )); DATA(insert ( 1995 25 25 1 400 )); DATA(insert ( 1997 1083 1083 1 452 )); + DATA(insert ( 1998 1700 1700 1 432 )); DATA(insert ( 1999 1184 1184 1 452 )); DATA(insert ( 2001 1266 1266 1 1696 )); DATA(insert ( 2040 1114 1114 1 452 )); Index: src/include/catalog/pg_opclass.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catal
Re: [PATCHES] Enable integer datetimes by default
On Sun, 2007-06-05 at 00:20 -0400, Andrew Dunstan wrote: > I object to the short notice. I think we need to give people a chance to > adjust their configs Sure, I can wait a few days (although if we're going to do this for 8.3, we should do it promptly). On reflection, it might actually be wiser to delay making this change until the beginning of the 8.4 cycle... -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Enable integer datetimes by default
On Sat, 2007-05-05 at 22:49 -0500, Michael Glaesemann wrote: > Would it make more sense to have phrase it in the positive sense? > i.e., --enable-floating-point-datetimes? I guess that's a bit longer, > but it says what you're doing, rather than what you're *not* doing. I think the primary reason people will want to use FP-based datetimes is because they can't use integer-based datetimes for compatibility reasons (e.g. no OS support for 64-bit integers, or they need to remain compatible with old applications). The situation is analogous to --without-spinlocks: we could call that --enable-slow-locking or something, but that would sound like we're enabling an additional feature. It would also mean there would be an implicit relationship between "--enable-integer-datetimes" and "--enable-fp-datetimes" (at most one can be true). IMHO it would be simpler to just keep a single boolean variable ("integer datetimes enabled or not"). -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Hash function for numeric (WIP)
On Mon, 2007-30-04 at 00:04 -0400, Tom Lane wrote: > I'm still not very comfortable with that. You're proposing to add a > pretty obvious failure mechanism --- any numeric-returning function > that failed to "normalize" its output would now create a subtle, > hard-to-find bug. What about teaching hash_numeric() to explicitly ignore leading and trailing zero digits? > Perhaps a suitable test would be to compare the number of > hash collisions in a large set of randomly-chosen-but-distinct > numeric values. Okay, I did a little testing. I created a table with ~2 million numerics, generated with equal probability by one of the two expressions: random()::numeric * ((random() * 100) ^ 20) * 100 random()::numeric * -100; There were 251 duplicate inputs that I didn't bother eliminating; of course, they should have effected both hash functions identically. Results: neilc=# create temp table r1 as select hash_numeric(a), count(*) from x group by hash_numeric(a); neilc=# create temp table r2 as select old_hash_numeric(a), count(*) from x group by old_hash_numeric(a); neilc=# select count(*) from r1 where count > 1; count 123342 (1 row) neilc=# select count(*) from r2 where count > 1; count --- 756 (1 row) old_hash_numeric() is the hash_any()-based hash, hash_numeric() is my coding of your suggested hash function (see attached patch). So it seems we need a better hash if we're not going to use hash_any(). The analysis required to write a robust hash function from scratch is precisely why I'd prefer to use hash_any() if possible. -Neil Index: src/backend/utils/adt/numeric.c === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/adt/numeric.c,v retrieving revision 1.101 diff -c -p -r1.101 numeric.c *** src/backend/utils/adt/numeric.c 27 Feb 2007 23:48:08 - 1.101 --- src/backend/utils/adt/numeric.c 3 May 2007 20:59:55 - *** *** 26,31 --- 26,32 #include #include + #include "access/hash.h" #include "catalog/pg_type.h" #include "libpq/pqformat.h" #include "utils/array.h" *** cmp_numerics(Numeric num1, Numeric num2) *** 1149,1154 --- 1150,1213 return result; } + Datum + old_hash_numeric(PG_FUNCTION_ARGS) + { + Numeric key = PG_GETARG_NUMERIC(0); + Datum digit_hash; + Datum result; + + /* If it's NaN, don't try to hash the rest of the fields */ + if (NUMERIC_IS_NAN(key)) + PG_RETURN_UINT32(0); + + /* If n_weight is zero, it is a zero, regardless of any other fields */ + if (NUMERIC_NDIGITS(key) == 0) + PG_RETURN_UINT32(-1); + + /* + * XXX: we don't hash on the Numeric's scale, since two numerics + * can compare equal but have different scales. We also don't hash + * on the sign, although we easily could (since a sign difference + * means inequality, it shouldn't affect correctness). + */ + digit_hash = hash_any((unsigned char *) NUMERIC_DIGITS(key), + NUMERIC_NDIGITS(key) * sizeof(NumericDigit)); + + /* Mix in the weight, via XOR */ + result = digit_hash ^ key->n_weight; + PG_RETURN_DATUM(result); + } + + Datum + hash_numeric(PG_FUNCTION_ARGS) + { + Numeric key = PG_GETARG_NUMERIC(0); + NumericDigit *digits; + uint32 hash; + int32 shift; + int i; + + /* If it's NaN, don't try to hash the rest of the fields */ + if (NUMERIC_IS_NAN(key)) + PG_RETURN_UINT32(0); + + hash = 0; + shift = 3 * key->n_weight; + digits = NUMERIC_DIGITS(key); + + for (i = 0; i < NUMERIC_NDIGITS(key); i++) + { + int32 this_shift = (shift & 31); + hash |= ((uint32) digits[i]) << this_shift; + if (this_shift > 0) + hash |= ((uint32) digits[i]) >> (32 - this_shift); + shift -= 3; + } + + PG_RETURN_UINT32(hash); + } + /* -- * Index: src/include/catalog/pg_amop.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amop.h,v retrieving revision 1.80 diff -c -p -r1.80 pg_amop.h *** src/include/catalog/pg_amop.h 2 Apr 2007 03:49:40 - 1.80 --- src/include/catalog/pg_amop.h 3 May 2007 19:02:44 - *** DATA(insert ( 2232 19 19 1 f 2334 405 *** 568,573 --- 568,575 DATA(insert ( 2235 1033 1033 1 f 974 405 )); /* uuid_ops */ DATA(insert ( 2969 2950 2950 1 f 2972 405 )); + /* numeric_ops */ + DATA(insert ( 1998 1700 1700 1 f 1752 405 )); /* Index: src/include/catalog/pg_amproc.h === RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/catalog/pg_amproc.h,v retrieving revision 1.64 diff -c -p -r1.64 pg_amproc.h *** src/include/catalog/pg_amproc.h 2 Apr 2007 03:49:40 - 1.64 --- src/include/catalog/pg_amproc.h 3 May 2007 19:02:44 - *** DATA(insert ( 1990 26 26 1 453 )); *** 148,153 ---
Re: [PATCHES] CREATE TABLE LIKE INCLUDING INDEXES support
On Wed, 2007-02-05 at 17:09 +0530, NikhilS wrote: > Since this patch is only supposed to copy unique/primary indexes, I > dont think we will ever have predicates associated to such indexes? Nope: neilc=# create table t1 (a int, b int); CREATE TABLE neilc=# create unique index t1_a_idx on t1 ((a + b)) where (a > 5); CREATE INDEX -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] actualised forgotten Magnus's patch for plpgsql MOVE statement
On Fri, 2007-04-20 at 09:46 +0200, Pavel Stehule wrote: > I refreshed Magnus's patch > http://archives.postgresql.org/pgsql-patches/2007-02/msg00275.php from > februar. Applied, thanks. BTW, I notice that the documentation for PL/PgSQL's FETCH command states that only the direction variants that fetch a *single* row are allowed. This is not actually the case: FETCH RELATIVE 2 FROM c INTO v results in assigning the first row from "c" into "v", and then discarding the second row. Is this the best behavior? At the least, we should describe it in the documentation. > p.s. scrollable cursors in plpgsql need little work still. I forgot for > nonstandard (postgresql extension) direction forward all, forward n, > backward n. Forward all propably hasn't sense. Yes, these are certainly needed for MOVE, and we may as well allow them for FETCH as well. -Neil ---(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