Re: BUG #18568: BUG: Result wrong when do group by on partition table!

On Tue, Nov 5, 2024 at 4:57 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Oct 24, 2024 at 11:04 AM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> > jian he <jian(dot)universality(at)gmail(dot)com> 于2024年10月23日周三 22:18写道：
> >>
> >> On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> >> >
> >> > I tried the patch I provided in [1], and the regression test cases all passed.
> >> >
> >>
> >> ////////////////////////
> >> ComputePartitionAttrs code snippet
> >> ELSE
> >> {
> >> /* Expression */
> >> Node *expr = pelem->expr;
> >> char partattname[16];
> >> Assert(expr != NULL);
> >> atttype = exprType(expr);
> >> attcollation = exprCollation(expr);
> >> }
> >> /*
> >> * Apply collation override if any
> >> */
> >> if (pelem->collation)
> >> attcollation = get_collation_oid(pelem->collation, false);
> >> partcollation[attn] = attcollation;
> >> ////////////////////////
> >>
> >> create table coll_pruning_multi (a text) partition by range (substr(a,
> >> 1) collate "POSIX", substr(a, 1) collate "C");
> >> PartitionElem->expr only cover "substr(a,1)".
> >> PartitionElem->collation is for explicitly COLLATION clauses.
> >> you can also see
> >> https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L4556
> >>
> >> From the above "collation override" comments, we can say
> >> exprCollation(PartitionElem->expr)
> >> does not always equal PartitionElem->collation
> >> PartitionElem->collation is the true collation OID.
> >>
> >> so you change in but didn't cover the ELSE branch.
> >>
> >> else
> >> {
> >> if (lc == NULL)
> >> elog(ERROR, "wrong number of partition key expressions");
> >> /* Re-stamp the expression with given varno. */
> >> partexpr = (Expr *) copyObject(lfirst(lc));
> >> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> >> lc = lnext(partkey->partexprs, lc);
> >> }
> >>
> >> as you mentioned partkey->partcollation is correct collation for PartitionKey.
> >> but the ELSE branch, we cannot do
> >> else
> >> {
> >> if (lc == NULL)
> >> elog(ERROR, "wrong number of partition key expressions");
> >> /* Re-stamp the expression with given varno. */
> >> partexpr = (Expr *) copyObject(lfirst(lc));
> >> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> >> exprSetCollation(Node *partexpr, Oid collation)
> >> lc = lnext(partkey->partexprs, lc);
> >> }
> >>
> >> because in struct inPartitionElem, collation and expr is seperated.
> >> that means after set_baserel_partition_key_exprs
> >> we still cannot be sure that RelOptInfo->partexprs have the correct
> >> PartitionKey collation information.
> >
> >
> > Yeah, you're right. I confirm this again. In set_baserel_partition_key_exprs(),
> > we copy partkey->partexprs not including partcollation, if it is not simple column reference.
> >
> > So I think how we can fix this thread issue and the [1] I reported by me using a uniform solution.
> > By the way, I re-started a new thread [2] to track the issue I found in [1]. I will reply to an email reflecting what you said here and cc you.
> >
> > [1] https://www.postgresql.org/message-id/CAHewXNnyWUEmdHrRK3yg4k2TzSbb5WnkKLWxyO%2BOVZPhPFX7ew%40mail.gmail.com
> >
> > [2] https://www.postgresql.org/message-id/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr%2BPS2Dwg%40mail.gmail.com
>
> FTR, the patch to fix the bug reported here is being discussed at [2].

I've pushed the fix for this issue to all the branches down to 12.

Thanks for the report and the analyses.

--
Thanks, Amit Langote