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
Re: [PATCHES] Avoid needless copy in nodeMaterial
Neil Conway <[EMAIL PROTECTED]> writes: > 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. Seems like this needs more comments about what's happening, rather than less ... 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? regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
[PATCHES] 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] Assertion failure with small block sizes
Gregory Stark <[EMAIL PROTECTED]> writes: > "Tom Lane" <[EMAIL PROTECTED]> writes: >> Hmm. I'm inclined to reverse the tests (there are 3 not just 1) in >> heapam.c, so that it explicitly tries to toast only in plain tables, >> rather than adding more exclusion cases. Thoughts? > Well RELKIND_UNCATALOGED can be usefully toasted in that data can be > compressed internally. But by the time we are inserting any data that needs compression, the relkind should not be that anymore. This would only be an issue if pg_proc.h itself contained DATA() lines long enough to need toasting. I argue that that isn't true and isn't likely to become true. (See ts_debug() for an example of deliberately avoiding such a case...) regards, tom lane ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Assertion failure with small block sizes
"Tom Lane" <[EMAIL PROTECTED]> writes: > Gregory Stark <[EMAIL PROTECTED]> writes: >> If I push the TOAST_TUPLES_PER_PAGE up to 16 I get another failure on the >> same >> line from trying to toast a sequence. If I add RELKIND_SEQUENCE to the >> assertion then it passes all regression tests even if I push >> TOAST_TUPLES_PER_PAGE up to 1024 -- ie, try to toast everything as far as >> possible. Perhaps heapam.c:1761 should just check for RELKIND_SEQUENCE as >> well. > > Hmm. I'm inclined to reverse the tests (there are 3 not just 1) in > heapam.c, so that it explicitly tries to toast only in plain tables, > rather than adding more exclusion cases. Thoughts? Well RELKIND_UNCATALOGED can be usefully toasted in that data can be compressed internally. I suppose we know none normally receive such treatment at normal block sizes. If we want to make the tuple threshold configurable down the line would we want it affecting initdb bootstrapping? My experiments show it isn't a problem but I don't particularly see any reason why it's advantageous. We may want some future relkinds to be toastable but I don't see a problem adding new cases to the test. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com ---(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] Assertion failure with small block sizes
Gregory Stark <[EMAIL PROTECTED]> writes: > If I push the TOAST_TUPLES_PER_PAGE up to 16 I get another failure on the same > line from trying to toast a sequence. If I add RELKIND_SEQUENCE to the > assertion then it passes all regression tests even if I push > TOAST_TUPLES_PER_PAGE up to 1024 -- ie, try to toast everything as far as > possible. Perhaps heapam.c:1761 should just check for RELKIND_SEQUENCE as > well. Hmm. I'm inclined to reverse the tests (there are 3 not just 1) in heapam.c, so that it explicitly tries to toast only in plain tables, rather than adding more exclusion cases. Thoughts? regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] quote_literal with NULL
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > I'm all for the prevalance of sanity, but I'm not really clear on what > about the above scenario is not sane. Well, a situation like that just calls into question whether there's been a mistake --- in particular whether the underlying function is really null-safe or not. We could remove the opr_sanity test, but to me that'd be akin to disabling a compiler warning. Our project policy has generally been to write warning-free code, instead. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Updated patch for tsearch contrib examples
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > Isn't dict_xsym like the built in synonym dictionary, but with support > for multiple replacement words? Why don't we just replace the built in > synonym dictionary with this? Well, we'd still need a contrib example that uses a config file, so that there's an example of the right way to do that. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Updated patch for tsearch contrib examples
Tom Lane wrote: > I've reviewed and tested Sergey Karpov's proposed contrib modules > dict_int, dict_xsyn, and test_parser. Isn't dict_xsym like the built in synonym dictionary, but with support for multiple replacement words? Why don't we just replace the built in synonym dictionary with this? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] quote_literal with NULL
On 10/12/07, Simon Riggs <[EMAIL PROTECTED]> wrote: > I think you should add some examples to show how we would handle an > INSERT or an UPDATE SET with quite_nullable() and a SELECT WHERE clause > with quote_literal. The difference is a subtle one, which is why nobody > mentioned it before, so it needs some better docs too. > > A cross-ref to the functions page would help also. Alright, I've improved the documentation along the lines suggested by Simon. There's a full example on doing a null-safe dynamic UPDATE, as well as a brief discussion about being wary of using comparison operators with NULLs (e.g., in WHERE clauses). Cross references abound. I did make a version of the patch which has the pg_proc entries for quote_literal and quote_nullable both pointing to the same internal function. I thought this was a tidier solution, but it failed regression test #5 in opr_sanity; apparently two entries in pg_proc can't have the same prosrc and differing proisstrict? Cheers, BJ quote-nullable_1.diff.bz2 Description: BZip2 compressed data ---(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
