MIME-Version: 1.0
References: <0714C8B8-8D82-4ABB-9F8D-A0C3657E7B6E@kleczek.org>
 <CAApHDvpfyTA3Pnf_P6Kx8s5rNSUBxPfsZxiFz6DpUyMiwqHERw@mail.gmail.com> <CAExHW5taz6hX05P55_S1cbx+P5CcNN21rD7oLY5iXJcShrAbaQ@mail.gmail.com>
In-Reply-To: <CAExHW5taz6hX05P55_S1cbx+P5CcNN21rD7oLY5iXJcShrAbaQ@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Fri, 8 Mar 2024 15:12:46 +1300
Message-ID: <CAApHDvrx5YTZYeSDq_WMA_gha5B1-Di9omKGUytGaYAuZ7293Q@mail.gmail.com>
Subject: Re: Invalid query generated by postgres_fdw with UNION ALL and ORDER BY
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: =?UTF-8?B?TWljaGHFgiBLxYJlY3plaw==?= <michal@kleczek.org>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000000eea3506131cbb66"
Archived-At: <https://www.postgresql.org/message-id/CAApHDvrx5YTZYeSDq_WMA_gha5B1-Di9omKGUytGaYAuZ7293Q%40mail.gmail.com>
Precedence: bulk

--0000000000000eea3506131cbb66
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 8 Mar 2024 at 00:54, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 4:39=E2=80=AFPM 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 =3D 1. append=
OrderByClause() 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

--0000000000000eea3506131cbb66
Content-Type: text/plain; charset="US-ASCII"; name="postgres_fdw_order_by_const_fix_v2.patch"
Content-Disposition: attachment; 
	filename="postgres_fdw_order_by_const_fix_v2.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lti0npjr0>
X-Attachment-Id: f_lti0npjr0

ZGlmZiAtLWdpdCBhL2NvbnRyaWIvcG9zdGdyZXNfZmR3L2RlcGFyc2UuYyBiL2NvbnRyaWIvcG9z
dGdyZXNfZmR3L2RlcGFyc2UuYwppbmRleCA4ZmM2NmZhMTFjLi5hMzBmMDAxNzllIDEwMDY0NAot
LS0gYS9jb250cmliL3Bvc3RncmVzX2Zkdy9kZXBhcnNlLmMKKysrIGIvY29udHJpYi9wb3N0Z3Jl
c19mZHcvZGVwYXJzZS5jCkBAIC0zOTEyLDEzICszOTEyLDEyIEBAIGFwcGVuZE9yZGVyQnlDbGF1
c2UoTGlzdCAqcGF0aGtleXMsIGJvb2wgaGFzX2ZpbmFsX3NvcnQsCiB7CiAJTGlzdENlbGwgICAq
bGNlbGw7CiAJaW50CQkJbmVzdGxldmVsOwotCWNvbnN0IGNoYXIgKmRlbGltID0gIiAiOwogCVN0
cmluZ0luZm8JYnVmID0gY29udGV4dC0+YnVmOworCWJvb2wJCWdvdG9uZSA9IGZhbHNlOwogCiAJ
LyogTWFrZSBzdXJlIGFueSBjb25zdGFudHMgaW4gdGhlIGV4cHJzIGFyZSBwcmludGVkIHBvcnRh
Ymx5ICovCiAJbmVzdGxldmVsID0gc2V0X3RyYW5zbWlzc2lvbl9tb2RlcygpOwogCi0JYXBwZW5k
U3RyaW5nSW5mb1N0cmluZyhidWYsICIgT1JERVIgQlkiKTsKIAlmb3JlYWNoKGxjZWxsLCBwYXRo
a2V5cykKIAl7CiAJCVBhdGhLZXkgICAgKnBhdGhrZXkgPSBsZmlyc3QobGNlbGwpOwpAQCAtMzk1
MSw2ICszOTUwLDI1IEBAIGFwcGVuZE9yZGVyQnlDbGF1c2UoTGlzdCAqcGF0aGtleXMsIGJvb2wg
aGFzX2ZpbmFsX3NvcnQsCiAKIAkJZW1fZXhwciA9IGVtLT5lbV9leHByOwogCisJCS8qCisJCSAq
IElmIHRoZSBtZW1iZXIgaXMgYSBDb25zdCBleHByZXNzaW9uIHRoZW4gd2UgbmVlZG4ndCBhZGQg
aXQgdG8gdGhlCisJCSAqIE9SREVSIEJZIGNsYXVzZS4gIFRoaXMgY2FuIGhhcHBlbiBpbiBVTklP
TiBBTEwgcXVlcmllcyB3aGVyZSB0aGUKKwkJICogdW5pb24gY2hpbGQgdGFyZ2V0bGlzdCBoYXMg
YSBDb25zdC4gIEFkZGluZyB0aGVzZSB3b3VsZCBiZSB3YXN0ZWZ1bCwKKwkJICogYnV0IGFsc28s
IGZvciBJTlQgY29sdW1ucywgYW4gaW50ZWdlciBsaXRlcmFsIHdpbGwgYmUgc2VlbiBhcyBhbgor
CQkgKiBvcmRpbmFsIGNvbHVtbiBwb3NpdGlvbiByYXRoZXIgdGhhbiBhIHZhbHVlIHRvIHNvcnQg
YnksIHNvIHdlIG11c3QKKwkJICogc2tpcCB0aGVzZS4KKwkJICovCisJCWlmIChJc0EoZW1fZXhw
ciwgQ29uc3QpKQorCQkJY29udGludWU7CisKKwkJaWYgKCFnb3RvbmUpCisJCXsKKwkJCWFwcGVu
ZFN0cmluZ0luZm9TdHJpbmcoYnVmLCAiIE9SREVSIEJZICIpOworCQkJZ290b25lID0gdHJ1ZTsK
KwkJfQorCQllbHNlCisJCQlhcHBlbmRTdHJpbmdJbmZvU3RyaW5nKGJ1ZiwgIiwgIik7CisKIAkJ
LyoKIAkJICogTG9va3VwIHRoZSBvcGVyYXRvciBjb3JyZXNwb25kaW5nIHRvIHRoZSBzdHJhdGVn
eSBpbiB0aGUgb3BjbGFzcy4KIAkJICogVGhlIGRhdGF0eXBlIHVzZWQgYnkgdGhlIG9wZmFtaWx5
IGlzIG5vdCBuZWNlc3NhcmlseSB0aGUgc2FtZSBhcwpAQCAtMzk2NSw3ICszOTgzLDYgQEAgYXBw
ZW5kT3JkZXJCeUNsYXVzZShMaXN0ICpwYXRoa2V5cywgYm9vbCBoYXNfZmluYWxfc29ydCwKIAkJ
CQkgcGF0aGtleS0+cGtfc3RyYXRlZ3ksIGVtLT5lbV9kYXRhdHlwZSwgZW0tPmVtX2RhdGF0eXBl
LAogCQkJCSBwYXRoa2V5LT5wa19vcGZhbWlseSk7CiAKLQkJYXBwZW5kU3RyaW5nSW5mb1N0cmlu
ZyhidWYsIGRlbGltKTsKIAkJZGVwYXJzZUV4cHIoZW1fZXhwciwgY29udGV4dCk7CiAKIAkJLyoK
QEAgLTM5NzUsNyArMzk5Miw2IEBAIGFwcGVuZE9yZGVyQnlDbGF1c2UoTGlzdCAqcGF0aGtleXMs
IGJvb2wgaGFzX2ZpbmFsX3NvcnQsCiAJCWFwcGVuZE9yZGVyQnlTdWZmaXgob3ByaWQsIGV4cHJU
eXBlKChOb2RlICopIGVtX2V4cHIpLAogCQkJCQkJCXBhdGhrZXktPnBrX251bGxzX2ZpcnN0LCBj
b250ZXh0KTsKIAotCQlkZWxpbSA9ICIsICI7CiAJfQogCXJlc2V0X3RyYW5zbWlzc2lvbl9tb2Rl
cyhuZXN0bGV2ZWwpOwogfQpkaWZmIC0tZ2l0IGEvY29udHJpYi9wb3N0Z3Jlc19mZHcvZXhwZWN0
ZWQvcG9zdGdyZXNfZmR3Lm91dCBiL2NvbnRyaWIvcG9zdGdyZXNfZmR3L2V4cGVjdGVkL3Bvc3Rn
cmVzX2Zkdy5vdXQKaW5kZXggYzM1NWU4ZjNmNy4uZjMzMzliNGFjOCAxMDA2NDQKLS0tIGEvY29u
dHJpYi9wb3N0Z3Jlc19mZHcvZXhwZWN0ZWQvcG9zdGdyZXNfZmR3Lm91dAorKysgYi9jb250cmli
L3Bvc3RncmVzX2Zkdy9leHBlY3RlZC9wb3N0Z3Jlc19mZHcub3V0CkBAIC05MTksNiArOTE5LDQ0
IEBAIEVYUExBSU4gKFZFUkJPU0UsIENPU1RTIE9GRikKICAgICAgICAgIFJlbW90ZSBTUUw6IFNF
TEVDVCAiQyAxIiwgYzIsIGMzLCBjNCwgYzUsIGM2LCBjNywgYzggRlJPTSAiUyAxIi4iVCAxIgog
KDYgcm93cykKIAorLS0gRW5zdXJlIHdlIGRvbid0IHB1c2ggT1JERVIgQlkgZXhwcmVzc2lvbnMg
d2hpY2ggYXJlIENvbnN0cyBhdCB0aGUgVU5JT04KKy0tIGNoaWxkIGxldmVsIHRvIHRoZSBmb3Jl
aWduIHNlcnZlci4KK0VYUExBSU4gKFZFUkJPU0UsIENPU1RTIE9GRikKK1NFTEVDVCAqIEZST00g
KAorICAgIFNFTEVDVCAxIEFTIHR5cGUsYzEgRlJPTSBmdDEKKyAgICBVTklPTiBBTEwKKyAgICBT
RUxFQ1QgMiBBUyB0eXBlLGMxIEZST00gZnQyCispIE9SREVSIEJZIHR5cGUsYzE7CisgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIFFVRVJZIFBMQU4gICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAKKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQorIE1lcmdlIEFwcGVu
ZAorICAgU29ydCBLZXk6ICgxKSwgZnQxLmMxCisgICAtPiAgRm9yZWlnbiBTY2FuIG9uIHB1Ymxp
Yy5mdDEKKyAgICAgICAgIE91dHB1dDogMSwgZnQxLmMxCisgICAgICAgICBSZW1vdGUgU1FMOiBT
RUxFQ1QgIkMgMSIgRlJPTSAiUyAxIi4iVCAxIiBPUkRFUiBCWSAiQyAxIiBBU0MgTlVMTFMgTEFT
VAorICAgLT4gIEZvcmVpZ24gU2NhbiBvbiBwdWJsaWMuZnQyCisgICAgICAgICBPdXRwdXQ6IDIs
IGZ0Mi5jMQorICAgICAgICAgUmVtb3RlIFNRTDogU0VMRUNUICJDIDEiIEZST00gIlMgMSIuIlQg
MSIgT1JERVIgQlkgIkMgMSIgQVNDIE5VTExTIExBU1QKKyg4IHJvd3MpCisKK0VYUExBSU4gKFZF
UkJPU0UsIENPU1RTIE9GRikKK1NFTEVDVCAqIEZST00gKAorICAgIFNFTEVDVCAxIEFTIHR5cGUs
YzEgRlJPTSBmdDEKKyAgICBVTklPTiBBTEwKKyAgICBTRUxFQ1QgMiBBUyB0eXBlLGMxIEZST00g
ZnQyCispIE9SREVSIEJZIHR5cGU7CisgICAgICAgICAgICAgICAgICAgIFFVRVJZIFBMQU4gICAg
ICAgICAgICAgICAgICAgICAKKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLQorIE1lcmdlIEFwcGVuZAorICAgU29ydCBLZXk6ICgxKQorICAgLT4gIEZv
cmVpZ24gU2NhbiBvbiBwdWJsaWMuZnQxCisgICAgICAgICBPdXRwdXQ6IDEsIGZ0MS5jMQorICAg
ICAgICAgUmVtb3RlIFNRTDogU0VMRUNUICJDIDEiIEZST00gIlMgMSIuIlQgMSIKKyAgIC0+ICBG
b3JlaWduIFNjYW4gb24gcHVibGljLmZ0MgorICAgICAgICAgT3V0cHV0OiAyLCBmdDIuYzEKKyAg
ICAgICAgIFJlbW90ZSBTUUw6IFNFTEVDVCAiQyAxIiBGUk9NICJTIDEiLiJUIDEiCisoOCByb3dz
KQorCiAtLSB1c2VyLWRlZmluZWQgb3BlcmF0b3IvZnVuY3Rpb24KIENSRUFURSBGVU5DVElPTiBw
b3N0Z3Jlc19mZHdfYWJzKGludCkgUkVUVVJOUyBpbnQgQVMgJCQKIEJFR0lOCmRpZmYgLS1naXQg
YS9jb250cmliL3Bvc3RncmVzX2Zkdy9zcWwvcG9zdGdyZXNfZmR3LnNxbCBiL2NvbnRyaWIvcG9z
dGdyZXNfZmR3L3NxbC9wb3N0Z3Jlc19mZHcuc3FsCmluZGV4IDgxMmU3NjQ2ZTEuLmJjOGZkYjY0
MTkgMTAwNjQ0Ci0tLSBhL2NvbnRyaWIvcG9zdGdyZXNfZmR3L3NxbC9wb3N0Z3Jlc19mZHcuc3Fs
CisrKyBiL2NvbnRyaWIvcG9zdGdyZXNfZmR3L3NxbC9wb3N0Z3Jlc19mZHcuc3FsCkBAIC0zNjIs
NiArMzYyLDIyIEBAIEVYUExBSU4gKFZFUkJPU0UsIENPU1RTIE9GRikKIEVYUExBSU4gKFZFUkJP
U0UsIENPU1RTIE9GRikKIAlTRUxFQ1QgKiBGUk9NIGZ0MiBPUkRFUiBCWSBmdDIuYzEsIGZ0Mi5j
MyBjb2xsYXRlICJDIjsKIAorLS0gRW5zdXJlIHdlIGRvbid0IHB1c2ggT1JERVIgQlkgZXhwcmVz
c2lvbnMgd2hpY2ggYXJlIENvbnN0cyBhdCB0aGUgVU5JT04KKy0tIGNoaWxkIGxldmVsIHRvIHRo
ZSBmb3JlaWduIHNlcnZlci4KK0VYUExBSU4gKFZFUkJPU0UsIENPU1RTIE9GRikKK1NFTEVDVCAq
IEZST00gKAorICAgIFNFTEVDVCAxIEFTIHR5cGUsYzEgRlJPTSBmdDEKKyAgICBVTklPTiBBTEwK
KyAgICBTRUxFQ1QgMiBBUyB0eXBlLGMxIEZST00gZnQyCispIE9SREVSIEJZIHR5cGUsYzE7CisK
K0VYUExBSU4gKFZFUkJPU0UsIENPU1RTIE9GRikKK1NFTEVDVCAqIEZST00gKAorICAgIFNFTEVD
VCAxIEFTIHR5cGUsYzEgRlJPTSBmdDEKKyAgICBVTklPTiBBTEwKKyAgICBTRUxFQ1QgMiBBUyB0
eXBlLGMxIEZST00gZnQyCispIE9SREVSIEJZIHR5cGU7CisKIC0tIHVzZXItZGVmaW5lZCBvcGVy
YXRvci9mdW5jdGlvbgogQ1JFQVRFIEZVTkNUSU9OIHBvc3RncmVzX2Zkd19hYnMoaW50KSBSRVRV
Uk5TIGludCBBUyAkJAogQkVHSU4K
--0000000000000eea3506131cbb66--