Horton

Thread

Commits

  1. Fix deparsing of Consts in postgres_fdw ORDER BY

  1. Invalid query generated by postgres_fdw with UNION ALL and ORDER BY

    Michał Kłeczek <michal@kleczek.org> — 2024-03-07T06:08:40Z

    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.

This filtering would also be pretty useful in the following scenario (which is also why I went through UNION ALL route and discovered this issue):

I have a table partitioned by year, partitions are remote tables.
On remote servers I have a GIST index that does not support ordering ([1]) so I would like to avoid sending ORDER BY year to remote servers.
Ideally redundant ordering should be filtered out.

[1] https://www.postgresql.org/message-id/B2AC13F9-6655-4E27-BFD3-068844E5DC91%40kleczek.org


—
Kind regards,
Michal

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

    David Rowley <dgrowleyml@gmail.com> — 2024-03-07T11:08:42Z

    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

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-03-07T11:54:40Z

    On Thu, Mar 7, 2024 at 4:39 PM David Rowley <dgrowleyml@gmail.com> wrote:

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

deparseSortGroupClause() calls deparseConst() with showtype = 1.
appendOrderByClause() may want to do something similar for consistency. Or
remove it from deparseSortGroupClause() as well?


>
> Something like the attached patch I think should work.
>
> I wonder if we need a test...
>

Yes.

-- 
Best Wishes,
Ashutosh Bapat

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

    David Rowley <dgrowleyml@gmail.com> — 2024-03-08T02:12:46Z

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

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

    Richard Guo <guofenglinux@gmail.com> — 2024-03-08T10:14:00Z

    On Fri, Mar 8, 2024 at 10:13 AM David Rowley <dgrowleyml@gmail.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

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

    David Rowley <dgrowleyml@gmail.com> — 2024-03-10T21:56:35Z

    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

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-03-11T06:03:24Z

    On Fri, Mar 8, 2024 at 7:43 AM David Rowley <dgrowleyml@gmail.com> wrote:

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