Обсуждение: parallel append vs. simple UNION ALL
As I mentioned in the commit message for the Parallel Append commit (ab72716778128fb63d54ac256adf7fe6820a1185), it's kind of sad that this doesn't work with UNION ALL queries, which are an obvious candidate for such parallelization. It turns out that it actually does work to a limited degree: assuming that the UNION ALL query can be converted to a simple appendrel, it can consider a parallel append of non-partial paths only. The attached patch lets it consider a parallel append of partial paths by doing the following things: 1. Teaching set_subquery_pathlist to create *partial* SubqueryScan paths as well as non-partial ones. 2. Teaching grouping_planner to create partial paths for the final rel if not at the outermost query level. 3. Modifying finalize_plan to allow the gather_param to be passed across subquery boundaries. #3 is the only part I'm really unsure about; the other stuff looks pretty cut and dried. I have a draft patch that handles the case where the union can't be converted to a simple appendrel, too, but that's not quite baked enough to post yet. For those for whom the above may be too technical to follow, here's an example: pgbench -i 40 explain (costs off) select a.bid from pgbench_accounts a, pgbench_branches b where a.bid = b.bid and aid % 1000 = 0 union all select a.bid from pgbench_accounts a where aid % 1000 = 0; Unpatched: Append -> Gather Workers Planned: 2 -> Hash Join Hash Cond: (a.bid = b.bid) -> Parallel Seq Scan on pgbench_accounts a Filter: ((aid % 1000) = 0) -> Hash -> Seq Scan on pgbench_branches b -> Gather Workers Planned: 2 -> Parallel Seq Scan on pgbench_accounts a_1 Filter: ((aid % 1000) = 0) Patched: Gather Workers Planned: 2 -> Parallel Append -> Hash Join Hash Cond: (a.bid = b.bid) -> Parallel Seq Scan on pgbench_accounts a Filter: ((aid % 1000) = 0) -> Hash -> Seq Scan on pgbench_branches b -> Parallel Seq Scan on pgbench_accounts a_1 Filter: ((aid % 1000) = 0) In this particular case the change doesn't buy very much, but the second plan is better because avoid shutting down one set of workers and starting a new set. That's more efficient, plus it allows the two branches to be worked in parallel rather than serially. On a small enough scale factor, even without the patch, you get this... Gather Workers Planned: 2 -> Parallel Append -> Nested Loop Join Filter: (a.bid = b.bid) -> Seq Scan on pgbench_branches b -> Seq Scan on pgbench_accounts a Filter: ((aid % 1000) = 0) -> Seq Scan on pgbench_accounts a_1 Filter: ((aid % 1000) = 0) ...but that's not good because now we have regular sequential scans instead of partial sequential scans. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Sat, Dec 23, 2017 at 4:53 PM, Robert Haas <robertmhaas@gmail.com> wrote: > As I mentioned in the commit message for the Parallel Append commit > (ab72716778128fb63d54ac256adf7fe6820a1185), it's kind of sad that this > doesn't work with UNION ALL queries, which are an obvious candidate > for such parallelization. It turns out that it actually does work to > a limited degree: assuming that the UNION ALL query can be converted > to a simple appendrel, it can consider a parallel append of > non-partial paths only. The attached patch lets it consider a > parallel append of partial paths ... Here's an extended series of patches that now handles both the simple UNION ALL case (where we flatten it) and the unflattened case: 0001 is pretty much the same as the subquery-smarts.patch file I attached to the previous email. I don't see much reason not to go ahead and commit this, although it could use a test case. It makes the simple/flattened case work. After some study I think that the gather-parameter handling is correct, although if somebody felt like reviewing that portion especially I wouldn't say no. 0002 rewrites recurse_union_children to work iteratively rather than recursively and renames it to plan_union_children. This probably isn't 100% necessary, but it seems to me that the resulting code is easier to understand, and it reduces the risk of blowing out the stack. There should be no user-visible behavior change. 0003 rewrites the setop planner to create a separate upper rel for each stage of setop planning and uses them to return paths instead of returning paths directly. This is necessary preparatory work for anything that wants to consider multiple possible paths for queries that go through the full setop planner, but it shouldn't have any visible impact all by itself. 0004 causes generate_union_path() to consider both the traditional method and also Gather -> Parallel Append -> [partial path for each subquery]. This is still a bit rough around the edges and there's a lot more that could be done here, but I'm posting what I have for now in the (perhaps vain) hope of getting some feedback. With this, you can use Parallel Append for the UNION ALL step of a query like SELECT .. UNION ALL .. SELECT ... EXCEPT SELECT ... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Sat, Feb 24, 2018 at 2:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
0001 is pretty much the same as the subquery-smarts.patch file I
attached to the previous email. I don't see much reason not to go
ahead and commit this, although it could use a test case. It makes
the simple/flattened case work. After some study I think that the
gather-parameter handling is correct, although if somebody felt like
reviewing that portion especially I wouldn't say no.
I have applied 0001 on pg-head, and while playing with regression tests, it crashed with below test case.
postgres=# SET min_parallel_table_scan_size=0;
SET
postgres=# SELECT * FROM information_schema.foreign_data_wrapper_options ORDER BY 1, 2, 3;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
--logfile
2018-02-26 22:06:07.331 IST [43508] LOG: database system is ready to accept connections
TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813)
2018-02-26 22:06:42.345 IST [43508] LOG: server process (PID 43519) was terminated by signal 6: Aborted
2018-02-26 22:06:42.345 IST [43508] DETAIL: Failed process was running: SELECT * FROM information_schema.foreign_data_wrapper_options ORDER BY 1, 2, 3;
postgres=# SET min_parallel_table_scan_size=0;
SET
postgres=# SELECT * FROM information_schema.foreign_data_wrapper_options ORDER BY 1, 2, 3;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
--logfile
2018-02-26 22:06:07.331 IST [43508] LOG: database system is ready to accept connections
TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813)
2018-02-26 22:06:42.345 IST [43508] LOG: server process (PID 43519) was terminated by signal 6: Aborted
2018-02-26 22:06:42.345 IST [43508] DETAIL: Failed process was running: SELECT * FROM information_schema.foreign_data_wrapper_options ORDER BY 1, 2, 3;
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
> On Sat, Feb 24, 2018 at 2:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> 0001 is pretty much the same as the subquery-smarts.patch file I >> attached to the previous email. I don't see much reason not to go >> ahead and commit this, although it could use a test case. It makes >> the simple/flattened case work. After some study I think that the >> gather-parameter handling is correct, although if somebody felt like >> reviewing that portion especially I wouldn't say no. I had a look at 0001 patch. Other than the issue raised by Rajkumar, it looks good functionally. Regarding the finalize_plan() changes, I see that in the patch, the Gather rescan param is now included in the valid_params while calling finalize_plan() for the SubqueryScan, which looks correct. But I was thinking that instead of doing that just before the recursive finalize_plan(), it looks better if we do that at the initial section of finalize_plan(). We already add initSetParam to valid_params. There itself we can also add gather_params. Something like this : @@ -2314,6 +2314,10 @@ finalize_plan(PlannerInfo *root, Plan *plan, if (initSetParam) valid_params = bms_union(valid_params, initSetParam); + /* Same applies for Gather rescan param */ + if (gather_param >= 0) + valid_params = bms_add_member(valid_params, gather_param); On 27 February 2018 at 16:51, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote: > > > I have applied 0001 on pg-head, and while playing with regression tests, it > crashed with below test case. > > postgres=# SET min_parallel_table_scan_size=0; > SET > postgres=# SELECT * FROM information_schema.foreign_data_wrapper_options > ORDER BY 1, 2, 3; > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. This is happening because there is a ProjectionPath as one of the subpaths, and that is not parallel safe. Sort Sort Key: ((current_database())::information_schema.sql_identifier), ((w.fdwname)::information_schema.sql_identifier), ((((pg_options_to_table(w.fdwoptions))).option_name)::information_schema.sql_identifier) -> Result -> ProjectSet -> Hash Join Hash Cond: (w.fdwowner = u.oid) -> Seq Scan on pg_foreign_data_wrapper w Filter: (pg_has_role(fdwowner, 'USAGE'::text) OR has_foreign_data_wrapper_privilege(oid, 'USAGE'::text)) -> Hash -> Seq Scan on pg_authid u In grouping_planner() where partial paths are generated for final_rel, we can skip non-parallel-safe paths. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
On Sat, Feb 24, 2018 at 2:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Hi,
With all 0001,0002,0003 and 0004 patch applied on head, I am getting a strange crash, while trying to change table name
in a query by using "TAB" key.
Same test case working fine with only 0001 applied and also on PG-head.
below are steps to reproduce.
--run below sqls
SET parallel_setup_cost=0;
SET parallel_tuple_cost=0;
SET min_parallel_table_scan_size=0;
CREATE TABLE tbl_union_t1 (c1 INTEGER PRIMARY KEY,c2 INTEGER,c3 CHAR(10));
INSERT INTO tbl_union_t1 SELECT i, i % 125, to_char(i % 4, 'FM0000') FROM generate_series(0, 499,2) i;
CREATE TABLE tbl_union_t2 (c1 INTEGER PRIMARY KEY,c2 INTEGER,c3 CHAR(10));
INSERT INTO tbl_union_t2 SELECT i, i % 125, to_char(i % 4, 'FM0000') FROM generate_series(0, 499,3) i;
ANALYSE tbl_union_t1;
ANALYSE tbl_union_t2;
EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tbl_union_t1 EXCEPT SELECT c1,c2 FROM tbl_union_t2 WHERE c1 % 25 =0 )UA;
--now try modifying tbl_union_t1 in the above query
--remove "_union_t1" and press TAB key, It crashed for me.
EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tbl<PRESS TAB KEY HERE>EXCEPT SELECT c1,c2 FROM tbl_union_t2 WHERE c1 % 25 =0 )UA;
postgres=# EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tblWARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat your command.
--logfile says something like this
2018-03-01 18:37:36.456 IST [50071] LOG: database system is ready to accept connections
2018-03-01 18:38:38.668 IST [50071] LOG: background worker "parallel worker" (PID 51703) was terminated by signal 11: Segmentation fault
2018-03-01 18:38:38.668 IST [50071] DETAIL: Failed process was running: SELECT pg_catalog.quote_ident(c.relname) FROM pg_catalog.pg_class c WHERE c.relkind IN ('r', 'S', 'v', 'm', 'f', 'p') AND substring(pg_catalog.quote_ident(c.relname),1,3)='tbl' AND pg_catalog.pg_table_is_visible(c.oid) AND c.relnamespace <> (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'pg_catalog')
UNION
SELECT pg_catalog.quote_ident(n.nspname) || '.' FROM pg_catalog.pg_namespace n WHERE substring(pg_catalog.quote_ident(n.nspname) || '.',1,3)='tbl' AND (SELECT pg_catalog.count(*) FROM pg_catalog.pg_namespace WHERE substring(pg_catalog.quote_ident(nspname) || '.',1,3) = substring('tbl',1,pg_catalog.length(pg_catalog.quote_ident(nspname))+1)) > 1
UNION
SELECT pg_catalog.quote_ident(n.nspname) || '.' || pg_catalog.quote_ident(c.relname) FROM pg_catalog.pg_class c, pg_catalog.pg_namespace n WHERE c.relnamespace = n.oid AND c.relkind IN ('r', 'S', 'v', 'm', 'f', 'p') AND substring(pg_catalog.quote_ident(n.nspname) || '.' || pg_catalog.quote_ident(c.relname),1,3)='tbl' AND substring(pg_catalog.quote_id
2018-03-01 18:38:38.668 IST [50071] LOG: terminating any other active server processes
2018-03-01 18:38:38.668 IST [50082] WARNING: terminating connection because of crash of another server process
2018-03-01 18:38:38.668 IST [50082] DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
2018-03-01 18:38:38.668 IST [50082] HINT: In a moment you should be able to reconnect to the database and repeat your command.
2018-03-01 18:38:38.670 IST [50076] WARNING: terminating connection because of crash of another server process
2018-03-01 18:38:38.670 IST [50076] DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
2018-03-01 18:38:38.670 IST [50076] HINT: In a moment you should be able to reconnect to the database and repeat your command.
2018-03-01 18:38:38.675 IST [50071] LOG: all server processes terminated; reinitializing
2018-03-01 18:38:38.702 IST [51712] LOG: database system was interrupted; last known up at 2018-03-01 18:37:36 IST
2018-03-01 18:38:38.723 IST [51712] LOG: database system was not properly shut down; automatic recovery in progress
2018-03-01 18:38:38.724 IST [51712] LOG: redo starts at 0/1639510
2018-03-01 18:38:38.726 IST [51712] LOG: invalid record length at 0/1669488: wanted 24, got 0
2018-03-01 18:38:38.726 IST [51712] LOG: redo done at 0/1669420
2018-03-01 18:38:38.726 IST [51712] LOG: last completed transaction was at log time 2018-03-01 18:38:36.53573+05:30
2018-03-01 18:38:38.744 IST [50071] LOG: database system is ready to accept connections
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
0004 causes generate_union_path() to consider both the traditional
method and also Gather -> Parallel Append -> [partial path for each
subquery]. This is still a bit rough around the edges and there's a
lot more that could be done here, but I'm posting what I have for now
in the (perhaps vain) hope of getting some feedback. With this, you
can use Parallel Append for the UNION ALL step of a query like SELECT
.. UNION ALL .. SELECT ... EXCEPT SELECT ...
Hi,
With all 0001,0002,0003 and 0004 patch applied on head, I am getting a strange crash, while trying to change table name
in a query by using "TAB" key.
Same test case working fine with only 0001 applied and also on PG-head.
below are steps to reproduce.
--run below sqls
SET parallel_setup_cost=0;
SET parallel_tuple_cost=0;
SET min_parallel_table_scan_size=0;
CREATE TABLE tbl_union_t1 (c1 INTEGER PRIMARY KEY,c2 INTEGER,c3 CHAR(10));
INSERT INTO tbl_union_t1 SELECT i, i % 125, to_char(i % 4, 'FM0000') FROM generate_series(0, 499,2) i;
CREATE TABLE tbl_union_t2 (c1 INTEGER PRIMARY KEY,c2 INTEGER,c3 CHAR(10));
INSERT INTO tbl_union_t2 SELECT i, i % 125, to_char(i % 4, 'FM0000') FROM generate_series(0, 499,3) i;
ANALYSE tbl_union_t1;
ANALYSE tbl_union_t2;
EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tbl_union_t1 EXCEPT SELECT c1,c2 FROM tbl_union_t2 WHERE c1 % 25 =0 )UA;
--now try modifying tbl_union_t1 in the above query
--remove "_union_t1" and press TAB key, It crashed for me.
EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tbl<PRESS TAB KEY HERE>EXCEPT SELECT c1,c2 FROM tbl_union_t2 WHERE c1 % 25 =0 )UA;
postgres=# EXPLAIN SELECT AVG(c1),SUM(c2) FROM (SELECT c1,c2 FROM tblWARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat your command.
--logfile says something like this
2018-03-01 18:37:36.456 IST [50071] LOG: database system is ready to accept connections
2018-03-01 18:38:38.668 IST [50071] LOG: background worker "parallel worker" (PID 51703) was terminated by signal 11: Segmentation fault
2018-03-01 18:38:38.668 IST [50071] DETAIL: Failed process was running: SELECT pg_catalog.quote_ident(c.relname) FROM pg_catalog.pg_class c WHERE c.relkind IN ('r', 'S', 'v', 'm', 'f', 'p') AND substring(pg_catalog.quote_ident(c.relname),1,3)='tbl' AND pg_catalog.pg_table_is_visible(c.oid) AND c.relnamespace <> (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'pg_catalog')
UNION
SELECT pg_catalog.quote_ident(n.nspname) || '.' FROM pg_catalog.pg_namespace n WHERE substring(pg_catalog.quote_ident(n.nspname) || '.',1,3)='tbl' AND (SELECT pg_catalog.count(*) FROM pg_catalog.pg_namespace WHERE substring(pg_catalog.quote_ident(nspname) || '.',1,3) = substring('tbl',1,pg_catalog.length(pg_catalog.quote_ident(nspname))+1)) > 1
UNION
SELECT pg_catalog.quote_ident(n.nspname) || '.' || pg_catalog.quote_ident(c.relname) FROM pg_catalog.pg_class c, pg_catalog.pg_namespace n WHERE c.relnamespace = n.oid AND c.relkind IN ('r', 'S', 'v', 'm', 'f', 'p') AND substring(pg_catalog.quote_ident(n.nspname) || '.' || pg_catalog.quote_ident(c.relname),1,3)='tbl' AND substring(pg_catalog.quote_id
2018-03-01 18:38:38.668 IST [50071] LOG: terminating any other active server processes
2018-03-01 18:38:38.668 IST [50082] WARNING: terminating connection because of crash of another server process
2018-03-01 18:38:38.668 IST [50082] DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
2018-03-01 18:38:38.668 IST [50082] HINT: In a moment you should be able to reconnect to the database and repeat your command.
2018-03-01 18:38:38.670 IST [50076] WARNING: terminating connection because of crash of another server process
2018-03-01 18:38:38.670 IST [50076] DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
2018-03-01 18:38:38.670 IST [50076] HINT: In a moment you should be able to reconnect to the database and repeat your command.
2018-03-01 18:38:38.675 IST [50071] LOG: all server processes terminated; reinitializing
2018-03-01 18:38:38.702 IST [51712] LOG: database system was interrupted; last known up at 2018-03-01 18:37:36 IST
2018-03-01 18:38:38.723 IST [51712] LOG: database system was not properly shut down; automatic recovery in progress
2018-03-01 18:38:38.724 IST [51712] LOG: redo starts at 0/1639510
2018-03-01 18:38:38.726 IST [51712] LOG: invalid record length at 0/1669488: wanted 24, got 0
2018-03-01 18:38:38.726 IST [51712] LOG: redo done at 0/1669420
2018-03-01 18:38:38.726 IST [51712] LOG: last completed transaction was at log time 2018-03-01 18:38:36.53573+05:30
2018-03-01 18:38:38.744 IST [50071] LOG: database system is ready to accept connections
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Sat, Feb 24, 2018 at 2:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > Here's an extended series of patches that now handles both the simple > UNION ALL case (where we flatten it) and the unflattened case: > The patches look clean. I particularly looked at 0003. patch 0001 + /* + * Generate partial paths for final_rel, too, if outer query levels might + * be able to make use of them. + */ I am not able to understand the construct esp. the if clause. Did you want to say "... if there are outer query levels. Those might ..." or something like that? 0002 (op->all == top_union->all || op->all) && This isn't really your change. Checking op->all is cheaper than checking equality, so may be we should check that first and take advantage of short-circuit condition evaluation. If we do that above condition reduces to (op->all || !top_union->all) which is two boolean conditions, even cheaper. But may be the second optimization is not worth the loss of readability. "identically-propertied UNIONs" may be "UNIONs with identical properties". 0003 Probably we want to rename generate_union_path() as generate_union_rel() or generate_union_paths() since the function doesn't return a path anymore. Similarly for generate_nonunion_path(). In recurse_set_operations() - return NULL; /* keep compiler quiet */ This line is deleted and instead rel is initialized to NULL. That way we loose any chance to detect a future bug because of a block leaving rel uninitialized through compiler warning. May be we should replace "return NULL" with "rel = NULL", which will not be executed because of the error. + /* Build path list and relid set. */ + foreach(lc, rellist) + { With the changes in this patch, we could actually use add_paths_to_append_rel() to create an append path. That function builds paths with different pathkeys, parameterization (doesn't matter here) and also handles parallel append. So we can avoid code duplication and also leverage more optimizations like using MergeAppend instead of overall sort etc. But that function doesn't have ability to add a final node like make_union_unique(). A similar requirement has arisen in partition-wise join where we need to add a final node for finalising aggregate on top of paths created by add_paths_to_append_rel(). May be we can change that function to return a list of paths, which are then finalized by the caller and added to "append" rel. But I don't think doing all that is in the scope of this patch set. 0004 + if (!op->all) + ppath = make_union_unique(op, ppath, tlist, root); We could probably push the grouping/sorting down to the parallel workers. But again not part of this patchset. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Tue, Feb 27, 2018 at 6:21 AM, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote: > I have applied 0001 on pg-head, and while playing with regression tests, it > crashed with below test case. > > postgres=# SET min_parallel_table_scan_size=0; > SET > postgres=# SELECT * FROM information_schema.foreign_data_wrapper_options > ORDER BY 1, 2, 3; > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. Hmm, nice catch. I think part of the problem here is that commit 69f4b9c85f168ae006929eec44fc44d569e846b9, wherein Andres introduced the ProjectSet node, didn't really do the right thing in terms of testing parallel-safety. Before that commit, is_parallel_safe(root, (Node *) scanjoin_target->exprs)) was really testing whether the tlist produced during the scan/join phase was parallel-safe. However, after that commit, scanjoin_target->exprs wasn't really the final target for the scan/join phase any more; instead, it was the first of possibly several targets computed by split_pathtarget_at_srfs(). Really, the right thing to do is to test the *last* element in that list for parallel-safety, but as the code stands we end up testing the *first* element. So, if there's a parallel-restricted item in the target list (this query ends up putting a CoerceToDomain in the target list, which we currently consider parallel-restricted), it looks we can nevertheless end up trying to project it in what is supposed to be a partial path. There are a large number of cases where this doesn't end up mattering because the next upper_rel created will not get marked consider_parallel because its target list will also contain the same parallel-restricted construct, and therefore the partial paths generated for the scan/join rel will never get used -- except to generate Gather/Gather Merge paths, which has already happened; but that step didn't know about the rest of the scan/join targets either, so it won't have used them. However, both create_distinct_paths() and the code in grouping_planner that populates final_rel assume that they don't need to retest the target for parallel-safety because no projection is done at those levels; they just inherit the parallel-safety marking of the input rel, so in those cases if the input rel's marking is wrong the result is populated upward. There's another way final_rel->consider_parallel can be wrong, too: if the FROM-list is empty, then we create a join rel and set its consider_parallel flag without regard to the parallel-safety of the target list. There are comments in query_planner() says that this will be dealt with "later", but this seems not to be true. (Before Tom's commit da1c91631e3577ea5818f855ebb5bd206d559006, the comments simply ignored the question of whether a check was needed here, but Tom seems to have inserted an incorrect justification for the already-wrong code.) I'm not sure to what degree, if at all, any of these problems are visible given that we don't use final_rel->consider_parallel for much of anything. Certainly, it gets much easier to trigger a problem with 0001 applied, as the test case shows. But I'm not entirely convinced that there's no problem even without that. It seems like every upper rel that is setting its consider_parallel flag based on the first element of some list of targets rather than the last is potentially vulnerable to ending up with the wrong answer, and I'm afraid that might have some adverse consequence that I haven't quite pinned down yet. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi,
With 0001 applied on PG-head, I got reference leak warning and later a server crash.
this crash is reproducible with enable_parallel_append=off also.
below is the test case to reproduce this.
SET enable_parallel_append=off;
SET parallel_setup_cost=0;
SET parallel_tuple_cost=0;
SET min_parallel_table_scan_size=0;
CREATE TABLE foo (a numeric PRIMARY KEY);
INSERT INTO foo VALUES (1);
INSERT INTO foo VALUES (2);
ANALYZE foo;
CREATE TABLE bar (a numeric PRIMARY KEY);
INSERT INTO bar VALUES (6);
INSERT INTO bar VALUES (7);
ANALYZE bar;
Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
/*
postgres=# Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
WARNING: relcache reference leak: relation "foo" not closed
WARNING: relcache reference leak: relation "bar" not closed
WARNING: Snapshot reference leak: Snapshot 0x22f9168 still referenced
WARNING: Snapshot reference leak: Snapshot 0x22f9298 still referenced
a
---
1
(1 row)
postgres=# Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>
*/
Stack-trace for the crash is given below :
/*
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `postgres: parallel worker for PID 7574 '.
Program terminated with signal 11, Segmentation fault.
#0 0x0000000000712283 in ExecProcNode (node=0x0) at ../../../src/include/executor/executor.h:236
236 if (node->chgParam != NULL) /* something changed? */
Missing separate debuginfos, use: debuginfo-install keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64 libcom_err-1.41.12-23.el6.x86_64 libselinux-2.0.94-7.el6.x86_64 openssl-1.0.1e-57.el6.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 0x0000000000712283 in ExecProcNode (node=0x0) at ../../../src/include/executor/executor.h:236
#1 0x0000000000714304 in ExecSetParamPlan (node=0x2311dd0, econtext=0x2312660) at nodeSubplan.c:1056
#2 0x00000000006ce19f in ExecEvalParamExec (state=0x231ab10, op=0x231ac28, econtext=0x2312660) at execExprInterp.c:2225
#3 0x00000000006cc106 in ExecInterpExpr (state=0x231ab10, econtext=0x2312660, isnull=0x7ffddfed6e47 "") at execExprInterp.c:1024
#4 0x00000000006cd828 in ExecInterpExprStillValid (state=0x231ab10, econtext=0x2312660, isNull=0x7ffddfed6e47 "") at execExprInterp.c:1819
#5 0x00000000006e02a1 in ExecEvalExprSwitchContext (state=0x231ab10, econtext=0x2312660, isNull=0x7ffddfed6e47 "") at ../../../src/include/executor/executor.h:305
#6 0x00000000006e038e in ExecQual (state=0x231ab10, econtext=0x2312660) at ../../../src/include/executor/executor.h:374
#7 0x00000000006e066b in ExecScan (node=0x2311cb8, accessMtd=0x70e4dc <SeqNext>, recheckMtd=0x70e5b3 <SeqRecheck>) at execScan.c:190
#8 0x000000000070e600 in ExecSeqScan (pstate=0x2311cb8) at nodeSeqscan.c:129
#9 0x00000000006deac2 in ExecProcNodeFirst (node=0x2311cb8) at execProcnode.c:446
#10 0x00000000006e9219 in ExecProcNode (node=0x2311cb8) at ../../../src/include/executor/executor.h:239
#11 0x00000000006e94a1 in ExecAppend (pstate=0x23117a8) at nodeAppend.c:203
#12 0x00000000006deac2 in ExecProcNodeFirst (node=0x23117a8) at execProcnode.c:446
#13 0x00000000006d5469 in ExecProcNode (node=0x23117a8) at ../../../src/include/executor/executor.h:239
#14 0x00000000006d7dc2 in ExecutePlan (estate=0x2310e08, planstate=0x23117a8, use_parallel_mode=0 '\000', operation=CMD_SELECT, sendTuples=1 '\001', numberTuples=0,
direction=ForwardScanDirection, dest=0x22ea108, execute_once=1 '\001') at execMain.c:1721
#15 0x00000000006d5a3b in standard_ExecutorRun (queryDesc=0x22ff1d8, direction=ForwardScanDirection, count=0, execute_once=1 '\001') at execMain.c:361
#16 0x00000000006d5857 in ExecutorRun (queryDesc=0x22ff1d8, direction=ForwardScanDirection, count=0, execute_once=1 '\001') at execMain.c:304
#17 0x00000000006dcb39 in ParallelQueryMain (seg=0x22561d0, toc=0x7f49097c0000) at execParallel.c:1313
#18 0x0000000000535cdb in ParallelWorkerMain (main_arg=737000409) at parallel.c:1397
#19 0x00000000007fcb8d in StartBackgroundWorker () at bgworker.c:841
#20 0x000000000080ffb1 in do_start_bgworker (rw=0x227cea0) at postmaster.c:5741
#21 0x000000000081031d in maybe_start_bgworkers () at postmaster.c:5954
#22 0x000000000080f382 in sigusr1_handler (postgres_signal_arg=10) at postmaster.c:5134
#23 <signal handler called>
#24 0x0000003dd26e1603 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:82
#25 0x000000000080ab76 in ServerLoop () at postmaster.c:1721
#26 0x000000000080a365 in PostmasterMain (argc=3, argv=0x2254180) at postmaster.c:1365
#27 0x000000000073f0b0 in main (argc=3, argv=0x2254180) at main.c:228
*/
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
With 0001 applied on PG-head, I got reference leak warning and later a server crash.
this crash is reproducible with enable_parallel_append=off also.
below is the test case to reproduce this.
SET enable_parallel_append=off;
SET parallel_setup_cost=0;
SET parallel_tuple_cost=0;
SET min_parallel_table_scan_size=0;
CREATE TABLE foo (a numeric PRIMARY KEY);
INSERT INTO foo VALUES (1);
INSERT INTO foo VALUES (2);
ANALYZE foo;
CREATE TABLE bar (a numeric PRIMARY KEY);
INSERT INTO bar VALUES (6);
INSERT INTO bar VALUES (7);
ANALYZE bar;
Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
/*
postgres=# Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
WARNING: relcache reference leak: relation "foo" not closed
WARNING: relcache reference leak: relation "bar" not closed
WARNING: Snapshot reference leak: Snapshot 0x22f9168 still referenced
WARNING: Snapshot reference leak: Snapshot 0x22f9298 still referenced
a
---
1
(1 row)
postgres=# Select a from foo where a=(select * from foo where a=1)
UNION All
SELECT a FROM bar where a=(select * from bar where a=1);
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>
*/
Stack-trace for the crash is given below :
/*
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `postgres: parallel worker for PID 7574 '.
Program terminated with signal 11, Segmentation fault.
#0 0x0000000000712283 in ExecProcNode (node=0x0) at ../../../src/include/executor/executor.h:236
236 if (node->chgParam != NULL) /* something changed? */
Missing separate debuginfos, use: debuginfo-install keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64 libcom_err-1.41.12-23.el6.x86_64 libselinux-2.0.94-7.el6.x86_64 openssl-1.0.1e-57.el6.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 0x0000000000712283 in ExecProcNode (node=0x0) at ../../../src/include/executor/executor.h:236
#1 0x0000000000714304 in ExecSetParamPlan (node=0x2311dd0, econtext=0x2312660) at nodeSubplan.c:1056
#2 0x00000000006ce19f in ExecEvalParamExec (state=0x231ab10, op=0x231ac28, econtext=0x2312660) at execExprInterp.c:2225
#3 0x00000000006cc106 in ExecInterpExpr (state=0x231ab10, econtext=0x2312660, isnull=0x7ffddfed6e47 "") at execExprInterp.c:1024
#4 0x00000000006cd828 in ExecInterpExprStillValid (state=0x231ab10, econtext=0x2312660, isNull=0x7ffddfed6e47 "") at execExprInterp.c:1819
#5 0x00000000006e02a1 in ExecEvalExprSwitchContext (state=0x231ab10, econtext=0x2312660, isNull=0x7ffddfed6e47 "") at ../../../src/include/executor/executor.h:305
#6 0x00000000006e038e in ExecQual (state=0x231ab10, econtext=0x2312660) at ../../../src/include/executor/executor.h:374
#7 0x00000000006e066b in ExecScan (node=0x2311cb8, accessMtd=0x70e4dc <SeqNext>, recheckMtd=0x70e5b3 <SeqRecheck>) at execScan.c:190
#8 0x000000000070e600 in ExecSeqScan (pstate=0x2311cb8) at nodeSeqscan.c:129
#9 0x00000000006deac2 in ExecProcNodeFirst (node=0x2311cb8) at execProcnode.c:446
#10 0x00000000006e9219 in ExecProcNode (node=0x2311cb8) at ../../../src/include/executor/executor.h:239
#11 0x00000000006e94a1 in ExecAppend (pstate=0x23117a8) at nodeAppend.c:203
#12 0x00000000006deac2 in ExecProcNodeFirst (node=0x23117a8) at execProcnode.c:446
#13 0x00000000006d5469 in ExecProcNode (node=0x23117a8) at ../../../src/include/executor/executor.h:239
#14 0x00000000006d7dc2 in ExecutePlan (estate=0x2310e08, planstate=0x23117a8, use_parallel_mode=0 '\000', operation=CMD_SELECT, sendTuples=1 '\001', numberTuples=0,
direction=ForwardScanDirection, dest=0x22ea108, execute_once=1 '\001') at execMain.c:1721
#15 0x00000000006d5a3b in standard_ExecutorRun (queryDesc=0x22ff1d8, direction=ForwardScanDirection, count=0, execute_once=1 '\001') at execMain.c:361
#16 0x00000000006d5857 in ExecutorRun (queryDesc=0x22ff1d8, direction=ForwardScanDirection, count=0, execute_once=1 '\001') at execMain.c:304
#17 0x00000000006dcb39 in ParallelQueryMain (seg=0x22561d0, toc=0x7f49097c0000) at execParallel.c:1313
#18 0x0000000000535cdb in ParallelWorkerMain (main_arg=737000409) at parallel.c:1397
#19 0x00000000007fcb8d in StartBackgroundWorker () at bgworker.c:841
#20 0x000000000080ffb1 in do_start_bgworker (rw=0x227cea0) at postmaster.c:5741
#21 0x000000000081031d in maybe_start_bgworkers () at postmaster.c:5954
#22 0x000000000080f382 in sigusr1_handler (postgres_signal_arg=10) at postmaster.c:5134
#23 <signal handler called>
#24 0x0000003dd26e1603 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:82
#25 0x000000000080ab76 in ServerLoop () at postmaster.c:1721
#26 0x000000000080a365 in PostmasterMain (argc=3, argv=0x2254180) at postmaster.c:1365
#27 0x000000000073f0b0 in main (argc=3, argv=0x2254180) at main.c:228
*/
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Tue, Mar 6, 2018 at 12:27 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 27, 2018 at 6:21 AM, Rajkumar Raghuwanshi
<rajkumar.raghuwanshi@enterprisedb.com> wrote:
> I have applied 0001 on pg-head, and while playing with regression tests, it
> crashed with below test case.
>
> postgres=# SET min_parallel_table_scan_size=0;
> SET
> postgres=# SELECT * FROM information_schema.foreign_data_wrapper_options
> ORDER BY 1, 2, 3;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
Hmm, nice catch. I think part of the problem here is that commit
69f4b9c85f168ae006929eec44fc44d569e846b9, wherein Andres introduced
the ProjectSet node, didn't really do the right thing in terms of
testing parallel-safety. Before that commit, is_parallel_safe(root,
(Node *) scanjoin_target->exprs)) was really testing whether the tlist
produced during the scan/join phase was parallel-safe. However, after
that commit, scanjoin_target->exprs wasn't really the final target for
the scan/join phase any more; instead, it was the first of possibly
several targets computed by split_pathtarget_at_srfs(). Really, the
right thing to do is to test the *last* element in that list for
parallel-safety, but as the code stands we end up testing the *first*
element. So, if there's a parallel-restricted item in the target list
(this query ends up putting a CoerceToDomain in the target list, which
we currently consider parallel-restricted), it looks we can
nevertheless end up trying to project it in what is supposed to be a
partial path.
There are a large number of cases where this doesn't end up mattering
because the next upper_rel created will not get marked
consider_parallel because its target list will also contain the same
parallel-restricted construct, and therefore the partial paths
generated for the scan/join rel will never get used -- except to
generate Gather/Gather Merge paths, which has already happened; but
that step didn't know about the rest of the scan/join targets either,
so it won't have used them. However, both create_distinct_paths() and
the code in grouping_planner that populates final_rel assume that they
don't need to retest the target for parallel-safety because no
projection is done at those levels; they just inherit the
parallel-safety marking of the input rel, so in those cases if the
input rel's marking is wrong the result is populated upward.
There's another way final_rel->consider_parallel can be wrong, too: if
the FROM-list is empty, then we create a join rel and set its
consider_parallel flag without regard to the parallel-safety of the
target list. There are comments in query_planner() says that this
will be dealt with "later", but this seems not to be true. (Before
Tom's commit da1c91631e3577ea5818f855ebb5bd206d559006, the comments
simply ignored the question of whether a check was needed here, but
Tom seems to have inserted an incorrect justification for the
already-wrong code.)
I'm not sure to what degree, if at all, any of these problems are
visible given that we don't use final_rel->consider_parallel for much
of anything. Certainly, it gets much easier to trigger a problem with
0001 applied, as the test case shows. But I'm not entirely convinced
that there's no problem even without that. It seems like every upper
rel that is setting its consider_parallel flag based on the first
element of some list of targets rather than the last is potentially
vulnerable to ending up with the wrong answer, and I'm afraid that
might have some adverse consequence that I haven't quite pinned down
yet.
On Wed, Mar 7, 2018 at 5:36 AM, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote: > With 0001 applied on PG-head, I got reference leak warning and later a > server crash. > this crash is reproducible with enable_parallel_append=off also. > below is the test case to reproduce this. New patches attached, fixing all 3 of the issues you reported: 0001 is a new patch to fix the incorrect parallel safety marks on upper relations. I don't know of a visible effect of this patch by itself, but there might be one. 0002 is the same as the old 0001, but I made a fix in SS_charge_for_initplans() which fixed your most recent crash report. Either this or the previous change also fixed the crash you saw when using tab-completion. Also, I added some test cases based on your failing examples. 0003-0005 are the same as the old 0002-0004. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
- 0005-Consider-Parallel-Append-as-a-way-to-implement-a-uni.patch
- 0004-Generate-a-separate-upper-relation-for-each-stage-of.patch
- 0003-Rewrite-recurse_union_children-to-iterate-rather-tha.patch
- 0002-Let-Parallel-Append-over-simple-UNION-ALL-have-parti.patch
- 0001-Correctly-assess-parallel-safety-of-tlists-when-SRFs.patch
On Thu, Mar 8, 2018 at 12:27 AM, Robert Haas <robertmhaas@gmail.com> wrote:
New patches attached, fixing all 3 of the issues you reported:
Thanks. new patches applied cleanly on head and fixing all reported issue.
Thanks & Regards,
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Thu, Mar 8, 2018 at 2:46 AM, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote: > On Thu, Mar 8, 2018 at 12:27 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> New patches attached, fixing all 3 of the issues you reported: > > Thanks. new patches applied cleanly on head and fixing all reported issue. Great. Committed 0001. Are you planning any further testing of this patch series? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Mar 1, 2018 at 8:30 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > The patches look clean. I particularly looked at 0003. > > patch 0001 > + /* > + * Generate partial paths for final_rel, too, if outer query levels might > + * be able to make use of them. > + */ > I am not able to understand the construct esp. the if clause. Did you want to > say "... if there are outer query levels. Those might ..." or something like > that? Well, that's what I meant, but I didn't think it was necessary to spell it out in quite that much detail. > 0002 > (op->all == top_union->all || op->all) && > This isn't really your change. Checking > op->all is cheaper than checking equality, so may be we should check that first > and take advantage of short-circuit condition evaluation. If we do that above > condition reduces to (op->all || !top_union->all) which is two boolean > conditions, even cheaper. But may be the second optimization is not worth the > loss of readability. I doubt this makes any difference. The compiler should be smart enough to do this the same way regardless of exactly how we write it, and if it's not, it can't make more than a few instructions worth of difference. This code is not nearly performance-critical enough for that to matter. Also, it's not the job of this patch to whack this around. > "identically-propertied UNIONs" may be "UNIONs with identical properties". Likewise, I didn't write those words, so I don't plan to change them just because I might have written them differently. > 0003 > Probably we want to rename generate_union_path() as generate_union_rel() or > generate_union_paths() since the function doesn't return a path anymore. > Similarly for generate_nonunion_path(). Good point. Changed. > In recurse_set_operations() > - return NULL; /* keep compiler quiet */ > This line is deleted and instead rel is initialized to NULL. That way we loose > any chance to detect a future bug because of a block leaving rel uninitialized > through compiler warning. May be we should replace "return NULL" with "rel = > NULL", which will not be executed because of the error. I don't think this is really a problem. If some code path fails to initialize rel, it's going to crash when postprocess_setop_rel() calls set_cheapest(). Any warning the compiler gives is more likely to be a false-positive than an actual problem. > + /* Build path list and relid set. */ > + foreach(lc, rellist) > + { > With the changes in this patch, we could actually use add_paths_to_append_rel() > to create an append path. That function builds paths with different pathkeys, > parameterization (doesn't matter here) and also handles parallel append. So we > can avoid code duplication and also leverage more optimizations like using > MergeAppend instead of overall sort etc. But that function doesn't have ability > to add a final node like make_union_unique(). A similar requirement has arisen > in partition-wise join where we need to add a final node for finalising > aggregate on top of paths created by add_paths_to_append_rel(). May be we can > change that function to return a list of paths, which are then finalized by the > caller and added to "append" rel. But I don't think doing all that is in the > scope of this patch set. Yeah, I thought about all of that and came to similar conclusions. > 0004 > + if (!op->all) > + ppath = make_union_unique(op, ppath, tlist, root); > We could probably push the grouping/sorting down to the parallel workers. But > again not part of this patchset. Yep. There's a lot more work that could be done to improve setop planning, but I think getting even this much done for v11 would be a significant step forward. Updated patches attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company<div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 1, 2018 at 8:30 AM, Ashutosh Bapat <span dir="ltr"><<a href="mailto:ashutosh.bapat@enterprisedb.com" target="_blank">ashutosh.bapat@enterprisedb.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Sat, Feb 24, 2018 at 2:55 AM, Robert Haas <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>> wrote:<br> <br> ><br> </span><span class="">> Here's an extended series of patches that now handles both the simple<br> > UNION ALL case (where we flatten it) and the unflattened case:<br> ><br> <br> </span>The patches look clean. I particularly looked at 0003.<br> <br> patch 0001<br> + /*<br> + * Generate partial paths for final_rel, too, if outer query levels might<br> + * be able to make use of them.<br> + */<br> I am not able to understand the construct esp. the if clause. Did you want to<br> say "... if there are outer query levels. Those might ..." or something like<br> that?<br> <br> 0002<br> (op->all == top_union->all || op->all) &&<br> This isn't really your change. Checking<br> op->all is cheaper than checking equality, so may be we should check that first<br> and take advantage of short-circuit condition evaluation. If we do that above<br> condition reduces to (op->all || !top_union->all) which is two boolean<br> conditions, even cheaper. But may be the second optimization is not worth the<br> loss of readability.<br> <br> "identically-propertied UNIONs" may be "UNIONs with identical properties".<br> <br> 0003<br> Probably we want to rename generate_union_path() as generate_union_rel() or<br> generate_union_paths() since the function doesn't return a path anymore.<br> Similarly for generate_nonunion_path().<br> <br> In recurse_set_operations()<br> - return NULL; /* keep compiler quiet */<br> This line is deleted and instead rel is initialized to NULL. That way we loose<br> any chance to detect a future bug because of a block leaving rel uninitialized<br> through compiler warning. May be we should replace "return NULL" with "rel =<br> NULL", which will not be executed because of the error.<br> <br> + /* Build path list and relid set. */<br> + foreach(lc, rellist)<br> + {<br> With the changes in this patch, we could actually use add_paths_to_append_rel()<br> to create an append path. That function builds paths with different pathkeys,<br> parameterization (doesn't matter here) and also handles parallel append. So we<br> can avoid code duplication and also leverage more optimizations like using<br> MergeAppend instead of overall sort etc. But that function doesn't have ability<br> to add a final node like make_union_unique(). A similar requirement has arisen<br> in partition-wise join where we need to add a final node for finalising<br> aggregate on top of paths created by add_paths_to_append_rel(). May be we can<br> change that function to return a list of paths, which are then finalized by the<br> caller and added to "append" rel. But I don't think doing all that is in the<br> scope of this patch set.<br> <br> 0004<br> + if (!op->all)<br> + ppath = make_union_unique(op, ppath, tlist, root);<br> We could probably push the grouping/sorting down to the parallel workers. But<br> again not part of this patchset.<br> <span class="HOEnZb"><font color="#888888"><br> --<br> Best Wishes,<br> Ashutosh Bapat<br> </font></span><div class="HOEnZb"><div class="h5">EnterpriseDB Corporation<br> The Postgres Database Company<br> </div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Robert Haas<br>EnterpriseDB: <a href="http://www.enterprisedb.com" target="_blank">http://www.enterprisedb.com</a><br>The Enterprise PostgreSQL Company</div> </div>
Вложения
On Fri, Mar 9, 2018 at 1:28 AM, Robert Haas <robertmhaas@gmail.com> wrote: > >> 0003 >> Probably we want to rename generate_union_path() as generate_union_rel() or >> generate_union_paths() since the function doesn't return a path anymore. >> Similarly for generate_nonunion_path(). > > Good point. Changed. It looks like it was not changed in all the places. make falied. I have fixed all the instances of these two functions in the attached patchset (only 0003 changes). Please check. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
On Tue, Mar 13, 2018 at 12:35 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > It looks like it was not changed in all the places. make falied. I > have fixed all the instances of these two functions in the attached > patchset (only 0003 changes). Please check. Oops. Thanks. I'm going to go ahead and commit 0001 here. Any more thoughts on the rest? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 14, 2018 at 2:09 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Mar 13, 2018 at 12:35 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> It looks like it was not changed in all the places. make falied. I >> have fixed all the instances of these two functions in the attached >> patchset (only 0003 changes). Please check. > > Oops. Thanks. > > I'm going to go ahead and commit 0001 here. Any more thoughts on the rest? Nope. I am good with the patchset. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Fri, Mar 9, 2018 at 1:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Great. Committed 0001. Are you planning any further testing of this
patch series?
Sorry I missed the mail.
Yes, I have further tested patches and find no more issues.
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Fri, Mar 16, 2018 at 7:35 AM, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote: > On Fri, Mar 9, 2018 at 1:04 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> Great. Committed 0001. Are you planning any further testing of this >> patch series? > > Sorry I missed the mail. > Yes, I have further tested patches and find no more issues. OK, thanks to both you and Ashutosh Bapat. Committed 0002 and 0003. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 19, 2018 at 11:57 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Mar 16, 2018 at 7:35 AM, Rajkumar Raghuwanshi > <rajkumar.raghuwanshi@enterprisedb.com> wrote: >> On Fri, Mar 9, 2018 at 1:04 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> Great. Committed 0001. Are you planning any further testing of this >>> patch series? >> >> Sorry I missed the mail. >> Yes, I have further tested patches and find no more issues. > > OK, thanks to both you and Ashutosh Bapat. Committed 0002 and 0003. And now committed 0004. This area could use significantly more work, but I think it's better to have this much than not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company