On Sun, May 19, 2024 at 9:39 AM David Rowley wrote:
> For #1, the locks taken for SELECT queries are less likely to conflict
> with other locks obtained by PostgreSQL, but at least at the moment if
> someone is getting deadlocks with a DDL type operation, they can
> change their query or DDL
On Sun, 19 May 2024 at 13:27, Tom Lane wrote:
>
> David Rowley writes:
> > 1. No ability to control the order that the locks are obtained. The
> > order in which the locks are taken will be at the mercy of the plan
> > the planner chooses.
>
> I do not think I buy this argument, because
David Rowley writes:
> With the caveat of not yet having looked at the latest patch, my
> thoughts are that having the executor startup responsible for taking
> locks is a bad idea and I don't think we should go down this path.
OK, it's certainly still up for argument, but ...
> 1. No ability
On Fri, 20 Jan 2023 at 08:39, Tom Lane wrote:
> I spent some time re-reading this whole thread, and the more I read
> the less happy I got. We are adding a lot of complexity and introducing
> coding hazards that will surely bite somebody someday. And after awhile
> I had what felt like an
Hi Andrey,
On Sun, Mar 31, 2024 at 2:03 PM Andrey M. Borodin wrote:
> > On 6 Dec 2023, at 23:52, Robert Haas wrote:
> >
> > I hope that it's at least somewhat useful.
>
> > On 5 Jan 2024, at 15:46, vignesh C wrote:
> >
> > There is a leak reported
>
> Hi Amit,
>
> this is a kind reminder that
> On 6 Dec 2023, at 23:52, Robert Haas wrote:
>
> I hope that it's at least somewhat useful.
>
> On 5 Jan 2024, at 15:46, vignesh C wrote:
>
> There is a leak reported
Hi Amit,
this is a kind reminder that some feedback on your patch[0] is waiting for your
reply.
Thank you for your
On Mon, 20 Nov 2023 at 10:00, Amit Langote wrote:
>
> On Thu, Sep 28, 2023 at 5:26 PM Amit Langote wrote:
> > On Tue, Sep 26, 2023 at 10:06 PM Amit Langote
> > wrote:
> > > After sleeping on this, I think we do need the checks after all the
> > > ExecInitNode() calls too, because we have many
Reviewing 0001:
Perhaps ExecEndCteScan needs an adjustment. What if node->leader was never set?
Other than that, I think this is in good shape. Maybe there are other
things we'd want to adjust here, or maybe there aren't, but there
doesn't seem to be any good reason to bundle more changes into
On Wed, Sep 6, 2023 at 11:20 PM Robert Haas wrote:
> On Wed, Sep 6, 2023 at 5:12 AM Amit Langote wrote:
> > Attached updated patches. Thanks for the review.
>
> I think 0001 looks ready to commit. I'm not sure that the commit
> message needs to mention future patches here, since this code
On Wed, Sep 6, 2023 at 5:12 AM Amit Langote wrote:
> Attached updated patches. Thanks for the review.
I think 0001 looks ready to commit. I'm not sure that the commit
message needs to mention future patches here, since this code cleanup
seems like a good idea regardless, but if you feel
On Tue, Sep 5, 2023 at 3:13 AM Amit Langote wrote:
> Attached 0001 removes unnecessary cleanup calls from ExecEnd*() routines.
It also adds a few random Assert()s to verify that unrelated pointers
are not NULL. I suggest that it shouldn't do that.
The commit message doesn't mention the removal
On Fri, Aug 11, 2023 at 9:50 AM Amit Langote wrote:
> After removing the unnecessary cleanup code from most node types’ ExecEnd*
> functions, one thing I’m tempted to do is remove the functions that do
> nothing else but recurse to close the outerPlan, innerPlan child nodes. We
> could
On Fri, Aug 11, 2023 at 14:31 Amit Langote wrote:
> On Wed, Aug 9, 2023 at 1:05 AM Robert Haas wrote:
> > On Tue, Aug 8, 2023 at 10:32 AM Amit Langote
> wrote:
> > > But should ExecInitNode() subroutines return the partially initialized
> > > PlanState node or NULL on detecting invalidation?
On Tue, Aug 8, 2023 at 10:32 AM Amit Langote wrote:
> But should ExecInitNode() subroutines return the partially initialized
> PlanState node or NULL on detecting invalidation? If I'm
> understanding how you think this should be working correctly, I think
> you mean the former, because if it
On Tue, Aug 8, 2023 at 12:36 AM Robert Haas wrote:
> On Thu, Aug 3, 2023 at 4:37 AM Amit Langote wrote:
> > Here's a patch set where the refactoring to move the ExecutorStart()
> > calls to be closer to GetCachedPlan() (for the call sites that use a
> > CachedPlan) is extracted into a separate
On Mon, Aug 7, 2023 at 11:44 AM Tom Lane wrote:
> Right, I doubt that changing that is going to work out well.
> Hash joins might have issues with it too.
I thought about the case, because Hash and Hash Join are such closely
intertwined nodes, but I don't see any problem there. It doesn't
really
Robert Haas writes:
> Second, I wondered whether the ordering of cleanup operations could be
> an issue. Right now, a node can position cleanup code before, after,
> or both before and after recursing to child nodes, whereas with this
> design change, the cleanup code will always be run before
On Thu, Aug 3, 2023 at 4:37 AM Amit Langote wrote:
> Here's a patch set where the refactoring to move the ExecutorStart()
> calls to be closer to GetCachedPlan() (for the call sites that use a
> CachedPlan) is extracted into a separate patch, 0002. Its commit
> message notes an aspect of this
On Tue, 18 Jul 2023, 08:26 Amit Langote, wrote:
> Hi Thom,
>
> On Tue, Jul 18, 2023 at 1:33 AM Thom Brown wrote:
> > On Thu, 13 Jul 2023 at 13:59, Amit Langote
> wrote:
> > > In an absolutely brown-paper-bag moment, I realized that I had not
> > > updated src/backend/executor/README to reflect
On Thu, 13 Jul 2023 at 13:59, Amit Langote wrote:
> In an absolutely brown-paper-bag moment, I realized that I had not
> updated src/backend/executor/README to reflect the changes to the
> executor's control flow that this patch makes. That is, after
> scrapping the old design back in January
> On 8 Jun 2023, at 16:23, Amit Langote wrote:
>
> Here is a new version.
The local planstate variable in the hunk below is shadowing the function
parameter planstate which cause a compiler warning:
@@ -1495,18 +1556,15 @@ ExecEndPlan(PlanState *planstate, EState *estate)
ListCell
On Tue, Apr 4, 2023 at 10:29 PM Amit Langote
wrote:
> On Tue, Apr 4, 2023 at 6:41 AM Tom Lane wrote:
> > A few concrete thoughts:
> >
> > * I understand that your plan now is to acquire locks on all the
> > originally-named tables, then do permissions checks (which will
> > involve only those
On Tue, Apr 4, 2023 at 6:41 AM Tom Lane wrote:
> Amit Langote writes:
> > [ v38 patchset ]
>
> I spent a little bit of time looking through this, and concluded that
> it's not something I will be wanting to push into v16 at this stage.
> The patch doesn't seem very close to being committable on
Amit Langote writes:
> [ v38 patchset ]
I spent a little bit of time looking through this, and concluded that
it's not something I will be wanting to push into v16 at this stage.
The patch doesn't seem very close to being committable on its own
terms, and even if it was now is not a great time
On Wed, Feb 8, 2023 at 7:31 PM Amit Langote wrote:
> On Tue, Feb 7, 2023 at 23:38 Andres Freund wrote:
>> The tests seem to frequently hang on freebsd:
>> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478
>
> Thanks for the heads up. I’ve noticed this one too,
On Tue, Feb 7, 2023 at 23:38 Andres Freund wrote:
> Hi,
>
> On 2023-02-03 22:01:09 +0900, Amit Langote wrote:
> > I've added a test case under src/modules/delay_execution by adding a
> > new ExecutorStart_hook that works similarly as
> > delay_execution_planner(). The test works by allowing a
Hi,
On 2023-02-03 22:01:09 +0900, Amit Langote wrote:
> I've added a test case under src/modules/delay_execution by adding a
> new ExecutorStart_hook that works similarly as
> delay_execution_planner(). The test works by allowing a concurrent
> session to drop an object being referenced in a
On Thu, Feb 2, 2023 at 11:49 PM Amit Langote wrote:
> On Fri, Jan 27, 2023 at 4:01 PM Amit Langote wrote:
> > I didn't actually go with calling the plancache on every lock taken on
> > a relation, that is, in ExecGetRangeTableRelation(). One thing about
> > doing it that way that I didn't quite
On Fri, Jan 27, 2023 at 4:01 PM Amit Langote wrote:
> On Fri, Jan 20, 2023 at 12:52 PM Amit Langote wrote:
> > Alright, I'll try to get something out early next week. Thanks for
> > all the pointers.
>
> Sorry for the delay. Attached is what I've come up with so far.
>
> I didn't actually go
On Fri, Jan 20, 2023 at 12:58 PM Tom Lane wrote:
> Amit Langote writes:
> > On Fri, Jan 20, 2023 at 12:31 PM Tom Lane wrote:
> >> It might be possible to incorporate this pointer into PlannedStmt
> >> instead of passing it separately.
>
> > Yeah, that would be less churn. Though, I wonder if
Amit Langote writes:
> On Fri, Jan 20, 2023 at 12:31 PM Tom Lane wrote:
>> It might be possible to incorporate this pointer into PlannedStmt
>> instead of passing it separately.
> Yeah, that would be less churn. Though, I wonder if you still hold
> that PlannedStmt should not be scribbled upon
On Fri, Jan 20, 2023 at 12:31 PM Tom Lane wrote:
> Amit Langote writes:
> > On Fri, Jan 20, 2023 at 4:39 AM Tom Lane wrote:
> >> I had what felt like an epiphany: the whole problem arises because the
> >> system is wrongly factored. We should get rid of AcquireExecutorLocks
> >> altogether,
Amit Langote writes:
> On Fri, Jan 20, 2023 at 4:39 AM Tom Lane wrote:
>> I had what felt like an epiphany: the whole problem arises because the
>> system is wrongly factored. We should get rid of AcquireExecutorLocks
>> altogether, allowing the plancache to hand back a generic plan that
>>
On Fri, Jan 20, 2023 at 4:39 AM Tom Lane wrote:
> I spent some time re-reading this whole thread, and the more I read
> the less happy I got.
Thanks a lot for your time on this.
> We are adding a lot of complexity and introducing
> coding hazards that will surely bite somebody someday. And
I spent some time re-reading this whole thread, and the more I read
the less happy I got. We are adding a lot of complexity and introducing
coding hazards that will surely bite somebody someday. And after awhile
I had what felt like an epiphany: the whole problem arises because the
system is
Alvaro Herrera writes:
> This version of the patch looks not entirely unreasonable to me. I'll
> set this as Ready for Committer in case David or Tom or someone else
> want to have a look and potentially commit it.
I will have a look during the January CF.
regards, tom
On Wed, Dec 21, 2022 at 7:18 PM Alvaro Herrera wrote:
> This version of the patch looks not entirely unreasonable to me. I'll
> set this as Ready for Committer in case David or Tom or someone else
> want to have a look and potentially commit it.
Thank you, Alvaro.
--
Thanks, Amit Langote
EDB:
This version of the patch looks not entirely unreasonable to me. I'll
set this as Ready for Committer in case David or Tom or someone else
want to have a look and potentially commit it.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Wed, Dec 14, 2022 at 5:35 PM Amit Langote wrote:
> I have moved the original functionality of GetCachedPlan() to
> GetCachedPlanInternal(), turning the former into a sort of controller
> as described shortly. The latter's CheckCachedPlan() part now only
> locks the "minimal" set of,
On Tue, Dec 13, 2022 at 2:24 AM Alvaro Herrera wrote:
> On 2022-Dec-12, Amit Langote wrote:
> > I started feeling like putting all the new logic being added
> > by this patch into plancache.c at the heart of GetCachedPlan() and
> > tweaking its API in kind of unintuitive ways may not have been
On 2022-Dec-12, Amit Langote wrote:
> I started feeling like putting all the new logic being added
> by this patch into plancache.c at the heart of GetCachedPlan() and
> tweaking its API in kind of unintuitive ways may not have been such a
> good idea to begin with. So I started thinking again
On Fri, Dec 9, 2022 at 8:37 PM Alvaro Herrera wrote:
> On 2022-Dec-09, Amit Langote wrote:
>
> > Pruning will be done afresh on every fetch of a given cached plan when
> > CheckCachedPlan() is called on it, so the part_prune_results_list part
> > will be discarded and rebuilt as many times as the
On 2022-Dec-09, Amit Langote wrote:
> Pruning will be done afresh on every fetch of a given cached plan when
> CheckCachedPlan() is called on it, so the part_prune_results_list part
> will be discarded and rebuilt as many times as the plan is executed.
> You'll find a description around
On Fri, Dec 9, 2022 at 7:49 PM Alvaro Herrera wrote:
> On 2022-Dec-09, Amit Langote wrote:
> > On Fri, Dec 9, 2022 at 6:52 PM Alvaro Herrera
> > wrote:
> > > Remind me again why is part_prune_results_list not part of struct
> > > CachedPlan then? I tried to understand that based on comments
On 2022-Dec-09, Amit Langote wrote:
> On Fri, Dec 9, 2022 at 6:52 PM Alvaro Herrera wrote:
> > Remind me again why is part_prune_results_list not part of struct
> > CachedPlan then? I tried to understand that based on comments upthread,
> > but I was unable to find anything.
>
> It used to be
On Fri, Dec 9, 2022 at 6:52 PM Alvaro Herrera wrote:
> On 2022-Dec-09, Amit Langote wrote:
> > On Wed, Dec 7, 2022 at 4:00 AM Alvaro Herrera
> > wrote:
> > > I find the API of GetCachedPlans a little weird after this patch.
>
> > David, in his Apr 7 reply on this thread, also sounded to suggest
On 2022-Dec-09, Amit Langote wrote:
> On Wed, Dec 7, 2022 at 4:00 AM Alvaro Herrera wrote:
> > I find the API of GetCachedPlans a little weird after this patch.
> David, in his Apr 7 reply on this thread, also sounded to suggest
> something similar.
>
> Hmm, I was / am not so sure if
Thanks for the review.
On Wed, Dec 7, 2022 at 4:00 AM Alvaro Herrera wrote:
> I find the API of GetCachedPlans a little weird after this patch. I
> think it may be better to have it return a pointer of a new struct --
> one that contains both the CachedPlan pointer and the list of pruning
>
I find the API of GetCachedPlans a little weird after this patch. I
think it may be better to have it return a pointer of a new struct --
one that contains both the CachedPlan pointer and the list of pruning
results. (As I understand, the sole caller that isn't interested in the
pruning results,
On Mon, Dec 5, 2022 at 12:00 PM Amit Langote wrote:
> On Fri, Dec 2, 2022 at 7:40 PM Amit Langote wrote:
> > Thought it might be good for PartitionPruneResult to also have
> > root_parent_relids that matches with the corresponding
> > PartitionPruneInfo. ExecInitPartitionPruning() does a sanity
On Fri, Dec 2, 2022 at 7:40 PM Amit Langote wrote:
> On Thu, Dec 1, 2022 at 9:43 PM Amit Langote wrote:
> > On Thu, Dec 1, 2022 at 8:21 PM Alvaro Herrera
> > wrote:
> > > On 2022-Dec-01, Amit Langote wrote:
> > > > Hmm, how about keeping the [Merge]Append's parent relation's RT index
> > > >
On Thu, Dec 1, 2022 at 9:43 PM Amit Langote wrote:
> On Thu, Dec 1, 2022 at 8:21 PM Alvaro Herrera wrote:
> > On 2022-Dec-01, Amit Langote wrote:
> > > Hmm, how about keeping the [Merge]Append's parent relation's RT index
> > > in the PartitionPruneInfo and passing it down to
> > >
On Thu, Dec 1, 2022 at 8:21 PM Alvaro Herrera wrote:
> On 2022-Dec-01, Amit Langote wrote:
> > Hmm, how about keeping the [Merge]Append's parent relation's RT index
> > in the PartitionPruneInfo and passing it down to
> > ExecInitPartitionPruning() from ExecInit[Merge]Append() for
> >
On 2022-Dec-01, Amit Langote wrote:
> Hmm, how about keeping the [Merge]Append's parent relation's RT index
> in the PartitionPruneInfo and passing it down to
> ExecInitPartitionPruning() from ExecInit[Merge]Append() for
> cross-checking? Both Append and MergeAppend already have a
> 'apprelids'
Hi Alvaro,
Thanks for looking at this one.
On Thu, Dec 1, 2022 at 3:12 AM Alvaro Herrera wrote:
> Looking at 0001, I wonder if we should have a crosscheck that a
> PartitionPruneInfo you got from following an index is indeed constructed
> for the relation that you think it is: previously, you
Looking at 0001, I wonder if we should have a crosscheck that a
PartitionPruneInfo you got from following an index is indeed constructed
for the relation that you think it is: previously, you were always sure
that the prune struct is for this node, because you followed a pointer
that was set up in
On Thu, Oct 27, 2022 at 11:41 AM Amit Langote wrote:
> On Mon, Oct 17, 2022 at 6:29 PM Amit Langote wrote:
> > On Wed, Oct 12, 2022 at 4:36 PM Amit Langote
> > wrote:
> > > On Fri, Jul 29, 2022 at 1:20 PM Amit Langote
> > > wrote:
> > > > On Thu, Jul 28, 2022 at 1:27 AM Robert Haas
> > > >
On Mon, Oct 17, 2022 at 6:29 PM Amit Langote wrote:
> On Wed, Oct 12, 2022 at 4:36 PM Amit Langote wrote:
> > On Fri, Jul 29, 2022 at 1:20 PM Amit Langote
> > wrote:
> > > On Thu, Jul 28, 2022 at 1:27 AM Robert Haas wrote:
> > > > 0001 adds es_part_prune_result but does not use it, so maybe
On Wed, Oct 12, 2022 at 4:36 PM Amit Langote wrote:
> On Fri, Jul 29, 2022 at 1:20 PM Amit Langote wrote:
> > On Thu, Jul 28, 2022 at 1:27 AM Robert Haas wrote:
> > > 0001 adds es_part_prune_result but does not use it, so maybe the
> > > introduction of that field should be deferred until it's
On Fri, Jul 29, 2022 at 12:47 PM Tom Lane wrote:
> > I'm just uncertain whether what Amit has implemented is the
> > least-annoying way to go about it... any thoughts on that,
> > specifically as it pertains to this patch?
>
> I haven't looked at this patch at all. I'll try to make some
> time
Robert Haas writes:
> ... it's
> always struck me as a little unfortunate that we basically test
> whether a var is equal by testing whether the varno and varlevelsup
> are equal. That only works if you assume that you can never end up
> comparing two vars from thoroughly unrelated parts of the
On Fri, Jul 29, 2022 at 11:04 AM Tom Lane wrote:
> We could probably make that work, but I'm skeptical that it would
> really be an improvement overall, for a couple of reasons.
>
> (1) The need for merge-rangetables-and-renumber-Vars logic doesn't
> go away. It just moves from setrefs.c to the
Robert Haas writes:
> That's not quite my question, though. Why do we ever build a non-flat
> range table in the first place? Like, instead of assigning indexes
> relative to the current subquery level, why not just assign them
> relative to the whole query from the start?
We could probably make
On Fri, Jul 29, 2022 at 12:55 AM Tom Lane wrote:
> It would not be profitable to flatten the range table before we've
> done remove_useless_joins. We'd end up with useless entries from
> subqueries that ultimately aren't there. We could perhaps do it
> after we finish that phase, but I don't
Amit Langote writes:
> On Thu, Jul 28, 2022 at 1:27 AM Robert Haas wrote:
>> I wonder whether it's really necessary to added the PartitionPruneInfo
>> objects to a list in PlannerInfo first and then roll them up into
>> PlannerGlobal later. I know we do that for range table entries, but
>> I've
On Thu, Jul 28, 2022 at 1:27 AM Robert Haas wrote:
> On Tue, Jul 26, 2022 at 11:01 PM Amit Langote wrote:
> > Needed to be rebased again, over 2d04277121f this time.
Thanks for looking.
> 0001 adds es_part_prune_result but does not use it, so maybe the
> introduction of that field should be
On Tue, Jul 26, 2022 at 11:01 PM Amit Langote wrote:
> Needed to be rebased again, over 2d04277121f this time.
0001 adds es_part_prune_result but does not use it, so maybe the
introduction of that field should be deferred until it's needed for
something.
I wonder whether it's really necessary
On Wed, Jul 13, 2022 at 4:03 PM Amit Langote wrote:
> On Wed, Jul 13, 2022 at 3:40 PM Amit Langote wrote:
> > Rebased over 964d01ae90c.
>
> Sorry, left some pointless hunks in there while rebasing. Fixed in
> the attached.
Needed to be rebased again, over 2d04277121f this time.
--
Thanks,
On Wed, Jul 13, 2022 at 3:40 PM Amit Langote wrote:
> Rebased over 964d01ae90c.
Sorry, left some pointless hunks in there while rebasing. Fixed in
the attached.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
v19-0001-Move-PartitioPruneInfo-out-of-plan-nodes-into-Pl.patch
Rebased over 964d01ae90c.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
v18-0002-Optimize-AcquireExecutorLocks-by-locking-only-un.patch
Description: Binary data
v18-0001-Move-PartitioPruneInfo-out-of-plan-nodes-into-Pl.patch
Description: Binary data
On Fri, May 27, 2022 at 1:09 AM Amit Langote wrote:
> 0001 contains the mechanical changes of moving PartitionPruneInfo out
> of Append/MergeAppend into a list in PlannedStmt.
>
> 0002 is the main patch to "Optimize AcquireExecutorLocks() by locking
> only unpruned partitions".
This patchset
On Fri, May 27, 2022 at 1:10 AM Amit Langote
wrote:
> On Mon, Apr 11, 2022 at 12:53 PM Zhihong Yu wrote:
> > On Sun, Apr 10, 2022 at 8:05 PM Amit Langote
> wrote:
> >> Sending v15 that fixes that to keep the cfbot green for now.
> >
> > Hi,
> >
> > + /* RT index of the partitione
On Sun, Apr 10, 2022 at 8:05 PM Amit Langote
wrote:
> On Fri, Apr 8, 2022 at 8:45 PM Amit Langote
> wrote:
> > Most looked fine changes to me except a couple of typos, so I've
> > adopted those into the attached new version, even though I know it's
> > too late to try to apply it.
> >
> > + *
On Fri, Apr 8, 2022 at 8:45 PM Amit Langote wrote:
> Most looked fine changes to me except a couple of typos, so I've
> adopted those into the attached new version, even though I know it's
> too late to try to apply it.
>
> + * XXX is it worth doing a bms_copy() on glob->minLockRelids if
> + *
Hi David,
On Fri, Apr 8, 2022 at 8:16 PM David Rowley wrote:
> On Fri, 8 Apr 2022 at 17:49, Amit Langote wrote:
> > Attached updated patch with these changes.
> Thanks for making the changes. I started looking over this patch but
> really feel like it needs quite a few more iterations of what
On Fri, 8 Apr 2022 at 17:49, Amit Langote wrote:
> Attached updated patch with these changes.
Thanks for making the changes. I started looking over this patch but
really feel like it needs quite a few more iterations of what we've
just been doing to get it into proper committable shape. There
On Thu, Apr 7, 2022 at 9:41 PM David Rowley wrote:
> On Thu, 7 Apr 2022 at 20:28, Amit Langote wrote:
> > Here's an updated version. In Particular, I removed
> > part_prune_results list from PortalData, in favor of anything that
> > needs to look at the list can instead get it from the
On Thu, 7 Apr 2022 at 20:28, Amit Langote wrote:
> Here's an updated version. In Particular, I removed
> part_prune_results list from PortalData, in favor of anything that
> needs to look at the list can instead get it from the CachedPlan
> (PortalData.cplan). This makes things better in 2
On Wed, Apr 6, 2022 at 4:20 PM Amit Langote wrote:
> And here is a version like that that passes make check-world. Maybe
> still a WIP as I think comments could use more editing.
>
> Here's how the new implementation works:
>
> AcquireExecutorLocks() calls ExecutorDoInitialPruning(), which in
On Fri, Apr 1, 2022 at 5:36 PM Amit Langote wrote:
> On Fri, Apr 1, 2022 at 5:20 PM David Rowley wrote:
> > On Fri, 1 Apr 2022 at 19:58, Amit Langote wrote:
> > > Yes, the ExecLockRelsInfo node in the current patch, that first gets
> > > added to the QueryDesc and subsequently to the EState of
On Tue, Apr 5, 2022 at 7:00 PM Alvaro Herrera wrote:
> On 2022-Apr-05, Amit Langote wrote:
> > While at it, maybe it's better to rename ExecInitPruningContext() to
> > InitPartitionPruneContext(), which I've done in the attached updated
> > patch.
>
> Good call. I had changed that name too, but
On 2022-Apr-05, Amit Langote wrote:
> While at it, maybe it's better to rename ExecInitPruningContext() to
> InitPartitionPruneContext(), which I've done in the attached updated
> patch.
Good call. I had changed that name too, but yours seems a better
choice.
I made a few other cosmetic
On Mon, Apr 4, 2022 at 9:55 PM Amit Langote wrote:
> On Sun, Apr 3, 2022 at 8:33 PM Alvaro Herrera wrote:
> > I think the names ExecCreatePartitionPruneState and
> > ExecInitPartitionPruning are too confusingly similar. Maybe the former
> > should be renamed to somehow make it clear that it is
Thanks for the review.
On Sun, Apr 3, 2022 at 8:33 PM Alvaro Herrera wrote:
> I noticed a definitional problem in 0001 that's also a bug in some
> conditions -- namely that the bitmapset "validplans" is never explicitly
> initialized to NIL. In the original coding, the BMS was always returned
>
I noticed a definitional problem in 0001 that's also a bug in some
conditions -- namely that the bitmapset "validplans" is never explicitly
initialized to NIL. In the original coding, the BMS was always returned
from somewhere; in the new code, it is passed from an uninitialized
stack variable
On Fri, Apr 1, 2022 at 5:20 PM David Rowley wrote:
> On Fri, 1 Apr 2022 at 19:58, Amit Langote wrote:
> > Yes, the ExecLockRelsInfo node in the current patch, that first gets
> > added to the QueryDesc and subsequently to the EState of the query,
> > serves as that stashing place. Not sure if
On Fri, 1 Apr 2022 at 19:58, Amit Langote wrote:
> Yes, the ExecLockRelsInfo node in the current patch, that first gets
> added to the QueryDesc and subsequently to the EState of the query,
> serves as that stashing place. Not sure if you've looked at
> ExecLockRelInfo in detail in your review
On Fri, Apr 1, 2022 at 12:45 PM Tom Lane wrote:
> Amit Langote writes:
> > On Fri, Apr 1, 2022 at 10:32 AM David Rowley wrote:
> >> 1. You've changed the signature of various functions by adding
> >> ExecLockRelsInfo *execlockrelsinfo. I'm wondering why you didn't just
> >> put the
On Fri, Apr 1, 2022 at 1:08 PM David Rowley wrote:
> On Fri, 1 Apr 2022 at 16:09, Amit Langote wrote:
> > definition of PlannedStmt says this:
> >
> > /*
> > * PlannedStmt node
> > *
> > * The output of the planner
> >
> > With the ideas that you've outlined below,
On Fri, 1 Apr 2022 at 16:09, Amit Langote wrote:
> definition of PlannedStmt says this:
>
> /*
> * PlannedStmt node
> *
> * The output of the planner
>
> With the ideas that you've outlined below, perhaps we can frame most
> of the things that the patch wants to do as the
Amit Langote writes:
> On Fri, Apr 1, 2022 at 10:32 AM David Rowley wrote:
>> 1. You've changed the signature of various functions by adding
>> ExecLockRelsInfo *execlockrelsinfo. I'm wondering why you didn't just
>> put the ExecLockRelsInfo as a new field in PlannedStmt?
> I'm worried about
Thanks a lot for looking into this.
On Fri, Apr 1, 2022 at 10:32 AM David Rowley wrote:
> I've been looking over the v8 patch and I'd like to propose semi-baked
> ideas to improve things. I'd need to go and write them myself to
> fully know if they'd actually work ok.
>
> 1. You've changed the
On Thu, 31 Mar 2022 at 16:25, Amit Langote wrote:
> Rebased.
I've been looking over the v8 patch and I'd like to propose semi-baked
ideas to improve things. I'd need to go and write them myself to
fully know if they'd actually work ok.
1. You've changed the signature of various functions by
On Thu, Mar 31, 2022 at 6:55 PM Alvaro Herrera wrote:
> I'm looking at 0001 here with intention to commit later. I see that
> there is some resistance to 0004, but I think a final verdict on that
> one doesn't materially affect 0001.
Thanks.
While the main goal of the refactoring patch is to
I'm looking at 0001 here with intention to commit later. I see that
there is some resistance to 0004, but I think a final verdict on that
one doesn't materially affect 0001.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos"
On Tue, Mar 15, 2022 at 3:19 PM Amit Langote wrote:
> On Tue, Mar 15, 2022 at 5:06 AM Robert Haas wrote:
> > On Mon, Mar 14, 2022 at 3:38 PM Tom Lane wrote:
> > > Also, while I've not spent much time at all reading this patch,
> > > it seems rather desperately undercommented, and a lot of the
>
On Tue, Mar 15, 2022 at 5:06 AM Robert Haas wrote:
> On Mon, Mar 14, 2022 at 3:38 PM Tom Lane wrote:
> > What I am skeptical about is that this work actually accomplishes
> > anything under real-world conditions. That's because if pruning would
> > save enough to make skipping the
On Mon, Mar 14, 2022 at 3:38 PM Tom Lane wrote:
> ... like EXPLAIN, for example?
Exactly! I think that's the foremost example, but extension modules
like auto_explain or even third-party extensions are also a risk. I
think there was some discussion of this previously.
> If "pruning" means
Robert Haas writes:
> In my opinion, the most important theoretical issue here is around
> reuse of plans that are no longer entirely valid, but the parts that
> are no longer valid are certain to be pruned. If, because we know that
> some parameter has some particular value, we skip locking a
On Fri, Mar 11, 2022 at 9:35 AM Amit Langote wrote:
> Attached is v5, now broken into 3 patches:
>
> 0001: Some refactoring of runtime pruning code
> 0002: Add a plan_tree_walker
> 0003: Teach AcquireExecutorLocks to skip locking pruned relations
So is any other committer planning to look at
1 - 100 of 124 matches
Mail list logo
