Re: BUG #17540: Prepared statement: PG switches to a generic query plan which is consistently much slower

On Wed, Nov 29, 2023 at 8:48 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> I rebased your v7 patch on top of 930d2b442 and updated the expected
> results of some new regression tests which now have their NullTest
> clauses removed.

Thanks for your rebase.

> On looking deeper, I see you're overwriting the rinfo_serial of the
> const-false RestrictInfo with the one from the original RestrictInfo.
> If that's the correct thing to do then the following comment would
> need to be updated to mention this exception of why the rinfo_serial
> isn't unique.

Right, that's what we need to do.

> Looking at the tests, I see:
>
> select * from pred_tab t1 left join pred_tab t2 on true left join
> pred_tab t3 on t2.a is null;
>
> I'm wondering if you can come up with a better test for this? I don't
> quite see any reason why varnullingrels can't be empty for t2.a in the
> join qual as the "ON true" join condition between t1 and t2 means that
> there shouldn't ever be any NULL t2.a rows. My thoughts are that if
> we improve how varnullingrels are set in the future then this test
> will be broken.
>
> Also, I also like to write exactly what each test is testing so that
> it's easier in the future to maintain the expected results. It's
> often tricky when making planner changes to know if some planner
> changes makes a test completely useless or if the expected results
> just need to be updated. If someone changes varnullingrels to be
> empty for this case, then if they accept the actual results as
> expected results then the test becomes useless. I tend to do this
> with comments in the .sql file along the lines of "-- Ensure ..."
>
> I also would rather see the SQLs in the test wrap their lines before
> each join and the keywords to be upper case.

Thanks for the suggestions on the tests. I had a go at improving the
test queries and their comments.

BTW, I changed the subject of this patch to 'Reduce NullTest quals to
constant TRUE or FALSE', which seems more accurate to me, because this
patch also reduces IS NULL clauses to constant-FALSE when applicable, in
addition to ignoring redundant NOT NULL clauses.

Thanks
Richard