Re: Inconsistent RestrictInfo serial numbers

On Fri, Oct 11, 2024 at 3:02 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> 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?

Well, the resetting of the last_rinfo_serial counter in
deconstruct_distribute_oj_quals is to ensure that multiple clones of
the same qual receive the same serial number. As the comment there
explains, it works only if we ensure that we are not changing the qual
list in any way that'd affect the number of RestrictInfos built from
it.

However, in b262ad440, we did not get this memo promptly. As I
explained earlier, the has_clone version of qual 't1.a is null'
would be reduced to constant-FALSE, while the is_clone version would
not. That is to say, for the has_clone version, we'd build three
RestrictInfos, whereas for the is_clone version, we'd build only two.
This discrepancy in numbers is the root cause of the issue.

So, resetting last_rinfo_serial in deconstruct_distribute_oj_quals()
is not 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.

I don't see how this change would prevent add_base_clause_to_rel and
add_join_clause_to_rels from being used in other context than
deconstruct_distribute_oj_quals. Could you please provide an example?

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

Hmm, I don't think that's necessary. As long as we'd build the same
number of RestrictInfos from the qual list for different versions,
we're good.

> 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.

Would you mind proposing a patch for this method so we can discuss the
details?

Thanks
Richard