On 04.10.2018 06:19, David Rowley wrote:
> On 12 September 2018 at 08:32, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Also the patch proposed by you is much simple and does mostly the same. Yes,
>> it is not covering CHECK constraints,
> I started to look at this and found a problem in regards to varno
> during the predicate_implied_by() test. The problem is that the
> partition bound is always stored as varno=1 (For example, see how
> get_qual_for_list() calls makeVar()). This causes the patch to fail in
> cases where the partitioned table is not varno=1. You're also
> incorrectly using rinfo->clause to pass to predicate_implied_by().
> This is a problem because stored here have not been translated to have
> the child varattnos. childqual is the correct thing to use as that's
> just been translated. You may have not used it as the varnos will have
> been converted to the child's varno, which will never be varno=1, so
> you might have found that not to work due to the missing code to
> change the varnos to 1.
>
> I've attached the diff for allpaths.c (only) that I ended up with to
> make it work. This causes the output of many other regression test to
> change, so you'll need to go over these and verify everything is
> correct again.
>
> Please, can you also add a test which tests this code which has a
> partition with columns in a different order than it's parent. Having
> an INT and a TEXT column is best as if the translations are done
> incorrectly it's likely to result in a crash which will alert us to
> the issue. It would be good to also verify the test causes a crash if
> you temporarily put the code back to using the untranslated qual.
>
> Thanks for working on this.
>
Thank you very much for detecting and fixing this problem.
I have checked that all changes in plan caused by this fix are correct.
Updated version of the patch is attached.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company