Re: BUG #18568: BUG: Result wrong when do group by on partition table! - Mailing list pgsql-bugs

On Tue, Nov 5, 2024 at 4:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Oct 24, 2024 at 11:04 AM Tender Wang <tndrwang@gmail.com> wrote:
> > jian he <jian.universality@gmail.com> 于2024年10月23日周三 22:18写道：
> >>
> >> On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang@gmail.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
whatyou 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