On Wed, Apr 7, 2021 at 5:18 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Wed, Apr 7, 2021 at 8:24 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > I also could not get excited about postponing initialization of RETURNING > > or WITH CHECK OPTIONS expressions. I grant that that can be helpful > > when those features are used, but I doubt that RETURNING is used that > > heavily, and WITH CHECK OPTIONS is surely next door to nonexistent > > in performance-critical queries. If the feature isn't used, the cost > > of the existing code is about zero. So I couldn't see that it was worth > > the amount of code thrashing and risk of new bugs involved. > > Okay. > > > The bit you > > noted about EXPLAIN missing a subplan is pretty scary in this connection; > > I'm not at all sure that that's just cosmetic. > > Yeah, this and...
I looked into this and can't see why this isn't just cosmetic as far as ModifyTable is concerned. "EXPLAIN missing a subplan" here just means that ModifyTableState.PlanState.subPlan is not set. Besides ExplainNode(), only ExecReScan() looks at PlanState.subPlan, and that does not seem relevant to ModifyTable, because it doesn't support rescanning. I don't see any such problems with creating RETURNING projections on-demand either. > > (Having said that, I'm wondering if there are bugs in these cases for > > cross-partition updates that target a previously-not-used partition. > > So we might have things to fix anyway.) > > ...this would need to be looked at a bit more closely, which I'll try > to do sometime later this week. Given the above, I can't think of any undiscovered problems related to WCO and RETURNING expression states in the cases where cross-partition updates target partitions that need to be initialized by ExecInitPartitionInfo(). Here is the result for the test case in updatable_views.sql modified to use partitioning and cross-partition updates: CREATE TABLE base_tbl (a int) partition by range (a); CREATE TABLE base_tbl1 PARTITION OF base_tbl FOR VALUES FROM (1) TO (6); CREATE TABLE base_tbl2 PARTITION OF base_tbl FOR VALUES FROM (6) TO (11); CREATE TABLE base_tbl3 PARTITION OF base_tbl FOR VALUES FROM (11) TO (15); CREATE TABLE ref_tbl (a int PRIMARY KEY); INSERT INTO ref_tbl SELECT * FROM generate_series(1,10); CREATE VIEW rw_view1 AS SELECT * FROM base_tbl b WHERE EXISTS(SELECT 1 FROM ref_tbl r WHERE r.a = b.a) WITH CHECK OPTION; INSERT INTO rw_view1 VALUES (1); INSERT 0 1 INSERT INTO rw_view1 VALUES (11); ERROR: new row violates check option for view "rw_view1" DETAIL: Failing row contains (11). -- Both are cross-partition updates where the target relation is -- lazily initialized in ExecInitPartitionInfo(), along with the WCO -- qual ExprState UPDATE rw_view1 SET a = a + 5 WHERE a = 1; UPDATE 1 UPDATE rw_view1 SET a = a + 5 WHERE a = 6; ERROR: new row violates check option for view "rw_view1" DETAIL: Failing row contains (11). EXPLAIN (costs off) INSERT INTO rw_view1 VALUES (5); QUERY PLAN ---------------------- Insert on base_tbl b -> Result (2 rows) EXPLAIN (costs off) UPDATE rw_view1 SET a = a + 5 WHERE a = 1; QUERY PLAN -------------------------------------------------------- Update on base_tbl b Update on base_tbl1 b_1 -> Nested Loop -> Index Scan using ref_tbl_pkey on ref_tbl r Index Cond: (a = 1) -> Seq Scan on base_tbl1 b_1 Filter: (a = 1) (7 rows) EXPLAIN (costs off) UPDATE rw_view1 SET a = a + 5 WHERE a = 6; QUERY PLAN -------------------------------------------------------- Update on base_tbl b Update on base_tbl2 b_1 -> Nested Loop -> Index Scan using ref_tbl_pkey on ref_tbl r Index Cond: (a = 6) -> Seq Scan on base_tbl2 b_1 Filter: (a = 6) (7 rows) Patch attached. I tested the performance benefit of doing this by modifying the update query used in earlier benchmarks to have a RETURNING * clause, getting the following TPS numbers: -Mprepared (plan_cache_mode=force_generic_plan) nparts 10cols 20cols 40cols HEAD 64 10909 9067 7171 128 6903 5624 4161 256 3748 3056 2219 1024 953 738 427 Patched 64 13817 13395 12754 128 9271 9102 8279 256 5345 5207 5083 1024 1463 1443 1389 Also, I don't see much impact of checking if (node->returningLists) in the per-result-rel initialization loop in the common cases where there's no RETURNING. -- Amit Langote EDB: http://www.enterprisedb.com
0001-Initialize-WITH-CHECK-OPTIONS-and-RETURNING-expressi.patch
Description: Binary data