Skip to content

Commit e167191

Browse files
akorotkovakorotkov
committed
Get rid of ojrelid local variable in remove_rel_from_query()
As spotted by Coverity, the calculation of ojrelid mixes signed and unsigned types causes possible overflow and undefined behavior. Instead of trying to fix the expression, this commit eliminates the relied local variable. The explicit branching is used to replace the -1 value. That, in turn, requires changing the signature of the remove_rel_from_eclass() function. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/914330.1740330169%40sss.pgh.pa.us Reviewed-by: Andrei Lepikhov <lepihov@gmail.com>
1 parent 55918f7 commit e167191

File tree

1 file changed

+53
-28
lines changed

1 file changed

+53
-28
lines changed

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

Lines changed: 53 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ static void remove_leftjoinrel_from_query(PlannerInfo *root, int relid,
5959
static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
6060
int relid, int ojrelid);
6161
static void remove_rel_from_eclass(EquivalenceClass *ec,
62-
int relid, int ojrelid, int subst);
62+
SpecialJoinInfo *sjinfo,
63+
int relid, int subst);
6364
static List *remove_rel_from_joinlist(List *joinlist, int relid, int *nremoved);
6465
static bool rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel);
6566
static bool rel_is_distinct_for(PlannerInfo *root, RelOptInfo *rel,
@@ -324,17 +325,22 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
324325
Relids joinrelids)
325326
{
326327
int relid = rel->relid;
327-
int ojrelid = (sjinfo != NULL) ? sjinfo->ojrelid : -1;
328328
Index rti;
329329
ListCell *l;
330330

331331
/*
332332
* Update all_baserels and related relid sets.
333333
*/
334334
root->all_baserels = adjust_relid_set(root->all_baserels, relid, subst);
335-
root->outer_join_rels = adjust_relid_set(root->outer_join_rels, ojrelid, subst);
336335
root->all_query_rels = adjust_relid_set(root->all_query_rels, relid, subst);
337-
root->all_query_rels = adjust_relid_set(root->all_query_rels, ojrelid, subst);
336+
337+
if (sjinfo != NULL)
338+
{
339+
root->outer_join_rels = bms_del_member(root->outer_join_rels,
340+
sjinfo->ojrelid);
341+
root->all_query_rels = bms_del_member(root->all_query_rels,
342+
sjinfo->ojrelid);
343+
}
338344

339345
/*
340346
* Likewise remove references from SpecialJoinInfo data structures.
@@ -366,22 +372,30 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
366372

367373
if (sjinfo != NULL)
368374
{
369-
Assert(subst <= 0 && ojrelid > 0);
370-
371-
/* Remove ojrelid bits from the sets: */
372-
sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, ojrelid);
373-
sjinf->min_righthand = bms_del_member(sjinf->min_righthand, ojrelid);
374-
sjinf->syn_lefthand = bms_del_member(sjinf->syn_lefthand, ojrelid);
375-
sjinf->syn_righthand = bms_del_member(sjinf->syn_righthand, ojrelid);
375+
Assert(subst <= 0);
376+
377+
/* Remove sjinfo->ojrelid bits from the sets: */
378+
sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand,
379+
sjinfo->ojrelid);
380+
sjinf->min_righthand = bms_del_member(sjinf->min_righthand,
381+
sjinfo->ojrelid);
382+
sjinf->syn_lefthand = bms_del_member(sjinf->syn_lefthand,
383+
sjinfo->ojrelid);
384+
sjinf->syn_righthand = bms_del_member(sjinf->syn_righthand,
385+
sjinfo->ojrelid);
376386
/* relid cannot appear in these fields, but ojrelid can: */
377-
sjinf->commute_above_l = bms_del_member(sjinf->commute_above_l, ojrelid);
378-
sjinf->commute_above_r = bms_del_member(sjinf->commute_above_r, ojrelid);
379-
sjinf->commute_below_l = bms_del_member(sjinf->commute_below_l, ojrelid);
380-
sjinf->commute_below_r = bms_del_member(sjinf->commute_below_r, ojrelid);
387+
sjinf->commute_above_l = bms_del_member(sjinf->commute_above_l,
388+
sjinfo->ojrelid);
389+
sjinf->commute_above_r = bms_del_member(sjinf->commute_above_r,
390+
sjinfo->ojrelid);
391+
sjinf->commute_below_l = bms_del_member(sjinf->commute_below_l,
392+
sjinfo->ojrelid);
393+
sjinf->commute_below_r = bms_del_member(sjinf->commute_below_r,
394+
sjinfo->ojrelid);
381395
}
382396
else
383397
{
384-
Assert(subst > 0 && ojrelid == -1);
398+
Assert(subst > 0);
385399

386400
ChangeVarNodes((Node *) sjinf->semi_rhs_exprs, relid, subst, 0);
387401
}
@@ -408,7 +422,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
408422
Assert(sjinfo == NULL || !bms_is_member(relid, phinfo->ph_lateral));
409423
if (bms_is_subset(phinfo->ph_needed, joinrelids) &&
410424
bms_is_member(relid, phinfo->ph_eval_at) &&
411-
(sjinfo == NULL || !bms_is_member(ojrelid, phinfo->ph_eval_at)))
425+
(sjinfo == NULL || !bms_is_member(sjinfo->ojrelid, phinfo->ph_eval_at)))
412426
{
413427
root->placeholder_list = foreach_delete_current(root->placeholder_list,
414428
l);
@@ -419,7 +433,9 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
419433
PlaceHolderVar *phv = phinfo->ph_var;
420434

421435
phinfo->ph_eval_at = adjust_relid_set(phinfo->ph_eval_at, relid, subst);
422-
phinfo->ph_eval_at = adjust_relid_set(phinfo->ph_eval_at, ojrelid, subst);
436+
if (sjinfo != NULL)
437+
phinfo->ph_eval_at = adjust_relid_set(phinfo->ph_eval_at,
438+
sjinfo->ojrelid, subst);
423439
Assert(!bms_is_empty(phinfo->ph_eval_at)); /* checked previously */
424440
/* Reduce ph_needed to contain only "relation 0"; see below */
425441
if (bms_is_member(0, phinfo->ph_needed))
@@ -437,7 +453,9 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
437453
/* ph_lateral might or might not be empty */
438454

439455
phv->phrels = adjust_relid_set(phv->phrels, relid, subst);
440-
phv->phrels = adjust_relid_set(phv->phrels, ojrelid, subst);
456+
if (sjinfo != NULL)
457+
phv->phrels = adjust_relid_set(phv->phrels,
458+
sjinfo->ojrelid, subst);
441459
Assert(!bms_is_empty(phv->phrels));
442460

443461
ChangeVarNodes((Node *) phv->phexpr, relid, subst, 0);
@@ -454,8 +472,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
454472
EquivalenceClass *ec = (EquivalenceClass *) lfirst(l);
455473

456474
if (bms_is_member(relid, ec->ec_relids) ||
457-
(sjinfo == NULL || bms_is_member(ojrelid, ec->ec_relids)))
458-
remove_rel_from_eclass(ec, relid, ojrelid, subst);
475+
(sjinfo == NULL || bms_is_member(sjinfo->ojrelid, ec->ec_relids)))
476+
remove_rel_from_eclass(ec, sjinfo, relid, subst);
459477
}
460478

461479
/*
@@ -671,7 +689,8 @@ remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
671689
}
672690

673691
/*
674-
* Remove any references to relid or ojrelid from the EquivalenceClass.
692+
* Remove any references to relid or sjinfo->ojrelid (if sjinfo != NULL)
693+
* from the EquivalenceClass.
675694
*
676695
* Like remove_rel_from_restrictinfo, we don't worry about cleaning out
677696
* any nullingrel bits in contained Vars and PHVs. (This might have to be
@@ -680,13 +699,16 @@ remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
680699
* level(s).
681700
*/
682701
static void
683-
remove_rel_from_eclass(EquivalenceClass *ec, int relid, int ojrelid, int subst)
702+
remove_rel_from_eclass(EquivalenceClass *ec, SpecialJoinInfo *sjinfo,
703+
int relid, int subst)
684704
{
685705
ListCell *lc;
686706

687707
/* Fix up the EC's overall relids */
688708
ec->ec_relids = adjust_relid_set(ec->ec_relids, relid, subst);
689-
ec->ec_relids = adjust_relid_set(ec->ec_relids, ojrelid, subst);
709+
if (sjinfo != NULL)
710+
ec->ec_relids = adjust_relid_set(ec->ec_relids,
711+
sjinfo->ojrelid, subst);
690712

691713
/*
692714
* Fix up the member expressions. Any non-const member that ends with
@@ -698,11 +720,14 @@ remove_rel_from_eclass(EquivalenceClass *ec, int relid, int ojrelid, int subst)
698720
EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc);
699721

700722
if (bms_is_member(relid, cur_em->em_relids) ||
701-
(ojrelid != -1 && bms_is_member(ojrelid, cur_em->em_relids)))
723+
(sjinfo != NULL && bms_is_member(sjinfo->ojrelid,
724+
cur_em->em_relids)))
702725
{
703726
Assert(!cur_em->em_is_const);
704727
cur_em->em_relids = adjust_relid_set(cur_em->em_relids, relid, subst);
705-
cur_em->em_relids = adjust_relid_set(cur_em->em_relids, ojrelid, subst);
728+
if (sjinfo != NULL)
729+
cur_em->em_relids = adjust_relid_set(cur_em->em_relids,
730+
sjinfo->ojrelid, subst);
706731
if (bms_is_empty(cur_em->em_relids))
707732
ec->ec_members = foreach_delete_current(ec->ec_members, lc);
708733
}
@@ -713,10 +738,10 @@ remove_rel_from_eclass(EquivalenceClass *ec, int relid, int ojrelid, int subst)
713738
{
714739
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
715740

716-
if (ojrelid == -1)
741+
if (sjinfo == NULL)
717742
ChangeVarNodes((Node *) rinfo, relid, subst, 0);
718743
else
719-
remove_rel_from_restrictinfo(rinfo, relid, ojrelid);
744+
remove_rel_from_restrictinfo(rinfo, relid, sjinfo->ojrelid);
720745
}
721746

722747
/*

0 commit comments

Comments
 (0)