Re: Invalid query generated by postgres_fdw with UNION ALL and ORDER BY

On Fri, Mar 8, 2024 at 7:43 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Fri, 8 Mar 2024 at 00:54, Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 7, 2024 at 4:39 PM David Rowley <dgrowleyml(at)gmail(dot)com>
> wrote:
> >> I think the fix should go in appendOrderByClause(). It's at that
> >> point we look for the EquivalenceMember for the relation and can
> >> easily discover if the em_expr is a Const. I think we can safely just
> >> skip doing any ORDER BY <const> stuff and not worry about if the
> >> literal format of the const will appear as a reference to an ordinal
> >> column position in the ORDER BY clause.
> >
> > deparseSortGroupClause() calls deparseConst() with showtype = 1.
> appendOrderByClause() may want to do something similar for consistency. Or
> remove it from deparseSortGroupClause() as well?
>
> The fix could also be to use deparseConst() in appendOrderByClause()
> and have that handle Const EquivalenceMember instead. I'd rather just
> skip them. To me, that seems less risky than ensuring deparseConst()
> handles all Const types correctly.
>
> Also, as far as adjusting GROUP BY to eliminate Consts, I don't think
> that's material for a bug fix. If we want to consider doing that,
> that's for master only.
>

If appendOrderByClause() would have been using deparseConst() since the
beginning this bug would not be there. Instead of maintaining two different
ways of deparsing ORDER BY clause, we could maintain just one. I think we
should unify those. If we should do it in only master be it so. I am fine
to leave back branches with two methods.

>
> >> I wonder if we need a test...
> >
> > Yes.
>
> I've added two of those in the attached.
>
> Thanks. They look fine to me.

--
Best Wishes,
Ashutosh Bapat