[PG19] Stale state after the join removal restart gives wrong results

master returns 29 for the first column, the answer is 3.
enable_self_join_elimination = off gives 3. 0001 clears the flag at
the top of that function.

master(wrong) returns 30, not 29.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

1. adjust_group_pathkeys_for_groupagg() sets aggpresorted on the
aggregates it picks and nothing clears it. When self join elimination
removes a rel and the second pass picks a different presorted group,
the stale flag makes the executor skip the sort.

Hmm. I've not looked at exactly what that function does, but just
going by the name, why is it being called inside the restart loop
rather than later? I feel like anything to do with identifying
pathkeys could/should wait till after join removal.

2. A removed rel keeps its tablesample. When the sample argument
refers to another rel that the second pass then removes,
ChangeVarNodes hits Assert(context->new_index != INVALID_VAR) in
rewriteManip.c. Non assert builds write INVALID_VAR into a dead rte
and seem fine. Richard looked at the rte fields in [2] and thought
tablesample was safe, this is the case that is not. 0002 clears
rte->tablesample next to securityQuals.

Good catch, although I kind of wonder why we're allowing lateral
refs in tablesample clauses in the first place.

regards, tom lane

I wrote:

shihao zhong <zhong950419@gmail.com> writes:

1. adjust_group_pathkeys_for_groupagg() sets aggpresorted on the
aggregates it picks and nothing clears it. When self join elimination
removes a rel and the second pass picks a different presorted group,
the stale flag makes the executor skip the sort.

Hmm. I've not looked at exactly what that function does, but just
going by the name, why is it being called inside the restart loop
rather than later? I feel like anything to do with identifying
pathkeys could/should wait till after join removal.

Concretely, the attached fixes the given test case and passes
check-world; but I've not tried harder than that to break it.

regards, tom lane

Hi Tom,

Tom Lane <tgl@sss.pgh.pa.us> writes:

Concretely, the attached fixes the given test case and passes
check-world; but I've not tried harder than that to break it.

I like this better than my 0001, and I could not break it. I kept
0001 small only because of where 19 is. If you think moving the call
is right for 19 and 18 too, I am fine with that.

I did a simple fuzz test generating 2200 random self join queries giving
the same results as 18.6 and the same plans as master. Tests pass
on REL_19_STABLE and REL_18_STABLE too.

Good catch, although I kind of wonder why we're allowing lateral
refs in tablesample clauses in the first place.

0002 is still needed, the Assert fires with only your patch.

I also hit one more of that kind. Self join elimination leaves the
removed rel's securityQuals in its rte, now pointing at the kept rel.
When a later pass removes the kept rel too, the same Assert fires.

create table r (k int primary key, v int);
alter table r enable row level security;
create policy p on r using (v > 0);
create table x (k int primary key);
create role u1;
grant select on r, x to u1;
set role u1;
select x.k from x left join (r a join r b on a.k = b.k) on x.k = a.k;

I will send a patch to address ^^^^^ later today.

Thanks,
Shihao

Hi Tom,

Thanks for committing 6b3f8857aa8. Both Asserts I mentioned still
fire on master, so here is v2.

0001 is my old 0002. It clears tablesample of a rel removed by outer
join removal. If you would rather forbid lateral refs in TABLESAMPLE,
I am happy to write that.
0001 would still be needed for the back branches.

0002 clears securityQuals of a rel removed by self join elimination.
The kept rel has the same quals because SJE requires the same
checkAsUser and the same row mark type, so RLS still filters.

0003 adds both cases to join.sql.

The tablesample Assert also fires on the REL_16_STABLE and
REL_17_STABLE heads, since 2ebf25e7d70 went back to 16.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

Thanks for committing 6b3f8857aa8. Both Asserts I mentioned still
fire on master, so here is v2.

Right, pushed. I concluded that the test cases probably weren't
worth the cycles forevermore, so didn't include them.

regards, tom lane