Re: [HACKERS] Memory leak in GIN index build
I had the possibility to perform tests on 9.5, and can confirm the memory leak I was seeing is solved with the patch (and that's great :) ) Regards Marc On 18/04/2016 17:53, Julien Rouhaud wrote: > On 18/04/2016 16:33, Tom Lane wrote: >> I poked at this over the weekend, and got more unhappy the more I poked. >> Aside from the memory leakage issue, there are multiple coding-rule >> violations besides the one you noted about scope of the critical sections. >> One example is that in the page-split path for an inner page, we modify >> the contents of childbuf long before getting into the critical section. >> The number of out-of-date comments was depressingly large as well. >> >> I ended up deciding that the most reasonable fix was along the lines of >> my first alternative above. In the attached draft patches, the >> "placeToPage" method is split into two methods, "beginPlaceToPage" and >> "execPlaceToPage", where the second method is what's supposed to happen >> inside the critical section for a non-page-splitting update. Nothing >> that's done inside beginPlaceToPage is critical. >> >> I've tested this to the extent of verifying that it passes make >> check-world and stops the memory leak in your test case, but it could use >> more eyeballs on it. >> > Thanks! I also started working on it but it was very far from being > complete (and already much more ugly...). > > I couldn't find any problem in the patch. > > I wonder if asserting being in a critical section would be valuable in > the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in > dataExecPlaceToPageLeaf(), since it's filled far from this function. > >> Attached are draft patches against HEAD and 9.5 (they're the same except >> for BufferGetPage calls). I think we'd also want to back-patch to 9.4, >> but that will take considerable additional work because of the XLOG API >> rewrite that happened in 9.5. I also noted that some of the coding-rule >> violations seem to be new in 9.5, so the problems may be less severe in >> 9.4 --- the memory leak definitely exists there, though. >> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] star schema and the optimizer
On 27/02/2015 20:01, Marc Cousin wrote: On 27/02/2015 19:45, Tom Lane wrote: I wrote: I had actually thought that we'd fixed this type of problem in recent versions, and that you should be able to get a plan that would look like Nestloop - scan dim1 - Nestloop - scan dim2 - indexscan fact table using dim1.a and dim2.b After closer study, I think this is an oversight in commit e2fa76d80ba571d4de8992de6386536867250474, which quoth +It can be useful for the parameter value to be passed down through +intermediate layers of joins, for example: + +NestLoop +- Seq Scan on A +Hash Join +Join Condition: B.Y = C.W +- Seq Scan on B +- Index Scan using C_Z_IDX on C +Index Condition: C.Z = A.X + +If all joins are plain inner joins then this is unnecessary, because +it's always possible to reorder the joins so that a parameter is used +immediately below the nestloop node that provides it. But in the +presence of outer joins, join reordering may not be possible, and then +this option can be critical. Before version 9.2, Postgres used ad-hoc This reasoning overlooked the fact that if we need parameters from more than one relation, and there's no way to join those relations to each other directly, then we have to allow passing the dim1 parameter down through the join to dim2. The attached patch seems to fix it (modulo the need for some updates in the README, and maybe a regression test). Could you see if this produces satisfactory plans for you? From what I see, it's just perfect. I'll give it a more thorough look a bit later, but it seems to be exactly what I was waiting for. Thanks a lot. Regards I gave it another look this morning. It works very well with the initial test schema. The situation is much improved for me. I still have one issue: I've extended the test to more than 2 dimensions. marc=# explain analyze SELECT * FROM dim1,dim2,dim3,dim4,facts WHERE facts.dim1=dim1.a and facts.dim2=dim2.a and facts.dim3=dim3.a and facts.dim4=dim4.a and dim1.b between 10 and 60 AND dim2.b between 30 and 50 and dim3.b=8526 and dim4.b between 70 and 90; QUERY PLAN -- Nested Loop (cost=15.00..957710.99 rows=1 width=200) (actual time=793.247..793.247 rows=0 loops=1) - Nested Loop (cost=14.71..957704.75 rows=1 width=192) (actual time=793.245..793.245 rows=0 loops=1) Join Filter: (facts.dim3 = dim3.a) Rows Removed by Join Filter: 942 - Index Scan using idxdim32 on dim3 (cost=0.29..3300.29 rows=10 width=8) (actual time=49.498..67.923 rows=1 loops=1) Filter: (b = 8526) Rows Removed by Filter: 9 - Materialize (cost=14.41..954262.56 rows=962 width=184) (actual time=0.552..724.988 rows=942 loops=1) - Nested Loop (cost=14.41..954257.75 rows=962 width=184) (actual time=0.542..723.380 rows=942 loops=1) - Index Scan using idxdim22 on dim2 (cost=0.29..3648.29 rows=192 width=16) (actual time=0.074..47.445 rows=229 loops=1) Filter: ((b = 30) AND (b = 50)) Rows Removed by Filter: 99771 - Nested Loop (cost=14.12..4946.08 rows=501 width=168) (actual time=2.575..2.946 rows=4 loops=229) - Bitmap Heap Scan on dim1 (cost=13.43..573.60 rows=501 width=16) (actual time=0.170..0.647 rows=509 loops=229) Recheck Cond: ((b = 10) AND (b = 60)) Heap Blocks: exact=78547 - Bitmap Index Scan on idxdim11 (cost=0.00..13.30 rows=501 width=0) (actual time=0.112..0.112 rows=509 loops=229) Index Cond: ((b = 10) AND (b = 60)) - Index Scan using idxfacts on facts (cost=0.69..8.72 rows=1 width=152) (actual time=0.004..0.004 rows=0 loops=116561) Index Cond: ((dim1 = dim1.a) AND (dim2 = dim2.a)) - Index Scan using idxdim42 on dim4 (cost=0.29..6.24 rows=1 width=8) (never executed) Index Cond: (a = facts.dim4) Filter: ((b = 70) AND (b = 90)) Planning time: 8.092 ms Execution time: 793.621 ms (25 lignes) marc=# \d facts Table « public.facts » Colonne | Type | Modificateurs -++--- dim1| bigint | dim2| bigint | dim3| bigint | dim4| bigint | dim5| bigint | dim6| bigint | dim7| bigint | dim8| bigint | dim9| bigint | dim10 | bigint | data1 | text | data2 | text | data3 | text
Re: [HACKERS] star schema and the optimizer
On 27/02/2015 15:08, Tom Lane wrote: Marc Cousin cousinm...@gmail.com writes: So I gave a look at the optimizer's code to try to understand why I got this problem. If I understand correctly, the optimizer won't do cross joins, except if it has no choice. That's right, and as you say, the planning-speed consequences of doing otherwise would be disastrous. However, all you need to help it find the right plan is some dummy join condition between the dimension tables, which will allow the join path you want to be considered. Perhaps you could do something like SELECT * FROM dim1,dim2,facts WHERE facts.dim1=dim1.a and facts.dim2=dim2.a and dim1.b=12 AND dim2.b=17 and (dim1.a+dim2.a) is not null; No I can't. I cannot rewrite the query at all, in my context. What do you mean by disastrous ? I've given it a few tries here, and with 8 joins (same model, 7 dimensions), planning time is around 100ms. At least in my context, it's well worth the planning time, to save minutes of execution. I perfectly understand that it's not something that should be by default, that would be crazy. But in a datawarehouse, it seems to me that accepting one, or even a few seconds of planning time to save minutes of execution is perfectly legetimate. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] star schema and the optimizer
On 27/02/2015 19:45, Tom Lane wrote: I wrote: I had actually thought that we'd fixed this type of problem in recent versions, and that you should be able to get a plan that would look like Nestloop - scan dim1 - Nestloop - scan dim2 - indexscan fact table using dim1.a and dim2.b After closer study, I think this is an oversight in commit e2fa76d80ba571d4de8992de6386536867250474, which quoth +It can be useful for the parameter value to be passed down through +intermediate layers of joins, for example: + + NestLoop + - Seq Scan on A + Hash Join + Join Condition: B.Y = C.W + - Seq Scan on B + - Index Scan using C_Z_IDX on C + Index Condition: C.Z = A.X + +If all joins are plain inner joins then this is unnecessary, because +it's always possible to reorder the joins so that a parameter is used +immediately below the nestloop node that provides it. But in the +presence of outer joins, join reordering may not be possible, and then +this option can be critical. Before version 9.2, Postgres used ad-hoc This reasoning overlooked the fact that if we need parameters from more than one relation, and there's no way to join those relations to each other directly, then we have to allow passing the dim1 parameter down through the join to dim2. The attached patch seems to fix it (modulo the need for some updates in the README, and maybe a regression test). Could you see if this produces satisfactory plans for you? From what I see, it's just perfect. I'll give it a more thorough look a bit later, but it seems to be exactly what I was waiting for. Thanks a lot. Regards -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] star schema and the optimizer
On 27/02/2015 15:27, Marc Cousin wrote: On 27/02/2015 15:08, Tom Lane wrote: Marc Cousin cousinm...@gmail.com writes: So I gave a look at the optimizer's code to try to understand why I got this problem. If I understand correctly, the optimizer won't do cross joins, except if it has no choice. That's right, and as you say, the planning-speed consequences of doing otherwise would be disastrous. However, all you need to help it find the right plan is some dummy join condition between the dimension tables, which will allow the join path you want to be considered. Perhaps you could do something like SELECT * FROM dim1,dim2,facts WHERE facts.dim1=dim1.a and facts.dim2=dim2.a and dim1.b=12 AND dim2.b=17 and (dim1.a+dim2.a) is not null; No I can't. I cannot rewrite the query at all, in my context. What do you mean by disastrous ? I've given it a few tries here, and with 8 joins (same model, 7 dimensions), planning time is around 100ms. At least in my context, it's well worth the planning time, to save minutes of execution. I perfectly understand that it's not something that should be by default, that would be crazy. But in a datawarehouse, it seems to me that accepting one, or even a few seconds of planning time to save minutes of execution is perfectly legetimate. I have given it a bit more thought. Could it be possible, to mitigate this, to permit only a few (few being to define) cross joins ? Still optional, of course, it still has an important cost. Only allowing cross joins for the first 3 levels, and keeping this to left-right sided joins, I can plan up to 11 joins on my small test machine in 500ms (instead of 150ms with the unpatched one), and get a good plan, good meaning 100 times faster. Regards -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] star schema and the optimizer
Hi all, I've been facing an issue with star schemas for a while. PostgreSQL's performance is not that good without rewriting the query (or at least I couldn't find a way to make it do what I want). Here is a very basic mockup schema I used for the rest of this mail. It's of course too small to see very long runtimes (I see minutes, not tenths of seconds, with the schema that triggered this work): create table dim1 (a int, b int); create table dim2 (a int, b int); insert into dim1 select i,(random()*1000)::int from generate_series(1,1) g(i); insert into dim2 select i,(random()*1000)::int from generate_series(1,1) g(i); create index idxdim11 on dim1(b); create index idxdim12 on dim1(a); create index idxdim21 on dim2(b); create index idxdim22 on dim2(a); create table facts (dim1 bigint, dim2 bigint, data1 text, data2 text, data3 text, data4 text, data5 text, data6 text); insert into facts select (random()*1000)::int, (random()*1000)::int,i,i,i,i,i from generate_series(1,1000) g(i); create index idxfacts on facts(dim1,dim2); analyze; Here is the query that I want to make faster: SELECT * FROM dim1,dim2,facts WHERE facts.dim1=dim1.a and facts.dim2=dim2.a and dim1.b=12 AND dim2.b=17; PostgreSQL creates this plan: Nested Loop (cost=233.98..207630.07 rows=8 width=99) (actual time=131.891..131.891 rows=0 loops=1) Join Filter: (facts.dim2 = dim2.a) Rows Removed by Join Filter: 239796 - Index Scan using idxdim22 on dim2 (cost=0.29..343.29 rows=9 width=8) (actual time=0.672..4.436 rows=12 loops=1) Filter: (b = 17) Rows Removed by Filter: 9988 - Materialize (cost=233.70..206094.28 rows=9000 width=91) (actual time=0.471..6.673 rows=19983 loops=12) - Nested Loop (cost=233.70..206049.28 rows=9000 width=91) (actual time=5.633..53.121 rows=19983 loops=1) - Index Scan using idxdim12 on dim1 (cost=0.29..343.29 rows=9 width=8) (actual time=0.039..4.359 rows=10 loops=1) Filter: (b = 12) Rows Removed by Filter: 9990 - Bitmap Heap Scan on facts (cost=233.41..22756.32 rows=9990 width=83) (actual time=1.113..4.146 rows=1998 loops=10) Recheck Cond: (dim1 = dim1.a) Heap Blocks: exact=19085 - Bitmap Index Scan on idxfacts (cost=0.00..230.92 rows=9990 width=0) (actual time=0.602..0.602 rows=1998 loops=10) Index Cond: (dim1 = dim1.a) Planning time: 1.896 ms Execution time: 132.588 ms What I used to do was to set join_collapse_limit to 1 and rewrite the query like this: EXPLAIN ANALYZE SELECT * FROM dim1 cross join dim2 join facts on (facts.dim1=dim1.a and facts.dim2=dim2.a) where dim1.b=12 AND dim2.b=17; QUERY PLAN Nested Loop (cost=13.25..3661.66 rows=8 width=99) (actual time=0.758..0.758 rows=0 loops=1) - Nested Loop (cost=8.71..57.82 rows=81 width=16) (actual time=0.065..0.151 rows=120 loops=1) - Bitmap Heap Scan on dim1 (cost=4.35..28.39 rows=9 width=8) (actual time=0.040..0.057 rows=10 loops=1) Recheck Cond: (b = 12) Heap Blocks: exact=10 - Bitmap Index Scan on idxdim11 (cost=0.00..4.35 rows=9 width=0) (actual time=0.033..0.033 rows=10 loops=1) Index Cond: (b = 12) - Materialize (cost=4.35..28.44 rows=9 width=8) (actual time=0.002..0.006 rows=12 loops=10) - Bitmap Heap Scan on dim2 (cost=4.35..28.39 rows=9 width=8) (actual time=0.021..0.040 rows=12 loops=1) Recheck Cond: (b = 17) Heap Blocks: exact=11 - Bitmap Index Scan on idxdim21 (cost=0.00..4.35 rows=9 width=0) (actual time=0.017..0.017 rows=12 loops=1) Index Cond: (b = 17) - Bitmap Heap Scan on facts (cost=4.54..44.39 rows=10 width=83) (actual time=0.004..0.004 rows=0 loops=120) Recheck Cond: ((dim1 = dim1.a) AND (dim2 = dim2.a)) - Bitmap Index Scan on idxfacts (cost=0.00..4.53 rows=10 width=0) (actual time=0.004..0.004 rows=0 loops=120) Index Cond: ((dim1 = dim1.a) AND (dim2 = dim2.a)) Planning time: 0.520 ms Execution time: 0.812 ms The cost is 100 times lower. So this plan seems to be a good candidate. Or at least it keeps my users happy. That rewriting worked for me, but today, I'm in a context where I cannot rewrite the query... it's generated. So I gave a look at the optimizer's code to try to understand why I got this problem. If I understand correctly, the optimizer won't do cross joins, except if it has no choice. A funny sidenote before continuing: having dim1.b = dim2.b gives the right plan
Re: [HACKERS] segfault with contrib lo
On Tuesday 08 October 2013 12:28:46 Robert Haas wrote: On Mon, Oct 7, 2013 at 12:32 PM, Marc Cousin cousinm...@gmail.com wrote: I was using the lo contrib a few days ago and wasn't paying attention, and forgot the for each row in the create trigger command... PostgreSQL segfaulted, when the trigger tried to access the row's attributes. Please find attached a patch to control that the trigger is correctly defined (as in the example): a before trigger, for each row, and a parameter (if the parameter was omitted, it segfaulted too). I hope I did this correctly. Thanks for the patch. Please add it to the next CommitFest. https://commitfest.postgresql.org/action/commitfest_view/open Done. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] segfault with contrib lo
I was using the lo contrib a few days ago and wasn't paying attention, and forgot the for each row in the create trigger command... PostgreSQL segfaulted, when the trigger tried to access the row's attributes. Please find attached a patch to control that the trigger is correctly defined (as in the example): a before trigger, for each row, and a parameter (if the parameter was omitted, it segfaulted too). I hope I did this correctly. Regards, Marc.diff --git a/contrib/lo/lo.c b/contrib/lo/lo.c index 9dbbbce..2b9477a 100644 --- a/contrib/lo/lo.c +++ b/contrib/lo/lo.c @@ -50,6 +50,13 @@ lo_manage(PG_FUNCTION_ARGS) tupdesc = trigdata-tg_relation-rd_att; args = trigdata-tg_trigger-tgargs; + if (!TRIGGER_FIRED_FOR_ROW(trigdata-tg_event)) /* internal error */ + elog(ERROR, not fired by a for each row trigger); + + if (!TRIGGER_FIRED_BEFORE(trigdata-tg_event)) /* internal error */ + elog(ERROR, not fired as a before trigger); + + /* tuple to return to Executor */ if (TRIGGER_FIRED_BY_UPDATE(trigdata-tg_event)) rettuple = newtuple; @@ -59,6 +66,9 @@ lo_manage(PG_FUNCTION_ARGS) /* Are we deleting the row? */ isdelete = TRIGGER_FIRED_BY_DELETE(trigdata-tg_event); + if (args == NULL) /* internal error */ + elog (ERROR, no column name provided in the trigger definition); + /* Get the column we're interested in */ attnum = SPI_fnumber(tupdesc, args[0]); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance problem in PLPgSQL
On 24/08/2013 21:16, Tom Lane wrote: Marc Cousin cousinm...@gmail.com writes: On 23/08/2013 23:55, Tom Lane wrote: My previous suggestion was to estimate planning cost as 10 * (length(plan-rangetable) + 1) but on reflection it ought to be scaled by one of the cpu cost constants, so perhaps 1000 * cpu_operator_cost * (length(plan-rangetable) + 1) which'd mean a custom plan has to be estimated to save a minimum of about 5 cost units (more if more than 1 table is used) before it'll be chosen. I'm tempted to make the multiplier be 1 not 1000, but it seems better to be conservative about changing the behavior until we see how well this works in practice. Objections, better ideas? No better idea as far as I'm concerned, of course :) But it is a bit tricky to understand what is going on when you get hit by it, and using a very approximated cost of the planning time seems the most logical to me. So I'm all for this solution. I've pushed a patch along this line. I verified it fixes your original example, but maybe you could try it on your real application? http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=005f583ba4e6d4d19b62959ef8e70a3da4d188a5 regards, tom lane I think that won't be possible :( It's one of those environments where you have to ask lots of permissions before doing anything. I'll do my best to have them do a test with this patch. Thanks a lot. Marc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance problem in PLPgSQL
On 23/08/2013 23:55, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: please, can you send a self explained test this issue should be fixed, and we need a examples. We already had a perfectly good example at the beginning of this thread. What's missing is a decision on how we ought to approximate the cost of planning (relative to execution costs). As I mentioned upthread, it doesn't seem unreasonable to me to do something quick-and-dirty based on the length of the plan's rangetable. Pretty nearly anything would fix these specific situations where the estimated execution cost is negligible. It's possible that there are more complicated cases where we'll need a more accurate estimate, but we've not seen an example of that yet. My previous suggestion was to estimate planning cost as 10 * (length(plan-rangetable) + 1) but on reflection it ought to be scaled by one of the cpu cost constants, so perhaps 1000 * cpu_operator_cost * (length(plan-rangetable) + 1) which'd mean a custom plan has to be estimated to save a minimum of about 5 cost units (more if more than 1 table is used) before it'll be chosen. I'm tempted to make the multiplier be 1 not 1000, but it seems better to be conservative about changing the behavior until we see how well this works in practice. Objections, better ideas? regards, tom lane No better idea as far as I'm concerned, of course :) But it is a bit tricky to understand what is going on when you get hit by it, and using a very approximated cost of the planning time seems the most logical to me. So I'm all for this solution. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Performance problem in PLPgSQL
Hi, I've been trying to diagnose a severe performance regression we've been having in one of our plpgsql procedures. The example below is of course extremely simplified, and obviously not what we are really doing in the database, but it exhibits the slowdown between 9.1.9 and 9.2.4. So here is the example: create table table_test_int (col_01 int); create table table_test_numeric (col_01 numeric); CREATE OR REPLACE FUNCTION public.test_insert(nb integer) RETURNS text LANGUAGE plpgsql AS $function$ DECLARE time_start timestamp; time_stop timestamp; tmp_numeric numeric; BEGIN time_start :=clock_timestamp(); FOR i IN 1..nb LOOP INSERT INTO table_test_int(col_01) VALUES (i); END LOOP; time_stop :=clock_timestamp(); RAISE NOTICE 'time for int:%',time_stop-time_start; time_start :=clock_timestamp(); FOR i IN 1..nb LOOP INSERT INTO table_test_numeric(col_01) VALUES (i); END LOOP; time_stop :=clock_timestamp(); RAISE NOTICE 'time for numeric:%',time_stop-time_start; time_start :=clock_timestamp(); FOR i IN 1..nb LOOP INSERT INTO table_test_numeric(col_01) VALUES (i::numeric); END LOOP; time_stop :=clock_timestamp(); RAISE NOTICE 'time for numeric, casted:%',time_stop-time_start; time_start :=clock_timestamp(); FOR i IN 1..nb LOOP tmp_numeric:=cast(i as numeric); INSERT INTO table_test_numeric(col_01) VALUES (tmp_numeric); END LOOP; time_stop :=clock_timestamp(); RAISE NOTICE 'time for numeric with tmp variable:%',time_stop-time_start; RETURN 1; END; $function$ ; It just inserts nb records in a loop in 4 different maneers: - Directly in an int field - Then in a numeric field (that's where we're having problems) - Then in the same numeric field, but trying a cast (it doesn't change a thing) - Then tries with an intermediary temp variable of numeric type (which solves the problem). Here are the runtimes (tables were truncated beforehand): 9.1.9: select test_insert(100); NOTICE: time for int:00:00:09.526009 NOTICE: time for numeric:00:00:10.557126 NOTICE: time for numeric, casted:00:00:10.821369 NOTICE: time for numeric with tmp variable:00:00:10.850847 9.2.4: select test_insert(100); NOTICE: time for int:00:00:09.477044 NOTICE: time for numeric:00:00:24.757032 NOTICE: time for numeric, casted:00:00:24.791016 NOTICE: time for numeric with tmp variable:00:00:10.89332 I really don't know exactly where the problem comes from… but it's been hurting a function very badly (there are several of these static queries with types mismatch). And of course, the problem is not limited to numeric… text has the exact same problem. Regards, Marc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Problem with background worker
Hi,
I'm trying to write a background writer, and I'm facing a problem with
timestamps. The following code is where I'm having a problem (it's just a demo
for
the problem):
BackgroundWorkerInitializeConnection(test, NULL);
while (!got_sigterm)
{
int ret;
/* Wait 1s */
ret = WaitLatch(MyProc-procLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
1000L);
ResetLatch(MyProc-procLatch);
/* Insert dummy for now */
StartTransactionCommand();
SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
ret = SPI_execute(INSERT INTO log VALUES
(now(),statement_timestamp(),clock_timestamp()), false, 0);
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
}
\d log
Column| Type | Modifiers
-+--+---
now | timestamp with time zone |
statement_timestamp | timestamp with time zone |
clock_timestamp | timestamp with time zone |
Here is its content. Only the clock_timestamp is right. There are missing
records at the
beginning because i truncated the table for this example.
now | statement_timestamp |
clock_timestamp
---+---+---
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:52.77683+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:53.784301+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:54.834212+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:55.848497+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:56.872671+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:57.888461+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:58.912448+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:01:59.936335+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:02:00.951247+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:02:01.967937+01
2013-03-20 15:01:44.618623+01 | 2013-03-20 15:01:44.618623+01 | 2013-03-20
15:02:02.983613+01
Most of the code is copy/paste from worker_spi (I don't really understand the
PushActiveSnapshot(GetTransactionSnapshot()) and PopActiveSnapshot() but the
behaviour is the same with or without them, and they were in worker_spi).
I guess I'm doing something wrong, but I really dont't know what it is.
Should I attach the whole code ?
Regards,
Marc
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Problem with background worker
On 20/03/2013 16:33, Alvaro Herrera wrote:
Marc Cousin escribió:
Hi,
I'm trying to write a background writer, and I'm facing a problem with
timestamps. The following code is where I'm having a problem (it's just a demo
for
the problem):
BackgroundWorkerInitializeConnection(test, NULL);
while (!got_sigterm)
{
int ret;
/* Wait 1s */
ret = WaitLatch(MyProc-procLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
1000L);
ResetLatch(MyProc-procLatch);
/* Insert dummy for now */
StartTransactionCommand();
SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
ret = SPI_execute(INSERT INTO log VALUES
(now(),statement_timestamp(),clock_timestamp()), false, 0);
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
}
Ah. The reason for this problem is that the statement start time (which
also sets the transaction start time, when it's the first statement) is
set by postgres.c, not the transaction-control functions in xact.c. So
you'd need to add a SetCurrentStatementStartTimestamp() call somewhere
in your loop.
Yes, that works. Thanks a lot !
Maybe this should be added to the worker_spi example ?
Regards
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] small bug on 3-digit years in 9.2-dev
Hi,
While working on the What's new in 9.2, I think I found a small bug:
SELECT to_date('519-07-02','YYY-MM-DD');
to_date
0519-07-02
(1 row)
It comes, I think, from the year 519 case not being handled in the following
code. Patch attached
+ if (year 70)
+ return year + 2000;
+ /* Force 70-99 into the 1900's */
+ else if (year = 70 year 100)
+ return year + 1900;
+ /* Force 100-519 into the 2000's */
+ else if (year = 100 year 519)
+ return year + 2000;
+ /* Force 520-999 into the 1000's */
+ else if (year = 520 year 1000)
+ return year + 1000;
+ else
+ return year;
Regards
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 765c6aa..8eb7d5d 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -1997,7 +1997,7 @@ adjust_partial_year_to_2020(int year)
else if (year = 70 year 100)
return year + 1900;
/* Force 100-519 into the 2000's */
- else if (year = 100 year 519)
+ else if (year = 100 year = 519)
return year + 2000;
/* Force 520-999 into the 1000's */
else if (year = 520 year 1000)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] lock_timeout and common SIGALRM framework
On Mon, 2012-04-23 at 10:53 +0200, Boszormenyi Zoltan wrote: 2012-04-10 09:02 keltezéssel, Boszormenyi Zoltan írta: 2012-04-06 14:47 keltezéssel, Cousin Marc írta: On 05/04/12 08:02, Boszormenyi Zoltan wrote: 2012-04-04 21:30 keltezéssel, Alvaro Herrera írta: I think this patch is doing two things: first touching infrastructure stuff and then adding lock_timeout on top of that. Would it work to split the patch in two pieces? Sure. Attached is the split version. Best regards, Zoltán Böszörményi Hi, I've started looking at and testing both patches. Technically speaking, I think the source looks much better than the first version of lock timeout, and may help adding other timeouts in the future. I haven't tested it in depth though, because I encountered the following problem: While testing the patch, I found a way to crash PG. But what's weird is that it crashes also with an unpatched git version. Here is the way to reproduce it (I have done it with a pgbench schema): - Set a small statement_timeout (just to save time during the tests) Session1: =#BEGIN; =#lock TABLE pgbench_accounts ; Session 2: =#BEGIN; =#lock TABLE pgbench_accounts ; ERROR: canceling statement due to statement timeout =# lock TABLE pgbench_accounts ; I'm using \set ON_ERROR_ROLLBACK INTERACTIVE by the way. It can also be done with a rollback to savepoint of course. Session 2 crashes with this : TRAP : FailedAssertion(« !(locallock-holdsStrongLockCount == 0) », fichier : « lock.c », ligne : 749). It can also be done without a statement_timeout, and a control-C on the second lock table. I didn't touch anything but this. It occurs everytime, when asserts are activated. I tried it on 9.1.3, and I couldn't make it crash with the same sequence of events. So maybe it's something introduced since ? Or is the assert still valid ? Cheers Attached are the new patches. I rebased them to current GIT and they are expected to be applied after Robert Haas' patch in the bug in fast-path locking thread. Now it survives the above scenario. Best regards, Zoltán Böszörményi New patch attached, rebased to today's GIT. Best regards, Zoltán Böszörményi Ok, I've done what was missing from the review (from when I had a bug in locking the other day), so here is the full review. By the way, this patch doesn't belong to current commitfest, but to the next one. Is the patch in context diff format? Yes Does it apply cleanly to the current git master? Yes Does it include reasonable tests, necessary doc patches, etc? The new lock_timeout GUC is documented. There are regression tests. Read what the patch is supposed to do, and consider: Does the patch actually implement that? Yes Do we want that? I do. Mostly for administrative jobs which could lock the whole application. It would be much easier to run reindexes, vacuum full, etc… without worrying about bringing application down because of lock contention. Do we already have it? No. Does it follow SQL spec, or the community-agreed behavior? I don't know if there is a consensus on this new GUC. statement_timeout is obviously not in the SQL spec. Does it include pg_dump support (if applicable)? Not applicable Are there dangers? Yes, as it rewrites all the timeout code. I feel it is much cleaner this way, as there is a generic set of function managing all sigalarm code, but it heavily touches this part. Have all the bases been covered? I tried all sql statements I could think of (select, insert, update, delete, truncate, drop, create index, adding constraint, lock. I tried having statement_timeout, lock_timeout and deadlock_timeout at very short and close or equal values. It worked too. Rollback to savepoint while holding locks dont crash PostgreSQL anymore. Other timeouts such as archive_timeout and checkpoint_timeout still work. Does the feature work as advertised? Yes Are there corner cases the author has failed to consider? I didn't find any. Are there any assertion failures or crashes? No. Does the patch slow down simple tests? No If it claims to improve performance, does it? Not applicable Does it slow down other things? No Does it follow the project coding guidelines? I think so Are there portability issues? No, all the portable code (acquiring locks and manipulating sigalarm) is the same as before. Will it work on Windows/BSD etc? It should. I couldn't test it though. Are the comments sufficient and accurate? Yes Does it do what it says, correctly? Yes Does it produce compiler warnings? No Can you make it crash? Not anymore Is everything done in a way that fits together coherently with other features/modules? Yes, I think so. The new way of handling sigalarm seems more robust to me. Are there interdependencies that can cause problems? I don't see any. Regards, Marc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To
[HACKERS] Review for EXPLAIN and nfiltered
Here is my review for EXPLAIN and nfiltered (http://archives.postgresql.org/message-id/4e6e9e83.7070...@2ndquadrant.com) - Is the patch in context diff format? It's in git diff format - Does it apply cleanly to the current git master? Yes - Does it include reasonable tests, necessary doc patches, etc? I think so. Comments in execnodes.h are not updated with new fields (ss_qualnremoved and others). They are commented directly in the definition. Very minor, comments should just be moved around. I have the same problem as explained in the original mail finding an appropriate place in the documentation. It could be added on EXPLAIN's page, but that would mean changing the example statement for one with a filter. - Does the patch actually implement that? Yes - Do we want that? I'd like to have that. It makes it easier to determine if an index is selective enough. Some people might prefer having it through an option of EXPLAIN (such as EXPLAIN (filters on) ), I don't really know. - Do we already have it? No. - Does it follow SQL spec, or the community-agreed behavior? Nothing about this in the SQL spec, obviously, and I didn't see anyone disagreeing with the proposal. - Does it include pg_dump support (if applicable)? Not applicable - Are there dangers? Not that I think of - Have all the bases been covered? Yes, I think so. - Does the feature work as advertised? Yes - Are there corner cases the author has failed to consider? I didn't find any - Are there any assertion failures or crashes? No - Does the patch slow down simple tests? No - If it claims to improve performance, does it? Not applicable - Does it slow down other things? No - Does it follow the project coding guidelines? Yes - Are there portability issues? No - Will it work on Windows/BSD etc? Yes - Are the comments sufficient and accurate? Yes, except for the minor inconsistancy in execnodes.h - Does it do what it says, correctly? Yes - Does it produce compiler warnings? No - Can you make it crash? No - Is everything done in a way that fits together coherently with other features/modules? Yes - Are there interdependencies that can cause problems? Not that I can see -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review for EXPLAIN and nfiltered
2011/9/19 Robert Haas robertmh...@gmail.com: On Mon, Sep 19, 2011 at 12:51 PM, Marc Cousin cousinm...@gmail.com wrote: Here is my review for EXPLAIN and nfiltered (http://archives.postgresql.org/message-id/4e6e9e83.7070...@2ndquadrant.com) Please add this review to the CommitFest app here: https://commitfest.postgresql.org/action/patch_view?id=638 ...and also update the patch status as appropriate. Sounds like Ready for Committer would be the way to go in this case, since your review found only trivial issues. Sorry, I wanted to, but I didn't get the email id fast enough, and had to go. It's done now. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Review: rollback sequence reset for TRUNCATE ... RESTART IDENTITY
Hi, Here is my review of 'rollback sequence reset for TRUNCATE ... RESTART IDENTITY' patch. - Is the patch in context diff format? It's in git diff format. I guess it's OK ? - Does it apply cleanly to the current git master? Yes - Does it include reasonable tests, necessary doc patches, etc? Doc: Yes, it removes the warning about TRUNCATE ... RESTART IDENTITY, which is the point of the patch Tests: There is a new regression test added for restart identity. And 'make check' passes (tested on linux). - Usability review (skills needed: test-fu, ability to find and read spec) - Read what the patch is supposed to do, and consider: - Does the patch actually implement that? Yes. - Do we want that? I think so, it removes a trap limitation of truncate - Do we already have it? No - Does it follow SQL spec, or the community-agreed behavior? I think so - Does it include pg_dump support (if applicable)? Not applicable - Are there dangers? Not that I think of - Have all the bases been covered? I think so - Feature test (skills needed: patch, configure, make, pipe errors to log) - Apply the patch, compile it and test: - Does the feature work as advertised? Yes. It works consistently, isn't fooled by savepoints or multiple serials in a table, or concurrent transactions - Are there corner cases the author has failed to consider? I don't think so - Are there any assertion failures or crashes? No Performance review (skills
Re: [HACKERS] Review: rollback sequence reset for TRUNCATE ... RESTART IDENTITY
The Wednesday 17 November 2010 15:50:36, Jaime Casanova wrote : On Wed, Nov 17, 2010 at 8:13 AM, Marc Cousin cousinm...@gmail.com wrote: - Does the feature work as advertised? Yes. It works consistently, isn't fooled by savepoints or multiple serials in a table, or concurrent transactions i haven't tested this nor readed the patch but i wondering what happens in the presence of a prepared transaction (2PC), did you try with concurrent transactions with different serialization levels? I haven't tested with 2PC. I didn't check with different isolations levels either. I just verified that locking was happening as it should : truncate is blocked by a transaction already locking the table with an AccessShareLock and vice- versa. And that Rollbacking and rollbacking to savepoint restores the sequence to the correct state : the sequence isn't restored to its value at the savepoint, but at its last value before the truncate. I don't see a special test-case with different isolation levels or 2PC. What do you have in mind ? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: rollback sequence reset for TRUNCATE ... RESTART IDENTITY
The Wednesday 17 November 2010 19:41:19, Tom Lane wrote : Marc Cousin cousinm...@gmail.com writes: - Does the feature work as advertised? Yes. It works consistently, isn't fooled by savepoints or multiple serials in a table, or concurrent transactions I think there's a rather nasty problem here, which is what to do with the cached nextval/currval state. As submitted, the patch does the same thing as ALTER SEQUENCE RESTART (to wit, clear any cached unissued nextval values, but don't touch currval) at the time of resetting the sequence. That's fine, but what if the transaction later rolls back? The cached state is untouched by rollback, so if the transaction had done any nextval()s meanwhile, the cache will be out of step with the rolled-back sequence contents. Yes, I completely missed testing with non default cache value. And it fails, of course, some values are generated a second time twice after a rollback We never had to worry about this before because sequence operations didn't roll back, by definition. If we're going to add a situation where they do roll back, we need to consider the case. I think we can arrange to clear cached unissued values on the next attempt to nextval() the sequence, by dint of adding the relfilenode to SeqTable entries and clearing cached state whenever we note that it doesn't match the current relfilenode of the sequence. However, I'm unsure what ought to happen to currval. It doesn't seem too practical to try to roll it back to its pre-transaction value. Should we leave it alone (ie, possibly reflecting a value that was assigned inside the failed transaction)? The other alternative would be to clear it as though nextval had never been issued at all in the session. Should currval really be used after a failed transaction ? Right now, we can have a value that has been generated inside a rollbacked transaction too. I'd vote for leave it alone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC][PATCH]: CRC32 is limiting at COPY/CTAS/INSERT ... SELECT + speeding it up
The Saturday 30 October 2010 11:05:17, Andres Freund wrote : Hi, This thread died after me not implementing a new version and some potential license problems. I still think its worthwile (and I used it in production for some time) so I would like to implement a version fit for the next commitfest. The code where I started out from is under the zlib license - which is to my knowledge compatible with PGs licence. Whats the position of HACKERS there? There already is some separately licenced code around and were already linking to zlib licenced code... For simplicitly I asked Mark Adler (the original Copyright Owner) if he would be willing to relicence - he is not. For anybody not hording all old mail like me here is a link to the archives about my old patch: http://archives.postgresql.org/message- id/201005202227.49990.and...@anarazel.de Andres I forgot to report this a few months ago: I had a very intensive COPY load, and this patch helped. The context was a server that was CPU bound on loading data (8 COPY on the same table in parallel, not indexed). This patch gave me a 10% boost in load time. I don't have the figures right now, but I could try to do this test again if this can help. At that time, I just tried it out of curiosity, but the load time was sufficient without it, so I didn't spend more time on it. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] lock_timeout GUC patch - Review
The Monday 02 August 2010 13:59:59, Boszormenyi Zoltan wrote : Also, I made sure that only one or two timeout causes (one of deadlock_timeout and lock_timeout in the first case or statement_timeout plus one of the other two) can be active at a time. A little clarification is needed. The above statement is not entirely true. There can be a case when all three timeout causes can be active, but only when deadlock_timeout is the smallest of the three. If the fin_time value for the another timeout cause is smaller than for deadlock_timeout then there's no point to make deadlock_timeout active. And in case deadlock_timeout triggers and CheckDeadLock() finds that there really is a deadlock then the possibly active lock_timeout gets disabled to avoid calling RemoveFromWaitQueue() twice. I hope the comments in the code explain it well. Previously I was able to trigger a segfault with the default 1sec deadlock_timeout and lock_timeout = 999 or 1001. Effectively, the system's clock resolution makes the lock_timeout and deadlock_timeout equal and RemoveFromWaitQueue() was called twice. This way it's a lot more robust. Ok, I've tested this new version: This time, it's this case that doesn't work : Session 1 : lock a table Session 2 : set lock_timeout to a large value, just to make it obvious (10 seconds). It times out after 1 s (the deadlock timeout), returning 'could not obtain lock'. Of course, it should wait 10 seconds. I really feel that the timeout framework is the way to go here. I know what you said about it before, but I think that the current code is getting too complicated, with too many special cases all over. As this is only my second review and I'm sure I am missing things here, could someone with more experience have a look and give advice ? Cheers Marc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] lock_timeout GUC patch - Review
The Thursday 29 July 2010 13:55:38, Boszormenyi Zoltan wrote : I fixed this by adding CheckLockTimeout() function that works like CheckStatementTimeout() and ensuring that the same start time is used for both deadlock_timeout and lock_timeout if both are active. The preference of errors if their timeout values are equal is: statement_timeout lock_timeout deadlock_timeout As soon as lock_timeout is bigger than deadlock_timeout, it doesn't work, with this new version. Keeping the deadlock_timeout to 1s, when lock_timeout = 1001, lock_timeout doesn't trigger anymore. * Consider the changes to the code in the context of the project as a whole: * Is everything done in a way that fits together coherently with other features/modules? I have a feeling that enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a very specific problem, and it gets complicated because there is no infrastructure in the code to handle several timeouts at the same time with sigalarm, so each timeout has its own dedicated and intertwined code. But I'm still discovering this part of the code. This WIP patch is also attached for reference, too. I would prefer this way, but I don't have more time to work on it and there are some interdependencies in the signal handler when e.g. disable_sig_alarm(true); means to disable ALL timers not just the statement_timeout. The specifically coded lock_timeout patch goes with the flow and doesn't change the semantics and works. If someone wants to pick up the timer framework patch and can make it work, fine. But I am not explicitely submitting it for the commitfest. The original patch with the fixes works and needs only a little more review. Ok, understood. But I like the principle of this framework much more (the rest of the code seems simpler to me as a consequence of this framework). But it goes far beyond the initial intent of the patch. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] lock_timeout GUC patch - Review
Hi, I've been reviewing this patch for the last few days. Here it is : * Submission review * Is the patch in context diff format? Yes * Does it apply cleanly to the current CVS HEAD? Yes * Does it include reasonable tests, necessary doc patches, etc? Doc patches are there. There are no regression tests. Should there be ? * Usability review * Does the patch actually implement that? Yes * Do we want that? I think so. At least I'd like to have this feature :) * Do we already have it? No * Does it follow SQL spec, or the community-agreed behavior? I didn't see a clear conclusion from the -hackers thread on this (GUC vs SQL statement extension) * Does it include pg_dump support (if applicable)? Not applicable. Or should pg_dump and/or pg_restore put lock_timeout to 0 ? * Are there dangers? As it is a guc, it could be set globally, is that a danger ? * Have all the bases been covered? I honestly don't know. It touches alarm signal handling. * Apply the patch, compile it and test: * Does the feature work as advertised? I only tested it with Linux. The code is very OS-dependent. It works as advertised with Linux. I found only one corner case (see below) * Are there corner cases the author has failed to consider? The feature almost works as advertised : it fails when lock_timeout = deadlock_timeout. Then the lock_timeout isn't detected. I think this case isn't considered in handle_sig_alarm(). * Are there any assertion failures or crashes? No * Performance review * Does the patch slow down simple tests? No * If it claims to improve performance, does it? Not applicable * Does it slow down other things? No. Maybe alarm signal handling and enabling will be slower, as there is more work done there (for instance, a GetCurrentTimestamp, that was only done when log_lock_waits was activated until now. But I couldn't measure a slowdown. * Read the changes to the code in detail and consider: * Does it follow the project coding guidelines? I think so * Are there portability issues? It seems to have already been adressed, from the previous discussion in the thread. * Will it work on Windows/BSD etc? It should. I couldn't test it though. Infrastructure is there. * Are the comments sufficient and accurate? Yes * Does it do what it says, correctly? Yes * Does it produce compiler warnings? No * Can you make it crash? No * Consider the changes to the code in the context of the project as a whole: * Is everything done in a way that fits together coherently with other features/modules? I have a feeling that enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a very specific problem, and it gets complicated because there is no infrastructure in the code to handle several timeouts at the same time with sigalarm, so each timeout has its own dedicated and intertwined code. But I'm still discovering this part of the code. * Are there interdependencies that can cause problems? I don't think so. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TRUNCATE+COPY optimization and --jobs=1 in pg_restore
2010/2/10 Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp Tom Lane t...@sss.pgh.pa.us wrote: Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes: We have an optimization to bulkload date in pg_restore, but the code only works in parallel restore (--jobs = 2). Why don't we do the same optimization in the serial restore (--jobs = 1) ? The code is only trying to substitute for something you can't have in parallel restore, ie --single-transaction. Yeah, the comment says so. But it does not necessarily mean that we cannot optimize the copy also in non-single-transaction restore. The attached patch improve the judgment condition, I'll add it to the next commit-fest. Hi This is my first review, so I hope I won't do too many things wrong. Please tell me how to improve. I've taken all the points from the 'reviewing a patch' wiki page. But to sum up what is below, I did see a large performance improvement (in a very simple test case) and no problems with the patch. Submission review Is the patch in context diff format? Yes Does it apply cleanly to the current CVS HEAD? Yes Does it include reasonable tests, necessary doc patches, etc? Not applicable: two lines updated Usability review Not applicable, doesn't change the use of pg_restore Read what the patch is supposed to do, and consider: Does the patch actually implement that? Yes: it wraps the truncate table + copy in a single transaction when doing pg_restore -c Do we want that? I think so, see the improvements below. And it makes the performance consistent between -j, -1 and 'simple' uses of pg_restore. Do we already have it? No Does it follow SQL spec, or the community-agreed behavior? Yes: if this is the way to do restore with -j, there is no point in not doing it with simple restores Does it include pg_dump support (if applicable)? Not applicable Are there dangers? I dont think so. The patch replaces the (is_parallel te-created) with (!ropt-single_txn te-created) to wrap every copy during restore in a transaction, preceding it with a truncate. This can only be done if the restore isn't already 'single transaction' (and there is no point in doing it in that case), and if we successfully created the table in our restore work, so the table is empty. The purpose being not doing unnecessary wal logging if possible. Feature test Apply the patch, compile it and test: Does the feature work as advertised? Yes. COPY is preceded by BEGIN and TRUNCATE when doing restore, and followed by COMMIT. This happens if and only if the table has been created during the restore. If the table was already there and restore appends data in it, only COPY is run. This was checked when explicitely restoring only data (-a, no truncate), and when restoring structure AND data (truncate only if creating is really done, not in the case of error because the table was already there). No WAL was generated. Are there corner cases the author has failed to consider? I don't think so Are there any assertion failures or crashes? Not during my tests Performance review Does the patch slow down simple tests? No If it claims to improve performance, does it? Yes. Test case : one 10 million rows table, single int column, no indexes. One single SATA 5400rpm disk, laptop. Dump was generated with pg_dump -Fc -Z0. A checkpoint was triggered between each run. wal_sync_method to fdatasync. First test (default configuration), wal_buffers=64kB, checkpoint_segments=3, shared_buffers=32MB. With patch, restore in 15s, without, 38s. Second test, wal_buffers=512kB, checkpoint_segments=32, shared_buffers=512MB. With patch, restore in 15s, without, 36s (this is a laptop, it is IO-bound during this test). Does it slow down other things? It didn't seem to, and there is no reason why it should. Coding review Read the changes to the code in detail and consider: Does it follow the project coding guidelines? Yes, to my knowledge Are there portability issues? No Will it work on Windows/BSD etc? Yes Are the comments sufficient and accurate? Yes, comments were modified to explain the new behaviour Does it do what it says, correctly? Yes Does it produce compiler warnings? No Can you make it crash? No Architecture review Consider the changes to the code in the context of the project as a whole: Is everything done in a way that fits together coherently with other features/modules? Yes, it's a 2 line-patch for the code, and 5 lnes of doc. Are there interdependencies that can cause problems? No Again, this is my first. Please tell me how to improve.
Re: [HACKERS] Cached Query Plans (was: global prepared statements)
Another issue with plan caches, besides contention, in Oracle at least, is shared memory fragmentation (as plans aren't all the same size in memory ...) But this cache is very helpful for developments where every query is done via prepare/execute/deallocate. I've seen it a lot on java apps, the purpose being to benefit from the advantages of prepared statements, but without having to deal with storing those prepared statements somewhere. And of course, as said before, the statistics associated with those plans can be very helpful, mostly for all those very small queries that are run very frequently (a badly developped app most of the time, but that happens). Le Sunday 13 April 2008 06:21:41 Jonah H. Harris, vous avez écrit : On Sat, Apr 12, 2008 at 10:17 PM, Tom Lane [EMAIL PROTECTED] wrote: Yes, this is worthless on large active databases. The logging overhead alone starts to affect performance. But somehow, all that stuff with cached plans is free? Of course not. The first time you execute a query, it is cached... so you pay the same penalty you do in PG, but in many cases, only once. In regards to plan re-use, sure there's going to be some contention on the hash buckets... but that can be mitigated in a lot of ways. In addition to that, Oracle collects over two thousand other statistics in real-time... yet somehow Oracle is quite fast. So, I would say that the usual complaint about collecting stats should be more an issue of proper implementation than a complaint about the act of collection itself. -- Jonah H. Harris, Sr. Software Architect | phone: 732.331.1324 EnterpriseDB Corporation | fax: 732.331.1301 499 Thornall Street, 2nd Floor | [EMAIL PROTECTED] Edison, NJ 08837 | http://www.enterprisedb.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
