One thing that is currently severly broken in master and in v4-1-test is running with CTDB. dbwrap_ctdb and dbwrap_tdb have different semantics when it comes to deleted records that break code at multiple places and lead not only to ugly messages spammed into the logs but also to functional breakage. e.g. dbwrap_exists() will report that a record that was deleted briefly before still exists. It will currently only go away once fast vacuuming has kicked in. In the dbwrap_ctdb case, there is currently no way to distinguish between an empty and a deleted record as records to be deleted will be reduced to the CTDB header and then added to the fast deletion queue (and left for fast vacuuming to delete them finally). dbwrap_tdb will delete the record immediately We'll probably need a DELETED or INACTIVE flag in the CTDB ltdb header that dbwrap_ctdb can look at. This then also needs to be respected in the CTDB recovery, R/O copy and vacuuming code.
Any news on this one? It's a blocker for 4.1.0...
This requires significant development effort in CTDB and then corresponding changes in Samba. I am not sure if I can find any time to fix this in next few weeks. Since CTDB is not yet part of Samba tree, can this be pushed to 4.2?
Well, this behaviour has been around in CTDB since the beginning. The design flaw is that that there is no distinction between deleted and validly stored (active) empty records. So this is not new. And it can (as Amitay has commented) not be solved with a trivial patch: If we treat all empty records either as existing or as deletetd, dbwrap_exists() either procudes false positives or false negatives. It can only be solved with introducing a new flag in the CTDB record header (as Christian has proposed) that signals that a record has been deleted (or is active). This is actually the right thing to do (tm), and I have proposed such changes 3 or 4 years ago already. But it was always rejected as too invasive and not upgrade-compatible. Indeed, the use of such a flag needs changes in both ctdb code and samba code (dbwrap_ctdb) to be aligned to work. Up to Samba 3.6 this issue was not bad enough to trigger a discussion at this level. But it seems that (with 4.0 already) the usage of the dbwrap_exists() from Samba code has changed, so that the known issue pops up much more prominently in the logs. So what to do? - We can either fix it, which is as Amitay says something that needs some time, since it has to be done very carefully (even if the resulting patches would not be huge). (Applicable if this is mostly cosmetic.) - We can raise the debug level of the nagging messages and fix this properly for 4.2. Opinions? I'm undecided..
From my point of view, raising the debug level for 4.1.0 and fixing it properly for 4.2 sounds reasonable in this case.
I do not think this is the correct way to go forward with for 4.1. It will silently break code that uses dbwrap_exists(). How about treating empty records as deleted? I have some incomplete patches for that. We probably have less code that expects records to be possibly empty than we have code that can deal with deleted records to be reported as empty and therefore trying to parse their contents.
(In reply to comment #5) > I do not think this is the correct way to go forward with for 4.1. > It will silently break code that uses dbwrap_exists(). > > How about treating empty records as deleted? I have some incomplete patches for > that. We probably have less code that expects records to be possibly empty than > we have code that can deal with deleted records to be reported as empty and > therefore trying to parse their contents. We need decisions/patches soon. The release branch will be frozen in one week. It's still a blocker.
Christian, would Volker's patch to treat all empty records as non-existing records do for now?
(In reply to comment #7) > Christian, would Volker's patch to treat all empty records as non-existing > records do for now? For 4.1, that should do it. But in the long run, we'll need to be able to distinguish properly between empty and deleted records.
Created attachment 9200 [details] Patch
Comment on attachment 9200 [details] Patch ACK
==> Karolin for inclusion into 4.1
Pushed to autobuild-v4-1-test.
(In reply to comment #12) > Pushed to autobuild-v4-1-test. Pushed to v4-1-test. Re-assigning to Volker. Volker, can we close this one or should we leave it as a reminder for the further changes proposed by ambi?
(In reply to comment #13) > Volker, can we close this one or should we leave it as a reminder for the > further changes proposed by ambi? Your choice. It's on the long, long list of possible enhancements now. Volker
Let's set this one to fixed