Обсуждение: Fix a wrong comment in setrefs.c
I noticed a wrong comment in search_indexed_tlist_for_sortgroupref().
foreach(lc, itlist->tlist)
{
TargetEntry *tle = (TargetEntry *) lfirst(lc);
/* The equal() check should be redundant, but let's be paranoid */
if (tle->ressortgroupref == sortgroupref &&
equal(node, tle->expr))
{
It turns out that the equal() check is necessary, because the given
sort/group expression might be type of FuncExpr which converts integer
to numeric. In this case we need to modify its args not itself to
reference the matching subplan output expression. As an example,
consider
explain (costs off, verbose)
SELECT 1.1 AS two UNION (SELECT 2 UNION ALL SELECT 2);
QUERY PLAN
-------------------------------------
HashAggregate
Output: (1.1)
Group Key: (1.1)
-> Append
-> Result
Output: 1.1
-> Result
Output: (2)
-> Append
-> Result
Output: 2
-> Result
Output: 2
(13 rows)
If we remove the equal() check, this query would cause crash in
execution.
I'm considering changing the comment as below.
- /* The equal() check should be redundant, but let's be paranoid */
+ /*
+ * The equal() check is necessary, because expressions with the same
+ * sortgroupref might be different, e.g., the given sort/group
+ * expression can be type of FuncExpr which converts integer to
+ * numeric, and we need to modify its args not itself to reference the
+ * matching subplan output expression in this case.
+ */
Any thoughts?
Thanks
Richard
foreach(lc, itlist->tlist)
{
TargetEntry *tle = (TargetEntry *) lfirst(lc);
/* The equal() check should be redundant, but let's be paranoid */
if (tle->ressortgroupref == sortgroupref &&
equal(node, tle->expr))
{
It turns out that the equal() check is necessary, because the given
sort/group expression might be type of FuncExpr which converts integer
to numeric. In this case we need to modify its args not itself to
reference the matching subplan output expression. As an example,
consider
explain (costs off, verbose)
SELECT 1.1 AS two UNION (SELECT 2 UNION ALL SELECT 2);
QUERY PLAN
-------------------------------------
HashAggregate
Output: (1.1)
Group Key: (1.1)
-> Append
-> Result
Output: 1.1
-> Result
Output: (2)
-> Append
-> Result
Output: 2
-> Result
Output: 2
(13 rows)
If we remove the equal() check, this query would cause crash in
execution.
I'm considering changing the comment as below.
- /* The equal() check should be redundant, but let's be paranoid */
+ /*
+ * The equal() check is necessary, because expressions with the same
+ * sortgroupref might be different, e.g., the given sort/group
+ * expression can be type of FuncExpr which converts integer to
+ * numeric, and we need to modify its args not itself to reference the
+ * matching subplan output expression in this case.
+ */
Any thoughts?
Thanks
Richard
Вложения
Richard Guo <guofenglinux@gmail.com> writes:
> I noticed a wrong comment in search_indexed_tlist_for_sortgroupref().
> /* The equal() check should be redundant, but let's be paranoid */
> It turns out that the equal() check is necessary, because the given
> sort/group expression might be type of FuncExpr which converts integer
> to numeric.
Hmm. This kind of makes me itch, because in principle a ressortgroupref
identifier should uniquely identify a sorting/grouping column. If it
fails to do so here, maybe there are outright bugs lurking elsewhere?
I poked into it a little and determined that the problematic
ressortgroupref values are being assigned during prepunion.c,
which says
* By convention, all non-resjunk columns in a setop tree have
* ressortgroupref equal to their resno. In some cases the ref isn't
* needed, but this is a cleaner way than modifying the tlist later.
So at the end of that, we can have Vars in the upper relations'
targetlists that are associated by ressortgroupref with values
in the setop input relations' tlists, but don't match.
(You are right that added-on implicit coercions are one reason for
the expressions to be different, but it's not the only one.)
I'm inclined to write the comment more like "Usually the equal()
check is redundant, but in setop plans it may not be, since
prepunion.c assigns ressortgroupref equal to the column resno
without regard to whether that matches the topmost level's
sortgrouprefs and without regard to whether any implicit coercions
are added in the setop tree. We might have to clean that up someday;
but for now, just ignore any false matches."
regards, tom lane
On Tue, Sep 26, 2023 at 5:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmm. This kind of makes me itch, because in principle a ressortgroupref
identifier should uniquely identify a sorting/grouping column. If it
fails to do so here, maybe there are outright bugs lurking elsewhere?
I poked into it a little and determined that the problematic
ressortgroupref values are being assigned during prepunion.c,
which says
* By convention, all non-resjunk columns in a setop tree have
* ressortgroupref equal to their resno. In some cases the ref isn't
* needed, but this is a cleaner way than modifying the tlist later.
So at the end of that, we can have Vars in the upper relations'
targetlists that are associated by ressortgroupref with values
in the setop input relations' tlists, but don't match.
(You are right that added-on implicit coercions are one reason for
the expressions to be different, but it's not the only one.)
Ah. Thanks for the investigation.
I'm inclined to write the comment more like "Usually the equal()
check is redundant, but in setop plans it may not be, since
prepunion.c assigns ressortgroupref equal to the column resno
without regard to whether that matches the topmost level's
sortgrouprefs and without regard to whether any implicit coercions
are added in the setop tree. We might have to clean that up someday;
but for now, just ignore any false matches."
+1. It explains the situation much more clearly and accurately.
Thanks
Richard
Thanks
Richard
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation: not tested That looks correct for me The new status of this patch is: Ready for Committer
On Tue, Sep 26, 2023 at 9:51 AM Richard Guo <guofenglinux@gmail.com> wrote:
On Tue, Sep 26, 2023 at 5:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:I'm inclined to write the comment more like "Usually the equal()
check is redundant, but in setop plans it may not be, since
prepunion.c assigns ressortgroupref equal to the column resno
without regard to whether that matches the topmost level's
sortgrouprefs and without regard to whether any implicit coercions
are added in the setop tree. We might have to clean that up someday;
but for now, just ignore any false matches."+1. It explains the situation much more clearly and accurately.
To make it easier to review, I've updated the patch to be so.
Thanks
Richard
Thanks
Richard
Вложения
On 03/11/2023 08:10, Richard Guo wrote: > On Tue, Sep 26, 2023 at 9:51 AM Richard Guo <guofenglinux@gmail.com > <mailto:guofenglinux@gmail.com>> wrote: > > On Tue, Sep 26, 2023 at 5:45 AM Tom Lane <tgl@sss.pgh.pa.us > <mailto:tgl@sss.pgh.pa.us>> wrote: > > I'm inclined to write the comment more like "Usually the equal() > check is redundant, but in setop plans it may not be, since > prepunion.c assigns ressortgroupref equal to the column resno > without regard to whether that matches the topmost level's > sortgrouprefs and without regard to whether any implicit coercions > are added in the setop tree. We might have to clean that up > someday; > but for now, just ignore any false matches." > > > +1. It explains the situation much more clearly and accurately. > > To make it easier to review, I've updated the patch to be so. Committed, thanks! -- Heikki Linnakangas Neon (https://neon.tech)
On Tue, Nov 28, 2023 at 8:19 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 03/11/2023 08:10, Richard Guo wrote:
> On Tue, Sep 26, 2023 at 9:51 AM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
> On Tue, Sep 26, 2023 at 5:45 AM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
> I'm inclined to write the comment more like "Usually the equal()
> check is redundant, but in setop plans it may not be, since
> prepunion.c assigns ressortgroupref equal to the column resno
> without regard to whether that matches the topmost level's
> sortgrouprefs and without regard to whether any implicit coercions
> are added in the setop tree. We might have to clean that up
> someday;
> but for now, just ignore any false matches."
>
> +1. It explains the situation much more clearly and accurately.
>
> To make it easier to review, I've updated the patch to be so.
Committed, thanks!
Thanks for pushing!
Thanks
Richard
Thanks
Richard