Skip to content

Commit 5ee190f

Browse files
tglsfdctglsfdc
committed
Rationalize use of list_concat + list_copy combinations.
In the wake of commit 1cff1b9, the result of list_concat no longer shares the ListCells of the second input. Therefore, we can replace "list_concat(x, list_copy(y))" with just "list_concat(x, y)". To improve call sites that were list_copy'ing the first argument, or both arguments, invent "list_concat_copy()" which produces a new list sharing no ListCells with either input. (This is a bit faster than "list_concat(list_copy(x), y)" because it makes the result list the right size to start with.) In call sites that were not list_copy'ing the second argument, the new semantics mean that we are usually leaking the second List's storage, since typically there is no remaining pointer to it. We considered inventing another list_copy variant that would list_free the second input, but concluded that for most call sites it isn't worth worrying about, given the relative compactness of the new List representation. (Note that in cases where such leakage would happen, the old code already leaked the second List's header; so we're only discussing the size of the leak not whether there is one. I did adjust two or three places that had been troubling to free that header so that they manually free the whole second List.) Patch by me; thanks to David Rowley for review. Discussion: https://postgr.es/m/11587.1550975080@sss.pgh.pa.us
1 parent 251c8e3 commit 5ee190f

File tree

26 files changed

+130
-148
lines changed

26 files changed

+130
-148
lines changed

‎contrib/postgres_fdw/deparse.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1497,7 +1497,7 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
14971497
if (fpinfo->jointype == JOIN_INNER)
14981498
{
14991499
*ignore_conds = list_concat(*ignore_conds,
1500-
list_copy(fpinfo->joinclauses));
1500+
fpinfo->joinclauses);
15011501
fpinfo->joinclauses = NIL;
15021502
}
15031503

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2667,7 +2667,7 @@ estimate_path_cost_size(PlannerInfo *root,
26672667
* baserestrictinfo plus any extra join_conds relevant to this
26682668
* particular path.
26692669
*/
2670-
remote_conds = list_concat(list_copy(remote_param_join_conds),
2670+
remote_conds = list_concat(remote_param_join_conds,
26712671
fpinfo->remote_conds);
26722672

26732673
/*
@@ -5102,23 +5102,23 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
51025102
{
51035103
case JOIN_INNER:
51045104
fpinfo->remote_conds = list_concat(fpinfo->remote_conds,
5105-
list_copy(fpinfo_i->remote_conds));
5105+
fpinfo_i->remote_conds);
51065106
fpinfo->remote_conds = list_concat(fpinfo->remote_conds,
5107-
list_copy(fpinfo_o->remote_conds));
5107+
fpinfo_o->remote_conds);
51085108
break;
51095109

51105110
case JOIN_LEFT:
51115111
fpinfo->joinclauses = list_concat(fpinfo->joinclauses,
5112-
list_copy(fpinfo_i->remote_conds));
5112+
fpinfo_i->remote_conds);
51135113
fpinfo->remote_conds = list_concat(fpinfo->remote_conds,
5114-
list_copy(fpinfo_o->remote_conds));
5114+
fpinfo_o->remote_conds);
51155115
break;
51165116

51175117
case JOIN_RIGHT:
51185118
fpinfo->joinclauses = list_concat(fpinfo->joinclauses,
5119-
list_copy(fpinfo_o->remote_conds));
5119+
fpinfo_o->remote_conds);
51205120
fpinfo->remote_conds = list_concat(fpinfo->remote_conds,
5121-
list_copy(fpinfo_i->remote_conds));
5121+
fpinfo_i->remote_conds);
51225122
break;
51235123

51245124
case JOIN_FULL:

‎src/backend/commands/indexcmds.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,8 @@ DefineIndex(Oid relationId,
516516
* is part of the key columns, and anything equal to and over is part of
517517
* the INCLUDE columns.
518518
*/
519-
allIndexParams = list_concat(list_copy(stmt->indexParams),
520-
list_copy(stmt->indexIncludingParams));
519+
allIndexParams = list_concat_copy(stmt->indexParams,
520+
stmt->indexIncludingParams);
521521
numberOfAttributes = list_length(allIndexParams);
522522

523523
if (numberOfAttributes <= 0)

‎src/backend/executor/functions.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -719,8 +719,7 @@ init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
719719
fcache->pinfo,
720720
NULL);
721721
queryTree_list = lappend(queryTree_list, queryTree_sublist);
722-
flat_query_list = list_concat(flat_query_list,
723-
list_copy(queryTree_sublist));
722+
flat_query_list = list_concat(flat_query_list, queryTree_sublist);
724723
}
725724

726725
check_sql_fn_statements(flat_query_list);

‎src/backend/nodes/list.c

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -501,12 +501,15 @@ lcons_oid(Oid datum, List *list)
501501
}
502502

503503
/*
504-
* Concatenate list2 to the end of list1, and return list1. list1 is
505-
* destructively changed, list2 is not. (However, in the case of pointer
506-
* lists, list1 and list2 will point to the same structures.) Callers
507-
* should be sure to use the return value as the new pointer to the
508-
* concatenated list: the 'list1' input pointer may or may not be the
509-
* same as the returned pointer.
504+
* Concatenate list2 to the end of list1, and return list1.
505+
*
506+
* This is equivalent to lappend'ing each element of list2, in order, to list1.
507+
* list1 is destructively changed, list2 is not. (However, in the case of
508+
* pointer lists, list1 and list2 will point to the same structures.)
509+
*
510+
* Callers should be sure to use the return value as the new pointer to the
511+
* concatenated list: the 'list1' input pointer may or may not be the same
512+
* as the returned pointer.
510513
*/
511514
List *
512515
list_concat(List *list1, const List *list2)
@@ -534,6 +537,41 @@ list_concat(List *list1, const List *list2)
534537
return list1;
535538
}
536539

540+
/*
541+
* Form a new list by concatenating the elements of list1 and list2.
542+
*
543+
* Neither input list is modified. (However, if they are pointer lists,
544+
* the output list will point to the same structures.)
545+
*
546+
* This is equivalent to, but more efficient than,
547+
* list_concat(list_copy(list1), list2).
548+
* Note that some pre-v13 code might list_copy list2 as well, but that's
549+
* pointless now.
550+
*/
551+
List *
552+
list_concat_copy(const List *list1, const List *list2)
553+
{
554+
List *result;
555+
int new_len;
556+
557+
if (list1 == NIL)
558+
return list_copy(list2);
559+
if (list2 == NIL)
560+
return list_copy(list1);
561+
562+
Assert(list1->type == list2->type);
563+
564+
new_len = list1->length + list2->length;
565+
result = new_list(list1->type, new_len);
566+
memcpy(result->elements, list1->elements,
567+
list1->length * sizeof(ListCell));
568+
memcpy(result->elements + list1->length, list2->elements,
569+
list2->length * sizeof(ListCell));
570+
571+
check_list_invariants(result);
572+
return result;
573+
}
574+
537575
/*
538576
* Truncate 'list' to contain no more than 'new_size' elements. This
539577
* modifies the list in-place! Despite this, callers should use the

‎src/backend/optimizer/path/allpaths.c

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1266,7 +1266,7 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
12661266
if (rel->part_scheme)
12671267
rel->partitioned_child_rels =
12681268
list_concat(rel->partitioned_child_rels,
1269-
list_copy(childrel->partitioned_child_rels));
1269+
childrel->partitioned_child_rels);
12701270

12711271
/*
12721272
* Child is live, so add it to the live_childrels list for use below.
@@ -1347,9 +1347,8 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
13471347
component = root->simple_rel_array[relid];
13481348
Assert(component->part_scheme != NULL);
13491349
Assert(list_length(component->partitioned_child_rels) >= 1);
1350-
partrels =
1351-
list_concat(partrels,
1352-
list_copy(component->partitioned_child_rels));
1350+
partrels = list_concat(partrels,
1351+
component->partitioned_child_rels);
13531352
}
13541353

13551354
partitioned_rels = list_make1(partrels);
@@ -2048,8 +2047,7 @@ accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths)
20482047

20492048
if (!apath->path.parallel_aware || apath->first_partial_path == 0)
20502049
{
2051-
/* list_copy is important here to avoid sharing list substructure */
2052-
*subpaths = list_concat(*subpaths, list_copy(apath->subpaths));
2050+
*subpaths = list_concat(*subpaths, apath->subpaths);
20532051
return;
20542052
}
20552053
else if (special_subpaths != NULL)
@@ -2072,8 +2070,7 @@ accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths)
20722070
{
20732071
MergeAppendPath *mpath = (MergeAppendPath *) path;
20742072

2075-
/* list_copy is important here to avoid sharing list substructure */
2076-
*subpaths = list_concat(*subpaths, list_copy(mpath->subpaths));
2073+
*subpaths = list_concat(*subpaths, mpath->subpaths);
20772074
return;
20782075
}
20792076

‎src/backend/optimizer/path/costsize.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4443,8 +4443,7 @@ get_parameterized_baserel_size(PlannerInfo *root, RelOptInfo *rel,
44434443
* restriction clauses. Note that we force the clauses to be treated as
44444444
* non-join clauses during selectivity estimation.
44454445
*/
4446-
allclauses = list_concat(list_copy(param_clauses),
4447-
rel->baserestrictinfo);
4446+
allclauses = list_concat_copy(param_clauses, rel->baserestrictinfo);
44484447
nrows = rel->tuples *
44494448
clauselist_selectivity(root,
44504449
allclauses,

‎src/backend/optimizer/path/indxpath.c

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,7 @@ get_join_index_paths(PlannerInfo *root, RelOptInfo *rel,
656656
}
657657
}
658658

659-
/* Add restriction clauses (this is nondestructive to rclauseset) */
659+
/* Add restriction clauses */
660660
clauseset.indexclauses[indexcol] =
661661
list_concat(clauseset.indexclauses[indexcol],
662662
rclauseset->indexclauses[indexcol]);
@@ -1204,8 +1204,7 @@ build_paths_for_OR(PlannerInfo *root, RelOptInfo *rel,
12041204
{
12051205
/* Form all_clauses if not done already */
12061206
if (all_clauses == NIL)
1207-
all_clauses = list_concat(list_copy(clauses),
1208-
other_clauses);
1207+
all_clauses = list_concat_copy(clauses, other_clauses);
12091208

12101209
if (!predicate_implied_by(index->indpred, all_clauses, false))
12111210
continue; /* can't use it at all */
@@ -1270,7 +1269,7 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
12701269
* We can use both the current and other clauses as context for
12711270
* build_paths_for_OR; no need to remove ORs from the lists.
12721271
*/
1273-
all_clauses = list_concat(list_copy(clauses), other_clauses);
1272+
all_clauses = list_concat_copy(clauses, other_clauses);
12741273

12751274
foreach(lc, clauses)
12761275
{
@@ -1506,8 +1505,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths)
15061505
pathinfo = pathinfoarray[i];
15071506
paths = list_make1(pathinfo->path);
15081507
costsofar = bitmap_scan_cost_est(root, rel, pathinfo->path);
1509-
qualsofar = list_concat(list_copy(pathinfo->quals),
1510-
list_copy(pathinfo->preds));
1508+
qualsofar = list_concat_copy(pathinfo->quals, pathinfo->preds);
15111509
clauseidsofar = bms_copy(pathinfo->clauseids);
15121510

15131511
for (j = i + 1; j < npaths; j++)
@@ -1543,10 +1541,8 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths)
15431541
{
15441542
/* keep new path in paths, update subsidiary variables */
15451543
costsofar = newcost;
1546-
qualsofar = list_concat(qualsofar,
1547-
list_copy(pathinfo->quals));
1548-
qualsofar = list_concat(qualsofar,
1549-
list_copy(pathinfo->preds));
1544+
qualsofar = list_concat(qualsofar, pathinfo->quals);
1545+
qualsofar = list_concat(qualsofar, pathinfo->preds);
15501546
clauseidsofar = bms_add_members(clauseidsofar,
15511547
pathinfo->clauseids);
15521548
}
@@ -1849,7 +1845,7 @@ find_indexpath_quals(Path *bitmapqual, List **quals, List **preds)
18491845

18501846
*quals = lappend(*quals, iclause->rinfo->clause);
18511847
}
1852-
*preds = list_concat(*preds, list_copy(ipath->indexinfo->indpred));
1848+
*preds = list_concat(*preds, ipath->indexinfo->indpred);
18531849
}
18541850
else
18551851
elog(ERROR, "unrecognized node type: %d", nodeTag(bitmapqual));

‎src/backend/optimizer/plan/createplan.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -559,8 +559,8 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
559559
* For paranoia's sake, don't modify the stored baserestrictinfo list.
560560
*/
561561
if (best_path->param_info)
562-
scan_clauses = list_concat(list_copy(scan_clauses),
563-
best_path->param_info->ppi_clauses);
562+
scan_clauses = list_concat_copy(scan_clauses,
563+
best_path->param_info->ppi_clauses);
564564

565565
/*
566566
* Detect whether we have any pseudoconstant quals to deal with. Then, if

‎src/backend/optimizer/plan/initsplan.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -973,7 +973,6 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
973973
*postponed_qual_list = lappend(*postponed_qual_list, pq);
974974
}
975975
}
976-
/* list_concat is nondestructive of its second argument */
977976
my_quals = list_concat(my_quals, (List *) j->quals);
978977

979978
/*

‎src/backend/optimizer/plan/planner.c

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3572,10 +3572,6 @@ reorder_grouping_sets(List *groupingsets, List *sortclause)
35723572
}
35733573
}
35743574

3575-
/*
3576-
* Safe to use list_concat (which shares cells of the second arg)
3577-
* because we know that new_elems does not share cells with anything.
3578-
*/
35793575
previous = list_concat(previous, new_elems);
35803576

35813577
gs->set = list_copy(previous);
@@ -4287,8 +4283,8 @@ consider_groupingsets_paths(PlannerInfo *root,
42874283
*/
42884284
if (!rollup->hashable)
42894285
return;
4290-
else
4291-
sets_data = list_concat(sets_data, list_copy(rollup->gsets_data));
4286+
4287+
sets_data = list_concat(sets_data, rollup->gsets_data);
42924288
}
42934289
foreach(lc, sets_data)
42944290
{
@@ -4473,7 +4469,7 @@ consider_groupingsets_paths(PlannerInfo *root,
44734469
{
44744470
if (bms_is_member(i, hash_items))
44754471
hash_sets = list_concat(hash_sets,
4476-
list_copy(rollup->gsets_data));
4472+
rollup->gsets_data);
44774473
else
44784474
rollups = lappend(rollups, rollup);
44794475
++i;
@@ -5642,8 +5638,7 @@ make_pathkeys_for_window(PlannerInfo *root, WindowClause *wc,
56425638
errdetail("Window ordering columns must be of sortable datatypes.")));
56435639

56445640
/* Okay, make the combined pathkeys */
5645-
window_sortclauses = list_concat(list_copy(wc->partitionClause),
5646-
list_copy(wc->orderClause));
5641+
window_sortclauses = list_concat_copy(wc->partitionClause, wc->orderClause);
56475642
window_pathkeys = make_pathkeys_for_sortclauses(root,
56485643
window_sortclauses,
56495644
tlist);

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -893,7 +893,7 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
893893
splan->resultRelIndex = list_length(root->glob->resultRelations);
894894
root->glob->resultRelations =
895895
list_concat(root->glob->resultRelations,
896-
list_copy(splan->resultRelations));
896+
splan->resultRelations);
897897

898898
/*
899899
* If the main target relation is a partitioned table, also

‎src/backend/optimizer/prep/prepjointree.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2659,7 +2659,6 @@ reduce_outer_joins_pass2(Node *jtnode,
26592659
pass_nonnullable_rels = find_nonnullable_rels(f->quals);
26602660
pass_nonnullable_rels = bms_add_members(pass_nonnullable_rels,
26612661
nonnullable_rels);
2662-
/* NB: we rely on list_concat to not damage its second argument */
26632662
pass_nonnullable_vars = find_nonnullable_vars(f->quals);
26642663
pass_nonnullable_vars = list_concat(pass_nonnullable_vars,
26652664
nonnullable_vars);

‎src/backend/optimizer/prep/prepqual.c

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,6 @@ pull_ands(List *andlist)
328328
{
329329
Node *subexpr = (Node *) lfirst(arg);
330330

331-
/*
332-
* Note: we can destructively concat the subexpression's arglist
333-
* because we know the recursive invocation of pull_ands will have
334-
* built a new arglist not shared with any other expr. Otherwise we'd
335-
* need a list_copy here.
336-
*/
337331
if (is_andclause(subexpr))
338332
out_list = list_concat(out_list,
339333
pull_ands(((BoolExpr *) subexpr)->args));
@@ -360,12 +354,6 @@ pull_ors(List *orlist)
360354
{
361355
Node *subexpr = (Node *) lfirst(arg);
362356

363-
/*
364-
* Note: we can destructively concat the subexpression's arglist
365-
* because we know the recursive invocation of pull_ors will have
366-
* built a new arglist not shared with any other expr. Otherwise we'd
367-
* need a list_copy here.
368-
*/
369357
if (is_orclause(subexpr))
370358
out_list = list_concat(out_list,
371359
pull_ors(((BoolExpr *) subexpr)->args));

0 commit comments

Comments
 (0)