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
mmapstorage doesn't work with kdb export #2419
Comments
Thank you for trying this and for this detailed report!
Yes, I fully agree. Maybe we even should have a |
Looking at the code (and the error message) I think the current problem is that we cannot call In any case we could simply check whether the output file is
If anything, I would have a status |
I take that back... DON'T TRY THAT! Calling |
I think the best solution is that |
I think plugins should create the temporary file themselves, if needed. That way we avoid the performance penalties for plugins that can directly output to TTYs. AFAIK Also for |
Then the plugins (or more specifically mmap) would need to know if it is used within KDB or @mpranj What is your opinion? Can this be fixed within the mmap plugin?
In which cases is this performance penalty a problem? Maybe in the backup/restore for every test case?
For export yes. But for import we had several plugins that do not work. If a plugin uses fseek or similar it obviously cannot work, e.g. csvstorage or mozprefs. (dump should now be fixed)
You mean tests/shell/check_export.sh line 46? |
Actually, probably never. As long as you use the 3 argument versions of export/import instead of piping. See proposal below.
Yes, but that is broken, because I think to solve this quickly and easily we should do the following:
PS. This might be |
Thank you for reporting that is_not_rw_storage is broken, I opened #2423 |
Sorry, I was away for a week so I could not look into it. The mmap error messages might be misleading. From POSIX:
As for solutions within mmapstorage, we can check if it is a regular file with
I'm open to other solutions too. The solutions above do not change much of the mmapstorage logic. Reworking mmapstorage to work with pipes or stdout directly does not make too much sense to me. |
Thank you for your reply!
Please improve the error messages.
Please add that info to the error messages.
As your plugin is currently the only affected one, it makes sense that you do the stat and copy everything if needed. Then we could close this issue without larger changes in the framework. |
We should also update the man pages for |
Thank you both for reporting the issue and for the input! Unfortunately, I will make the requested changes in a PR this week. |
Maybe we do not need the argument to specify the file? The argument would conflict once
Thank you! |
Then we have to support
We could easily move to using the options |
Yes, we can add -i, -o options. But fixing the plugins (or the import/export framework) so that also stdin/stdout works would be nice in any case. |
To fix this problem with This works now: To make the solution consistent (thus completely compatible with non-regular files) it would be needed to solve it for the kdbGet() function too. There are roughly three solutions for this:
Is any of this desirable or do we ignore it? |
I think we can ignore it for now, as we already have quickdump. Simply document it as not being suitable for serialization. We will now rework the import/export framework anyway, then we will find a proper solution. @mpranj I unassigned you for now. |
#2639 closes all problems here. It works fine on Linux, I just need to fix a bug so it works on the BSDs too. |
I had a really nice solution (using I have decided to throw it away, thus not making mmapstorage completely compatible with non-regular files. It will only work with kdb import/export. The new solution will simply check if kdb import/export is used, by checking if the file is " |
This is the commented out solution?
You are right, the framework should handle this. I created #2640.
Also with the
Where are the differences? Can this code be moved to the import/export framework? |
I left it in the history but removed the lines later. The imho better solution was up until a523f9b. Note that it worked well on Linux but not BSD. One problem I encountered is that stdin/stdout can not be open()ed on BSDs. The other is that you really have to use the realpath to stat() the file and determine whether it is a regular file. Otherwise stat only resolved one level of symlinks for me. This approach failed on BSDs for me, because realpath resolved to a nonexistent file somehow.
Yes!
The relevant part is the same, sorry for the confusion. What I meant is that we strcmp for /dev/stdin instead of using stat to determine whether it is a regular file. That means that it will still fail if we use /dev/fd/. My other solution used stat instead of strcmp so it worked with any path. Edit:
Yes I think the code was almost completely there, but I had no time to fix the BSD problems properly. |
Steps to Reproduce the Problem
Expected Result
A file called
test.mmap
is created, which can be re-imported withkdb import
.Actual Result
An empty file called
test.mmap
is created and the following message is printed (includes logs fromENABLE_LOGGER
):System Information
Further Information
Works when(EDIT: see comment below)kdb export
is called as root.kdb export user/tests/hello mmapstorage test.mmap
also works, but is a completely undocumented feature ofkdb export
. So a better error message and updated documentation might be all we need.The text was updated successfully, but these errors were encountered: