Re: Removing unneeded self joins

On Mon, Jun 17, 2024 at 3:00 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> On Mon, May 6, 2024 at 11:55 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > I want to go on record right now as disagreeing with the plan proposed
> > > in the commit message for the revert commit, namely, committing this
> > > again early in the v18 cycle. I don't think Tom would have proposed
> > > reverting this feature unless he believed that it had more serious
> > > problems than could be easily fixed in a short period of time. I think
> > > that concern is well-founded, given the number of fixes that were
> > > committed. It seems likely that the patch needs significant rework and
> > > stabilization before it gets committed again, and I think it shouldn't
> > > be committed again without explicit agreement from Tom or one of the
> > > other committers who have significant experience with the query
> > > planner.
> >
> > FWIW I accept some of the blame here, for not having paid any
> > attention to the SJE work earlier. I had other things on my mind
> > for most of last year, and not enough bandwidth to help.
> >
> > The main thing I'd like to understand before we try this again is
> > why SJE needed so much new query-tree-manipulation infrastructure.
> > I would have expected it to be very similar to the left-join
> > elimination we do already, and therefore to mostly just share the
> > existing infrastructure. (I also harbor suspicions that some of
> > the new code existed just because someone didn't research what
> > was already there --- for instance, the now-removed replace_varno
> > sure looks like ChangeVarNodes should have been used instead.)
> >
>
> i have looked around the code.
> about replace_varno and ChangeVarNodes:
>
> ChangeVarNodes
> have
> ````
> if (IsA(node, RangeTblRef))
> {
> RangeTblRef *rtr = (RangeTblRef *) node;
>
> if (context->sublevels_up == 0 &&
> rtr->rtindex == context->rt_index)
> rtr->rtindex = context->new_index;
> /* the subquery itself is visited separately */
> return false;
> }
> ````
> if ChangeVarNodes executed the above code in remove_useless_self_joins and
> remove_self_joins_recurse. the joinlist(RangeTblRef) will change from (1,2)
> to (2,2). then later, remove_rel_from_joinlist cannot remove the 1,
> *nremoved will be zero.
> then the below code error branch will be executed.
> ````
> joinlist = remove_rel_from_joinlist(joinlist, relid, &nremoved);
> if (nremoved != 1)
> elog(ERROR, "failed to find relation %d in joinlist", relid);
> ```

Did you manage to overcome this problem in your patch? If not, why do
regression tests pass while this seems to affect pretty much every
self-join removal? If so, how did you do that?

>
> ---------------------------------------------------------------------
> replace_varno and replace_varno_walker didn't replace
> Query->resultRelation, Query->mergeTargetRelation
> as ChangeVarNodes did.
>
> then replace_varno will have problems with DELETE, UPDATE, MERGE
> someway.
> ChangeVarNodes solved this problem.
>
>
> > Another thing that made me pretty sad was 8c441c082 (Forbid SJE with
> > result relation). While I don't claim that that destroyed the entire
> > use case for SJE, it certainly knocked its usefulness down by many
> > notches, maybe even to the point where it's not worth putting in the
> > effort needed to get it to re-committability. So I think we need to
> > look harder at finding a way around that. Is the concern that
> > RETURNING should return either old or new values depending on which
> > RTE is mentioned? If so, maybe the feature Dean has proposed to
> > allow RETURNING to access old values [1] is a prerequisite to moving
> > forward. Alternatively, perhaps it'd be good enough to forbid SJE
> > only when the non-target relation is actually mentioned in RETURNING.
> >
> > regards, tom lane
> >
> > [1] https://www.postgresql.org/message-id/flat/CAEZATCWx0J0-v=Qjc6gXzR=KtsdvAE7Ow=D=mu50AgOe+pvisQ(at)mail(dot)gmail(dot)com
>
> if only SELECT, no worth to make it being committed,
> do you think support DML but no support RETURNING worth the effort?

It appears you didn't try to address the EPQ problem, which seems to
me even more serious than the RETURNING problem.

See the following example.

Session 1
# create table test (id int primary key, val int);
# insert into test values (1,1);
# begin;
# update test set val = val + 1 where id = 1;

Session 2
# update test set val = t.val + 1 from test t where test.id = t.id;
(wait)

Session 1
# commit;

With v3 patch the query of session 2 fails on assert even before
starting to wait for the tuple lock. But even if we fix that, I
expect that after SJE this example would result in val = 3. Without
SJE, it would result with val = 2, because during EPQ alias t still
references the row version read according to the snapshot. In order
to overcome that we need to distinguish Var, which points to the
latest version during EPQ, and Var, which points to the snapshot
version during EPQ. Probably I'm wrong, but this change seems to have
quite terrific complexity.

The way to workaround that could be to apply SJE for target relations
only on REPEATABLE READ or SERIALIZABLE. But that would require
introducing dependency of query planning on the isolation level. I'm
not quite happy with this already. Also, this would lead to very
non-obvious user-visible gotcha that data-modification queries have
better performance on higher isolation levels.

BTW, I don't think SJE for just SELECTs doesn't make sense. For me,
it seems reasonable to take a step-by-step approach to first nail down
SJE just for SELECTs (that looks enough challenge for a single
release). And then attack the problems we have with data-modification
queries.

Links.
1. https://www.postgresql.org/message-id/flat/2285d5d0-f330-e8b6-9ee5-b2b90e44409b%40postgrespro.ru#4a665faeb7cca32979d7e389fe89c97c

------
Regards,
Alexander Korotkov
Supabase