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 version check that r
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 measurements it's st
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
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 disp
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
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* of
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.
>
> > I'm unconvinced
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 compiler's options
>
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 PG_DISABLE_CROSSJUMPING),
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 __GNUC__ / __GNUC_MINOR_
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 tests
> > >> show that with GCC 6.2 adding
>
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 GCC optimize ("no-crossjumping")
> >>
> >>
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 op tail jumps.
>>
>> Some quick and dirt
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 :-(. It's not a total disaster: as best I
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 remain distinct. But
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(); \
>
> instea
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 to do that?
Ah-hah.
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
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 = &state->steps[stepno]; \
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 sca
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 call per source slot, bu
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 separa
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
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.
>
> > Right, the old a
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:
> * inputDesc can be NULL, bu
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
> >> other backend taki
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 I'm afraid that the
>>
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 = ExecCopySlotTuple(sl
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 s
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
>
> EEOP_
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 :/
+ */
+
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
EEOP_JUMP_IF_NUL
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 th
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.
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
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) too
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 SRFs. I feel
>> like
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
> constraints-returning-null
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 prob
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 confusing.
> Not th
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 m
... 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 (pgs
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 stuff to get the LOC m
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 tried to improve upon bot
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 nodes - then the whole
>
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 that in
>> this impl
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 to
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 preferable to
a firs
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 convi
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 the existing test case
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 ExecEvalCase,
> too.
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 similar to f0c7b789a here,
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 b/src/backend
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 something isn't done about t
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 get_last_attnums (and
> > inl
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 how ExprEvalSteps acqu
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) at execution time anym
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
> >> slot_getsomeattrs call in adv
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 contents.
> probably isn't
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
> >> predetermination of the last
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
>> tuple-deconstruction woul
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
> >> they really need
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 execGrouping.c's execT
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 it into
> >> execExpr.c. I suppose you s
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 LastAttnumInfo API change
>> so as to deco
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 get_last_attnums() from execUtils.c and stick it into
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 call of ExecGetLastAttnu
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
> symbols defined with HAVE
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 tha
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 unders
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 HAVE_COMPUTED_GOTO
I went back-and-fo
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 alleges it's the latter.
Al
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 somebody else
> > to lo
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,
> especially given that previ
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 tha
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
e
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 w
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 opcode
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 times... Seems like
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...
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
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
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.
My
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
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
>
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;
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,
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 pat
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
c
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 the basis for signif
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 followup
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 o
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 a
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 performanc
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 in
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, Remot
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 function
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 notice
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
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
EnterpriseDB: http://www.ente
100 matches
Mail list logo
