Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for building client with readline (bug1266386) #57

Merged
merged 1 commit into from
May 4, 2015

Conversation

tplavcic
Copy link
Member

This change is to enable building mysql client with readline library since upstream mysql switched to editline and there were multiple requests to enable building with readline again (bug1266386).

Build with system readline is here set as default but it can be enforced with -DWITH_READLINE=system
Basically options for building are:
-DWITH_EDITLINE=system|bundled or -DWITH_READLINE=system
And system readline is the default.

Here is the test build:
rpm: http://jenkins.percona.com/job/percona-server-5.6-redhat-binary/143/
deb: http://jenkins.percona.com/job/percona-server-5.6-debian-binary/137/
deb32: http://jenkins.percona.com/job/percona-server-5.6-debian-binary-32/122/
bin: http://jenkins.percona.com/job/percona-server-5.6-binaries-release/107/

Here is the param build:
http://jenkins.percona.com/job/percona-server-5.6-param/837/

Some testing:
readline link in mysql client lib
[vagrant@t-centos6-64 ~]$ ldd /usr/bin/mysql
...
libreadline.so.6 => /lib64/libreadline.so.6 (0x00007fe657bba000)

Running:
mysql> show variables;
Ctrl+w
mysql> show
Comment: in editline Ctrl+w deletes the whole line, while in readline it deletes the characters from pointer back to first space character.

Reverse search:
In readline:
Ctrl+r
(reverse-i-search)`sh': show engines;

In editline it looks like this:
mysql> show engines;
bck:show

@@ -8,7 +8,7 @@ diff -rup old/client/CMakeLists.txt new/client/CMakeLists.txt
-TARGET_LINK_LIBRARIES(mysql perconaserverclient)
+TARGET_LINK_LIBRARIES(mysql mysqlclient)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a change, it's part of the centos7 patch so github gui is marking it red/green since it contains +/- start of line.

@laurynas-biveinis
Copy link
Contributor

Did you test -DWITH_EDITLINE=system and bundled with this patch?

What happens if both WITH_EDITLINE and WITH_READLINE are specified?

Do completion and history across the sessions functions work in the
new client binaries? The reason I'm asking is because the fix seems to
be different from the WebScaleSQL one:
facebookarchive/webscalesql-5.6@4136922#diff-af3b638bc2a3e6c650974192a53c7291.
Removing libedit support aside, the differences are:

  • they link with curses. They are not alone, a patch contributed on
    http://bugs.mysql.com/bug.php?id=63130 says "(...) does not handle the
    case where the Readline library uses termcap functions, but does not
    link with the termcap or curses library itself." Do you have any
    idea why it's not needed for us then?
  • they require readline/history.h while we appear not to and trust the
    extern "C" declarations in mysql.cc to substitute? Replacing those
    declarations with an include seems more robust to me.
  • ...and maybe required there and in CMake tests as it is history.h
    which defines HIST_ENTRY, and so, under Ubuntu 14.10 we can see now

-- Performing Test READLINE_HAVE_HIST_ENTRY
-- Performing Test READLINE_HAVE_HIST_ENTRY - Failed

because only readline.h but not history.h is included.
This is also mentioned in the description of #63130.

Once this goes in, this might fix more bugs than 1266386 alone, please
follow the links from that bug report:

It should be verified that readline binaries fix the issues in the bug
reports, and the bugs updated accordingly. The upstream bugs that do
not have LP bugs can be mentioned in 1296192, they all seem to be
caused by history file incompatibility.

@tplavcic
Copy link
Member Author

I have tested building with -DWITH_EDITLINE=system and bundled previously and now with this latest commit and didn't have issues - I think I only tested running the client with previous patch with editline.

In the new commit I have added that if you specify both build options it exits with error.
Looks like this:
CMake Error at cmake/readline.cmake:261 (MESSAGE):
Cannot configure WITH_READLINE and WITH_EDITLINE! Use only one setting.

We link with curses also - variable ${MY_READLINE_LIBRARY} includes ${READLINE_LIBRARY} and ${CURSES_LIBRARY}.

In the second commit I changed the stuff you wrote about including the history.h stuff and extern functions. I tried something like this before but had some issues in some conditions so left it like it was - but that time I didn't see the patch you mentioned - so it seems it works.

From the bugs listed above the issues were:
history file format
now looks like this:
vagrant@t-debian7-64:~$ cat .mysql_history
show tables;
use test;
show tables;
show engines;
quit;

putty command line mixup when resizeing windows
I don't have proof for this and I only tried it with putty on linux (don't have windows).
Basically I tried to repeat the steps from bug: https://bugs.launchpad.net/percona-server/+bug/1332822
but it works ok for me.

general readline hotkey usage
Runs as described in my first comment.

I'll update the bugs once we agree that this is ok and it gets merged.

BUILDS:
build:
rpm: http://jenkins.percona.com/job/percona-server-5.6-redhat-binary/144/
deb: http://jenkins.percona.com/job/percona-server-5.6-debian-binary/138/
deb-32: http://jenkins.percona.com/job/percona-server-5.6-debian-binary-32/123/
bin: http://jenkins.percona.com/job/percona-server-5.6-debian-binary-32/123/

param:
http://jenkins.percona.com/job/percona-server-5.6-param/843/
I have incidently stopped the trusty-debug build while trying to do a new run with the commit I have later removed. If you think it's needed I can rerun it - although the one below is runing with that removed commit (which is practically the same thing).
http://jenkins.percona.com/job/percona-server-5.6-param/844/

Linking looks like this on debian7:
vagrant@t-debian7-64:~$ ldd /usr/bin/mysql
linux-vdso.so.1 => (0x00007fff921ff000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fdee17f1000)
libreadline.so.6 => /lib/x86_64-linux-gnu/libreadline.so.6 (0x00007fdee15aa000)
libncurses.so.5 => /lib/x86_64-linux-gnu/libncurses.so.5 (0x00007fdee1387000)
libtinfo.so.5 => /lib/x86_64-linux-gnu/libtinfo.so.5 (0x00007fdee115e000)
libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fdee0f47000)
librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fdee0d3e000)
libssl.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.0 (0x00007fdee0ade000)
libcrypto.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0 (0x00007fdee06fa000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fdee04f5000)
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fdee01ee000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fdedff6c000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fdedfd55000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fdedf9cb000)
/lib64/ld-linux-x86-64.so.2 (0x00007fdee1a14000)

CMAKE:
08:38:09 -- Found Curses: /usr/lib/x86_64-linux-gnu/libcurses.so
08:38:09 -- Looking for tputs in /usr/lib/x86_64-linux-gnu/libcurses.so
08:38:09 -- Looking for tputs in /usr/lib/x86_64-linux-gnu/libcurses.so - found
08:38:09 -- Looking for 3 include files stdio.h, ..., readline/history.h
08:38:09 -- Looking for 3 include files stdio.h, ..., readline/history.h - found
08:38:09 -- READLINE_INCLUDE_DIR /usr/include/readline
08:38:09 -- READLINE_LIBRARY /usr/lib/x86_64-linux-gnu/libreadline.so
08:38:09 -- Performing Test READLINE_HAVE_HIST_ENTRY
08:38:09 -- Performing Test READLINE_HAVE_HIST_ENTRY - Success
08:38:09 -- Performing Test READLINE_USE_LIBEDIT_INTERFACE
08:38:09 -- Performing Test READLINE_USE_LIBEDIT_INTERFACE - Success
08:38:09 -- Performing Test READLINE_USE_NEW_READLINE_INTERFACE
08:38:09 -- Performing Test READLINE_USE_NEW_READLINE_INTERFACE - Success

@laurynas-biveinis
Copy link
Contributor

Could you please flatten the two commits (i.e. git rebase -i) and then it's OK to merge

tplavcic added a commit that referenced this pull request May 4, 2015
Added support for building client with readline (bug1266386)
@tplavcic tplavcic merged commit 87b34cd into percona:5.6 May 4, 2015
@laurynas-biveinis
Copy link
Contributor

Thanks Tomislav, please update the bugs as well

@tplavcic tplavcic deleted the 5.6-ps-bug1266386 branch May 26, 2015 07:55
@mal
Copy link

mal commented Feb 17, 2016

Hi @tplavcic would you consider submitting this to upstream https://github.com/mysql/mysql-server?

I've successfully used your patch to restore sane readline functionality to my personal mysql client (my vim addled fingers thank you!), and there appears to be an open issue for doing just this: http://bugs.mysql.com/bug.php?id=69991

@tplavcic
Copy link
Member Author

Hi @mal I would consider it and will put it into my todo list but I make no further promises! IIRC this patch doesn't break the current editline builds so they might even consider it.
Cheers!

@mal
Copy link

mal commented Feb 18, 2016

That's all I could ask for, thanks!

george-lorch pushed a commit to george-lorch/percona-server that referenced this pull request May 7, 2016
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Test Plan: Ran mtr

Reviewers: maykov, jtolmer, yoshinorim, hermanlee4

Reviewed By: hermanlee4

Differential Revision: https://reviews.facebook.net/D38313
george-lorch pushed a commit to george-lorch/percona-server that referenced this pull request May 9, 2016
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Test Plan: Ran mtr

Reviewers: maykov, jtolmer, yoshinorim, hermanlee4

Reviewed By: hermanlee4

Differential Revision: https://reviews.facebook.net/D38313
inikep pushed a commit to inikep/percona-server that referenced this pull request Apr 23, 2020
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Differential Revision: https://reviews.facebook.net/D38313

fbshipit-source-id: 709bdf5aade
inikep pushed a commit to inikep/percona-server that referenced this pull request Feb 24, 2021
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Differential Revision: https://reviews.facebook.net/D38313

fbshipit-source-id: 709bdf5aade
inikep pushed a commit to inikep/percona-server that referenced this pull request Nov 15, 2021
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Differential Revision: https://reviews.facebook.net/D38313

fbshipit-source-id: 30de0110702
ldonoso pushed a commit to ldonoso/percona-server that referenced this pull request Mar 15, 2022
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Differential Revision: https://reviews.facebook.net/D38313
inikep pushed a commit to inikep/percona-server that referenced this pull request Apr 9, 2024
Summary:
Do exactly that: remember what locks we've held at the start of the
statement, and release all other locks if the statement is rolled back.

Differential Revision: https://reviews.facebook.net/D38313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants