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

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 what
yousaid 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].

--
Thanks, Amit Langote