Обсуждение: Invalid query generated by postgres_fdw with UNION ALL and ORDER BY

On Thu, 7 Mar 2024 at 19:09, Michał Kłeczek <michal@kleczek.org> wrote:
>
> The following query:
>
> SELECT * FROM (
>   SELECT 2023 AS year, * FROM remote_table_1
>   UNION ALL
>   SELECT 2022 AS year, * FROM remote_table_2
> )
> ORDER BY year DESC;
>
> yields the following remote query:
>
> SELECT [columns] FROM remote_table_1 ORDER BY 2023 DESC
>
> and subsequently fails remote execution.
>
>
> Not really sure where the problem is - the planner or postgres_fdw.
> I guess it is postgres_fdw not filtering out ordering keys.

Interesting.  I've attached a self-contained recreator for the casual passerby.

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.

Something like the attached patch I think should work.

I wonder if we need a test...

David

On Fri, 8 Mar 2024 at 00:54, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 4:39 PM David Rowley <dgrowleyml@gmail.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
similarfor 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

On Fri, 8 Mar 2024 at 23:14, Richard Guo <guofenglinux@gmail.com> wrote:
> 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().

This situation where the child member is a Const but the parent isn't
is unique to UNION ALL queries.  The only other cases where we have
child members are with partitioned and inheritance tables. In those
cases, the parent member just maps to the equivalent child member
replacing parent Vars with the corresponding child Var according to
the column mapping between the parent and child. It might be nice if
partitioning supported mapping to a Const as in many cases that could
save storing the same value in the table every time, but we don't
support that, so this can only happen with UNION ALL queries.

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

I can't see how there'd be any problems with a misinterpretation of a
pseudoconstant value as an ordinal column position on the remote
server. Surely it's only actual "Const" node types that we're just
going to call the type's output function which risks it yielding a
string of digits and the remote server thinking that we must mean an
ordinal column position.

> The patch looks good to me otherwise.

Thanks

David