Horton

Thread

Commits

  1. Remove AtSubStart_Notify.

  1. let's kill AtSubStart_Notify

    Robert Haas <robertmhaas@gmail.com> — 2019-09-11T12:52:21Z

    There are only four subsystems which require a callback at the
beginning of each subtransaction: the relevant functions are
AtSubStart_Memory, AtSubStart_ResourceOwner, AtSubStart_Notify, and
AfterTriggerBeginSubXact. The AtSubStart_Memory and
AtSubStart_ResourceOwner callbacks seem relatively unobjectionable,
because almost every subtransaction is going to allocate memory and
acquire some resource managed by a resource owner, but the others
represent initialization that has to be done whether or not the
corresponding feature is used.

Generally, a subsystem can avoid needing a callback at subtransaction
start (or transaction start) by detecting new levels of
subtransactions at time of use. A typical practice is to maintain a
stack which has entries only for those transaction nesting levels
where the functionality was used. The attached patch implements this
method for async.c. I was a little surprised to find that it makes a
pretty noticeable performance difference when starting and ending
trivial subtransactions.  I used this test case:

\timing
do $$begin for i in 1 .. 10000000 loop begin null; exception when
others then null; end; end loop; end;$$;

I ran the test four times with and without the patch and took the
median of the last three.  This was an attempt to exclude effects due
to starting up the database cluster. With the patch, the result was
3127.377 ms; without the patch, it was 3527.285 ms. That's a big
enough difference that I'm wondering whether I did something wrong
while testing this, so feel free to check my work and tell me whether
I'm all wet. Still, I don't find it wholly unbelievable, because I've
observed in the past that these code paths are lean enough that a few
palloc() calls can make a noticeable difference, and the effect of
this patch is to remove a few palloc() calls.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: let's kill AtSubStart_Notify

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-09-12T04:14:49Z

    On Wed, Sep 11, 2019 at 6:22 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> There are only four subsystems which require a callback at the
> beginning of each subtransaction: the relevant functions are
> AtSubStart_Memory, AtSubStart_ResourceOwner, AtSubStart_Notify, and
> AfterTriggerBeginSubXact. The AtSubStart_Memory and
> AtSubStart_ResourceOwner callbacks seem relatively unobjectionable,
> because almost every subtransaction is going to allocate memory and
> acquire some resource managed by a resource owner, but the others
> represent initialization that has to be done whether or not the
> corresponding feature is used.
>
> Generally, a subsystem can avoid needing a callback at subtransaction
> start (or transaction start) by detecting new levels of
> subtransactions at time of use. A typical practice is to maintain a
> stack which has entries only for those transaction nesting levels
> where the functionality was used. The attached patch implements this
> method for async.c. I was a little surprised to find that it makes a
> pretty noticeable performance difference when starting and ending
> trivial subtransactions.  I used this test case:
>
> \timing
> do $$begin for i in 1 .. 10000000 loop begin null; exception when
> others then null; end; end loop; end;$$;
>
> I ran the test four times with and without the patch and took the
> median of the last three.  This was an attempt to exclude effects due
> to starting up the database cluster. With the patch, the result was
> 3127.377 ms; without the patch, it was 3527.285 ms. That's a big
> enough difference that I'm wondering whether I did something wrong
> while testing this, so feel free to check my work and tell me whether
> I'm all wet. Still, I don't find it wholly unbelievable, because I've
> observed in the past that these code paths are lean enough that a few
> palloc() calls can make a noticeable difference, and the effect of
> this patch is to remove a few palloc() calls.

I did not read the patch but run the same case what you have given and
I can see the similar improvement with the patch.
With the patch 8832.988, without the patch 10252.701ms (median of three reading)

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  3. Re: let's kill AtSubStart_Notify

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-09-12T10:23:06Z

    At Thu, 12 Sep 2019 09:44:49 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in <CAFiTN-u8sp=1X+zk0hBPcYhZVYS6k1DcT+R3p+fucKu3iS7NHQ@mail.gmail.com>
> On Wed, Sep 11, 2019 at 6:22 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > trivial subtransactions.  I used this test case:
> >
> > \timing
> > do $$begin for i in 1 .. 10000000 loop begin null; exception when
> > others then null; end; end loop; end;$$;
> >
> > I ran the test four times with and without the patch and took the
> > median of the last three.  This was an attempt to exclude effects due
> > to starting up the database cluster. With the patch, the result was
> > 3127.377 ms; without the patch, it was 3527.285 ms. That's a big
> > enough difference that I'm wondering whether I did something wrong
> > while testing this, so feel free to check my work and tell me whether
> > I'm all wet. Still, I don't find it wholly unbelievable, because I've
> > observed in the past that these code paths are lean enough that a few
> > palloc() calls can make a noticeable difference, and the effect of
> > this patch is to remove a few palloc() calls.
> 
> I did not read the patch but run the same case what you have given and
> I can see the similar improvement with the patch.
> With the patch 8832.988, without the patch 10252.701ms (median of three reading)

I see the similar result. The patch let it run faster by about
25%. The gain is reduced to 3-6% by a crude check by adding { (in
TopTxCxt) lcons(0, p1); lcons(0, p2); } to the place where
AtSubStart_Notify was called and respective list_delete_first's
just after the call to AtSubCommit_Notfiy. At least around 20% of
the gain seems to be the result of removing palloc/pfree's.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: let's kill AtSubStart_Notify

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-09-27T09:41:02Z

    Hi Robert,

Generally, a subsystem can avoid needing a callback at subtransaction
> start (or transaction start) by detecting new levels of
> subtransactions at time of use.


Yes I agree with this argument.


> A typical practice is to maintain a
> stack which has entries only for those transaction nesting levels
> where the functionality was used. The attached patch implements this
> method for async.c.


I have reviewed your patch, and it seems correctly implementing the
actions per subtransactions using stack. Atleast I could not find
any flaw with your implementation here.


> I was a little surprised to find that it makes a
> pretty noticeable performance difference when starting and ending
> trivial subtransactions.  I used this test case:
>
> \timing
> do $$begin for i in 1 .. 10000000 loop begin null; exception when
> others then null; end; end loop; end;$$;
>

I ran your testcase and on my VM I get numbers like 3593.801 ms
without patch and 3593.801 with the patch, average of 5 runs each.
The runs were quite consistent.

Further make check also passing well.

Regards,
Jeevan Ladhe

  5. Re: let's kill AtSubStart_Notify

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-09-27T09:43:25Z

    >
> I did not read the patch but run the same case what you have given and
> I can see the similar improvement with the patch.
> With the patch 8832.988, without the patch 10252.701ms (median of three
> reading)
>

Possibly you had debug symbols enabled? With debug symbols enabled
I also get about similar number 10136.839 with patch vs 12900.044 ms
without the patch.

Regards,
Jeevan Ladhe

  6. Re: let's kill AtSubStart_Notify

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-09-27T09:49:40Z

    Correction -

On Fri, Sep 27, 2019 at 3:11 PM Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
wrote:

> I ran your testcase and on my VM I get numbers like 3593.801 ms
> without patch and 3593.801 with the patch, average of 5 runs each.
> The runs were quite consistent.
>

 3593.801 ms without patch and 3213.809 with the patch,
approx. 10% gain.

Regards,
Jeevan Ladhe

  7. Re: let's kill AtSubStart_Notify

    Robert Haas <robertmhaas@gmail.com> — 2019-10-04T12:25:56Z

    On Fri, Sep 27, 2019 at 5:41 AM Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> I have reviewed your patch, and it seems correctly implementing the
> actions per subtransactions using stack. Atleast I could not find
> any flaw with your implementation here.

Thanks for the review.  Based on this and other positive comments made
on this thread, I have committed the patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company