Re: Inconsistent RestrictInfo serial numbers

On Thu, Oct 10, 2024 at 7:37 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>
> > > > So may
> > > > be we should do this processing elsewhere and replace the original
> > > > clause itself?
> > >
> > > I’m not sure about this. Different versions of the same qual clause
> > > can lead to different conclusions about whether it can be reduced to
> > > constant-FALSE.
> >
> > Can you please provide an example?
>
> I think the query I provided in my initial email serves as an example.
> To quote what I said there:
>
> "
> In the query above, the has_clone version of qual 't1.a is null' would
> be reduced to constant-FALSE, while the is_clone version would not.
> "
>
> > > I don't think it is possible to replace the original
> > > clause; we need to do this processing on a version-by-version basis.
> > >
> >
> > Either way, each version will be represented by only one RestrictInfo.
> > Why can't we replace that version instead of creating a new
> > RestrictInfo?
>
> Sure we can do that. However, I find that manually altering a
> well-formed RestrictInfo's clause (along with its left_relids,
> right_relids, clause_relids, num_base_rels, etc.) seems too
> hacky to me.

That's fair.

1. What strikes me as odd in your patch is that the last_rinfo_serial
is reset so far away from the new clause that will be created with the
next last_rinfo_serial OR the clause whose clones are being created.
It either indicates another site where we are missing (possibly
future) to restore rinfo_serial in a clone OR reset of
last_serial_rinfo needs to happen somewhere else. Why isn't resetting
last_rinfo_serial in deconstruct_distribute_oj_quals() enough?

2. This change would also prevent add_base_clause_to_rel() and
add_join_clause_to_rels() from being used anywhere outside the context
of deconstruct_distribute_oj_quals() since these functions would reset
last_rinfo_serial when it's expected to be incremented. Moreover
another make_restrictinfo() call somewhere in the minions of
distribute_qual_to_rels() or distribute_quals_to_rels() would cause a
similar bug.

3. Do we need to reset last_serial_rinfo everywhere we reset
rinfo_serial e.g. create_join_clause()?

I suspect that b262ad440edecda0b1aba81d967ab560a83acb8a didn't do
anything wrong. It's effectively copying rinfo_serial of original
clause into the derived clause. I would have liked it better if it
would have used the same method as create_join_clause(). Some other
commti failed to notice the some minion of
distribute_quals_to_rels()->distribute_qual_to_rels() may increment
last_rinfo_serial while creating an alternate RestrictInfo for the
qual passed to distribute_qual_to_rels(). I think a robust fix is to
reset last_rinfo_serial in distribute_quals_to_rels() for every qual
and add an Assert(root->last_rinfo_serial == saved_last_rinfo_serial +
list_length(quals)) before resetting last_rinfo_serial in
deconstruct_distribute_oj_quals() to catch any future digressions.

--
Best Wishes,
Ashutosh Bapat