Horton

Re: Removing unneeded self joins

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Richard Guo <guofenglinux@gmail.com>, Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Alexander Lakhin <exclusion@gmail.com>, "Gregory Stark (as CFM)" <stark.cfm@gmail.com>, Michał Kłeczek <michal@kleczek.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2024-05-03T08:56:32Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove GUC_NOT_IN_SAMPLE from enable_self_join_elimination

  2. Put enable_self_join_elimination into postgresql.conf.sample

  3. Get rid of ojrelid local variable in remove_rel_from_query()

  4. Implement Self-Join Elimination

  5. Revert: Remove useless self-joins

  6. Replace lateral references to removed rels in subqueries

  7. Replace relids in lateral subquery parse tree during SJE

  8. Forbid SJE with result relation

  9. Fix misuse of RelOptInfo.unique_for_rels cache by SJE

  10. Replace the relid in some missing fields during SJE

  11. Revert 56-bit relfilenode change and follow-up commits.

  12. Stabilize timetz test across DST transitions.

  13. Speed up finding EquivalenceClasses for a given set of rels

  14. Fix mark-and-restore-skipping test case to not be a self-join.

Hi, Tom!

On Fri, May 3, 2024 at 2:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > I would appreciate your review of this patchset, and review from Andrei as well.
>
> I hate to say this ... but if we're still finding bugs this
> basic in SJE, isn't it time to give up on it for v17?
>
> I might feel better about it if there were any reason to think
> these were the last major bugs.  But you have already committed
> around twenty separate fixes for the original SJE patch, and
> now here you come with several more; so it doesn't seem like
> the defect rate has slowed materially.  There can be no doubt
> whatever that the original patch was far from commit-ready.

I think if we subtract from the SJE followup commits containing
improvements (extra comments, asserts) and fix for in-place Bitmapset
modification, which was there before, the number of fixes will be
closer to ten.  And the number of pending fixes will be two.  But I
totally get your concern that we're quite late in the release cycle
and new SJE-related issues continue to arise.  This leads to a
significant risk of raising many bugs for end users.

> I think we should revert SJE for v17 and do a thorough design
> review before trying again in v18.

I agree to revert it for v17, but I'm not exactly sure the issue is
design (nevertheless design review is very welcome as any other type
of review).  The experience of the bugs arising with the SJE doesn't
show me a particular weak spot in the feature.  It looks more like
this patch has to revise awfully a lot planner data structures to
replace one relid with another.  And I don't see the design, which
could avoid that.  Somewhere in the thread I have proposed a concept
of "alias relids".  However, I suspect that could leave us with more
lurking bugs instead of the bug-free code.

I suggest we should give this feature more review and testing, then
commit early v18.  That would leave us enough time to fix any other
issues before v18 release.

------
Regards,
Alexander Korotkov
Supabase