Thread: Delay locking partitions during INSERT and UPDATE
As a follow-on from [1] and also discussed in [2], I'd like to propose that we don't obtain locks on all partitions during INSERT into a partitioned table and UPDATE of a partitioned key and instead, only lock the partition when we first route a tuple to it. This means that the order that the locks are obtained is no longer well defined and is at the mercy of the order that tuples are INSERTed or UPDATEd. It seems worth relaxing this a bit for gains in performance, as when a partitioned table contains many partitions, the overhead of locking all partitions when inserting a single row, or just a few rows is often significantly higher than the cost of doing the actual insert. The current behaviour was added in 54cde0c4c058073 in order to minimise deadlock risk. It seems that the risk there only comes from AELs that could be taken when a partition directly receives a TRUNCATE / CREATE INDEX / VACUUM FULL / CLUSTER. There's obviously no conflict with other DML operations since two RowExclusiveLocks don't conflict with each other. I think all other AEL obtaining DDL must be performed on the top level partitioned table, for example, ADD COLUMN can't be done directly on a partition, so there's no added deadlock risk from those. For a deadlock to occur one of the above DDL commands would have to be executed inside a transaction in an order opposite to the order rows are being INSERTed or UPDATEd in the partitioned table. If required, such operations could LOCK TABLE the top partitioned table to block the DML operation. There's already a risk of similar deadlocks from such operations done on multiple separate tables when the order they're done is not the same as the order the tables are written in a query, although, in that case, the window for the deadlock is likely to be much smaller. With this done, the performance of an INSERT into a 10k partition partitioned table looks like: Setup: create table hashp (a int) partition by hash(a); select 'create table hashp'||x::Text || ' partition of hashp for values with (modulus 10000, remainder '||x::text||');' from generate_Series(0,9999) x; \gexec hashp_insert.sql: \set p_a random(1,1000) insert into hashp values(:p_a); Results: $ psql -c "truncate hashp;" postgres && pgbench -n -f hashp_insert.sql -M prepared -c 4 -j 4 -T 60 postgres Patched: tps = 27811.427620 (excluding connections establishing) tps = 28617.417308 (excluding connections establishing) Unpatched: tps = 130.446706 (excluding connections establishing) tps = 119.726641 (excluding connections establishing) The patch is attached. I'll park this here until the January commitfest. [1] https://www.postgresql.org/message-id/flat/CAKJS1f_1RJyFquuCKRFHTdcXqoPX-PYqAd7nz=GVBwvGh4a6xA@mail.gmail.com [2] https://www.postgresql.org/message-id/flat/25C1C6B2E7BE044889E4FE8643A58BA963B5796B%40G01JPEXMBKW03#3783596a794c6789a54a95a20971b6aa -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hi, On 11/23/18 1:14 AM, David Rowley wrote: > As a follow-on from [1] and also discussed in [2], I'd like to propose > that we don't obtain locks on all partitions during INSERT into a > partitioned table and UPDATE of a partitioned key and instead, only > lock the partition when we first route a tuple to it. This means that > the order that the locks are obtained is no longer well defined and is > at the mercy of the order that tuples are INSERTed or UPDATEd. It > seems worth relaxing this a bit for gains in performance, as when a > partitioned table contains many partitions, the overhead of locking > all partitions when inserting a single row, or just a few rows is > often significantly higher than the cost of doing the actual insert. > Yep, the locking seems like a significant bottleneck. I've done quite a bit of testing on two machines, using a slightly modified version of your test script with variable number of partitions (0 means not partitioned), and the results look like this: 1) xeon e5-2620v4 partitions 0 100 1000 10000 --------------------------------------------- master 16643 6956 1039 108 patched 16398 15522 15222 13228 2) i5-2500k partitions 0 100 1000 10000 ----------------------------------------- master 3901 2892 920 76 patched 3894 3838 3845 3522 When using UNLOGGED tables to minimize the external noise, it looks like this: 3) xeon e5-2620v4 partitions 0 100 1000 10000 -------------------------------------------- master 30806 8740 1091 107 patched 30455 28137 27582 24985 partitions 0 100 1000 10000 -------------------------------------------- master 27662 9013 1277 79 patched 28263 26474 25794 22434 So the performance benefit is pretty clear - up to 2 orders of magnitude with 10k partitions, and gets us fairly close to non-partitioned table. Me gusta. > The current behaviour was added in 54cde0c4c058073 in order to > minimise deadlock risk. It seems that the risk there only comes from > AELs that could be taken when a partition directly receives a TRUNCATE > / CREATE INDEX / VACUUM FULL / CLUSTER. There's obviously no conflict > with other DML operations since two RowExclusiveLocks don't conflict > with each other. I think all other AEL obtaining DDL must be > performed on the top level partitioned table, for example, ADD COLUMN > can't be done directly on a partition, so there's no added deadlock > risk from those. For a deadlock to occur one of the above DDL commands > would have to be executed inside a transaction in an order opposite to > the order rows are being INSERTed or UPDATEd in the partitioned table. > If required, such operations could LOCK TABLE the top partitioned > table to block the DML operation. There's already a risk of similar > deadlocks from such operations done on multiple separate tables when > the order they're done is not the same as the order the tables are > written in a query, although, in that case, the window for the > deadlock is likely to be much smaller. > Hmmm, yeah. Per the discussion in [1] the locking was necessary also to ensure partitions can't disappear while we're building the descriptors in RelationBuildPartitionDesc(). But AFAICS 3f2393edef fixed this. The other issue - as you note - is ensuring locking order, to prevent (or rather reduce the risk of) deadlocks. I agree with your assessment here, i.e. that locking the parent is a sufficient protection. Maybe there's an alternative solution with the same benefits and not sacrificing the lock ordering, but I fail to see how it would work. > > [1] https://www.postgresql.org/message-id/flat/CAKJS1f_1RJyFquuCKRFHTdcXqoPX-PYqAd7nz=GVBwvGh4a6xA@mail.gmail.com regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: tested, failed Documentation: tested, failed Hi, Increasing the number of clients, I benchmarked with a table partitioned into 1k partition. I confirmed that this patch improve performance by 10 times or more. master (commit: 90525d7b4e Date: Tue Jan 15 12:19:21 2019 -0800) cpu: Xeon(R) CPU E5-2667 v3 * 2 setup: create table history(aid int, delta int, mtime timestamp without time zone) partition by range(aid); \o /dev/null select 'create table history_' || x || ' partition of history for values from(' || x ||') to(' || x+1 || ')' from generate_series(1,1000) x; \gexec benchmark.sql: \set aid random(1, 1000) \set delta random(-5000, 5000) INSERT INTO history VALUES (:aid, :delta, CURRENT_TIMESTAMP); command line: pgbench -d testdb -f benchmark.sql -c number_of_clients -T 60 -r -n Results: clients | tps_patched | tps_unpatched | tps_unpatched / tps_patched ---------+-------------+---------------+----------------------------- 1 | 8890 | 841 | 11 2 | 17484 | 1470 | 12 4 | 29218 | 2474 | 12 8 | 48789 | 3876 | 13 16 | 68794 | 4030 | 17 32 | 69550 | 2888 | 24 64 | 71196 | 2555 | 28 128 | 71739 | 2295 | 31 256 | 66548 | 2105 | 32 I wonder why performance does not increase much when the number of clients exceeds 16. Even though it is caused by competition of lightweight locks I thought 16 clients are small. Also, I did make installcheck world, but test partition_prune failed. However, this test case failed even before applying a patch, so this patch is not a problem. One of the results is as follows. explain (analyze, costs off, summary off, timing off) execute ab_q1 (2, 2, 3); - QUERY PLAN ---------------------------------------------------------- + QUERY PLAN +------------------------------------------------------ Append (actual rows=0 loops=1) - Subplans Removed: 6 -> Seq Scan on ab_a2_b1 (actual rows=0 loops=1) - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) -> Seq Scan on ab_a2_b2 (actual rows=0 loops=1) - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) -> Seq Scan on ab_a2_b3 (actual rows=0 loops=1) - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) -(8 rows) + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) +(7 rows) regards, sho kato
On Fri, 18 Jan 2019 at 19:08, sho kato <kato-sho@jp.fujitsu.com> wrote: > I confirmed that this patch improve performance by 10 times or more. Thanks for testing this out. > Also, I did make installcheck world, but test partition_prune failed. > However, this test case failed even before applying a patch, so this patch is not a problem. > One of the results is as follows. > > explain (analyze, costs off, summary off, timing off) execute ab_q1 (2, 2, 3); > - QUERY PLAN > ---------------------------------------------------------- > + QUERY PLAN > +------------------------------------------------------ > Append (actual rows=0 loops=1) > - Subplans Removed: 6 > -> Seq Scan on ab_a2_b1 (actual rows=0 loops=1) > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > -> Seq Scan on ab_a2_b2 (actual rows=0 loops=1) > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > -> Seq Scan on ab_a2_b3 (actual rows=0 loops=1) > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > -(8 rows) > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > +(7 rows) Perhaps you're running with plan_cache_mode = force_custom_plan. You'll likely need it set to auto for these to pass. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 11/22/18, David Rowley <david.rowley@2ndquadrant.com> wrote: > If required, such operations could LOCK TABLE the top partitioned > table to block the DML operation. There's already a risk of similar > deadlocks from such operations done on multiple separate tables when > the order they're done is not the same as the order the tables are > written in a query, although, in that case, the window for the > deadlock is likely to be much smaller. Is this something that would need documentation anywhere? > With this done, the performance of an INSERT into a 10k partition > partitioned table looks like: > > Setup: > create table hashp (a int) partition by hash(a); > select 'create table hashp'||x::Text || ' partition of hashp for > values with (modulus 10000, remainder '||x::text||');' from > generate_Series(0,9999) x; > \gexec > > hashp_insert.sql: > \set p_a random(1,1000) > insert into hashp values(:p_a); > > Results: > $ psql -c "truncate hashp;" postgres && pgbench -n -f hashp_insert.sql > -M prepared -c 4 -j 4 -T 60 postgres I used a similar test, but with unlogged tables, and "-c 2", and got: normal table: 32000tps 10k partitions / master: 82tps 10k partitions / patch: 7000tps So far I haven't gotten quite as good performance as you and Tomas, although it's still a ~85x improvement. -John Naylor
On 1/19/19 12:05 AM, John Naylor wrote: > On 11/22/18, David Rowley <david.rowley@2ndquadrant.com> wrote: >> If required, such operations could LOCK TABLE the top partitioned >> table to block the DML operation. There's already a risk of similar >> deadlocks from such operations done on multiple separate tables when >> the order they're done is not the same as the order the tables are >> written in a query, although, in that case, the window for the >> deadlock is likely to be much smaller. > > Is this something that would need documentation anywhere? > Not sure. Initially I was going to say "no" because it's an internal implementation detail and the risk of the deadlock is already there anyway. But maybe this patch is making it more likely and we should at least mention how partitions are locked. >> With this done, the performance of an INSERT into a 10k partition >> partitioned table looks like: >> >> Setup: >> create table hashp (a int) partition by hash(a); >> select 'create table hashp'||x::Text || ' partition of hashp for >> values with (modulus 10000, remainder '||x::text||');' from >> generate_Series(0,9999) x; >> \gexec >> >> hashp_insert.sql: >> \set p_a random(1,1000) >> insert into hashp values(:p_a); >> >> Results: >> $ psql -c "truncate hashp;" postgres && pgbench -n -f hashp_insert.sql >> -M prepared -c 4 -j 4 -T 60 postgres > > I used a similar test, but with unlogged tables, and "-c 2", and got: > > normal table: 32000tps > 10k partitions / master: 82tps > 10k partitions / patch: 7000tps > > So far I haven't gotten quite as good performance as you and Tomas, > although it's still a ~85x improvement. > What hardware are you running the tests on? I wouldn't be surprised if you were hitting some CPU or I/O bottleneck, which we're not hitting. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Jan 19, 2019 at 10:59 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > > On 1/19/19 12:05 AM, John Naylor wrote: > > I used a similar test, but with unlogged tables, and "-c 2", and got: > > > > normal table: 32000tps > > 10k partitions / master: 82tps > > 10k partitions / patch: 7000tps > > > > So far I haven't gotten quite as good performance as you and Tomas, > > although it's still a ~85x improvement. > > What hardware are you running the tests on? I wouldn't be surprised if > you were hitting some CPU or I/O bottleneck, which we're not hitting. 9 year-old laptop, Core i3. Side note, I miswrote my test parameters above -- should be "-c4 -j2". -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/20/19 5:45 AM, John Naylor wrote: > On Sat, Jan 19, 2019 at 10:59 AM Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >> >> On 1/19/19 12:05 AM, John Naylor wrote: >>> I used a similar test, but with unlogged tables, and "-c 2", and got: >>> >>> normal table: 32000tps >>> 10k partitions / master: 82tps >>> 10k partitions / patch: 7000tps >>> >>> So far I haven't gotten quite as good performance as you and Tomas, >>> although it's still a ~85x improvement. >> >> What hardware are you running the tests on? I wouldn't be surprised if >> you were hitting some CPU or I/O bottleneck, which we're not hitting. > > 9 year-old laptop, Core i3. Side note, I miswrote my test parameters > above -- should be "-c4 -j2". > Hmmm, I wouldn't be surprised if it was getting throttled for some reasons (e.g. temperature). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, 19 Jan 2019 at 12:05, John Naylor <jcnaylor@gmail.com> wrote: > > On 11/22/18, David Rowley <david.rowley@2ndquadrant.com> wrote: > > If required, such operations could LOCK TABLE the top partitioned > > table to block the DML operation. There's already a risk of similar > > deadlocks from such operations done on multiple separate tables when > > the order they're done is not the same as the order the tables are > > written in a query, although, in that case, the window for the > > deadlock is likely to be much smaller. > > Is this something that would need documentation anywhere? Probably at least the release notes. I'm unsure where else to mention it. I don't feel the workaround of using LOCK TABLE is special to this case. The patch does, however, make it a possible requirement for performing DDL on individual partitions where it was not previously. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
on Fri, 18 2019 at 19:41, David Rowley <david.rowley@2ndquadrant.com> wrote: >Perhaps you're running with plan_cache_mode = force_custom_plan. >You'll likely need it set to auto for these to pass. Thank you. I was running with plan_cache_mode = force_custom_plan. When it set to auto, all tests are passed. regards, sho kato > -----Original Message----- > From: David Rowley [mailto:david.rowley@2ndquadrant.com] > Sent: Friday, January 18, 2019 7:41 PM > To: Kato, Sho/加藤 翔 <kato-sho@jp.fujitsu.com> > Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; David > Rowley <dgrowley@gmail.com> > Subject: Re: Delay locking partitions during INSERT and UPDATE > > On Fri, 18 Jan 2019 at 19:08, sho kato <kato-sho@jp.fujitsu.com> wrote: > > I confirmed that this patch improve performance by 10 times or more. > > Thanks for testing this out. > > > Also, I did make installcheck world, but test partition_prune failed. > > However, this test case failed even before applying a patch, so this > patch is not a problem. > > One of the results is as follows. > > > > explain (analyze, costs off, summary off, timing off) execute ab_q1 > (2, 2, 3); > > - QUERY PLAN > > ---------------------------------------------------------- > > + QUERY PLAN > > +------------------------------------------------------ > > Append (actual rows=0 loops=1) > > - Subplans Removed: 6 > > -> Seq Scan on ab_a2_b1 (actual rows=0 loops=1) > > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > > -> Seq Scan on ab_a2_b2 (actual rows=0 loops=1) > > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > > -> Seq Scan on ab_a2_b3 (actual rows=0 loops=1) > > - Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) > > -(8 rows) > > + Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) > > +(7 rows) > > Perhaps you're running with plan_cache_mode = force_custom_plan. > You'll likely need it set to auto for these to pass. > > > -- > David Rowley http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
The cfbot reports this patch no longer applies. -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, 23 Jan 2019 at 04:35, John Naylor <john.naylor@2ndquadrant.com> wrote: > The cfbot reports this patch no longer applies. Thanks. I've attached a rebased version. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Tue, Jan 22, 2019 at 4:31 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > Thanks. I've attached a rebased version. Hmm, now instead of an 85x speedup over master in the 10k partition case, I only get 20x. Anyone else see this? -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 24 Jan 2019 at 13:38, John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Tue, Jan 22, 2019 at 4:31 PM David Rowley > <david.rowley@2ndquadrant.com> wrote: > > Thanks. I've attached a rebased version. > > Hmm, now instead of an 85x speedup over master in the 10k partition > case, I only get 20x. Anyone else see this? What's it like with fsync off? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jan 23, 2019 at 7:56 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > On Thu, 24 Jan 2019 at 13:38, John Naylor <john.naylor@2ndquadrant.com> wrote: > > Hmm, now instead of an 85x speedup over master in the 10k partition > > case, I only get 20x. Anyone else see this? > > What's it like with fsync off? No change. Just in case, I tried git bisect and also recreated the cluster, but never got the same performance as my first test, so not sure what happened. -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/24/19 9:50 PM, John Naylor wrote: > On Wed, Jan 23, 2019 at 7:56 PM David Rowley > <david.rowley@2ndquadrant.com> wrote: >> On Thu, 24 Jan 2019 at 13:38, John Naylor <john.naylor@2ndquadrant.com> wrote: >>> Hmm, now instead of an 85x speedup over master in the 10k partition >>> case, I only get 20x. Anyone else see this? >> >> What's it like with fsync off? > > No change. Just in case, I tried git bisect and also recreated the > cluster, but never got the same performance as my first test, so not > sure what happened. I can still see about the same performance as before (on both clusters). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 24, 2019 at 4:17 PM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > I can still see about the same performance as before (on both clusters). Thanks for checking! I think the thing to do now is have a committer look at it. It's a small patch with obvious performance effects -- there's just the question of whether the locking behavior with concurrent DDL is as safe as it is now. Anyone have anything else? -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/24/19 10:34 PM, John Naylor wrote: > On Thu, Jan 24, 2019 at 4:17 PM Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >> I can still see about the same performance as before (on both clusters). > > Thanks for checking! I think the thing to do now is have a committer > look at it. It's a small patch with obvious performance effects -- > there's just the question of whether the locking behavior with > concurrent DDL is as safe as it is now. > > Anyone have anything else? > Yes, I don't see why would the patch change that and I've been looking for such cases. I think David was looking at that this week too, and I assume he'll send an update if he finds anything. If not, I plan to get it committed soon-ish (possibly next week after the FOSDEM dev meeting, unless there are some objections). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 24, 2019 at 4:43 PM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > Yes, I don't see why would the patch change that and I've been looking > for such cases. I think David was looking at that this week too, and I > assume he'll send an update if he finds anything. If not, I plan to get > it committed soon-ish (possibly next week after the FOSDEM dev meeting, > unless there are some objections). I have reviewed this patch and I am in favor of it. I think it likely needs minor rebasing because of the heap_open -> table_open renaming. I also agree that it's worth taking some deadlock risk for the rather massive performance gain, although I suspect it's likely that a few users are going to complain about deadlocks and I wonder whether we'll have to put some energy into that problem at some point. However, I think what we probably want to do there is reduce the probability of deadlocks through some trickery or maybe invent some new locking mechanisms that work around the problem. The alternative of trying to block this patch seems unpalatable. In short, +1 from me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, 1 Feb 2019 at 07:46, Robert Haas <robertmhaas@gmail.com> wrote: > I have reviewed this patch and I am in favor of it. I think it likely > needs minor rebasing because of the heap_open -> table_open renaming. Many thanks for looking at it. The v2 patch was based on top of the heap_open -> table_open change. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Jan 31, 2019 at 4:48 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > On Fri, 1 Feb 2019 at 07:46, Robert Haas <robertmhaas@gmail.com> wrote: > > I have reviewed this patch and I am in favor of it. I think it likely > > needs minor rebasing because of the heap_open -> table_open renaming. > > Many thanks for looking at it. The v2 patch was based on top of the > heap_open -> table_open change. Oops. I guess I opened the wrong version. I'm now wondering whether the same issues discussed in https://www.postgresql.org/message-id/CA%2BTgmoZN-80143F8OhN8Cn5-uDae5miLYVwMapAuc%2B7%2BZ7pyNg%40mail.gmail.com also need discussion with respect to this patch. But I haven't thought about it very hard, so I'm not sure whether they do or don't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, 2 Feb 2019 at 03:07, Robert Haas <robertmhaas@gmail.com> wrote: > I'm now wondering whether the same issues discussed in > https://www.postgresql.org/message-id/CA%2BTgmoZN-80143F8OhN8Cn5-uDae5miLYVwMapAuc%2B7%2BZ7pyNg%40mail.gmail.com > also need discussion with respect to this patch. But I haven't > thought about it very hard, so I'm not sure whether they do or don't. I really don't think it does, or if it does then the code is already broken as it is now. As the code is today, we obtain the locks well after the plan is locked in. The only difference with this patch is that we do the locking on routing tuples to the partition for the first time rather than do them all at once when setting up the routing data structure. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Feb 1, 2019 at 9:16 AM David Rowley <david.rowley@2ndquadrant.com> wrote: > On Sat, 2 Feb 2019 at 03:07, Robert Haas <robertmhaas@gmail.com> wrote: > > I'm now wondering whether the same issues discussed in > > https://www.postgresql.org/message-id/CA%2BTgmoZN-80143F8OhN8Cn5-uDae5miLYVwMapAuc%2B7%2BZ7pyNg%40mail.gmail.com > > also need discussion with respect to this patch. But I haven't > > thought about it very hard, so I'm not sure whether they do or don't. > > I really don't think it does, or if it does then the code is already > broken as it is now. > > As the code is today, we obtain the locks well after the plan is > locked in. The only difference with this patch is that we do the > locking on routing tuples to the partition for the first time rather > than do them all at once when setting up the routing data structure. OK, that is what I thought yesterday, and then I was just doubting myself. Thanks for thinking about it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 1/25/19, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > Yes, I don't see why would the patch change that and I've been looking > for such cases. I think David was looking at that this week too, and I > assume he'll send an update if he finds anything. If not, I plan to get > it committed soon-ish (possibly next week after the FOSDEM dev meeting, > unless there are some objections). I forgot to mark it ready for committer, so I've done that now. -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-01-31 13:46:33 -0500, Robert Haas wrote: > I have reviewed this patch and I am in favor of it. I think it likely > needs minor rebasing because of the heap_open -> table_open renaming. > I also agree that it's worth taking some deadlock risk for the rather > massive performance gain, although I suspect it's likely that a few > users are going to complain about deadlocks and I wonder whether we'll > have to put some energy into that problem at some point. However, I > think what we probably want to do there is reduce the probability of > deadlocks through some trickery or maybe invent some new locking > mechanisms that work around the problem. The alternative of trying to > block this patch seems unpalatable. Are you saying that such workarounds would have to be merged at the same time as this patch? Or that we'd address such complaints that way at a later time? Greetings, Andres Freund
What I'd like to understand about this patch is how it relates to Amit L.'s work on making the planner faster for partitioned UPDATE/DELETE cases (https://commitfest.postgresql.org/22/1778/). I think that that might render this moot? And if it doesn't, how much does it really matter? You can't really postpone taking a lock on a relation that the planner is going to do anything nontrivial with. regards, tom lane
On Tue, 19 Feb 2019 at 11:11, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > What I'd like to understand about this patch is how it relates > to Amit L.'s work on making the planner faster for partitioned > UPDATE/DELETE cases (https://commitfest.postgresql.org/22/1778/). It's close to being unrelated and completely unrelated for the INSERT case. It's true that currently we obtain locks on all partitions during the planning of an UPDATE to a partitioned table. During execution, we attempt to obtain all those locks again in ExecSetupPartitionTupleRouting(), looking up the local lock table isn't free, so it's beneficial to not wastefully do this again. In the case of PREPAREd UPDATE statements, delaying obtaining any partition lock until we first route a tuple to the partition can save big time. Patches further on in Amit's patch series will reduce the locking taken in the planner to just non-pruned partitions, these partitions might not be the same ones that tuples get routed to. Remember that tuples can go to any partition, not just the ones listed in the ModifyTable node's subnodes. So in that case, this patch just complements Amit's patch series. > I think that that might render this moot? And if it doesn't, > how much does it really matter? Quite a bit. Here's an INSERT benchmark from the first email in this thread. The test was done with 10k partitions. I wrote: > Patched: > tps = 27811.427620 (excluding connections establishing) > tps = 28617.417308 (excluding connections establishing) > > Unpatched: > tps = 130.446706 (excluding connections establishing) > tps = 119.726641 (excluding connections establishing) Remember that planner will obtain no locks on any partitions during INSERT. In the above case, the executor in the unpatched version obtains 10k locks when only 1 is needed. In the patched version the executor just takes a lock on the single partition that the tuple is routed to. > You can't really postpone taking > a lock on a relation that the planner is going to do anything > nontrivial with. Not sure what you mean here. This is an executor change. What does the planner care about what the executor does? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes: > On Tue, 19 Feb 2019 at 11:11, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> What I'd like to understand about this patch is how it relates >> to Amit L.'s work on making the planner faster for partitioned >> UPDATE/DELETE cases (https://commitfest.postgresql.org/22/1778/). >> I think that that might render this moot? And if it doesn't, >> how much does it really matter? > Quite a bit. Here's an INSERT benchmark from the first email in this > thread. The test was done with 10k partitions. Um, this doesn't really address my question, since you're comparing HEAD to the patch. I was wondering about the delta if we assume Amit's patch is in and then apply this on top (assuming it even applies...) >> You can't really postpone taking >> a lock on a relation that the planner is going to do anything >> nontrivial with. > Not sure what you mean here. This is an executor change. What does > the planner care about what the executor does? Are we talking about the same patch? The one I'm looking at seems to be mostly planner and plancache changes: src/backend/catalog/dependency.c | 1 + src/backend/commands/createas.c | 1 + src/backend/executor/execUtils.c | 20 +++++++++++--------- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/planner.c | 2 ++ src/backend/optimizer/util/inherit.c | 11 +++++++++++ src/backend/parser/parse_relation.c | 2 ++ src/backend/replication/logical/worker.c | 1 + src/backend/rewrite/rewriteHandler.c | 1 + src/backend/utils/adt/ri_triggers.c | 2 ++ src/backend/utils/adt/ruleutils.c | 3 +++ src/backend/utils/cache/plancache.c | 15 +++++++++++---- src/include/nodes/parsenodes.h | 2 ++ BTW, I'm doubtful that putting planner outputs into RangeTblEntry is a particularly good idea. regards, tom lane
Hi, On 2019-02-18 17:47:07 -0500, Tom Lane wrote: > David Rowley <david.rowley@2ndquadrant.com> writes: > > Not sure what you mean here. This is an executor change. What does > > the planner care about what the executor does? > > Are we talking about the same patch? The one I'm looking at seems > to be mostly planner and plancache changes: I think so - the patch in this thread is much simpler: diffstat < /tmp/v2-0001-Delay-locking-of-partitions-during-INSERT-and-UPD.patch execPartition.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) that's the patch upthread from here, at https://www.postgresql.org/message-id/CAKJS1f_hWSiC9j9HkZCfNVQs-bbHY-P2RqbH4bYcEHmWtpyLRw%40mail.gmail.com Greetings, Andres Freund
On Tue, 19 Feb 2019 at 11:47, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Are we talking about the same patch? The one I'm looking at seems > to be mostly planner and plancache changes: > > src/backend/catalog/dependency.c | 1 + > src/backend/commands/createas.c | 1 + > src/backend/executor/execUtils.c | 20 +++++++++++--------- > src/backend/nodes/copyfuncs.c | 1 + > src/backend/nodes/equalfuncs.c | 1 + > src/backend/nodes/outfuncs.c | 1 + > src/backend/nodes/readfuncs.c | 1 + > src/backend/optimizer/plan/planner.c | 2 ++ > src/backend/optimizer/util/inherit.c | 11 +++++++++++ > src/backend/parser/parse_relation.c | 2 ++ > src/backend/replication/logical/worker.c | 1 + > src/backend/rewrite/rewriteHandler.c | 1 + > src/backend/utils/adt/ri_triggers.c | 2 ++ > src/backend/utils/adt/ruleutils.c | 3 +++ > src/backend/utils/cache/plancache.c | 15 +++++++++++---- > src/include/nodes/parsenodes.h | 2 ++ Looks like you're looking at the patch from the "Delay locking partitions during query execution" thread [1]. Different thing altogether, although the names are confusingly similar. [1] https://commitfest.postgresql.org/22/1897/ -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes: > On Tue, 19 Feb 2019 at 11:47, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Are we talking about the same patch? The one I'm looking at seems >> to be mostly planner and plancache changes: > Looks like you're looking at the patch from the "Delay locking > partitions during query execution" thread [1]. Different thing > altogether, although the names are confusingly similar. My apologies --- I searched my inbox for "Delay locking partitions", and failed to notice that I was extracting a patch from the wrong thread with that in its title. (The questions I posed should be posted to that thread instead, as they still apply there.) Now that I've looked at *this* thread's patch, I agree that having the executor acquire locks based on find_all_inheritors() is a pretty horrid idea. However, it still seems like this is being done in a vacuum without attention to locks already acquired upstream. How much does the knowledge that the planner or plancache would've already locked everything mentioned in the rangetable affect the issues here? I'm inclined to think that if we already have lock on the parent partitioned table (thereby, IIUC, guaranteeing that its partitioning info can't change) that the order in which we acquire the same lock level on its partition(s) isn't very important. regards, tom lane
On Tue, 19 Feb 2019 at 12:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Now that I've looked at *this* thread's patch, I agree that having > the executor acquire locks based on find_all_inheritors() is a > pretty horrid idea. However, it still seems like this is being > done in a vacuum without attention to locks already acquired > upstream. How much does the knowledge that the planner or plancache > would've already locked everything mentioned in the rangetable > affect the issues here? For the INSERT with VALUES case, there should be no existing locks on the partition. Only the partitioned table that's the target of the INSERT will have been locked already. For the INSERT with SELECT case, well, it's possible that one of the partitions exist in the SELECT and it was already locked by the planner or in AcquireExecutorLocks(), but how is that different from having multiple RangeTblEntry objects for the same relation? We already try to lock those twice. If we had code to resolve those duplicates and take just the strongest of the locks for the relation then I might feel we'd better handle the locking better in this patch, but we don't, so I see no reason to add additional smarts for a corner case if we have none for a more common case. For the UPDATE case, we reuse result relations from the ModifyTable's subnodes. See ExecFindPartition() around: /* * We have not yet set up a ResultRelInfo for this partition, * but if we have a subplan hash table, we might have one * there. If not, we'll have to create one. */ if (proute->subplan_resultrel_htab) > I'm inclined to think that if we already have lock on the parent > partitioned table (thereby, IIUC, guaranteeing that its partitioning > info can't change) that the order in which we acquire the same lock > level on its partition(s) isn't very important. Yeah. I think most people seem to agree with that too. At least, nobody seemed to raise any concerns about increased deadlock risk, although it was talked about. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, 19 Feb 2019 at 12:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <david.rowley@2ndquadrant.com> writes: > > Looks like you're looking at the patch from the "Delay locking > > partitions during query execution" thread [1]. Different thing > > altogether, although the names are confusingly similar. > > My apologies --- I searched my inbox for "Delay locking partitions", > and failed to notice that I was extracting a patch from the wrong > thread with that in its title. (The questions I posed should > be posted to that thread instead, as they still apply there.) Easily done. Happy to answer the question there if you ask it there. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Feb 15, 2019 at 9:22 PM Andres Freund <andres@anarazel.de> wrote: > On 2019-01-31 13:46:33 -0500, Robert Haas wrote: > > I have reviewed this patch and I am in favor of it. I think it likely > > needs minor rebasing because of the heap_open -> table_open renaming. > > I also agree that it's worth taking some deadlock risk for the rather > > massive performance gain, although I suspect it's likely that a few > > users are going to complain about deadlocks and I wonder whether we'll > > have to put some energy into that problem at some point. However, I > > think what we probably want to do there is reduce the probability of > > deadlocks through some trickery or maybe invent some new locking > > mechanisms that work around the problem. The alternative of trying to > > block this patch seems unpalatable. > > Are you saying that such workarounds would have to be merged at the same > time as this patch? Or that we'd address such complaints that way at a > later time? Later. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Feb 18, 2019 at 6:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm inclined to think that if we already have lock on the parent > partitioned table (thereby, IIUC, guaranteeing that its partitioning > info can't change) that the order in which we acquire the same lock > level on its partition(s) isn't very important. Well, as it turns out, there's also a pending patch (series) to remove that guarantee which I would like to get committed. The thread for that is "ATTACH/DETACH PARTITION CONCURRENTLY". I believe I've more or less got the issues with that patch sorted out, but I'm concerned about how it interacts with all of the other things that are currently in flight. Delaying or skipping lock acquisition in some cases and weakening the locks we take in others is not a process that can continue indefinitely without at some point hitting a wall. But that being said, I don't think I quite see how the two things you mention here are connected to each other. If operation A locks parents before children, and operation B also locks parents before children, they generally won't deadlock against each other, even if each locks the children in a different order. An exception would be if the locks involved are of different levels such that the locks on the parents don't conflict but the locks on the children do, e.g. RowExclusiveLock on partitioned tables and AccessExclusiveLock on partitions. But that's a pretty weird setup and I don't know of any current or proposed code that does that. So my question is - what do you mean by the parenthetical note about the partitioning info not changing? Regardless of whether it does or does not, I think the same property holds. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, 20 Feb 2019 at 06:36, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Feb 18, 2019 at 6:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I'm inclined to think that if we already have lock on the parent > > partitioned table (thereby, IIUC, guaranteeing that its partitioning > > info can't change) that the order in which we acquire the same lock > > level on its partition(s) isn't very important. > > Well, as it turns out, there's also a pending patch (series) to remove > that guarantee which I would like to get committed. The thread for > that is "ATTACH/DETACH PARTITION CONCURRENTLY". I believe I've more > or less got the issues with that patch sorted out, but I'm concerned > about how it interacts with all of the other things that are currently > in flight. Delaying or skipping lock acquisition in some cases and > weakeni ng the locks we take in others is not a process that can > continue indefinitely without at some point hitting a wall. I'd say that here we should only discuss what this patch is doing, not anything else that's in flight that you're concerned will conflict with the ATTACH/DETACH PARTITION CONCURRENTLY patch. During INSERT and UPDATE, not all partitions will always be locked before executor startup. This patch removing the find_all_inheritors() call from ExecSetupPartitionTupleRouting() is not going to break ATTACH/DETACH PARTITION CONCURRENTLY if it wasn't already broken in the first place. With this patch, we're still obtaining locks after execution has begun, it just may delay the locking until a bit later. It was previously already happening after executor startup had begun, so the window for the problems that you describe were already non-zero. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Feb 19, 2019 at 4:07 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > I'd say that here we should only discuss what this patch is doing, not > anything else that's in flight that you're concerned will conflict > with the ATTACH/DETACH PARTITION CONCURRENTLY patch. > > During INSERT and UPDATE, not all partitions will always be locked > before executor startup. This patch removing the find_all_inheritors() > call from ExecSetupPartitionTupleRouting() is not going to break > ATTACH/DETACH PARTITION CONCURRENTLY if it wasn't already broken in > the first place. With this patch, we're still obtaining locks after > execution has begun, it just may delay the locking until a bit later. > It was previously already happening after executor startup had begun, > so the window for the problems that you describe were already > non-zero. The issue of whether it's OK to lock child partitions in variable order has come up on many different threads, and as far as I know this is the only thread where Tom has expressed any view on it. Since Tom is a smart guy,[1] I was hoping to get him to expand on those views a little bit. Even if that leads this thread to detour slightly from the matter immediately at hand, I think it will be worth it if it gets us to some kind of consensus on what has been a thorny question for some time now. On the other hand, Tom may not reply, in which case the rest of us will just have to keep faking it as best we can. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company [1] citation needed, except not really
On Tue, Feb 19, 2019 at 4:07 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > I'd say that here we should only discuss what this patch is doing, ... On that note, I spent some more time looking at what the patch is doing today. /* * We locked all the partitions in ExecSetupPartitionTupleRouting * including the leaf partitions. */ - partrel = table_open(dispatch->partdesc->oids[partidx], NoLock); + partrel = table_open(dispatch->partdesc->oids[partidx], RowExclusiveLock); It seems to me that the reason for this change is precisely that the comment is now false, and therefore the comment needs to be updated. Does that sound right? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Feb 18, 2019 at 6:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I'm inclined to think that if we already have lock on the parent >> partitioned table (thereby, IIUC, guaranteeing that its partitioning >> info can't change) that the order in which we acquire the same lock >> level on its partition(s) isn't very important. > But that being said, I don't think I quite see how the two things you > mention here are connected to each other. If operation A locks > parents before children, and operation B also locks parents before > children, they generally won't deadlock against each other, even if > each locks the children in a different order. Right, that's the same thing I was trying to say. > ... So my question is - what do > you mean by the parenthetical note about the partitioning info not > changing? Regardless of whether it does or does not, I think the same > property holds. What I was wondering about was the possibility of the set of tables-that-need-to-be-locked-at-all changing. Maybe that won't create an issue either, but I'm not quite sure. I do have to say that I find the idea of somebody changing the partitioning data concurrently with queries on that partitioned table to be stone cold petrifying. I don't think I believe you can make that work. regards, tom lane
On Wed, Feb 20, 2019 at 11:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Right, that's the same thing I was trying to say. OK, thanks. > > ... So my question is - what do > > you mean by the parenthetical note about the partitioning info not > > changing? Regardless of whether it does or does not, I think the same > > property holds. > > What I was wondering about was the possibility of the set of > tables-that-need-to-be-locked-at-all changing. Maybe that won't > create an issue either, but I'm not quite sure. That's pretty much what I was thinking, too. I think it might be fair to say, however, that if it does give rise to deadlock situations, they will be corner cases. For instance, suppose you lock are busy locking top-down and, meanwhile, somebody detaches a partition you haven't gotten around to locking yet and tries to attach it someplace higher up in the partition hierarchy. I think that there's a more-or-less unavoidable deadlock there. And there may be other cases where it is practically avoidable but we will fail to avoid it. But I don't think it's such a common scenario that we have two concurrent DDL commands on the same partitioning hierarchy that we should stress about it too much. If the common cases work OK, a theoretical deadlock risk in some more obscure case seems acceptable to me, if it means we get a significant performance boost. > I do have to say > that I find the idea of somebody changing the partitioning data > concurrently with queries on that partitioned table to be stone > cold petrifying. I don't think I believe you can make that work. I guess you won't be surprised to hear that I believe otherwise, but I agree it's a pretty thorny issue. I would welcome your comments on the patches and issues discussed on the ATTACH/DETACH CONCURRENTLY thread. To get things to (apparently) work, I have had to separately handle: (1) the case where things change as we are building the PartitionDesc, (2) the case where the same PartitionDesc changes during a planning cycle, (3) the case where the PartitionDesc changes between planning and execution, and (4) the case where the PartitionDesc changes during execution. And even then I'm only able so far to handle cases where a partition is added, just by ignoring it, not cases where a partition is removed. But it does seem to work. There isn't that much code yet that depends on the PartitionDesc not changing, so it seems to be an easier problem than, say, allowing a column to be added concurrently. As David says, we probably shouldn't hijack this thread to talk about that in detail, but I would definitely like it if you would comment on what I've done over there. I think it's good work but it is very easy to be wrong about such things -- and even if the basic approach is sound, there may be more cases of which I have not thought. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Feb 20, 2019 at 11:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> What I was wondering about was the possibility of the set of >> tables-that-need-to-be-locked-at-all changing. Maybe that won't >> create an issue either, but I'm not quite sure. > That's pretty much what I was thinking, too. I think it might be fair > to say, however, that if it does give rise to deadlock situations, > they will be corner cases. For instance, suppose you lock are busy > locking top-down and, meanwhile, somebody detaches a partition you > haven't gotten around to locking yet and tries to attach it someplace > higher up in the partition hierarchy. I think that there's a > more-or-less unavoidable deadlock there. And there may be other cases > where it is practically avoidable but we will fail to avoid it. But I > don't think it's such a common scenario that we have two concurrent > DDL commands on the same partitioning hierarchy that we should stress > about it too much. If the common cases work OK, a theoretical > deadlock risk in some more obscure case seems acceptable to me, if it > means we get a significant performance boost. I agree that any deadlock would have to involve somebody doing something quite odd --- not just one partition-oriented operation, but something taking multiple strong locks without regard to the partition structure. So I don't see a problem with taking that risk; people doing that sort of thing are probably at risk of deadlocks no matter what we do here. Looking at the patch itself, I agree that a bit more attention to comments is needed, and I wonder whether David has found all the places where it's now necessary to s/NoLock/RowExclusiveLock/. I don't have any other objections. regards, tom lane
On Wed, Feb 20, 2019 at 3:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I agree that any deadlock would have to involve somebody doing something > quite odd --- not just one partition-oriented operation, but something > taking multiple strong locks without regard to the partition structure. > So I don't see a problem with taking that risk; people doing that sort > of thing are probably at risk of deadlocks no matter what we do here. OK. > Looking at the patch itself, I agree that a bit more attention to comments > is needed, and I wonder whether David has found all the places where > it's now necessary to s/NoLock/RowExclusiveLock/. I don't have any > other objections. I spent some time thinking about that exact issue this morning and studying the code to try to figure that out. I wasn't able to find any other places that seemed to need updating, but it could be that I missed something that David also missed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, 21 Feb 2019 at 10:32, Robert Haas <robertmhaas@gmail.com> wrote: > I spent some time thinking about that exact issue this morning and > studying the code to try to figure that out. I wasn't able to find > any other places that seemed to need updating, but it could be that I > missed something that David also missed. It looks like the comment that claimed the table was already locked crept back in during a (seemingly) sloppy rebase after the relation_open() -> table_open() change. I've made a pass over this again and updated the header comments in functions that now obtain a lock to mention that fact. Also slightly updated commit msg in the patch. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Feb 20, 2019 at 3:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Looking at the patch itself, I agree that a bit more attention to comments >> is needed, and I wonder whether David has found all the places where >> it's now necessary to s/NoLock/RowExclusiveLock/. I don't have any >> other objections. > I spent some time thinking about that exact issue this morning and > studying the code to try to figure that out. I wasn't able to find > any other places that seemed to need updating, but it could be that I > missed something that David also missed. Actually, in the wake of b04aeb0a0, we probably need not be too stressed about the possibility that we missed something. Those assertions wouldn't detect doing an unwanted lock upgrade, but I think that's unlikely to be an issue here (or if it is, we have the problem anyway). regards, tom lane
On Wed, Feb 20, 2019 at 4:56 PM David Rowley <david.rowley@2ndquadrant.com> wrote: > I've made a pass over this again and updated the header comments in > functions that now obtain a lock to mention that fact. Thanks. I have committed this version. I know Tomas Vondra was planning to do that, but it's been close to a month since he mentioned those plans so I hope he will not mind me jumping in. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/21/19 5:33 PM, Robert Haas wrote: > On Wed, Feb 20, 2019 at 4:56 PM David Rowley > <david.rowley@2ndquadrant.com> wrote: >> I've made a pass over this again and updated the header comments in >> functions that now obtain a lock to mention that fact. > > Thanks. I have committed this version. I know Tomas Vondra was > planning to do that, but it's been close to a month since he mentioned > those plans so I hope he will not mind me jumping in. > Fine with me. It was mostly a sign that I'd like to get that committed eventually, trying to nudge others into looking at the patch a bit more. Relaxing locks is tricky, and I wasn't quite sure I haven't missed something obvious. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 22 Feb 2019 at 05:33, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Feb 20, 2019 at 4:56 PM David Rowley > <david.rowley@2ndquadrant.com> wrote: > > I've made a pass over this again and updated the header comments in > > functions that now obtain a lock to mention that fact. > > Thanks. I have committed this version. I know Tomas Vondra was > planning to do that, but it's been close to a month since he mentioned > those plans so I hope he will not mind me jumping in. Great. Thanks for doing that, and thanks to everyone who reviewed it too. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services