{"action":"created","issue":{"url":"https://api.github.com/repos/greenplum-db/gpdb/issues/965","repository_url":"https://api.github.com/repos/greenplum-db/gpdb","labels_url":"https://api.github.com/repos/greenplum-db/gpdb/issues/965/labels{/name}","comments_url":"https://api.github.com/repos/greenplum-db/gpdb/issues/965/comments","events_url":"https://api.github.com/repos/greenplum-db/gpdb/issues/965/events","html_url":"https://github.com/greenplum-db/gpdb/pull/965","id":166287499,"number":965,"title":"Fix annoyances in the regression test added for memtuple-forming bug.","user":{"login":"hlinnaka","id":191602,"avatar_url":"https://avatars.githubusercontent.com/u/191602?v=3","gravatar_id":"","url":"https://api.github.com/users/hlinnaka","html_url":"https://github.com/hlinnaka","followers_url":"https://api.github.com/users/hlinnaka/followers","following_url":"https://api.github.com/users/hlinnaka/following{/other_user}","gists_url":"https://api.github.com/users/hlinnaka/gists{/gist_id}","starred_url":"https://api.github.com/users/hlinnaka/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hlinnaka/subscriptions","organizations_url":"https://api.github.com/users/hlinnaka/orgs","repos_url":"https://api.github.com/users/hlinnaka/repos","events_url":"https://api.github.com/users/hlinnaka/events{/privacy}","received_events_url":"https://api.github.com/users/hlinnaka/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-07-19T09:33:06Z","updated_at":"2016-07-19T09:33:07Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/greenplum-db/gpdb/pulls/965","html_url":"https://github.com/greenplum-db/gpdb/pull/965","diff_url":"https://github.com/greenplum-db/gpdb/pull/965.diff","patch_url":"https://github.com/greenplum-db/gpdb/pull/965.patch"},"body":"Commit 785c9bea5524a4293e78d7b75bd5820bf23a4ac0, \"Fix memtuple forming bug\r\nwhen null-saved size is larger than 32763 bytes\", added a regression test\r\nthat was annoying for multiple reasons:\r\n\r\n* The 'aggregates' regression test is from the upstream. We're trying to keep\r\nupstream test files unchanged, to make diffing and merging with the upstream\r\neasier.\r\n\r\n* The query is huge, about 250000 bytes, and it's all on one line. Not all\r\ntools play well with such long lines. Emacs became really sluggish and the\r\nlaptop fan started screaming, when I moved the cursor over that line. When I\r\ntried to copy-paste that into psql, my terminal ground to a halt. It took\r\nminutes to process it, ie. just to paste the line to the terminal. (Note to\r\nself: get a better terminal program :-) ).\r\n\r\n* The query takes a very long time to run, about 30 seconds on my laptop. I\r\ndid some profiling: all the time is spent in the code in ExecInitAgg() that\r\nchecks for duplicate aggregates. That loop is O(n^2), and calls equal() on\r\nthe AggRef expressions on each iteration.\r\n\r\n* When the query doesn't crash because of the bug that was fixed, it produces\r\nan \"invalid timestamp\" error. Seems surprising that the expected result is\r\nan error, unless you're specifically testing for error handling. It also\r\nmakes me worried that the test might fail to test the right thing in the\r\nfuture, if the planner decides to rearrange things so that the cast to\r\ntimestamp is evaluated first, so that it errors out before even constructing\r\nthe array with the aggregate results.\r\n\r\nTo fix those issues:\r\n\r\n* Move the test query to 'bfv_aggregate' test. BFV stands for Bug Fix\r\nVerification, i.e. checking that old bugs haven't reappeared, and that's\r\nexactly what this is. (There are other GPDB additions in 'aggregates' test\r\nbesides this, but let's not make it worse.)\r\n\r\n* Split the test query into reasonably-sized lines.\r\n\r\n* Replace the aggregated expression with a simpler one. I'm hesitant to try\r\noptimizing the O(n^2) code, because code comes from the upstream. There\r\nhave been some changes in that area in recent versions of PostgreSQL, and\r\nI'm not sure if the recent changes will make this better or worse, but not\r\nmuch point spending time on it before we catch up. And then we should get\r\nany improvements there committed to the upstream first.\r\n\r\nReplacing the CASE-WHEN construct\" with something simpler doesn't fix the\r\nO(n^2) behavior, but it makes the constant factor smaller, by making the\r\nequal() calls cheaper. I replaced it with \"c4 % [number]\", and now the\r\nquery takes about 17 seconds on my laptop (was 30 s). That's still longer\r\nthan I'd wish, but every little helps.\r\n\r\n* Remove the cast to timestamp that was causing the error on succes.\r\n\r\nIn the passing, also add a comment explaining what the purpose of the test\r\nis, and some notes on the O(n^2) behavior."},"comment":{"url":"https://api.github.com/repos/greenplum-db/gpdb/issues/comments/233580072","html_url":"https://github.com/greenplum-db/gpdb/pull/965#issuecomment-233580072","issue_url":"https://api.github.com/repos/greenplum-db/gpdb/issues/965","id":233580072,"user":{"login":"gpdbdreddbot","id":15618396,"avatar_url":"https://avatars.githubusercontent.com/u/15618396?v=3","gravatar_id":"","url":"https://api.github.com/users/gpdbdreddbot","html_url":"https://github.com/gpdbdreddbot","followers_url":"https://api.github.com/users/gpdbdreddbot/followers","following_url":"https://api.github.com/users/gpdbdreddbot/following{/other_user}","gists_url":"https://api.github.com/users/gpdbdreddbot/gists{/gist_id}","starred_url":"https://api.github.com/users/gpdbdreddbot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gpdbdreddbot/subscriptions","organizations_url":"https://api.github.com/users/gpdbdreddbot/orgs","repos_url":"https://api.github.com/users/gpdbdreddbot/repos","events_url":"https://api.github.com/users/gpdbdreddbot/events{/privacy}","received_events_url":"https://api.github.com/users/gpdbdreddbot/received_events","type":"User","site_admin":false},"created_at":"2016-07-19T09:33:07Z","updated_at":"2016-07-19T09:33:07Z","body":"Hello hlinnaka!\n\nThanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.\n"}}
{"actor":{"display_login":"gpdbdreddbot"}}