The Samba-Bugzilla – Bug 10008
dbwrap_ctdb broken for deleted records in 4.1
Last modified: 2016-07-03 06:50:51 UTC
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
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
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]
Comment on attachment 9200 [details]
==> 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.
Let's set this one to fixed