Feedback-ID: i131946ab:Fastmail
Content-Type: multipart/mixed; boundary="------------jfgtuYrWYYYOESXvgxD0zcZh"
Message-ID: <bf604c8d-b6e9-284b-327a-cac8417a74e8@enterprisedb.com>
Date: Mon, 23 Jan 2023 14:27:13 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Generating code for query jumbling through gen_node_support.pl
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 "Drouvot, Bertrand" <bdrouvot@amazon.com>
References: <34491a4e-2664-6ea7-662c-b13d18d2a92d@enterprisedb.com>
 <Y8EOUuViEL8Ewvf/@paquier.xyz>
 <d74d2681-5c07-2f6e-d177-995905015311@enterprisedb.com>
 <Y8YakMwx2a6Y63oR@paquier.xyz>
 <e4801c28-cb5b-3b81-bda1-03af8df1fef9@enterprisedb.com>
 <Y8ZTvD6nEqNttAHJ@paquier.xyz> <Y8eZ9/3STyhF57RV@paquier.xyz>
 <50b571dc-e0c0-22be-9803-4a3480d2e792@enterprisedb.com>
 <Y8oaA5Vxnr4rzEDJ@paquier.xyz>
 <e4d61d52-4992-d190-b5ad-48c6c752942c@enterprisedb.com>
 <Y8tdmPlzFyNcrPR3@paquier.xyz>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <Y8tdmPlzFyNcrPR3@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/bf604c8d-b6e9-284b-327a-cac8417a74e8%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------jfgtuYrWYYYOESXvgxD0zcZh
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 21.01.23 04:35, Michael Paquier wrote:
>> I'll read the 0003 again more carefully.  I haven't studied the new 0004
>> yet.
> 
> Thanks, again.  Rebased version attached.

A couple of small fixes are attached.

There is something weird in _jumbleNode().  There are two switch 
(nodeTag(expr)) statements.  Maybe that's intentional, but then it 
should be commented better, because now it looks more like an editing 
mistake.

The handling of T_RangeTblEntry could be improved.  In other contexts we 
have for example a custom_copy attribute, which generates the switch 
entry but not the function.  Something like this could be useful here too.

Otherwise, this looks ok.  I haven't checked whether it maintains the 
exact behavior from before.  What is the test coverage situation for this?

For the 0004 patch, it should be documented why one would want one 
behavior or the other.  That's totally unclear right now.

I think if we are going to accept 0004, then it might be better to 
combine it with 0003.  Otherwise, 0004 is just undoing a lot of the code 
structure changes in JumbleQuery() that 0003 did.

--------------jfgtuYrWYYYOESXvgxD0zcZh
Content-Type: text/plain; charset=UTF-8;
 name="0001-fixup-Support-for-automated-query-jumble-with-all-No.patch"
Content-Disposition: attachment;
 filename*0="0001-fixup-Support-for-automated-query-jumble-with-all-No.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSAwNTk3Mjc1ZWY0MzlmMDcwYjFiZDMyYmEyNWNhMzU4MWNiYzJhZjMwIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogTW9uLCAyMyBKYW4gMjAyMyAxNDoxNjozOSArMDEwMApTdWJqZWN0
OiBbUEFUQ0hdIGZpeHVwISBTdXBwb3J0IGZvciBhdXRvbWF0ZWQgcXVlcnkganVtYmxlIHdp
dGggYWxsIE5vZGVzCgotLS0KIHNyYy9iYWNrZW5kL25vZGVzL3F1ZXJ5anVtYmxlZnVuY3Mu
YyB8IDIgKy0KIHNyYy9pbmNsdWRlL25vZGVzL25vZGVzLmggICAgICAgICAgICB8IDQgKyst
LQogc3JjL2luY2x1ZGUvbm9kZXMvcHJpbW5vZGVzLmggICAgICAgIHwgMSAtCiAzIGZpbGVz
IGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQg
YS9zcmMvYmFja2VuZC9ub2Rlcy9xdWVyeWp1bWJsZWZ1bmNzLmMgYi9zcmMvYmFja2VuZC9u
b2Rlcy9xdWVyeWp1bWJsZWZ1bmNzLmMKaW5kZXggMjc4MTUwZmJhMC4uYmRiNTYzNmIxYiAx
MDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvbm9kZXMvcXVlcnlqdW1ibGVmdW5jcy5jCisrKyBi
L3NyYy9iYWNrZW5kL25vZGVzL3F1ZXJ5anVtYmxlZnVuY3MuYwpAQCAtMjEsNyArMjEsNyBA
QAogICogdHJlZShzKSBnZW5lcmF0ZWQgZnJvbSB0aGUgcXVlcnkuICBUaGUgZXhlY3V0b3Ig
Y2FuIHRoZW4gdXNlIHRoaXMgdmFsdWUKICAqIHRvIGJsYW1lIHF1ZXJ5IGNvc3RzIG9uIHRo
ZSBwcm9wZXIgcXVlcnlJZC4KICAqCi0gKiBQb3J0aW9ucyBDb3B5cmlnaHQgKGMpIDE5OTYt
MjAyMiwgUG9zdGdyZVNRTCBHbG9iYWwgRGV2ZWxvcG1lbnQgR3JvdXAKKyAqIFBvcnRpb25z
IENvcHlyaWdodCAoYykgMTk5Ni0yMDIzLCBQb3N0Z3JlU1FMIEdsb2JhbCBEZXZlbG9wbWVu
dCBHcm91cAogICogUG9ydGlvbnMgQ29weXJpZ2h0IChjKSAxOTk0LCBSZWdlbnRzIG9mIHRo
ZSBVbml2ZXJzaXR5IG9mIENhbGlmb3JuaWEKICAqCiAgKgpkaWZmIC0tZ2l0IGEvc3JjL2lu
Y2x1ZGUvbm9kZXMvbm9kZXMuaCBiL3NyYy9pbmNsdWRlL25vZGVzL25vZGVzLmgKaW5kZXgg
NmVjZDk0NGE5MC4uZTdmOTJkZjdjYiAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvbm9kZXMv
bm9kZXMuaAorKysgYi9zcmMvaW5jbHVkZS9ub2Rlcy9ub2Rlcy5oCkBAIC01OSwxMiArNTks
MTIgQEAgdHlwZWRlZiBlbnVtIE5vZGVUYWcKICAqCiAgKiAtIG5vX2NvcHlfZXF1YWw6IFNo
b3J0aGFuZCBmb3IgYm90aCBub19jb3B5IGFuZCBub19lcXVhbC4KICAqCi0gKiAtIG5vX3F1
ZXJ5X2p1bWJsZTogRG9lcyBub3Qgc3VwcG9ydCBqdW1ibGUoKSBhdCBhbGwuCisgKiAtIG5v
X3F1ZXJ5X2p1bWJsZTogRG9lcyBub3Qgc3VwcG9ydCBKdW1ibGVRdWVyeSgpIGF0IGFsbC4K
ICAqCiAgKiAtIG5vX3JlYWQ6IERvZXMgbm90IHN1cHBvcnQgbm9kZVJlYWQoKSBhdCBhbGwu
CiAgKgogICogLSBub2RldGFnX29ubHk6IERvZXMgbm90IHN1cHBvcnQgY29weU9iamVjdCgp
LCBlcXVhbCgpLCBvdXROb2RlKCksCi0gKiAgIG9yIG5vZGVSZWFkKCkuCisgKiAgIG9yIG5v
ZGVSZWFkKCkuIFhYWAogICoKICAqIC0gc3BlY2lhbF9yZWFkX3dyaXRlOiBIYXMgc3BlY2lh
bCB0cmVhdG1lbnQgaW4gb3V0Tm9kZSgpIGFuZCBub2RlUmVhZCgpLgogICoKZGlmZiAtLWdp
dCBhL3NyYy9pbmNsdWRlL25vZGVzL3ByaW1ub2Rlcy5oIGIvc3JjL2luY2x1ZGUvbm9kZXMv
cHJpbW5vZGVzLmgKaW5kZXggZjgxNTE4YzJmZC4uOWVhODE5YTZhNyAxMDA2NDQKLS0tIGEv
c3JjL2luY2x1ZGUvbm9kZXMvcHJpbW5vZGVzLmgKKysrIGIvc3JjL2luY2x1ZGUvbm9kZXMv
cHJpbW5vZGVzLmgKQEAgLTc5OSw3ICs3OTksNiBAQCB0eXBlZGVmIE9wRXhwciBOdWxsSWZF
eHByOwogICogZmlsbGVkIGluIHJpZ2h0IGF3YXksIHNvIHdpbGwgYmUgaWdub3JlZCBmb3Ig
ZXF1YWxpdHkgaWYgdGhleSBhcmUgbm90IHNldAogICogeWV0LgogICoKLSAqCiAgKiBPSUQg
ZW50cmllcyBvZiB0aGUgaW50ZXJuYWwgZnVuY3Rpb24gdHlwZXMgYXJlIGlycmVsZXZhbnQg
Zm9yIHRoZSBxdWVyeQogICoganVtYmxpbmcsIGJ1dCB0aGUgb3BlcmF0b3IgT0lEIGFuZCB0
aGUgYXJndW1lbnRzIGFyZS4KICAqLwotLSAKMi4zOS4xCgo=

--------------jfgtuYrWYYYOESXvgxD0zcZh--