Обсуждение: Batch insert in CTAS/MatView code
Вложения
Hi, On Wed, Mar 06, 2019 at 10:06:27PM +0800, Paul Guo wrote: > The copy code has used batch insert with function heap_multi_insert() to > speed up. It seems that Create Table As or Materialized View could leverage > that code also to boost the performance also. Attached is a patch to > implement that. That was done by Taylor (cc-ed) and me. Please note that we are currently in the last commit fest of Postgres 12, and it is too late to propose new features. Please feel free to add an entry to the commit fest happening afterwards. -- Michael
Вложения
On 06/03/2019 22:06, Paul Guo wrote: > The patch also modifies heap_multi_insert() a bit to do a bit further > code-level optimization by using static memory, instead of using memory > context and dynamic allocation. If toasting is required, heap_prepare_insert() creates a palloc'd tuple. That is still leaked to the current memory context. Leaking into the current memory context is not a bad thing, because resetting a memory context is faster than doing a lot of pfree() calls. The callers just need to be prepared for that, and use a short-lived memory context. > By the way, while looking at the code, I noticed that there are 9 local > arrays with large length in toast_insert_or_update() which seems to be a > risk of stack overflow. Maybe we should put it as static or global. Hmm. We currently reserve 512 kB between the kernel's limit, and the limit we check in check_stack_depth(). See STACK_DEPTH_SLOP. Those arrays add up to 52800 bytes on a 64-bit maching, if I did my math right. So there's still a lot of headroom. I agree that it nevertheless seems a bit excessive, though. > With the patch, > > Time: 4728.142 ms (00:04.728) > Time: 14203.983 ms (00:14.204) > Time: 1008.669 ms (00:01.009) > > Baseline, > Time: 11096.146 ms (00:11.096) > Time: 13106.741 ms (00:13.107) > Time: 1100.174 ms (00:01.100) Nice speedup! > While for toast and large column size there is < 10% decrease but for > small column size the improvement is super good. Actually if I hardcode > the batch count as 4 all test cases are better but the improvement for > small column size is smaller than that with current patch. Pretty much > the number 4 is quite case specific so I can not hardcode that in the > patch. Of course we could further tune that but the current value seems > to be a good trade-off? Have you done any profiling, on why the multi-insert is slower with large tuples? In principle, I don't see why it should be slower. - Heikki
On 06/03/2019 22:06, Paul Guo wrote:
> The patch also modifies heap_multi_insert() a bit to do a bit further
> code-level optimization by using static memory, instead of using memory
> context and dynamic allocation.
If toasting is required, heap_prepare_insert() creates a palloc'd tuple.
That is still leaked to the current memory context.
Leaking into the current memory context is not a bad thing, because
resetting a memory context is faster than doing a lot of pfree() calls.
The callers just need to be prepared for that, and use a short-lived
memory context.
> By the way, while looking at the code, I noticed that there are 9 local
> arrays with large length in toast_insert_or_update() which seems to be a
> risk of stack overflow. Maybe we should put it as static or global.
Hmm. We currently reserve 512 kB between the kernel's limit, and the
limit we check in check_stack_depth(). See STACK_DEPTH_SLOP. Those
arrays add up to 52800 bytes on a 64-bit maching, if I did my math
right. So there's still a lot of headroom. I agree that it nevertheless
seems a bit excessive, though.
> With the patch,
>
> Time: 4728.142 ms (00:04.728)
> Time: 14203.983 ms (00:14.204)
> Time: 1008.669 ms (00:01.009)
>
> Baseline,
> Time: 11096.146 ms (00:11.096)
> Time: 13106.741 ms (00:13.107)
> Time: 1100.174 ms (00:01.100)
Nice speedup!
> While for toast and large column size there is < 10% decrease but for
> small column size the improvement is super good. Actually if I hardcode
> the batch count as 4 all test cases are better but the improvement for
> small column size is smaller than that with current patch. Pretty much
> the number 4 is quite case specific so I can not hardcode that in the
> patch. Of course we could further tune that but the current value seems
> to be a good trade-off?
Have you done any profiling, on why the multi-insert is slower with
large tuples? In principle, I don't see why it should be slower.
- Heikki
On Wed, Mar 06, 2019 at 10:06:27PM +0800, Paul Guo wrote: > Hello, Postgres hackers, > > The copy code has used batch insert with function heap_multi_insert() to > speed up. It seems that Create Table As or Materialized View could leverage > that code also to boost the performance also. Attached is a patch to > implement that. This is great! Is this optimization doable for multi-row INSERTs, either with tuples spelled out in the body of the query or in constructs like INSERT ... SELECT ...? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Wed, Mar 06, 2019 at 10:06:27PM +0800, Paul Guo wrote:
> Hello, Postgres hackers,
>
> The copy code has used batch insert with function heap_multi_insert() to
> speed up. It seems that Create Table As or Materialized View could leverage
> that code also to boost the performance also. Attached is a patch to
> implement that.
This is great!
Is this optimization doable for multi-row INSERTs, either with tuples
spelled out in the body of the query or in constructs like INSERT ...
SELECT ...?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> https://urldefense.proofpoint.com/v2/url?u=http-3A__fetter.org_&d=DwIBAg&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=Usi0ex6Ch92MsB5QQDgYFw&m=wgGDTDFzZV7nnMm0NFt-yGKmm_KZk18RXKP9HL8h6UE&s=tnaoLdajjR0Ew-93XUliHW1FUspVl09pIFd9aXxvqc8&e=
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
create table tt as select a,b,c from t11;
\timing
drop table tt;
create table t11 (a int, b int, c int, d int);
insert into t11 select s,s,s,s from generate_series(1, 10000000) s;
analyze t11;
-- tuples that are untoasted and tuple size is 1984 bytes.
create table t12 (a name, b name, c name, d name, e name, f name, g name, h name, i name, j name, k name, l name, m name, n name, o name, p name, q name, r name, s name, t name, u name, v name, w name, x name, y name, z name, a1 name, a2 name, a3 name, a4 name, a5 name);
insert into t12 select 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'a', 'b', 'c', 'd', 'e' from generate_series(1, 500000);
analyze t12;
-- tuples that are untoasted and tuple size is 2112 bytes.
create table t13 (a name, b name, c name, d name, e name, f name, g name, h name, i name, j name, k name, l name, m name, n name, o name, p name, q name, r name, s name, t name, u name, v name, w name, x name, y name, z name, a1 name, a2 name, a3 name, a4 name, a5 name, a6 name, a7 name);
insert into t13 select 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'a', 'b', 'c', 'd', 'e', 'f', 'g' from generate_series(1, 500000);
analyze t13;
-- tuples that are toastable and tuple compressed size is 1084.
create table t14 (a text, b text, c text, d text, e text, f text, g text, h text, i text, j text, k text, l text, m text, n text, o text, p text, q text, r text, s text, t text, u text, v text, w text, x text, y text, z text);
insert into t14 select i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i from (select repeat('123456789', 10000) from generate_series(1,5000)) i;
analyze t14;
baseline patch diff% SQL
10.1 5.57 44.85% create table tt as select * from t11;
10.7 5.52 48.41% create table tt as select a,b,c from t11;
9.57 10.2 -6.58% create table tt as select * from t12;
9.64 8.63 10.48% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4 from t12;
14.2 14.46 -1.83% create table tt as select * from t13;
11.88 12.05 -1.43% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4,a5,a6 from t13;
3.17 3.25 -2.52% create table tt as select * from t14;
2.93 3.12 -6.48% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y from t14;
scenario2: all related kernel caches are populated after previous testing.
baseline patch diff% SQL
9.6 4.97 48.23% create table tt as select * from t11;
10.41 5.32 48.90% create table tt as select a,b,c from t11;
9.12 9.52 -4.38% create table tt as select * from t12;
9.66 8.6 10.97% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4 from t12;
13.56 13.6 -0.30% create table tt as select * from t13;
11.36 11.7 -2.99% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4,a5,a6 from t13;
3.08 3.13 -1.62% create table tt as select * from t14;
2.95 3.03 -2.71% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y from t14;
On 06/03/2019 22:06, Paul Guo wrote:
> The patch also modifies heap_multi_insert() a bit to do a bit further
> code-level optimization by using static memory, instead of using memory
> context and dynamic allocation.
If toasting is required, heap_prepare_insert() creates a palloc'd tuple.
That is still leaked to the current memory context.
Leaking into the current memory context is not a bad thing, because
resetting a memory context is faster than doing a lot of pfree() calls.
The callers just need to be prepared for that, and use a short-lived
memory context.
> By the way, while looking at the code, I noticed that there are 9 local
> arrays with large length in toast_insert_or_update() which seems to be a
> risk of stack overflow. Maybe we should put it as static or global.
Hmm. We currently reserve 512 kB between the kernel's limit, and the
limit we check in check_stack_depth(). See STACK_DEPTH_SLOP. Those
arrays add up to 52800 bytes on a 64-bit maching, if I did my math
right. So there's still a lot of headroom. I agree that it nevertheless
seems a bit excessive, though.
> With the patch,
>
> Time: 4728.142 ms (00:04.728)
> Time: 14203.983 ms (00:14.204)
> Time: 1008.669 ms (00:01.009)
>
> Baseline,
> Time: 11096.146 ms (00:11.096)
> Time: 13106.741 ms (00:13.107)
> Time: 1100.174 ms (00:01.100)
Nice speedup!
> While for toast and large column size there is < 10% decrease but for
> small column size the improvement is super good. Actually if I hardcode
> the batch count as 4 all test cases are better but the improvement for
> small column size is smaller than that with current patch. Pretty much
> the number 4 is quite case specific so I can not hardcode that in the
> patch. Of course we could further tune that but the current value seems
> to be a good trade-off?
Have you done any profiling, on why the multi-insert is slower with
large tuples? In principle, I don't see why it should be slower.
- Heikki
Вложения
Hi all,I've been working other things until recently I restarted the work, profiling & refactoring the code.It's been a long time since the last patch was proposed. The new patch has now been firstly refactored due to4da597edf1bae0cf0453b5ed6fc4347b6334dfe1 (Make TupleTableSlots extensible, finish split of existing slot type).Now that TupleTableSlot, instead of HeapTuple is one argument of intorel_receive() so we can not get thetuple length directly. This patch now gets the tuple length if we know all columns are with fixed widths, elsewe calculate an avg. tuple length using the first MAX_MULTI_INSERT_SAMPLES (defined as 1000) tuplesand use for the total length of tuples in a batch.I noticed that to do batch insert, we might need additional memory copy sometimes comparing with "single insert"(that should be the reason that we previously saw a bit regressions) so a good solution seems to fall backto "single insert" if the tuple length is larger than a threshold. I set this as 2000 after quick testing.To make test stable and strict, I run checkpoint before each ctas, the test script looks like this:checkpoint;\timing
create table tt as select a,b,c from t11;
\timing
drop table tt;Also previously I just tested the BufferHeapTupleTableSlot (i.e. create table tt as select * from t11),this time I test VirtualTupleTableSlot (i.e. create table tt as select a,b,c from t11) additionally.It seems that VirtualTupleTableSlot is very common in real cases.I tested four kinds of tables, see below SQLs.-- tuples with small size.
create table t11 (a int, b int, c int, d int);
insert into t11 select s,s,s,s from generate_series(1, 10000000) s;
analyze t11;
-- tuples that are untoasted and tuple size is 1984 bytes.
create table t12 (a name, b name, c name, d name, e name, f name, g name, h name, i name, j name, k name, l name, m name, n name, o name, p name, q name, r name, s name, t name, u name, v name, w name, x name, y name, z name, a1 name, a2 name, a3 name, a4 name, a5 name);
insert into t12 select 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'a', 'b', 'c', 'd', 'e' from generate_series(1, 500000);
analyze t12;
-- tuples that are untoasted and tuple size is 2112 bytes.
create table t13 (a name, b name, c name, d name, e name, f name, g name, h name, i name, j name, k name, l name, m name, n name, o name, p name, q name, r name, s name, t name, u name, v name, w name, x name, y name, z name, a1 name, a2 name, a3 name, a4 name, a5 name, a6 name, a7 name);
insert into t13 select 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'a', 'b', 'c', 'd', 'e', 'f', 'g' from generate_series(1, 500000);
analyze t13;
-- tuples that are toastable and tuple compressed size is 1084.
create table t14 (a text, b text, c text, d text, e text, f text, g text, h text, i text, j text, k text, l text, m text, n text, o text, p text, q text, r text, s text, t text, u text, v text, w text, x text, y text, z text);
insert into t14 select i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i, i from (select repeat('123456789', 10000) from generate_series(1,5000)) i;
analyze t14;I also tested two scenarios for each testing.One is to clean up all kernel caches (page & inode & dentry on Linux) using the command below and then run the test,sync; echo 3 > /proc/sys/vm/drop_cachesAfter running all tests all relation files will be in kernel cache (my test system memory is large enough to accommodate all relation files),then I run the tests again. I run like this because in real scenario the result of the test should be among the two results. Also I reruneach test and finally I calculate the average results as the experiment results. Below are some results:scenario1: All related kernel caches are cleaned up (note the first two columns are time with second).baseline patch diff% SQL
10.1 5.57 44.85% create table tt as select * from t11;
10.7 5.52 48.41% create table tt as select a,b,c from t11;
9.57 10.2 -6.58% create table tt as select * from t12;
9.64 8.63 10.48% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4 from t12;
14.2 14.46 -1.83% create table tt as select * from t13;
11.88 12.05 -1.43% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4,a5,a6 from t13;
3.17 3.25 -2.52% create table tt as select * from t14;
2.93 3.12 -6.48% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y from t14;
scenario2: all related kernel caches are populated after previous testing.
baseline patch diff% SQL
9.6 4.97 48.23% create table tt as select * from t11;
10.41 5.32 48.90% create table tt as select a,b,c from t11;
9.12 9.52 -4.38% create table tt as select * from t12;
9.66 8.6 10.97% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4 from t12;
13.56 13.6 -0.30% create table tt as select * from t13;
11.36 11.7 -2.99% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z,a1,a2,a3,a4,a5,a6 from t13;
3.08 3.13 -1.62% create table tt as select * from t14;
2.95 3.03 -2.71% create table tt as select a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y from t14;
From above we can get some tentative conclusions:1. t11: For short-size tables, batch insert improves much (40%+).2. t12: For BufferHeapTupleTableSlot, the patch slows down 4.x%-6.x%, but for VirtualTupleTableSlot it improves 10.x%.If we look at execTuples.c, it looks like this is quite relevant to additional memory copy. It seems that VirtualTupleTableSlot ismore common than the BufferHeapTupleTableSlot so probably the current code should be fine for most real cases. Or it's possibleto determine multi-insert also according to the input slot tuple but this seems to be ugly in code. Or continue to lower the threshold a bitso that "create table tt as select * from t12;" also improves although this hurts the VirtualTupleTableSlot case.
relation files are in cache:
3. for t13, new code still uses single insert so the difference should be small. I just want to see the regression when even we use "single insert".4. For toast case t14, the degradation is small, not a big deal.By the way, did we try or think about allow better prefetch (on Linux) for seqscan. i.e. POSIX_FADV_SEQUENTIAL in posix_fadvise() to enlarge the kernel readahead window. Suppose this should help if seq tuple handling is faster than default kernel readahead setting.v2 patch is attached.On Thu, Mar 7, 2019 at 4:54 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:On 06/03/2019 22:06, Paul Guo wrote:
> The patch also modifies heap_multi_insert() a bit to do a bit further
> code-level optimization by using static memory, instead of using memory
> context and dynamic allocation.
If toasting is required, heap_prepare_insert() creates a palloc'd tuple.
That is still leaked to the current memory context.
Leaking into the current memory context is not a bad thing, because
resetting a memory context is faster than doing a lot of pfree() calls.
The callers just need to be prepared for that, and use a short-lived
memory context.
> By the way, while looking at the code, I noticed that there are 9 local
> arrays with large length in toast_insert_or_update() which seems to be a
> risk of stack overflow. Maybe we should put it as static or global.
Hmm. We currently reserve 512 kB between the kernel's limit, and the
limit we check in check_stack_depth(). See STACK_DEPTH_SLOP. Those
arrays add up to 52800 bytes on a 64-bit maching, if I did my math
right. So there's still a lot of headroom. I agree that it nevertheless
seems a bit excessive, though.
> With the patch,
>
> Time: 4728.142 ms (00:04.728)
> Time: 14203.983 ms (00:14.204)
> Time: 1008.669 ms (00:01.009)
>
> Baseline,
> Time: 11096.146 ms (00:11.096)
> Time: 13106.741 ms (00:13.107)
> Time: 1100.174 ms (00:01.100)
Nice speedup!
> While for toast and large column size there is < 10% decrease but for
> small column size the improvement is super good. Actually if I hardcode
> the batch count as 4 all test cases are better but the improvement for
> small column size is smaller than that with current patch. Pretty much
> the number 4 is quite case specific so I can not hardcode that in the
> patch. Of course we could further tune that but the current value seems
> to be a good trade-off?
Have you done any profiling, on why the multi-insert is slower with
large tuples? In principle, I don't see why it should be slower.
- Heikki
Вложения
On 17/06/2019 15:53, Paul Guo wrote: > I noticed that to do batch insert, we might need additional memory copy > sometimes comparing with "single insert" > (that should be the reason that we previously saw a bit regressions) so a > good solution seems to fall back > to "single insert" if the tuple length is larger than a threshold. I set > this as 2000 after quick testing. Where does the additional memory copy come from? Can we avoid doing it in the multi-insert case? - Heikki
On 17/06/2019 15:53, Paul Guo wrote:
> I noticed that to do batch insert, we might need additional memory copy
> sometimes comparing with "single insert"
> (that should be the reason that we previously saw a bit regressions) so a
> good solution seems to fall back
> to "single insert" if the tuple length is larger than a threshold. I set
> this as 2000 after quick testing.
Where does the additional memory copy come from? Can we avoid doing it
in the multi-insert case?
Sorry for the late reply. I took some time on looking at & debugging the code of TupleTableSlotOps
of various TupleTableSlot types carefully, especially the BufferHeapTupleTableSlot case on which
we seemed to see regression if no threshold is set, also debugging & testing more of the CTAS case.
I found my previous word "additional memory copy" (mainly tuple content copy against single insert)
t12_Virtual 8041.637 8191.317 8182.404
t12_Virtual 8167.792 7970.537 8106.874
t12_Virtual means: create table tt as select *partial columns* from t12;
This actually confused me a bit given we've cached the source table in shared buffers. I suspected checkpoint affects,
t12_Virtual 7445.742 7483.148 7593.583
t12_BufferHeapTuple 8186.302 7736.541 7759.056
t12_Virtual 8004.880 8096.712 7961.483
Вложения
On Mon, Sep 9, 2019 at 4:02 PM Paul Guo <pguo@pivotal.io> wrote:
>
> So in theory
> we should not worry about additional tuple copy overhead now, and then I tried the patch without setting
> multi-insert threshold as attached.
>
On 2019-Sep-25, Asim R P wrote: > I reviewed your patch today. It looks good overall. My concern is that > the ExecFetchSlotHeapTuple call does not seem appropriate. In a generic > place such as createas.c, we should be using generic tableam API only. > However, I can also see that there is no better alternative. We need to > compute the size of accumulated tuples so far, in order to decide whether > to stop accumulating tuples. There is no convenient way to obtain the > length of the tuple, given a slot. How about making that decision solely > based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call > altogether? ... maybe we should add a new operation to slots, that returns the (approximate?) size of a tuple? That would make this easy. (I'm not sure however what to do about TOAST considerations -- is it size in memory that we're worried about?) Also: + myState->mi_slots_size >= 65535) This magic number should be placed in a define next to the other one, but I'm not sure that heapam.h is a good location, since surely this applies to matviews in other table AMs too. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Sep 9, 2019 at 4:02 PM Paul Guo <pguo@pivotal.io> wrote:
>
> So in theory
> we should not worry about additional tuple copy overhead now, and then I tried the patch without setting
> multi-insert threshold as attached.
>I reviewed your patch today. It looks good overall. My concern is that the ExecFetchSlotHeapTuple call does not seem appropriate. In a generic place such as createas.c, we should be using generic tableam API only. However, I can also see that there is no better alternative. We need to compute the size of accumulated tuples so far, in order to decide whether to stop accumulating tuples. There is no convenient way to obtain the length of the tuple, given a slot. How about making that decision solely based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call altogether?
The multi insert copy code deals with index tuples also, which I don't see in the patch. Don't we need to consider populating indexes?
On 2019-Sep-25, Asim R P wrote:
> I reviewed your patch today. It looks good overall. My concern is that
> the ExecFetchSlotHeapTuple call does not seem appropriate. In a generic
> place such as createas.c, we should be using generic tableam API only.
> However, I can also see that there is no better alternative. We need to
> compute the size of accumulated tuples so far, in order to decide whether
> to stop accumulating tuples. There is no convenient way to obtain the
> length of the tuple, given a slot. How about making that decision solely
> based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call
> altogether?
... maybe we should add a new operation to slots, that returns the
(approximate?) size of a tuple? That would make this easy. (I'm not
sure however what to do about TOAST considerations -- is it size in
memory that we're worried about?)
Also:
+ myState->mi_slots_size >= 65535)
This magic number should be placed in a define next to the other one,
but I'm not sure that heapam.h is a good location, since surely this
applies to matviews in other table AMs too.
On Thu, Sep 26, 2019 at 7:13 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Sep-25, Asim R P wrote:
>
> > I reviewed your patch today. It looks good overall. My concern is that
> > the ExecFetchSlotHeapTuple call does not seem appropriate. In a generic
> > place such as createas.c, we should be using generic tableam API only.
> > However, I can also see that there is no better alternative. We need to
> > compute the size of accumulated tuples so far, in order to decide whether
> > to stop accumulating tuples. There is no convenient way to obtain the
> > length of the tuple, given a slot. How about making that decision solely
> > based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call
> > altogether?
>
> ... maybe we should add a new operation to slots, that returns the
> (approximate?) size of a tuple? That would make this easy. (I'm not
> sure however what to do about TOAST considerations -- is it size in
Hi, On 2019-09-26 10:43:27 -0300, Alvaro Herrera wrote: > On 2019-Sep-25, Asim R P wrote: > > > I reviewed your patch today. It looks good overall. My concern is that > > the ExecFetchSlotHeapTuple call does not seem appropriate. In a generic > > place such as createas.c, we should be using generic tableam API > > only. Indeed. > > However, I can also see that there is no better alternative. We need to > > compute the size of accumulated tuples so far, in order to decide whether > > to stop accumulating tuples. There is no convenient way to obtain the > > length of the tuple, given a slot. How about making that decision solely > > based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call > > altogether? > > ... maybe we should add a new operation to slots, that returns the > (approximate?) size of a tuple? Hm, I'm not convinced that it's worth adding that as a dedicated operation. It's not that clear what it'd exactly mean anyway - what would it measure? As referenced in the slot? As if it were stored on disk? etc? I wonder if the right answer wouldn't be to just measure the size of a memory context containing the batch slots, or something like that. > That would make this easy. (I'm not sure however what to do about > TOAST considerations -- is it size in memory that we're worried > about?) The in-memory size is probably fine, because in all likelihood the toasted cols are just going to point to on-disk datums, no? > Also: > > + myState->mi_slots_size >= 65535) > > This magic number should be placed in a define next to the other one, > but I'm not sure that heapam.h is a good location, since surely this > applies to matviews in other table AMs too. Right. I think it'd be better to move this into an AM independent place. Greetings, Andres Freund
Hi, On 2019-09-09 18:31:54 +0800, Paul Guo wrote: > diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c > index e9544822bf..8a844b3b5f 100644 > --- a/src/backend/access/heap/heapam.c > +++ b/src/backend/access/heap/heapam.c > @@ -2106,7 +2106,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, > CommandId cid, int options, BulkInsertState bistate) > { > TransactionId xid = GetCurrentTransactionId(); > - HeapTuple *heaptuples; > int i; > int ndone; > PGAlignedBlock scratch; > @@ -2115,6 +2114,10 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, > Size saveFreeSpace; > bool need_tuple_data = RelationIsLogicallyLogged(relation); > bool need_cids = RelationIsAccessibleInLogicalDecoding(relation); > + /* Declare it as static to let this memory be not on stack. */ > + static HeapTuple heaptuples[MAX_MULTI_INSERT_TUPLES]; > + > + Assert(ntuples <= MAX_MULTI_INSERT_TUPLES); > > /* currently not needed (thus unsupported) for heap_multi_insert() */ > AssertArg(!(options & HEAP_INSERT_NO_LOGICAL)); > @@ -2124,7 +2127,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, > HEAP_DEFAULT_FILLFACTOR); > > /* Toast and set header data in all the slots */ > - heaptuples = palloc(ntuples * sizeof(HeapTuple)); > for (i = 0; i < ntuples; i++) > { > HeapTuple tuple; I don't think this is a good idea. We shouldn't unnecessarily allocate 8KB on the stack. Is there any actual evidence this is a performance benefit? To me this just seems like it'll reduce the flexibility of the API, without any benefit. I'll also note that you've apparently not updated tableam.h to document this new restriction. Greetings, Andres Freund
Hi,
On 2019-09-09 18:31:54 +0800, Paul Guo wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index e9544822bf..8a844b3b5f 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -2106,7 +2106,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> CommandId cid, int options, BulkInsertState bistate)
> {
> TransactionId xid = GetCurrentTransactionId();
> - HeapTuple *heaptuples;
> int i;
> int ndone;
> PGAlignedBlock scratch;
> @@ -2115,6 +2114,10 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> Size saveFreeSpace;
> bool need_tuple_data = RelationIsLogicallyLogged(relation);
> bool need_cids = RelationIsAccessibleInLogicalDecoding(relation);
> + /* Declare it as static to let this memory be not on stack. */
> + static HeapTuple heaptuples[MAX_MULTI_INSERT_TUPLES];
> +
> + Assert(ntuples <= MAX_MULTI_INSERT_TUPLES);
>
> /* currently not needed (thus unsupported) for heap_multi_insert() */
> AssertArg(!(options & HEAP_INSERT_NO_LOGICAL));
> @@ -2124,7 +2127,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> HEAP_DEFAULT_FILLFACTOR);
>
> /* Toast and set header data in all the slots */
> - heaptuples = palloc(ntuples * sizeof(HeapTuple));
> for (i = 0; i < ntuples; i++)
> {
> HeapTuple tuple;
I don't think this is a good idea. We shouldn't unnecessarily allocate
8KB on the stack. Is there any actual evidence this is a performance
benefit? To me this just seems like it'll reduce the flexibility of the
API, without any benefit. I'll also note that you've apparently not
updated tableam.h to document this new restriction.
> > However, I can also see that there is no better alternative. We need to
> > compute the size of accumulated tuples so far, in order to decide whether
> > to stop accumulating tuples. There is no convenient way to obtain the
> > length of the tuple, given a slot. How about making that decision solely
> > based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call
> > altogether?
>
> ... maybe we should add a new operation to slots, that returns the
> (approximate?) size of a tuple?
Hm, I'm not convinced that it's worth adding that as a dedicated
operation. It's not that clear what it'd exactly mean anyway - what
would it measure? As referenced in the slot? As if it were stored on
disk? etc?
I wonder if the right answer wouldn't be to just measure the size of a
memory context containing the batch slots, or something like that.
Hi, On 2019-09-30 12:12:31 +0800, Paul Guo wrote: > > > > However, I can also see that there is no better alternative. We need > > to > > > > compute the size of accumulated tuples so far, in order to decide > > whether > > > > to stop accumulating tuples. There is no convenient way to obtain the > > > > length of the tuple, given a slot. How about making that decision > > solely > > > > based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple > > call > > > > altogether? > > > > > > ... maybe we should add a new operation to slots, that returns the > > > (approximate?) size of a tuple? > > > > Hm, I'm not convinced that it's worth adding that as a dedicated > > operation. It's not that clear what it'd exactly mean anyway - what > > would it measure? As referenced in the slot? As if it were stored on > > disk? etc? > > > > I wonder if the right answer wouldn't be to just measure the size of a > > memory context containing the batch slots, or something like that. > > > > > Probably a better way is to move those logic (append slot to slots, judge > when to flush, flush, clean up slots) into table_multi_insert()? That does not strike me as a good idea. The upper layer is going to need to manage some resources (e.g. it's the only bit that knows about how to manage lifetime of the incoming data), and by exposing it to each AM we're going to duplicate the necessary code too. > Generally the final implementation of table_multi_insert() should be > able to know the sizes easily. One concern is that currently just COPY > in the repo uses multi insert, so not sure if other callers in the > future want their own logic (or set up a flag to allow customization > but seems a bit over-designed?). And that is also a concern, it seems unlikely that we'll get the interface good. Greetings, Andres Freund
On Mon, Sep 30, 2019 at 12:38:02AM -0700, Andres Freund wrote: > That does not strike me as a good idea. The upper layer is going to need > to manage some resources (e.g. it's the only bit that knows about how to > manage lifetime of the incoming data), and by exposing it to each AM > we're going to duplicate the necessary code too. Latest status of the thread maps with a patch which still applies, still the discussion could go on as more review is needed. So I have moved it to next CF. -- Michael
Вложения
+ tuple = ExecFetchSlotHeapTuple(batchslot, true, NULL);
+
+ myState->mi_slots_num++;
+MemoryContextUsedSize(MemoryContext context)
+{
+ MemoryContextCounters total;
+
+ memset(&total, 0, sizeof(total));
+ context->methods->stats(context, NULL, NULL, &total);
+
+ return total.totalspace - total.freespace;
+}
As a reference, For "create table t1 as select * from t2", the above code returns
In an off-list discussion with Paul, we decided to withdraw this patch for now and instead create a new entry when there is a re-worked patch. This has now been done in the CF app. cheers ./daniel
Hello Paul-san, From: Daniel Gustafsson <daniel@yesql.se> > In an off-list discussion with Paul, we decided to withdraw this patch for now > and instead create a new entry when there is a re-worked patch. This has > now > been done in the CF app. Would you mind if I take over this patch for PG 15? I find this promising, as Bharath-san demonstrated good performanceby combining your patch and the parallel CTAS that Bharath-san has been working on. We'd like to do things thatenhance parallelism. Please allow me to start posting the revised patches next week if I can't get your reply. Regards Takayuki Tsunakawa
On Wed, May 26, 2021 at 12:18 PM tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> wrote: > > Hello Paul-san, > > From: Daniel Gustafsson <daniel@yesql.se> > > In an off-list discussion with Paul, we decided to withdraw this patch for now > > and instead create a new entry when there is a re-worked patch. This has > > now > > been done in the CF app. > > Would you mind if I take over this patch for PG 15? I find this promising, as Bharath-san demonstrated good performanceby combining your patch and the parallel CTAS that Bharath-san has been working on. We'd like to do things thatenhance parallelism. > > Please allow me to start posting the revised patches next week if I can't get your reply. Hi, I think the "New Table Access Methods for Multi and Single Inserts" patches at [1] make multi insert usage easy for COPY, CTAS/Create Mat View, Refresh Mat View and so on. It also has a patch for multi inserts in CTAS and Refresh Mat View (v6-0002-CTAS-and-REFRESH-Mat-View-With-New-Multi-Insert-T.patch). Please see that thread and feel free to review it. [1] - https://www.postgresql.org/message-id/CALj2ACXdrOmB6Na9amHWZHKvRT3Z0nwTRsCwoMT-npOBtmXLXg%40mail.gmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> > I think the "New Table Access Methods for Multi and Single Inserts" > patches at [1] make multi insert usage easy for COPY, CTAS/Create Mat > View, Refresh Mat View and so on. It also has a patch for multi > inserts in CTAS and Refresh Mat View > (v6-0002-CTAS-and-REFRESH-Mat-View-With-New-Multi-Insert-T.patch). > Please see that thread and feel free to review it. Ouch, I didn't notice that the patch was reborn in that thread. OK. Could you add it to the CF if you haven't yet? Regards Takayuki Tsunakawa
On Wed, May 26, 2021 at 12:49 PM tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> wrote: > > From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> > > I think the "New Table Access Methods for Multi and Single Inserts" > > patches at [1] make multi insert usage easy for COPY, CTAS/Create Mat > > View, Refresh Mat View and so on. It also has a patch for multi > > inserts in CTAS and Refresh Mat View > > (v6-0002-CTAS-and-REFRESH-Mat-View-With-New-Multi-Insert-T.patch). > > Please see that thread and feel free to review it. > > Ouch, I didn't notice that the patch was reborn in that thread. OK. > > Could you add it to the CF if you haven't yet? It already has one - https://commitfest.postgresql.org/33/2871/ With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com