server icon indicating copy to clipboard operation
server copied to clipboard

MDEV-32061 INTO OUTFILE should work with named pipes

Open XanC opened this issue 1 year ago • 11 comments
trafficstars

The Jira issue number for this PR is: MDEV-32061

Description

This patch allows named pipes (aka fifos) to be used as destinations for INTO OUTFILE statements.

In my case, the time and disk space required to make compressed snapshots of database tables are hugely reduced by this capability. There are probably many other creative uses of named pipes for INTO OUTFILE.

Release Notes

Allow INTO OUTFILE to write to named pipes.

https://mariadb.com/kb/en/select-into-outfile/ will need some updates.

Testing

An automated test for this feature is included in this pull request.

Basing the PR against the correct MariaDB version

  • [x ] This is a new feature and the PR is based against the latest MariaDB development branch.

PR quality check

  • [x ] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [x ] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

XanC avatar Feb 14 '24 02:02 XanC

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 14 '24 02:02 CLAassistant

Ok squashed it up and used --replace_result for test. Not sure why you added secure-priv - its the default and not needed, so removed.

Sorry the github suggestions weren't as easily apply-able, first time I've used them.

I'm chasing up why the --ps-protocol tests are failing (its executing SELECT queries twice).

grooverdan avatar Feb 15 '24 03:02 grooverdan

Thanks very much!

--replace_result is definitely the better approach. Is there anywhere that the different test "commands" are documented?

At one point I was getting a failure to write to a file because of secure-priv, and setting it to the empty string got around the error. I'm not sure how that would have happened if it were the default as you say, so that's a mystery. I was definitely chasing my tail for a while trying to get the paths to line up correctly between the SQL commands and the shell/test commands.

Well anyway, thanks for pushing this out. Would it also be appropriate for me to attempt to apply this change to 10.11?

XanC avatar Feb 15 '24 03:02 XanC

disabled the ps2-protocol around the critical bit that wouldn't execute twice.

--replace_result is definitely the better approach.

Is there anywhere that the different test "commands" are documented?

I had a look, couldn't find a good reference apart from the client/mysqltest.cc code and the existing test cases. :grimacing: should be a better answer sorry.

Well anyway, thanks for pushing this out. Would it also be appropriate for me to attempt to apply this change to 10.11?

Strictly is a new feature so its outside the engineering guidelines to backport. Currently plan is to have a new LTS release out later this year.

grooverdan avatar Feb 15 '24 03:02 grooverdan

secure-file-priv from the CI did need to be set. Not sure why. After a second review we'll merge this in.

grooverdan avatar Feb 16 '24 03:02 grooverdan

Well, at least I'm not crazy!

XanC avatar Feb 16 '24 03:02 XanC

Is there anything else I should be doing with this?

XanC avatar Mar 12 '24 20:03 XanC

There is a problem with it, in that those writes are not killable. It is not hard to imagine someone writes more than PIPE_BUF into FIFO, but nobody is reading from it. It hangs, the connection is not killable, query timeouts do not work, it can't release locks that it might have like global locks (FTWRL), or table locks, user locks, and row locks. It can't finish transaction.

Thus writes must be interruptable/killable, or at least maybe timeout-able. We have nothing like this in mysys at the moment.

Also, this Windows exclusion is superficial. TBH, currently the our code can't even open named pipe because the name gets translated from \\.\pipe\something in a way that it loses the third character, the dot 🤦 , thus the name becomes some UNC network name, but this is for another day. Important is that we should not be excluding Windows out of superstition, this is not very sound.

vaintroub avatar Aug 01 '24 19:08 vaintroub

One way to go could be (On Linux) non-blocking writes, then wait in a poll() for fd to be writable, with another fd used as signal for the interrupt. another hack (on Linux) is pthread_kill, and look for "killed" indicator on EINTR, not sure how good it works. Windows has cancellable IO, so given file handle, one can interrupt its IOs.

Note that with regular files we do not have to worry about such stiff, all IOs finish rather quicky

vaintroub avatar Aug 01 '24 19:08 vaintroub

@vaintroub, thank you for the feedback. Killing the write is a situation I had not considered. I will (as time permits!) experiment with the mechanisms you suggested for making the writes killable.

I'm a bit confused as to what your position is on Windows support. I didn't originally include Windows support because I don't have much Windows infrastructure for development and testing, and I don't happen to need this feature on Windows. If your position is not to include this feature until it also works on Windows, that's a very reasonable position.

You go on to give a much better reason that the feature won't work on Windows: currently, the code can't even open the named pipe because of some kind of path munging.

Unless I'm missing what you mean by "superstition", neither of those reasons seem to be superstitious.

Are you willing to merge this feature as a Linux-only feature, or do you require Windows support too?

Thank you!

XanC avatar Aug 05 '24 04:08 XanC

What I mean with Windows is that - if you do not know if something will work or not, just do not add #ifndef _WIN32. If you'll disable it, we'll never know that \.\pipe gets converted to network share in some my_convert_dir. So please so not add ifdefs and ifndefs, just in case, if you do not know exactly and whether or not you broke something. I understand that maybe, for you adding a test case is challenging. But do not worry, you won't be adding that test case, its ok when it compiles.

vaintroub avatar Aug 05 '24 10:08 vaintroub