Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Andres Freund
On 2017-03-27 13:30:11 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-27 12:18:37 -0400, Tom Lane wrote: > >> My feeling at this point is that we might be better off disabling > >> the computed-goto case by default. At the very least, we're going > >> to need a

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Tom Lane
Andres Freund writes: > On 2017-03-27 12:18:37 -0400, Tom Lane wrote: >> My feeling at this point is that we might be better off disabling >> the computed-goto case by default. At the very least, we're going >> to need a version check that restricts it to latest gcc. > In my

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Andres Freund
On 2017-03-27 12:18:37 -0400, Tom Lane wrote: > I wrote: > > As to the point of whether it actually helps or not ... > > on gcc 4.4.7 (RHEL 6), it makes things *WORSE*. We go from about half of > > the dispatches getting routed through a common location, to *all* of them > > (except one; for some

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Andres Freund
On 2017-03-27 11:52:05 -0400, Tom Lane wrote: > As to the point of whether it actually helps or not ... > > on gcc 6.3.1 (Fedora 25), yes it does. Seems to be one "jmp *something" > per EEO_NEXT or EEO_JUMP. > > on gcc 4.4.7 (RHEL 6), it makes things *WORSE*. We go from about half of > the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Tom Lane
I wrote: > As to the point of whether it actually helps or not ... > on gcc 4.4.7 (RHEL 6), it makes things *WORSE*. We go from about half of > the dispatches getting routed through a common location, to *all* of them > (except one; for some odd reason the first EEO_NEXT in EEOP_NULLIF > survives

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Tom Lane
As to the point of whether it actually helps or not ... on gcc 6.3.1 (Fedora 25), yes it does. Seems to be one "jmp *something" per EEO_NEXT or EEO_JUMP. on gcc 4.4.7 (RHEL 6), it makes things *WORSE*. We go from about half of the dispatches getting routed through a common location, to *all*

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Andres Freund
On 2017-03-27 11:22:40 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-27 09:33:43 -0400, Tom Lane wrote: > >> I think it would, primarily because if we find out that some other compiler > >> spells this differently, we could handle it totally within configure. >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Tom Lane
Andres Freund writes: > On 2017-03-27 09:33:43 -0400, Tom Lane wrote: >> I think it would, primarily because if we find out that some other compiler >> spells this differently, we could handle it totally within configure. > I'm unconvinced that we could sensibly map different

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Andres Freund
On 2017-03-27 09:33:43 -0400, Tom Lane wrote: > Andres Freund writes: > > Checking for this isn't entirely pretty - see my attached attempt at > > doing so. I considered hiding > > __attribute__((optimize("no-crossjumping"))) in execInterpExpr.c behind > > a macro (like

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-27 Thread Tom Lane
Andres Freund writes: >> I personally find per-function annotation ala >> __attribute__((optimize("no-crossjumping"))) >> cleaner anyway. I tested that, and it seems to work. >> >> Obviously we'd have to hide that behind a configure test. Could also do >> tests based on

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-26 Thread Andres Freund
On 2017-03-25 20:59:27 -0700, Andres Freund wrote: > On 2017-03-25 23:51:45 -0400, Tom Lane wrote: > > Andres Freund writes: > > > On March 25, 2017 4:56:11 PM PDT, Ants Aasma wrote: > > >> I haven't had the time to research this properly, but initial

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Andres Freund
On 2017-03-25 23:51:45 -0400, Tom Lane wrote: > Andres Freund writes: > > On March 25, 2017 4:56:11 PM PDT, Ants Aasma wrote: > >> I haven't had the time to research this properly, but initial tests > >> show that with GCC 6.2 adding > >> > >> #pragma

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Tom Lane
Andres Freund writes: > On March 25, 2017 4:56:11 PM PDT, Ants Aasma wrote: >> I haven't had the time to research this properly, but initial tests >> show that with GCC 6.2 adding >> >> #pragma GCC optimize ("no-crossjumping") >> >> fixes merging of the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Andres Freund
On March 25, 2017 4:56:11 PM PDT, Ants Aasma wrote: >On Sun, Mar 26, 2017 at 12:22 AM, Andres Freund >wrote: >>> At least with current gcc (6.3.1 on Fedora 25) at -O2, >>> what I see is multiple places jumping to the same indirect jump >>> instruction

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Ants Aasma
On Sun, Mar 26, 2017 at 12:22 AM, Andres Freund wrote: >> At least with current gcc (6.3.1 on Fedora 25) at -O2, >> what I see is multiple places jumping to the same indirect jump >> instruction :-(. It's not a total disaster: as best I can tell, all the >> uses of EEO_JUMP

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Andres Freund
On 2017-03-25 12:22:15 -0400, Tom Lane wrote: > More random musing ... have you considered making the jump-target fields > in expressions be relative rather than absolute indexes? That is, > EEO_JUMP would look like > > op += (stepno); \ > EEO_DISPATCH(); \ > >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Tom Lane
Andres Freund writes: > - The !caseExpr->defresult result branch is currently unreachable (and > its equivalent was before the patch) because transformCaseExpr() > generates a default expression. I'm inclined to replace the dead code > with an assertion. Any reason not

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Andres Freund
Hi, On 2017-03-24 17:16:15 -0400, Tom Lane wrote: > Attached is an updated patch. I believe this is committable, but > since I whacked it around quite a bit, I'm sure you'll want to look > it over first. Indeed. Points: - It's going to take a while for me to get used to execExprInterp.c - I

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-25 Thread Tom Lane
More random musing ... have you considered making the jump-target fields in expressions be relative rather than absolute indexes? That is, EEO_JUMP would look like op += (stepno); \ EEO_DISPATCH(); \ instead of op = >steps[stepno]; \

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-24 Thread Tom Lane
btw ... I just got around to looking at a code coverage report for this patched version, and that reminded me of something I'd already suspected: EEOP_INNER_SYSVAR and EEOP_OUTER_SYSVAR seem to be dead code. That's unsurprising, because we never try to access a tuple's system columns above the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-24 Thread Tom Lane
Andres Freund writes: > On 2017-03-24 11:26:27 -0400, Tom Lane wrote: >> Another modest proposal: >> >> I'm not really sold on the approach of using EEOP_FETCHSOME opcodes to >> trigger initial tupleslot de-forming. Certainly we want to have a single >> slot_getsomeattrs

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-24 Thread Andres Freund
Hi, On 2017-03-24 11:26:27 -0400, Tom Lane wrote: > Another modest proposal: > > I'm not really sold on the approach of using EEOP_FETCHSOME opcodes to > trigger initial tupleslot de-forming. Certainly we want to have a single > slot_getsomeattrs call per source slot, but as-is, we need a

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-24 Thread Tom Lane
Another modest proposal: I'm not really sold on the approach of using EEOP_FETCHSOME opcodes to trigger initial tupleslot de-forming. Certainly we want to have a single slot_getsomeattrs call per source slot, but as-is, we need a separate traversal over the expression tree just to precompute the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
On 2017-03-23 21:58:03 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-23 21:26:19 -0400, Tom Lane wrote: > >> Hmm, I see ... but that only works in the cases where the caller of > >> ExecBuildProjectionInfo supplied a source slot, and a lot of 'em > >> don't. >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Tom Lane
Andres Freund writes: > On 2017-03-23 21:26:19 -0400, Tom Lane wrote: >> Hmm, I see ... but that only works in the cases where the caller of >> ExecBuildProjectionInfo supplied a source slot, and a lot of 'em >> don't. > Right, the old and new code comment on that: > *

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
On 2017-03-23 21:26:19 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-23 20:36:32 -0400, Tom Lane wrote: > >> The problem here is that once we drop the buffer pin, any pointers we may > >> have into on-disk data are dangling pointers --- we're at risk of some >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Tom Lane
Andres Freund writes: > On 2017-03-23 20:36:32 -0400, Tom Lane wrote: >> The problem here is that once we drop the buffer pin, any pointers we may >> have into on-disk data are dangling pointers --- we're at risk of some >> other backend taking away that shared buffer. (So

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
On 2017-03-23 20:36:32 -0400, Tom Lane wrote: > So I think that we have got to fix ExecEvalWholeRowVar so that it doesn't > clobber the state of the slot. Right at the moment, the only way to do > that seems to be to do this instead of ExecFetchSlotTupleDatum: > > tuple =

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
On 2017-03-23 20:36:32 -0400, Tom Lane wrote: > I found a rather nasty bug :-( ... the comment in EEOP_INNER_VAR_FIRST about > > +/* > + * Can't assert tts_nvalid, as wholerow var evaluation or such > + * could have materialized the slot - but the contents are

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
Hi,m On 2017-03-23 17:40:55 -0400, Tom Lane wrote: > Stylistic thought ... I am wondering if it wouldn't be a good idea > to replace EEOP_CASE_WHEN_STEP, EEOP_CASE_THEN_STEP, EEOP_COALESCE, > and EEOP_ARRAYREF_CHECKINPUT with instructions defined in a less > usage-dependent way as > >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Tom Lane
I found a rather nasty bug :-( ... the comment in EEOP_INNER_VAR_FIRST about +/* + * Can't assert tts_nvalid, as wholerow var evaluation or such + * could have materialized the slot - but the contents are still + * valid :/ + */ +

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Tom Lane
Stylistic thought ... I am wondering if it wouldn't be a good idea to replace EEOP_CASE_WHEN_STEP, EEOP_CASE_THEN_STEP, EEOP_COALESCE, and EEOP_ARRAYREF_CHECKINPUT with instructions defined in a less usage-dependent way as EEOP_JUMP unconditional jump

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Andres Freund
Hi, On 2017-03-23 13:14:59 -0400, Tom Lane wrote: > Looking at some of the coding choices you made here, I see that you're > making a hard assumption that called functions never scribble on their > fcinfo->arg/argnull arrays. I do not believe that we've ever had such > an assumption before. I

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-23 Thread Tom Lane
Looking at some of the coding choices you made here, I see that you're making a hard assumption that called functions never scribble on their fcinfo->arg/argnull arrays. I do not believe that we've ever had such an assumption before. Are we comfortable with that? If so, we'd better document it.

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-22 Thread Andres Freund
On 2017-03-22 13:15:41 -0400, Tom Lane wrote: > BTW, I'm fairly concerned by what you did in nodeTidscan.c, ie delaying > compile of the TID expressions until TidListCreate. I think that probably > fails for cases involving, eg, subplans in the expressions; we need > subplans to get linked to the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-22 Thread Tom Lane
BTW, I'm fairly concerned by what you did in nodeTidscan.c, ie delaying compile of the TID expressions until TidListCreate. I think that probably fails for cases involving, eg, subplans in the expressions; we need subplans to get linked to the parent node, and this way won't do it till (maybe)

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-22 Thread Tom Lane
Andres Freund writes: > On 2017-03-22 10:41:06 -0400, Tom Lane wrote: >> * execQual.c doesn't seem to have a unifying reason to exist anymore. >> It certainly has little to do with evaluating typical qual expressions; >> what's left in there seems to be mostly concerned with

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-22 Thread Andres Freund
On 2017-03-22 10:41:06 -0400, Tom Lane wrote: > I've been busily hacking away on this, trying to make things cleaner and > fix a couple of bugs I stumbled across. (Confusion between ExecQual and > ExecCheck, for instance - we apparently lack regression tests exercising >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-22 Thread Tom Lane
I've been busily hacking away on this, trying to make things cleaner and fix a couple of bugs I stumbled across. (Confusion between ExecQual and ExecCheck, for instance - we apparently lack regression tests exercising constraints-returning-null in corner cases such as table rewrite.) It will

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-21 Thread Tom Lane
Andres Freund writes: > On 2017-03-20 16:06:27 -0400, Tom Lane wrote: >> ... is there a reason why resultnum for EEOP_ASSIGN_* steps is declared >> size_t and not just int? Since it's an array index, and one that >> certainly can't be bigger than AttrNumber, that seems rather

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-21 Thread Andres Freund
On 2017-03-20 16:06:27 -0400, Tom Lane wrote: > ... is there a reason why resultnum for EEOP_ASSIGN_* steps is declared > size_t and not just int? Since it's an array index, and one that > certainly can't be bigger than AttrNumber, that seems rather confusing. Not that I can see, no. I guess I

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-20 Thread Tom Lane
... is there a reason why resultnum for EEOP_ASSIGN_* steps is declared size_t and not just int? Since it's an array index, and one that certainly can't be bigger than AttrNumber, that seems rather confusing. regards, tom lane -- Sent via pgsql-hackers mailing list

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-20 Thread Tom Lane
Andres Freund writes: > Additionally I added a regression test for the nearly entirely untested > nodeTidscan.c, after I'd broken it previously without noticing (thanks > Andreas). I went ahead and pushed this part, since it seemed pretty uncontroversial. I added a bit more

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-20 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 20:09:03 -0400, Tom Lane wrote: >> I think it would be worth creating a README file giving an overview >> of how all of this patch is supposed to work. You also need to do a >> whole lot more work on the function-level comments. > I

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-19 Thread Andres Freund
Hi, On 2017-03-19 23:55:50 -0400, Tom Lane wrote: > Andres Freund writes: > > I've been pondering if we can't entirely get rid of CaseTest etc, the > > amount of hackery required seems not like a good thing. One way I'd > > prototyped was to replace them with PARAM_EXEC

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-19 Thread Tom Lane
Andres Freund writes: > On 2017-03-16 11:15:16 -0400, Tom Lane wrote: >> The thing that actually made the ExecEvalCase code into a bug was that >> we were using ExprContext-level fields to store the current caseValue, >> allowing aliasing to occur across nested CASEs. I think

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-19 Thread Andres Freund
Hi, On 2017-03-16 11:15:16 -0400, Tom Lane wrote: > The thing that actually made the ExecEvalCase code into a bug was that > we were using ExprContext-level fields to store the current caseValue, > allowing aliasing to occur across nested CASEs. I think that in > this implementation, it ought

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-17 Thread Tom Lane
Andres Freund writes: > That said, it seems this is something that has to wait for a later > release, I'm putting back in similar logic as there was before (not a > branch, but change the opcode to a non-checking variant). Yeah, I was wondering if changing the opcode would be

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-17 Thread Andres Freund
Hi, On 2017-03-17 11:36:30 -0400, Tom Lane wrote: > Having said all that, I think that 0001 is contributing very little to the > goals of this patch set. Andres stated that he wanted it so as to drop > some of the one-time checks that execQual.c currently does for Vars, but > I'm not really

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-17 Thread Tom Lane
Andres Freund writes: > [ latest patches ] I looked through 0001 (the composite-type-dependencies one). Although I agree that it'd be good to tighten things up in that area, I do not think we want this specific patch: it tightens things too much. Consider this variant of

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-16 Thread Tom Lane
I wrote: > Andres Freund writes: >> I don't think there's a danger similar to f0c7b789a here, because the >> "caller" (i.e. the node that needs the expression's result) expects >> resvalue/null to be overwritten. > Yeah, that's what I thought when I wrote the broken code in

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-16 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 20:09:03 -0400, Tom Lane wrote: >> That scares me quite a bit, because it smells exactly like the sort of >> premature optimization that bit us on the rear in CVE-2016-5423 (cf commit >> f0c7b789a). > I don't think there's a danger

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andreas Karlsson
Hi, I got a test failure with this version of the patch in the postges_fdw. It looks to me like it was caused by a typo in the source code which is fixed in the attached patch. After applying this patch check-world passes. Andreas diff --git a/src/backend/executor/nodeTidscan.c

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
Hi, On 2017-03-15 20:09:03 -0400, Tom Lane wrote: > Andres Freund writes: > > [ new patches ] > > I've started to look at 0004, and the first conclusion I've come to > is that it's *direly* short of documentation. To the point that I'd > vote against committing it if

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On 2017-03-15 18:48:28 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-15 18:16:57 -0400, Tom Lane wrote: > >> [ scratches head... ] What deforming logic do you think I'm proposing > >> removing? > > > I thought you were suggesting that we don't do the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > [ new patches ] I've started to look at 0004, and the first conclusion I've come to is that it's *direly* short of documentation. To the point that I'd vote against committing it if something isn't done about that. As an example, it's quite unclear

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 18:16:57 -0400, Tom Lane wrote: >> [ scratches head... ] What deforming logic do you think I'm proposing >> removing? > I thought you were suggesting that we don't do the get_last_attnums (and > inlined version in the isSimpleVar case)

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On 2017-03-15 18:16:57 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-15 17:33:46 -0400, Tom Lane wrote: > >> We could make the planner mark each table scan node with the highest > >> column number that the plan will access, and use that to drive a > >>

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 17:33:46 -0400, Tom Lane wrote: >> We could make the planner mark each table scan node with the highest >> column number that the plan will access, and use that to drive a >> slot_getsomeattrs call in advance of any access to tuple

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On 2017-03-15 17:33:46 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-15 16:07:14 -0400, Tom Lane wrote: > >> As for ExecHashGetHashValue, it's most likely going to be working from > >> virtual tuples passed up to the join, which won't benefit from > >>

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 16:07:14 -0400, Tom Lane wrote: >> As for ExecHashGetHashValue, it's most likely going to be working from >> virtual tuples passed up to the join, which won't benefit from >> predetermination of the last column to be accessed. The >>

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On 2017-03-15 16:07:14 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-15 15:41:22 -0400, Tom Lane wrote: > >> Color me dubious. Which specific other places have you got in mind, and > >> do they have expression trees at hand that would tell them which columns >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > On 2017-03-15 15:41:22 -0400, Tom Lane wrote: >> Color me dubious. Which specific other places have you got in mind, and >> do they have expression trees at hand that would tell them which columns >> they really need to pull out? > I was thinking of

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On 2017-03-15 15:41:22 -0400, Tom Lane wrote: > Andres Freund writes: > > On March 15, 2017 12:33:28 PM PDT, Tom Lane wrote: > >> So for my money, you should drop 0002 altogether and just have 0004 > >> remove get_last_attnums() from execUtils.c and stick

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > On March 15, 2017 12:33:28 PM PDT, Tom Lane wrote: >> So for my money, you should drop 0002 altogether and just have 0004 >> remove get_last_attnums() from execUtils.c and stick it into >> execExpr.c. I suppose you still need the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Andres Freund
On March 15, 2017 12:33:28 PM PDT, Tom Lane wrote: >Andres Freund writes: >> [ latest patches ] > >I looked through 0002-Make-get_last_attnums-more-generic.patch. >So for my money, you should drop 0002 altogether and just have 0004 >remove

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-15 Thread Tom Lane
Andres Freund writes: > [ latest patches ] I looked through 0002-Make-get_last_attnums-more-generic.patch. Although it seems relatively unobjectionable on its own, I'm not convinced that it's really useful to try to split it out like this. I see that 0004 removes the only

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Tom Lane
Andres Freund writes: > On 2017-03-14 19:34:12 -0400, Tom Lane wrote: >> It seems bizarre that you chose to spell the new configure symbol as >> HAVE__COMPUTED_GOTO rather than HAVE_COMPUTED_GOTO > I went back-and-forth about this a number of times. We have a bunch of >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
On 2017-03-14 23:10:25 -0400, Peter Eisentraut wrote: > On 3/14/17 19:40, Andres Freund wrote: > > Any idea why we introduce __ stuff? > > Because the symbols start with an underscore: > > /* Define to 1 if your compiler understands _Static_assert. */ > #undef HAVE__STATIC_ASSERT Oh, I guess

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Peter Eisentraut
On 3/14/17 19:40, Andres Freund wrote: > Any idea why we introduce __ stuff? Because the symbols start with an underscore: /* Define to 1 if your compiler understands _Static_assert. */ #undef HAVE__STATIC_ASSERT There is apparently some inconsistency when symbols start with more than one

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
Hi, On 2017-03-14 19:34:12 -0400, Tom Lane wrote: > Andres Freund writes: > > [ new patch versions ] > > About to leave, but I had time to read 0003: > > It seems bizarre that you chose to spell the new configure symbol as > HAVE__COMPUTED_GOTO rather than

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Tom Lane
Andres Freund writes: > [ new patch versions ] About to leave, but I had time to read 0003: It seems bizarre that you chose to spell the new configure symbol as HAVE__COMPUTED_GOTO rather than HAVE_COMPUTED_GOTO, especially so when the comment for PGAC_C_COMPUTED_GOTO

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
On 2017-03-14 14:19:18 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2017-03-14 08:44:24 -0300, Alvaro Herrera wrote: > >> It would be good to have someone at least read it before pushing, but > >> I don't think anyone other than you has done so. > > > I'd love for

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Douglas Doole
On Tue, Mar 14, 2017 at 3:16 PM Andres Freund wrote: > Hm. Right now ExprState's are allocated in several places - but we > could easily move that to a central place. Have a bit of a hard time > seing that that branch during *initialization* time is that expensive, >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
Hi, On 2017-03-14 22:03:45 +, Douglas Doole wrote: > I do have one observation based on my experiments with your first version > of the code. In my tests, I found that expression init becomes a lot more > expensive in this new model. (That's neither a surprise, nor a > concern.) I suspect

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Douglas Doole
Andres, sorry I haven't had a chance to look at this great stuff you've been doing. I've wanted to get to it, but work keeps getting in the way. ;-) I do have one observation based on my experiments with your first version of the code. In my tests, I found that expression init becomes a lot more

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
Hi, On 2017-03-14 20:28:51 +0200, Heikki Linnakangas wrote: > On 03/14/2017 07:31 PM, Andres Freund wrote: > > On 2017-03-14 16:58:54 +0200, Heikki Linnakangas wrote: > > > * How tight are we on space in the ExprEvalStep union? Currently, the > > > jump-threading preparation replaces the opcodes

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Heikki Linnakangas
On 03/14/2017 07:31 PM, Andres Freund wrote: On 2017-03-14 16:58:54 +0200, Heikki Linnakangas wrote: * How tight are we on space in the ExprEvalStep union? Currently, the jump-threading preparation replaces the opcodes with the goto labels, but it would be really nice to keep the original

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Tom Lane
Andres Freund writes: > On 2017-03-14 08:44:24 -0300, Alvaro Herrera wrote: >> It would be good to have someone at least read it before pushing, but >> I don't think anyone other than you has done so. > I'd love for somebody else > to look through it, I tried asking multiple

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
On 2017-03-14 08:44:24 -0300, Alvaro Herrera wrote: > Patch 0003 is huge. I suspect you mean 0004? If so - yes :(. I unfortunately don't think there's a useful way to split it in smaller chunks - I originally moved ops over one-by-one, but that required a lot of duplicated structs and such...

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
On 2017-03-14 08:28:02 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > > EEO_SWITCH(op->opcode) > > { > > EEO_CASE(EEO_DONE): > > goto out; > > Oh my. > > > which is a bit annoying. (the EEO_CASE is either a jump label or a case > > statement, depending on computed goto

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Andres Freund
Hi, On 2017-03-14 16:58:54 +0200, Heikki Linnakangas wrote: > On 03/14/2017 08:53 AM, Andres Freund wrote: > > Besides that, this version has: > > - pgindented most of the affected pieces (i.e. all significant new code > > has been reindent, not all touched one) > > I think you'll need to add

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Heikki Linnakangas
On 03/14/2017 08:53 AM, Andres Freund wrote: Besides that, this version has: - pgindented most of the affected pieces (i.e. all significant new code has been reindent, not all touched one) I think you'll need to add all the inner structs ExprEvalStep typedefs.list to indent them right.

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Alvaro Herrera
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index 3d6a3801c0..d205101b89 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -47,7 +47,14 @@ #include "utils/rel.h" -static bool get_last_attnums(Node *node, ProjectionInfo

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-14 Thread Alvaro Herrera
Andres Freund wrote: > EEO_SWITCH(op->opcode) > { > EEO_CASE(EEO_DONE): > goto out; Oh my. > which is a bit annoying. (the EEO_CASE is either a jump label or a case > statement, depending on computed goto availability). > > It seems we could either: > 1) live with the damage >

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-13 Thread Andres Freund
Hi, On 2017-03-13 01:03:51 -0700, Andres Freund wrote: > What's basically missing here is: > - pgindent run and minimizing resulting damage Running into a bit of an issue here - pgindent mangles something like EEO_SWITCH (op->opcode) { EEO_CASE(EEO_DONE): goto out;

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-13 Thread Tomas Vondra
On 03/13/2017 09:03 AM, Andres Freund wrote: Hi, On 2017-03-12 05:40:51 +0100, Tomas Vondra wrote: I wanted to do a bit of testing and benchmarking on this, but 0004 seems to be a bit broken. Well, "broken" in the sense that it's already outdated, because other stuff that got merged. Yes,

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-13 Thread Konstantin Knizhnik
On 13.03.2017 11:03, Andres Freund wrote: Hi, On 2017-03-12 05:40:51 +0100, Tomas Vondra wrote: I wanted to do a bit of testing and benchmarking on this, but 0004 seems to be a bit broken. Well, "broken" in the sense that it's already outdated, because other stuff that got merged. The

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-11 Thread Tomas Vondra
Hi, On 03/07/2017 04:01 AM, Andres Freund wrote: Hi, Attached is an updated version of the patchset, but more importantly some benchmark numbers. I wanted to do a bit of testing and benchmarking on this, but 0004 seems to be a bit broken. The patch does not apply anymore - there are some

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-11 Thread Robert Haas
On Fri, Mar 10, 2017 at 7:42 PM, Bruce Momjian wrote: > On Fri, Mar 10, 2017 at 07:15:59PM -0500, Peter Eisentraut wrote: >> On 3/10/17 14:40, Andres Freund wrote: >> > I'd really like to get it in. The performance improvements on its own >> > are significant, and it provides

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Bruce Momjian
On Fri, Mar 10, 2017 at 07:15:59PM -0500, Peter Eisentraut wrote: > On 3/10/17 14:40, Andres Freund wrote: > > I'd really like to get it in. The performance improvements on its own > > are significant, and it provides the basis for significantly larger > > improvements again (JIT) - those

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Peter Eisentraut
On 3/10/17 14:40, Andres Freund wrote: > I'd really like to get it in. The performance improvements on its own > are significant, and it provides the basis for significantly larger > improvements again (JIT) - those followup improvements are large patches > again though, so I'd rather not do all

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Peter Eisentraut
On 3/10/17 14:24, Andres Freund wrote: > What problem are you thinking of exactly, and what'd be the solution? > I'd guess that people wouldn't like being unable to change columns in a > table if some function returned the type. Well, that's pretty much the problem. For example: create type t1

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Andres Freund
On 2017-03-10 09:00:14 -0500, Peter Eisentraut wrote: > I have also looked at the 0002 and 0003 patches, and they seem OK, but > they are obviously not of much use without 0004. What is your ambition > for getting 0004 reviewed and committed for PG10? I'd really like to get it in. The

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Andres Freund
On 2017-03-10 08:51:25 -0500, Peter Eisentraut wrote: > On 3/7/17 19:14, Andres Freund wrote: > >> Why shouldn't the function itself also depend on the components of its > >> return type? > > Because that'd make it impossible to change the return type components - > > if the return type is baked

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Peter Eisentraut
I have also looked at the 0002 and 0003 patches, and they seem OK, but they are obviously not of much use without 0004. What is your ambition for getting 0004 reviewed and committed for PG10? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support,

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-10 Thread Peter Eisentraut
On 3/7/17 19:14, Andres Freund wrote: >> Why shouldn't the function itself also depend on the components of its >> return type? > Because that'd make it impossible to change the return type components - > if the return type is baked into the view that's necessary, but for a > "freestanding

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-07 Thread Andres Freund
Hi, On 2017-03-07 18:46:31 -0500, Peter Eisentraut wrote: > I looked over > 0001-Add-expression-dependencies-on-composite-type-whole-.patch. That > seems useful independent of the things you have following. > > Even though it is presented more as a preparatory change, it appears to > have

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-07 Thread Peter Eisentraut
I looked over 0001-Add-expression-dependencies-on-composite-type-whole-.patch. That seems useful independent of the things you have following. Even though it is presented more as a preparatory change, it appears to have noticeable user-facing impact, which we should analyze. For example, in the

Re: [HACKERS] WIP: Faster Expression Processing v4

2017-03-07 Thread Robert Haas
On Mon, Mar 6, 2017 at 10:01 PM, Andres Freund wrote: > My benchmarking script first prewarms the whole database, then runs the > tpch queries in sequence, repeated three times, and compares the shortes > execution time: Those numbers are stupendous. -- Robert Haas

  1   2   >