Re: Reduce palloc's in numeric operations.

Hello, I will propose reduce palloc's in numeric operations.

The numeric operations are slow by nature, but usually it is not
a problem for on-disk operations. Altough the slowdown is
enhanced on on-memory operations.

I inspcted them and found some very short term pallocs. These
palloc's are used for temporary storage for digits of unpaked
numerics.

The formats of numeric digits in packed and unpaked forms are
same. So we can kicked out a part of palloc's using digits in
packed numeric in-place to make unpakced one.

In this patch, I added new function set_var_from_num_nocopy() to
do this. And make use of it for operands which won't modified.

The performance gain seems quite moderate....

'SELECT SUM(numeric_column) FROM on_memory_table' for ten million
rows and about 8 digits numeric runs for 3480 ms aganst original
3930 ms. It's 11% gain. 'SELECT SUM(int_column) FROM
on_memory_table' needed 1570 ms.

Similary 8% gain for about 30 - 50 digits numeric. Performance of
avg(numeric) made no gain in contrast.

Do you think this worth doing?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 14.09.2012 11:25, Kyotaro HORIGUCHI wrote:
> Hello, I will propose reduce palloc's in numeric operations.
>
> The numeric operations are slow by nature, but usually it is not
> a problem for on-disk operations. Altough the slowdown is
> enhanced on on-memory operations.
>
> I inspcted them and found some very short term pallocs. These
> palloc's are used for temporary storage for digits of unpaked
> numerics.
>
> The formats of numeric digits in packed and unpaked forms are
> same. So we can kicked out a part of palloc's using digits in
> packed numeric in-place to make unpakced one.
>
> In this patch, I added new function set_var_from_num_nocopy() to
> do this. And make use of it for operands which won't modified.

Have to be careful to really not modify the operands. numeric_out() and
numeric_out_sci() are wrong; they call get_str_from_var(), which
modifies the argument. Same with numeric_expr(): it passes the argument
to numericvar_to_double_no_overflow(), which passes it to
get_str_from_var(). numericvar_to_int8() also modifies its argument, so
all the functions that use that, directly or indirectly, must make a copy.

Perhaps get_str_from_var(), and the other functions that currently
scribble on the arguments, should be modified to not do so. They could
easily make a copy of the argument within the function. Then the callers
could safely use set_var_from_num_nocopy(). The performance would be the
same, you would have the same number of pallocs, but you would get rid
of the surprising argument-modifying behavior of those functions.

> The performance gain seems quite moderate....
>
> 'SELECT SUM(numeric_column) FROM on_memory_table' for ten million
> rows and about 8 digits numeric runs for 3480 ms aganst original
> 3930 ms. It's 11% gain. 'SELECT SUM(int_column) FROM
> on_memory_table' needed 1570 ms.
>
> Similary 8% gain for about 30 - 50 digits numeric. Performance of
> avg(numeric) made no gain in contrast.
>
> Do you think this worth doing?

Yes, I think this is worthwhile. I'm seeing an even bigger gain, with
smaller numerics. I created a table with this:

CREATE TABLE numtest AS SELECT a::numeric AS col FROM generate_series(1,
10000000) a;

And repeated this query with \timing:

SELECT SUM(col) FROM numtest;

The execution time of that query fell from about 5300 ms to 4300 ms, ie.
about 20%.

- Heikki

Kyotaro HORIGUCHI wrote:
> Hello, I will propose reduce palloc's in numeric operations.
>
> The numeric operations are slow by nature, but usually it is not
> a problem for on-disk operations. Altough the slowdown is
> enhanced on on-memory operations.
>
> I inspcted them and found some very short term pallocs. These
> palloc's are used for temporary storage for digits of unpaked
> numerics.

This looks like a neat little patch. Some feedback has been provided by
Heikki (thanks!) and since we're still waiting for an updated version, I
have marked this Returned with Feedback for the time being. Please make
sure to address the remaining issues and submit to the next commitfest.
Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello. My colleague found that pg_stat_replication.sync_state
shows false state for some condition.

This prevents Pacemaker from completing fail-over that could
safely be done.

The point is in walsender.c, pg_stat_get_wal_senders() below, (as
of REL9_2_1)

1555: if (walsnd->pid != 0)
1556: {
1557: /*
1558: * Treat a standby such as a pg_basebackup background process
1559: * which always returns an invalid flush location, as an
1560: * asynchronous standby.
1561: */
! 1562: sync_priority[i] = XLogRecPtrIsInvalid(walsnd->flush) ?
1563: 0 : walsnd->sync_standby_priority;

Here, XLogRecPtrIsInvalid(walsnd->flush) is defined as
(walsnd->flush.xrecoff == 0) which becomes true as usual at every
WAL 'file's (not segments) boundary. xrecoff == 0 is certainly
invalid for the start point of WAL *RECORD*, but should be
considered valid in replication stream. This check was introduced
at 9.2.0 and the version up between 9.1.4 and 9.1.5.

| DEBUG: write 4/0 flush 3/FEFFEC68 apply 3/FEFFEC68
| DEBUG: write 4/0 flush 4/0 apply 3/FEFFEC68
| DEBUG: HOGE: flush = 3/FEFFEC68 sync_priority[0] = 1
| DEBUG: write 4/111C0 flush 4/0 apply 3/FEFFECC0
!| DEBUG: HOGE: flush = 4/0 sync_priority[0] = 0

This value zero of sync_priority[0] makes sync_status 'async'
errorneously and confuses Pacemaker.

# The log line marked with 'HOGE' above printed by applying the
# patch at the bottom of this message and invoking 'select
# sync_state from pg_stat_replication' periodically. To increase
# the chance to see the symptom, sleep 1 second for 'file'
# boundaries :-)

The Heikki's recent(?) commit
0ab9d1c4b31622e9176472b4276f3e9831e3d6ba which changes the format
of XLogRecPtr from logid:xrecoff struct to 64 bit linear address
would fix the false indication. But I suppose this patch won't be
applied to existing 9.1.x and 9.2.x because of the modification
onto streaming protocol.

As far as I see the patch, it would'nt change the meaning of
XLogRecPtr to change XLogRecPtrIsInvalid from (xrecoff == 0) to
(xrecoff == 0 && xlogid == 0). But this change affects rather
wide portion where handling WAL nevertheless what is needed here
is only to stop the false indication.

On the other hand, pg_basebackup seems return 0/0 as flush and
apply positions so it seems enough only to add xlogid == 0 into
the condition. The patch attached for REL9_2_1 does this and
yields the result following.

| DEBUG: write 2/FEFFFD48 flush 2/FEFFFD48 apply 2/FEFF7AB0
| DEBUG: write 3/0 flush 2/FEFFFD48 apply 2/FEFF7E88
| DEBUG: write 3/0 flush 3/0 apply 2/FEFFFD48
| DEBUG: HOGE: flush = 2/FEFFFD48 sync_priority[0] = 1
| DEBUG: write 3/E338 flush 3/0 apply 2/FEFFFF80
!| DEBUG: HOGE: flush = 3/0 sync_priority[0] = 1

I think this patch should be applied for 9.2.2 and 9.1.7.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

============================================================
===== The patch for this test.
============================================================
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 064ddd5..19f79d1 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -618,6 +618,10 @@ ProcessStandbyReplyMessage(void)
reply.flush.xlogid, reply.flush.xrecoff,
reply.apply.xlogid, reply.apply.xrecoff);

+ if (reply.write.xrecoff == 0 ||
+ reply.flush.xrecoff == 0)
+ sleep(1);
+
/*
* Update shared state for this WalSender process based on reply data from
* standby.
@@ -1561,7 +1565,10 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
*/
sync_priority[i] = XLogRecPtrIsInvalid(walsnd->flush) ?
0 : walsnd->sync_standby_priority;
-
+ elog(DEBUG1, "HOGE: flush = %X/%X sync_priority[%d] = %d",
+ walsnd->flush.xlogid, walsnd->flush.xrecoff,
+ i, sync_priority[i]);
+
if (walsnd->state == WALSNDSTATE_STREAMING &&
walsnd->sync_standby_priority > 0 &&
(priority == 0 ||

On Thu, Oct 18, 2012 at 5:42 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello. My colleague found that pg_stat_replication.sync_state
> shows false state for some condition.
>
> This prevents Pacemaker from completing fail-over that could
> safely be done.
>
> The point is in walsender.c, pg_stat_get_wal_senders() below, (as
> of REL9_2_1)
>
> 1555: if (walsnd->pid != 0)
> 1556: {
> 1557: /*
> 1558: * Treat a standby such as a pg_basebackup background process
> 1559: * which always returns an invalid flush location, as an
> 1560: * asynchronous standby.
> 1561: */
> ! 1562: sync_priority[i] = XLogRecPtrIsInvalid(walsnd->flush) ?
> 1563: 0 : walsnd->sync_standby_priority;
>
> Here, XLogRecPtrIsInvalid(walsnd->flush) is defined as
> (walsnd->flush.xrecoff == 0) which becomes true as usual at every
> WAL 'file's (not segments) boundary. xrecoff == 0 is certainly
> invalid for the start point of WAL *RECORD*, but should be
> considered valid in replication stream. This check was introduced
> at 9.2.0 and the version up between 9.1.4 and 9.1.5.
>
> | DEBUG: write 4/0 flush 3/FEFFEC68 apply 3/FEFFEC68
> | DEBUG: write 4/0 flush 4/0 apply 3/FEFFEC68
> | DEBUG: HOGE: flush = 3/FEFFEC68 sync_priority[0] = 1
> | DEBUG: write 4/111C0 flush 4/0 apply 3/FEFFECC0
> !| DEBUG: HOGE: flush = 4/0 sync_priority[0] = 0
>
> This value zero of sync_priority[0] makes sync_status 'async'
> errorneously and confuses Pacemaker.
>
> # The log line marked with 'HOGE' above printed by applying the
> # patch at the bottom of this message and invoking 'select
> # sync_state from pg_stat_replication' periodically. To increase
> # the chance to see the symptom, sleep 1 second for 'file'
> # boundaries :-)
>
> The Heikki's recent(?) commit
> 0ab9d1c4b31622e9176472b4276f3e9831e3d6ba which changes the format
> of XLogRecPtr from logid:xrecoff struct to 64 bit linear address
> would fix the false indication. But I suppose this patch won't be
> applied to existing 9.1.x and 9.2.x because of the modification
> onto streaming protocol.
>
> As far as I see the patch, it would'nt change the meaning of
> XLogRecPtr to change XLogRecPtrIsInvalid from (xrecoff == 0) to
> (xrecoff == 0 && xlogid == 0). But this change affects rather
> wide portion where handling WAL nevertheless what is needed here
> is only to stop the false indication.
>
> On the other hand, pg_basebackup seems return 0/0 as flush and
> apply positions so it seems enough only to add xlogid == 0 into
> the condition. The patch attached for REL9_2_1 does this and
> yields the result following.
>
> | DEBUG: write 2/FEFFFD48 flush 2/FEFFFD48 apply 2/FEFF7AB0
> | DEBUG: write 3/0 flush 2/FEFFFD48 apply 2/FEFF7E88
> | DEBUG: write 3/0 flush 3/0 apply 2/FEFFFD48
> | DEBUG: HOGE: flush = 2/FEFFFD48 sync_priority[0] = 1
> | DEBUG: write 3/E338 flush 3/0 apply 2/FEFFFF80
> !| DEBUG: HOGE: flush = 3/0 sync_priority[0] = 1
>
> I think this patch should be applied for 9.2.2 and 9.1.7.

Looks good to me, though I don't think the source code comment needs
to be updated in the way the patch does.

Regards,

--
Fujii Masao

Thank you for comment.

> > I think this patch should be applied for 9.2.2 and 9.1.7.
>
> Looks good to me, though I don't think the source code comment needs
> to be updated in the way the patch does.

Ok, the patch for walsender.c becomes 1 liner, quite simple.

However, I've forgotten to treat other three portions in
walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
which comes from WAL receiver>). This new patch includes the
changes for them.

By the way, XLogRecPtrIsInvliad() seems to be used also in
gist.c, gistget.c, gistvacuum.c, visibilitymap.c, clog.c, slru.c,
xlog.c.

In these files, LSN's fed to XLogRecPtrIsInvalid() looks to be
*valid* start point of WAL records, but I'm not sure of that.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Ouch! I'm sorry to have sent truly buggy version, please abandon
v2 patch sent just before.

Added include "access/transam.h" to syncrep.c and corrected the
name of XLByteEQ.

> Thank you for comment.
>
> > > I think this patch should be applied for 9.2.2 and 9.1.7.
> >
> > Looks good to me, though I don't think the source code comment needs
> > to be updated in the way the patch does.
>
> Ok, the patch for walsender.c becomes 1 liner, quite simple.
>
> However, I've forgotten to treat other three portions in
> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
> which comes from WAL receiver>). This new patch includes the
> changes for them.
>
> By the way, XLogRecPtrIsInvliad() seems to be used also in
> gist.c, gistget.c, gistvacuum.c, visibilitymap.c, clog.c, slru.c,
> xlog.c.
>
> In these files, LSN's fed to XLogRecPtrIsInvalid() looks to be
> *valid* start point of WAL records, but I'm not sure of that.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Oct 19, 2012 at 5:46 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Ouch! I'm sorry to have sent truly buggy version, please abandon
> v2 patch sent just before.
>
> Added include "access/transam.h" to syncrep.c and corrected the
> name of XLByteEQ.

Thanks for updating the patch! This looks good to me.

>> Thank you for comment.
>>
>> > > I think this patch should be applied for 9.2.2 and 9.1.7.
>> >
>> > Looks good to me, though I don't think the source code comment needs
>> > to be updated in the way the patch does.
>>
>> Ok, the patch for walsender.c becomes 1 liner, quite simple.
>>
>> However, I've forgotten to treat other three portions in
>> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
>> which comes from WAL receiver>). This new patch includes the
>> changes for them.

Good catch.

Regards,

--
Fujii Masao

On Fri, Oct 19, 2012 at 10:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Oct 19, 2012 at 5:46 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Ouch! I'm sorry to have sent truly buggy version, please abandon
>> v2 patch sent just before.
>>
>> Added include "access/transam.h" to syncrep.c and corrected the
>> name of XLByteEQ.
>
> Thanks for updating the patch! This looks good to me.
>
>>> Thank you for comment.
>>>
>>> > > I think this patch should be applied for 9.2.2 and 9.1.7.
>>> >
>>> > Looks good to me, though I don't think the source code comment needs
>>> > to be updated in the way the patch does.
>>>
>>> Ok, the patch for walsender.c becomes 1 liner, quite simple.
>>>
>>> However, I've forgotten to treat other three portions in
>>> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
>>> which comes from WAL receiver>). This new patch includes the
>>> changes for them.
>
> Good catch.

Does any commiter pick up this?

Regards,

--
Fujii Masao

Fujii Masao escribió:
> On Fri, Oct 19, 2012 at 10:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> >>> However, I've forgotten to treat other three portions in
> >>> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
> >>> which comes from WAL receiver>). This new patch includes the
> >>> changes for them.
> >
> > Good catch.
>
> Does any commiter pick up this?

If not, please add to next commitfest so that we don't forget.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 9, 2012 at 4:06 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Fujii Masao escribió:
>> On Fri, Oct 19, 2012 at 10:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
>> >>> However, I've forgotten to treat other three portions in
>> >>> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
>> >>> which comes from WAL receiver>). This new patch includes the
>> >>> changes for them.
>> >
>> > Good catch.
>>
>> Does any commiter pick up this?
>
> If not, please add to next commitfest so that we don't forget.

Yep, I added this to next CF. This is just a bug fix, so please feel free to
pick up this even before CF.

Regards,

--
Fujii Masao

Thanks for comments,

> Have to be careful to really not modify the
> operands. numeric_out() and numeric_out_sci() are wrong; they
> call get_str_from_var(), which modifies the argument. Same with
> numeric_expr(): it passes the argument to
> numericvar_to_double_no_overflow(), which passes it to
> get_str_from_var(). numericvar_to_int8() also modifies its
> argument, so all the functions that use that, directly or
> indirectly, must make a copy.

mmm. My carefulness was a bit short to pick up them...

I overlooked that get_str_from_var() and numeric_to_int8() calls
round_var() which rewrites the operand. I reverted numeric_out()
and numeric_int8(), numeric_int2().

Altough, I couldn't find in get_str_from_var_sci() where the
operand would be modified.

The lines where var showing in get_str_from_var_sci() is listed
below.

| 2:get_str_from_var_sci(NumericVar *var, int rscale)
| 21: if (var->ndigits > 0)
| 23: exponent = (var->weight + 1) * DEC_DIGITS;
| 29: exponent -= DEC_DIGITS - (int) log10(var->digits[0]);
| 59: div_var(var, &denominator, &significand, rscale, true);

The only suspect is div_var at this level, and do the same thing
for var1 in div_var.

| 2:div_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
| 20: int var1ndigits = var1->ndigits;
| 35: if (var1ndigits == 0)
| 47: if (var1->sign == var2->sign)
| 51: res_weight = var1->weight - var2->weight;
| 68: div_ndigits = Max(div_ndigits, var1ndigits);
| 83: memcpy(dividend + 1, var1->digits, var1ndigits * sizeof(NumericDigit));
| 132: for (i = var1ndigits; i >= 0; i--)

No line seems to modify var1 as far as I see so I've left
numeric_out_sci() modified in this patch.

Well, I found some other bugs in numeric_stddev_internal.
vN was errorniously freed and vsumX2 in is used as work.
They are fixed in this new patch.

> Perhaps get_str_from_var(), and the other functions that
> currently scribble on the arguments, should be modified to not
> do so. They could easily make a copy of the argument within the
> function. Then the callers could safely use
> set_var_from_num_nocopy(). The performance would be the same,
> you would have the same number of pallocs, but you would get
> rid of the surprising argument-modifying behavior of those
> functions.

I agree with that. const qualifiers on parameters would rule this
mechanically. I try this for the next version of this patch.

> SELECT SUM(col) FROM numtest;
>
> The execution time of that query fell from about 5300 ms to 4300 ms, ie. about 20%.

Wow, it seems more promising than I expected. Thanks.

regards,

--
堀口恭太郎

日本電信電話株式会社 NTTオープンソフトウェアセンタ
Phone: 03-5860-5115 / Fax: 03-5463-5490

On 09.11.2012 15:28, Fujii Masao wrote:
> On Fri, Nov 9, 2012 at 4:06 AM, Alvaro Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
>> Fujii Masao escribió:
>>> On Fri, Oct 19, 2012 at 10:29 PM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>>>>>> However, I've forgotten to treat other three portions in
>>>>>> walsender.c and syncrep.c also does XLogRecPtrIsInvalid(<XLogPtr
>>>>>> which comes from WAL receiver>). This new patch includes the
>>>>>> changes for them.
>>>>
>>>> Good catch.
>>>
>>> Does any commiter pick up this?
>>
>> If not, please add to next commitfest so that we don't forget.
>
> Yep, I added this to next CF. This is just a bug fix, so please feel free to
> pick up this even before CF.

Committed, thanks.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> Committed, thanks.

Doesn't seem to have been pushed to master?

regards, tom lane

On 23.11.2012 19:55, Tom Lane wrote:
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> writes:
>> Committed, thanks.
>
> Doesn't seem to have been pushed to master?

On purpose. Per commit message:

> 9.3 doesn't have this problem because XLogRecPtr is now a single 64-bit
> integer, so XLogRecPtrIsInvalid() does the right thing. Apply to 9.2, and
> 9.1 where pg_stat_replication view was introduced.

I considered applying it to master anyway, just to keep the branches in
sync. But XLogRecPtrIsInvalid(var) seems slightly more readable than
XLByteEQ(var, InvalidXLogRecPtr), so I decided not to.

- Heikki