Amit Langote writes:
> On 2018/07/16 2:02, Tom Lane wrote:
>> BTW, there'd be a lot to be said for having InitPlan just open all
>> the rels and build an array of Relation pointers that parallels the
>> RTE list, rather than doing heap_opens in random places elsewhere.
> +1 to this. Actually I
On 2018/07/16 2:02, Tom Lane wrote:
> Amit Langote writes:
>> On 2018/06/19 2:05, Tom Lane wrote:
>>> Or maybe what we should do is drop ExecLockNonLeafAppendTables/
>>> ExecOpenAppendPartitionedTables entirely and teach InitPlan to do it.
>
>> Hmm, for InitPlan to do what
On Sun, Jul 15, 2018 at 8:24 PM, David Rowley
wrote:
>> +1. In fact, maybe we ought to go a little further and have a
>> relation_reopen(oid, mode) that verifies that a lock in the specified
>> mode is held.
>
> Wouldn't it be better to just store the Relation indexed by its relid
> somewhere
David Rowley writes:
> On 16 July 2018 at 12:12, Robert Haas wrote:
>> On Sun, Jul 15, 2018 at 1:02 PM, Tom Lane wrote:
>>> What we'd be better off doing, if we go this route, is to install an
>>> assertion-build-only test that verifies during relation_open(NoLock)
>>> that some kind of lock is
On 16 July 2018 at 12:12, Robert Haas wrote:
> On Sun, Jul 15, 2018 at 1:02 PM, Tom Lane wrote:
>> What we'd be better off doing, if we go this route, is to install an
>> assertion-build-only test that verifies during relation_open(NoLock)
>> that some kind of lock is already held on the rel.
On Sun, Jul 15, 2018 at 1:02 PM, Tom Lane wrote:
> What we'd be better off doing, if we go this route, is to install an
> assertion-build-only test that verifies during relation_open(NoLock)
> that some kind of lock is already held on the rel. That would protect
> not only the executor, but a
On 2018/06/19 2:05, Tom Lane wrote:
> Amit Langote writes:
>> [ 0001-Open-partitioned-tables-during-Append-initialization.patch ]
>
> I took a look at this. While I'm in agreement with the general idea of
> holding open the partitioned relations' relcache entries throughout the
> query, I do
Amit Langote writes:
> [ 0001-Open-partitioned-tables-during-Append-initialization.patch ]
I took a look at this. While I'm in agreement with the general idea of
holding open the partitioned relations' relcache entries throughout the
query, I do not like anything about this patch:
* It seems
On 06/15/2018 10:02 AM, Tom Lane wrote:
Andrew Dunstan writes:
OK, lousyjack is back online with this, new and improved. It currently
takes 7.5 hours for a run. Should I also add -DCATCACHE_FORCE_RELEASE?
I did some experimentation yesterday with valgrind plus both
RELCACHE_FORCE_RELEASE
Andrew Dunstan writes:
> OK, lousyjack is back online with this, new and improved. It currently
> takes 7.5 hours for a run. Should I also add -DCATCACHE_FORCE_RELEASE?
I did some experimentation yesterday with valgrind plus both
RELCACHE_FORCE_RELEASE and CATCACHE_FORCE_RELEASE. I didn't
On 06/12/2018 07:47 AM, Andrew Dunstan wrote:
On 06/11/2018 06:41 PM, Andrew Dunstan wrote:
On 06/11/2018 06:24 PM, Tom Lane wrote:
If we had any buildfarm critters running valgrind on
RELCACHE_FORCE_RELEASE or CLOBBER_CACHE_ALWAYS builds, they'd have
detected use of uninitialized
On 2018/06/14 20:23, David Rowley wrote:
> On 14 June 2018 at 19:17, Amit Langote wrote:
>> I had sent a patch to try to get rid of the open(NoLock) there a couple of
>> months ago [1]. The idea was to both lock and open the relation in
>> ExecNonLeafAppendTables, which is the first time all
On 2018/06/14 23:40, Tom Lane wrote:
> Robert Haas writes:
>> On Thu, Jun 14, 2018 at 7:23 AM, David Rowley
>> wrote:
>>> However, I only spent about 10 mins looking into this, there may be
>>> some giant holes in the idea. It would need much more research.
>
>> It kind of flies in the face of
Robert Haas writes:
> On Thu, Jun 14, 2018 at 7:23 AM, David Rowley
> wrote:
>> However, I only spent about 10 mins looking into this, there may be
>> some giant holes in the idea. It would need much more research.
> It kind of flies in the face of the idea that a RangeTblEntry is just
> a
On Thu, Jun 14, 2018 at 7:23 AM, David Rowley
wrote:
> However, I only spent about 10 mins looking into this, there may be
> some giant holes in the idea. It would need much more research.
It kind of flies in the face of the idea that a RangeTblEntry is just
a node that can be freely copied
On 14 June 2018 at 19:17, Amit Langote wrote:
> I had sent a patch to try to get rid of the open(NoLock) there a couple of
> months ago [1]. The idea was to both lock and open the relation in
> ExecNonLeafAppendTables, which is the first time all partitioned tables in
> a given Append node are
On 14 June 2018 at 04:10, Tom Lane wrote:
> There's still one thing I'm a bit confused about here. I noticed that
> we weren't actually using the partopfamily and partopcintype fields in
> PartitionPruneContext, so I removed them. But that still leaves both
> partsupfunc and partcollation as
On 2018/06/13 23:39, Tom Lane wrote:
> Robert Haas writes:
>> Seems reasonable. Really, I think we should look for a way to hang
>> onto the relation at the point where it's originally opened and locked
>> instead of reopening it here. But that's probably more invasive than
>> we can really
David Rowley writes:
> On 13 June 2018 at 16:15, Tom Lane wrote:
>> It seems not to be that bad: we just need a shutdown call for the
>> PartitionPruneState, and then we can remember the open relation there.
>> The attached is based on David's patch from yesterday.
> I've looked over this and
On Wed, Jun 13, 2018 at 12:15 AM, Tom Lane wrote:
>> As for whether to change it at this point in the release cycle, I
>> guess that, to me, depends on how invasive the fix is.
>
> It seems not to be that bad: we just need a shutdown call for the
> PartitionPruneState, and then we can remember
On 13 June 2018 at 16:15, Tom Lane wrote:
> It seems not to be that bad: we just need a shutdown call for the
> PartitionPruneState, and then we can remember the open relation there.
> The attached is based on David's patch from yesterday.
>
> I'm still a bit annoyed at the fmgr_info_copy calls
Robert Haas writes:
> On Tue, Jun 12, 2018 at 12:54 PM, Tom Lane wrote:
>> While I've not looked into the exact reasons for that, my first guess
>> is that the partitioned table is not held open because it's not one
>> of the ones to be scanned. Are you prepared to change something like
>> that
On Tue, Jun 12, 2018 at 12:54 PM, Tom Lane wrote:
> Testing with valgrind + RELCACHE_FORCE_RELEASE is sufficient to disprove
> that, cf current results from lousyjack (which match my own testing).
> The partkey *is* disappearing under us.
>
> While I've not looked into the exact reasons for that,
Robert Haas writes:
> And there is
> code in RelationClearRelation to avoid changing rd_partkey and
> rd_partdesc if no logical change has occurred.
Oh, and by the way, what's actually in there is
keep_partkey = (relation->rd_partkey != NULL);
I would be interested to see an
Robert Haas writes:
> I think we DO hold relations open for the duration of execution
> (though not necessarily between planning and execution). And there is
> code in RelationClearRelation to avoid changing rd_partkey and
> rd_partdesc if no logical change has occurred.
Testing with valgrind +
On Tue, Jun 12, 2018 at 12:25 PM, Tom Lane wrote:
> Robert Haas writes:
>> On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane wrote:
>>> Not sure about a good fix for this. It seems annoying to copy the
>>> rel's whole partkey data structure into query-local storage, but
>>> I'm not sure we have any
Robert Haas writes:
> On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane wrote:
>> Not sure about a good fix for this. It seems annoying to copy the
>> rel's whole partkey data structure into query-local storage, but
>> I'm not sure we have any choice. On the bright side, there might
>> be an
On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane wrote:
> Not sure about a good fix for this. It seems annoying to copy the
> rel's whole partkey data structure into query-local storage, but
> I'm not sure we have any choice. On the bright side, there might
> be an opportunity to get rid of repeated
On Tue, Jun 12, 2018 at 3:54 AM, Tom Lane wrote:
>
> Not sure about a good fix for this. It seems annoying to copy the
> rel's whole partkey data structure into query-local storage, but
> I'm not sure we have any choice. On the bright side, there might
> be an opportunity to get rid of repeated
On 06/11/2018 06:41 PM, Andrew Dunstan wrote:
On 06/11/2018 06:24 PM, Tom Lane wrote:
If we had any buildfarm critters running valgrind on
RELCACHE_FORCE_RELEASE or CLOBBER_CACHE_ALWAYS builds, they'd have
detected use of uninitialized memory here ... but I don't think we have
any. (The
On 12 June 2018 at 10:24, Tom Lane wrote:
> After looking closer, that code isn't just inefficient, it's flat
> out broken. The reason is that ExecSetupPartitionPruneState thinks
> it can store some pointers into the target relation's relcache entry
> in the PartitionPruneContext, and then
Andrew Dunstan writes:
> On 06/11/2018 06:24 PM, Tom Lane wrote:
>> If we had any buildfarm critters running valgrind on
>> RELCACHE_FORCE_RELEASE or CLOBBER_CACHE_ALWAYS builds, they'd have
>> detected use of uninitialized memory here ... but I don't think we have
>> any. (The combination of
On 06/11/2018 06:24 PM, Tom Lane wrote:
If we had any buildfarm critters running valgrind on
RELCACHE_FORCE_RELEASE or CLOBBER_CACHE_ALWAYS builds, they'd have
detected use of uninitialized memory here ... but I don't think we have
any. (The combination of valgrind and CCA would probably be
I wrote:
> * I'm fairly suspicious of the fmgr_info and fmgr_info_copy calls in
> perform_pruning_base_step. Those seem likely to leak memory, and
> for sure they destroy any opportunity for the called comparison
> function to cache info in fn_extra --- something that's critical
> for performance
David Rowley writes:
> On 12 June 2018 at 02:26, Tom Lane wrote:
>>> ... I'd previously tried having NULL subnodes in
>>> the Append and I thought it required too much hacking work in
>>> explain.c,
>> No, that was pretty much exactly what I was envisioning.
> What you're proposing exchanges
David Rowley writes:
> On 11 June 2018 at 10:49, Tom Lane wrote:
>> * The business with ExecFindInitialMatchingSubPlans remapping the
>> subplan indexes seems very dubious to me. Surely, that is adding
>> way more complexity and fragility than it's worth.
> I think having the ability to prune
Thanks for working on and pushing those fixes.
On 11 June 2018 at 10:49, Tom Lane wrote:
> It's very unclear for example what the subplan_map and subpart_map
> arrays really are, eg what are they indexed by? I get the impression
> that only one of them can have a non-minus-1 value for a given
David Rowley writes:
> I've made a pass over the execPartition.c and partprune.c code
> attempting to resolve these issues. I have hopefully fixed them all,
> but I apologise if I've missed any.
> I also couldn't resist making a few other improvements to the code.
By the time this arrived, I'd
On 10 June 2018 at 09:00, Tom Lane wrote:
> I wrote:
>> One thing I'm wondering about is why in the world are PartitionPruneInfo
>> and its subsidiary struct types declared in primnodes.h?
That may have been a legacy thing that accidentally wasn't changed
from a previous version of the patch.
I wrote:
> One thing I'm wondering about is why in the world are PartitionPruneInfo
> and its subsidiary struct types declared in primnodes.h?
Oh, and while I'm bitching: it seems like there is hardly any part of
the partitioning code in which the comments aren't desperately in need
of a
David Rowley writes:
> I'm really hoping this is what you meant about the special-case code for
> Params.
> Does this look any better?
I'm starting to look this over and it seems like generally the right
thing, though I'm finding minor things I don't like much.
One thing I'm wondering about is
On 8 June 2018 at 18:14, David Rowley wrote:
> On 8 June 2018 at 15:22, Tom Lane wrote:
>> David Rowley writes:
>>> On 8 June 2018 at 03:43, Tom Lane wrote:
Maybe there's something I'm missing here, but I sort of hoped that this
patch would nuke all the special-case code for Params
On 8 June 2018 at 15:22, Tom Lane wrote:
> David Rowley writes:
>> On 8 June 2018 at 03:43, Tom Lane wrote:
>>> Maybe there's something I'm missing here, but I sort of hoped that this
>>> patch would nuke all the special-case code for Params in this area.
>>> Why is there any need to
On Fri, Jun 8, 2018 at 8:52 AM, Tom Lane wrote:
> David Rowley writes:
>> On 8 June 2018 at 03:43, Tom Lane wrote:
>>> Maybe there's something I'm missing here, but I sort of hoped that this
>>> patch would nuke all the special-case code for Params in this area.
>>> Why is there any need to
David Rowley writes:
> On 8 June 2018 at 03:43, Tom Lane wrote:
>> Maybe there's something I'm missing here, but I sort of hoped that this
>> patch would nuke all the special-case code for Params in this area.
>> Why is there any need to distinguish them from other stable expressions?
> We need
On 8 June 2018 at 03:43, Tom Lane wrote:
> Maybe there's something I'm missing here, but I sort of hoped that this
> patch would nuke all the special-case code for Params in this area.
> Why is there any need to distinguish them from other stable expressions?
>
> IOW, I was hoping for the code to
David Rowley writes:
> On 6 June 2018 at 18:05, Amit Langote wrote:
>> I wonder why we need to create those Bitmapsets in the planner? Why not
>> in ExecSetupPartitionPruneState()? For example, like how
>> context->exprstates is initialized.
> That seems like a good idea. Certainly much
On Thu, Jun 7, 2018 at 8:51 AM, David Rowley
wrote:
> On 7 June 2018 at 14:51, Amit Langote wrote:
>> Thanks David. This one looks good. I also like it that hasparamlessexprs
>> is no longer determined and set in the planner.
>
> Thanks for checking it.
>
>> I checked what happens with the
On 7 June 2018 at 14:51, Amit Langote wrote:
> Thanks David. This one looks good. I also like it that hasparamlessexprs
> is no longer determined and set in the planner.
Thanks for checking it.
> I checked what happens with the cases that Ashutosh complained about
> upthread and seems that
On 2018/06/06 18:52, David Rowley wrote:
> On 6 June 2018 at 18:05, Amit Langote wrote:
>> On 2018/06/06 14:10, David Rowley wrote:
>>> I then decided that
>>> I didn't like the way we need to check which params are in the Expr
>>> each time we call partkey_datum_from_expr. It seems better to
On 6 June 2018 at 18:05, Amit Langote wrote:
> On 2018/06/06 14:10, David Rowley wrote:
>> I then decided that
>> I didn't like the way we need to check which params are in the Expr
>> each time we call partkey_datum_from_expr. It seems better to prepare
>> this in advance when building the
On 2018/06/06 14:10, David Rowley wrote:
> I then decided that
> I didn't like the way we need to check which params are in the Expr
> each time we call partkey_datum_from_expr. It seems better to prepare
> this in advance when building the pruning steps. I started work on
> that, but soon
On 6 June 2018 at 03:07, David Rowley wrote:
> Please see the attached patch. I've only just finished with it and
> it's not fully done yet as there's still an XXX comment where I've not
> quite thought about Exprs with Vars from higher levels. These might
> always be converted to Params, so the
2018-06-05 17:07 GMT+02:00 David Rowley :
> On 5 June 2018 at 22:31, Amit Langote
> wrote:
> > Maybe, David (added to cc) has something to say about all this,
> especially
> > whether he considers this a feature and not a bug fix.
>
> Thanks, Amit. I had missed this thread.
>
> Yeah. I admit if
On Tue, Jun 5, 2018 at 6:24 PM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> On 5 June 2018 at 12:31, Amit Langote wrote:
>>
>> doesn't look quite right. What says expr is really a Param? The patch
>> appears to work because, by setting pinfo->execparams to *something*, it
>> triggers
Hi Dmitry,
Thanks for creating the patch. I looked at it and have some comments.
On 2018/06/04 22:30, Dmitry Dolgov wrote:
>> On 3 June 2018 at 19:11, Tom Lane wrote:
>> Dmitry Dolgov <9erthali...@gmail.com> writes:
>>> Just to clarify for myself, for evaluating any stable function here would
> On 3 June 2018 at 19:11, Tom Lane wrote:
> Dmitry Dolgov <9erthali...@gmail.com> writes:
>> Just to clarify for myself, for evaluating any stable function here would it
>> be
>> enough to handle all function-like expressions (FuncExpr / OpExpr /
>> DistinctExpr / NullIfExpr) and check a
Dmitry Dolgov <9erthali...@gmail.com> writes:
> Just to clarify for myself, for evaluating any stable function here would it
> be
> enough to handle all function-like expressions (FuncExpr / OpExpr /
> DistinctExpr / NullIfExpr) and check a corresponding function for provolatile,
> like in the
> On 1 June 2018 at 17:53, Tom Lane wrote:
> Ashutosh Bapat writes:
>> I think the patch is right if we were to handle only SQLValueFunction,
>> but the bigger picture here is that we aren't evaluating stable
>> functions before run-time partition pruning happens.
>
> I agree though that it
Jeff Janes writes:
> On Fri, Jun 1, 2018 at 11:53 AM, Tom Lane wrote:
>> It's worth questioning whether this is a bug fix or an improvement.
>> If the latter, it probably ought to wait for v12.
> If explaining the change requires reference to tokens from the source code,
> rather than something
On Sat, Jun 2, 2018 at 5:16 PM, Jeff Janes wrote:
> On Fri, Jun 1, 2018 at 11:53 AM, Tom Lane wrote:
>>
>>
>> I agree though that it seems strange to special-case SQLValueFunction
>> rather than any-stable-expression. As long as the evaluation happens
>> at executor start (i.e. with the query's
On Fri, Jun 1, 2018 at 11:53 AM, Tom Lane wrote:
>
> I agree though that it seems strange to special-case SQLValueFunction
> rather than any-stable-expression. As long as the evaluation happens
> at executor start (i.e. with the query's run-time snapshot) it should
> be reasonable to simplify
2018-06-01 17:53 GMT+02:00 Tom Lane :
> Ashutosh Bapat writes:
> > I think the patch is right if we were to handle only SQLValueFunction,
> > but the bigger picture here is that we aren't evaluating stable
> > functions before run-time partition pruning happens. I was under the
> > impression
Ashutosh Bapat writes:
> I think the patch is right if we were to handle only SQLValueFunction,
> but the bigger picture here is that we aren't evaluating stable
> functions before run-time partition pruning happens. I was under the
> impression that the stable functions/expressions get evaluated
On Fri, Jun 1, 2018 at 9:47 AM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> On 1 June 2018 at 07:19, Pavel Stehule wrote:
>>
>> Partition pruning is working now.
>>
>> Is it expected? Tested on fresh master.
>
> That's interesting. So there are two cases:
>
> * vlozeno > (select current_date)
> On 1 June 2018 at 07:19, Pavel Stehule wrote:
>
> Partition pruning is working now.
>
> Is it expected? Tested on fresh master.
That's interesting. So there are two cases:
* vlozeno > (select current_date) (pruning works)
* vlozeno > current_date (pruning doesn't work)
In
66 matches
Mail list logo
