On Thu, Dec 28, 2023 at 6:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Richard Guo <guofenglinux@gmail.com> writes:
> > I've looked into it a bit. The problem lies in how the SJE code handles
> > the transfer of qual clauses from the removed relation to the remaining
> > one.
>
> I am definitely starting to think that the SJE patch was not ready
> for prime time. We keep finding not only minor but major problems
> in it --- I'd call this one a "major" one. Is it time to revert
> and rethink it from scratch?
First, I'd like to admit that the SJE patch contained a set of
oversights, some of them were very unnecessary ones (like putting
non-node into RelOptInfo.unique_for_rels).
The distinction between minor and major issues is often a matter of
perspective. In the case of the SJE patch, I believe the issues yet
spotted are localized and do not necessitate a complete redesign of
the feature. This localized nature of the problems suggests that
targeted fixes could be sufficient to address the current concerns.
In terms of effort, the bitmapset issue was the toughest for me yet.
But I would have to say that this issue persisted in the code before.
As a result of the committed fixes, now we have the instrumentation to
detect problems like this in the future.
The integration of the SJE feature with the optimizer's data structure
and algorithms indeed presents a high risk of conflicts with other
patches. The experience with nullable vars is a testament to this
challenge. It's a delicate balance to maintain, and I recognize the
difficulties in predicting and managing such conflicts.
I am not advocating for the SJE feature unconditionally. I am fully
prepared to reconsider its implementation if a fundamental redesign
becomes necessary. However, at this stage, I believe that retaining
and refining the feature is more beneficial than a complete rollback.
Despite its imperfections, the SJE patch holds significant potential.
> > If we determine that avoiding duplicates is necessary, I think at least
> > we should compare the entire RestrictInfos not just their clauses. One
> > challenge with this approach is that the 'rinfo_serial' usually differs,
> > making direct comparison problematic. I'm wondering if we can make
> > 'rinfo_serial' equal_ignore. Not too sure about that.
>
> I'd say that that will break the cases rinfo_serial was introduced for.
> Now, I certainly don't love rinfo_serial and would be happier if we
> could do without it, but getting rid of it is another research project.
It's a pity that no regression tests detect that. The attached patch
implements the special comparison function, which ignores the
'rinfo_serial' field. This avoids marking 'rinfo_serial' as
pg_node_attr(equal_ignore).
Links.
1. https://www.postgresql.org/message-id/CAMbWs4_wJthNtYBL%2BSsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw%40mail.gmail.com
------
Regards,
Alexander Korotkov