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

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
Cc: Michał Kłeczek <michal(at)kleczek(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Invalid query generated by postgres_fdw with UNION ALL and ORDER BY
Date: 2024-03-08 02:12:46
Message-ID: CAApHDvrx5YTZYeSDq_WMA_gha5B1-Di9omKGUytGaYAuZ7293Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

>> I wonder if we need a test...
>
> Yes.

I've added two of those in the attached.

I also changed the way the delimiter stuff works as the exiting code
seems to want to avoid having a bool flag to record if we're adding
the first item. The change I'm making means the bool flag is now
required, so we may as well use that flag to deal with the delimiter
append too.

David

Attachment Content-Type Size
postgres_fdw_order_by_const_fix_v2.patch text/plain 4.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2024-03-08 02:53:43 Re: [PoC] Improve dead tuple storage for lazy vacuum
Previous Message Justin Pryzby 2024-03-08 02:02:00 Re: ALTER TABLE SET ACCESS METHOD on partitioned tables