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

From: Richard Guo <guofenglinux(at)gmail(dot)com>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, 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 10:14:00
Message-ID: CAMbWs4--3qi82XToCvfygf-uP83pjuqXGS7w3XV_RmGJTYMbcw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

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

I've looked at this patch a bit. I once wondered why we don't check
pathkey->pk_eclass->ec_has_const with EC_MUST_BE_REDUNDANT to see if the
pathkey is not needed. Then I realized that a child member would not be
marked as constant even if the child expr is a Const, as explained in
add_child_rel_equivalences().

BTW, I wonder if it is possible that we have a pseudoconstant expression
that is not of type Const. In such cases we would need to check
'bms_is_empty(pull_varnos(em_expr))' instead of 'IsA(em_expr, Const)'.
However, I'm unable to think of such an expression in this context.

The patch looks good to me otherwise.

Thanks
Richard

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2024-03-08 10:17:56 Re: Proposal to add page headers to SLRU pages
Previous Message Bharath Rupireddy 2024-03-08 10:06:30 Re: Add new error_action COPY ON_ERROR "log"