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

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

--
Thanks, Amit Langote