Re: Removing unneeded self joins

On Sun, Feb 23, 2025 at 7:02 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
> > I've corrected some spelling error reported by Alexander Lakhin
> > privately to me. Also, I've revised comments around ChangeVarNodes()
> > and ChangeVarNodesExtended(). I'm going to continue nitpicking this
> > patch during next couple days then push it if no objections.
>
> Coverity has another nit-pick:
>
> /srv/coverity/git/pgsql-git/postgresql/src/backend/optimizer/plan/analyzejoins.c: 327 in remove_rel_from_query()
> 321 static void
> 322 remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
> 323 int subst, SpecialJoinInfo *sjinfo,
> 324 Relids joinrelids)
> 325 {
> 326 int relid = rel->relid;
> >>> CID 1643155: Integer handling issues (INTEGER_OVERFLOW)
> >>> Expression "ojrelid", which is equal to 4294967295, where "(sjinfo != NULL) ? sjinfo->ojrelid : 4294967295U" is known to be equal to 4294967295, overflows the type that receives it, a signed integer 32 bits wide.
> 327 int ojrelid = (sjinfo != NULL) ? sjinfo->ojrelid : -1;
> 328 Index rti;
> 329 ListCell *l;
> 330
> 331 /*
> 332 * Update all_baserels and related relid sets.
>
> This is unhappy because sjinfo->ojrelid, which is declared Index
> (that is, unsigned int) is being converted to int. Admittedly
> there are plenty of other places that do likewise, but the additional
> load of assuming that -1 isn't a possible value of sjinfo->ojrelid
> seems to be enough to draw its ire.

Thank you for reporting this!

> I suggest finding another way to code this function that doesn't
> depend on that type pun. I think it's fairly accidental that
> adjust_relid_set doesn't misbehave on -1 anyway, so personally I'd
> get rid of that local variable entirely in favor of something like
>
> if (sjinfo != NULL)
> {
> root->outer_join_rels = adjust_relid_set(root->outer_join_rels,
> sjinfo->ojrelid, subst);
> root->all_query_rels = adjust_relid_set(root->all_query_rels,
> sjinfo->ojrelid, subst);
> }

There is my attempt to implement this approach. Getting rid of local
variable (and computation of the same value other way) required to
change arguments of remove_rel_from_eclass() as well. I'm going to
further polish this tomorrow.

------
Regards,
Alexander Korotkov
Supabase