[PATCH] delete entries with entryUSN > lastUSN when lastUSN changes

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/734

Patches 1 and 2 adds support sysdb functions.
Patch 3 uses them to remove those entries.

[Simo Sorce]

On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/734
>
> Patches 1 and 2 adds support sysdb functions.
> Patch 3 uses them to remove those entries.

Sorry but this implementation doesn't seem to do what the ticket says
you should do.

Why are you deleting entries comparing entryUsn to a totally new and
uncomparable lastUSN ?

What should be done is that you should remove all entryUSN *attributes*,
then do one enumeration to refresh them all, then remove any entry that
has no updated entryUSN.

If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
simply removing *all* entries for no good reason, withy a lot of churn
in the ldb files due to memebrship removals etc, and lost of cached
password for users.

I guess this is a NACK on the approach unless I grossly misunderstood
something.

Simo.

[Pavel Březina]

On 08/07/2012 04:04 PM, Simo Sorce wrote:
> On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/734
> >
> > Patches 1 and 2 adds support sysdb functions.
> > Patch 3 uses them to remove those entries.
>
> Sorry but this implementation doesn't seem to do what the ticket says
> you should do.
>
> Why are you deleting entries comparing entryUsn to a totally new and
> uncomparable lastUSN ?

Because it was written by Jan as a solution in the ticket comment.
> What should be done is that you should remove all entryUSN *attributes*,
> then do one enumeration to refresh them all, then remove any entry that
> has no updated entryUSN.

I suppose this should be done only if enumeration is enabled, right?
> If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> simply removing *all* entries for no good reason, withy a lot of churn
> in the ldb files due to memebrship removals etc, and lost of cached
> password for users.
>
> I guess this is a NACK on the approach unless I grossly misunderstood
> something.
>
> Simo.

[Pavel Březina]

On 08/07/2012 04:04 PM, Simo Sorce wrote:
> On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/734
> >
> > Patches 1 and 2 adds support sysdb functions.
> > Patch 3 uses them to remove those entries.
>
> Sorry but this implementation doesn't seem to do what the ticket says
> you should do.
>
> Why are you deleting entries comparing entryUsn to a totally new and
> uncomparable lastUSN ?
>
> What should be done is that you should remove all entryUSN *attributes*,
> then do one enumeration to refresh them all, then remove any entry that
> has no updated entryUSN.
>
> If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> simply removing *all* entries for no good reason, withy a lot of churn
> in the ldb files due to memebrship removals etc, and lost of cached
> password for users.
>
> I guess this is a NACK on the approach unless I grossly misunderstood
> something.
>
> Simo.

The patch is completely rewritten to follow the approach mentioned above.

[Jakub Hrozek]

On Thu, Aug 16, 2012 at 05:42:45PM +0200, Pavel Březina wrote:
> On 08/07/2012 04:04 PM, Simo Sorce wrote:
> > On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/734
> > >
> > > Patches 1 and 2 adds support sysdb functions.
> > > Patch 3 uses them to remove those entries.
> >
> > Sorry but this implementation doesn't seem to do what the ticket says
> > you should do.
> >
> > Why are you deleting entries comparing entryUsn to a totally new and
> > uncomparable lastUSN ?
> >
> > What should be done is that you should remove all entryUSN *attributes*,
> > then do one enumeration to refresh them all, then remove any entry that
> > has no updated entryUSN.
> >
> > If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> > simply removing *all* entries for no good reason, withy a lot of churn
> > in the ldb files due to memebrship removals etc, and lost of cached
> > password for users.
> >
> > I guess this is a NACK on the approach unless I grossly misunderstood
> > something.
> >
> > Simo.
>
> The patch is completely rewritten to follow the approach mentioned above.

I think you should call sysdb_set_enumerated() for the domain once the
process finishes. See ldap_id_enumerate_reschedule().

Otherwise looks good to me.

[Pavel Březina]

On 08/22/2012 05:37 PM, Jakub Hrozek wrote:
> On Thu, Aug 16, 2012 at 05:42:45PM +0200, Pavel Březina wrote:
> > On 08/07/2012 04:04 PM, Simo Sorce wrote:
> > > On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/734
> > > >
> > > > Patches 1 and 2 adds support sysdb functions.
> > > > Patch 3 uses them to remove those entries.
> > >
> > > Sorry but this implementation doesn't seem to do what the ticket says
> > > you should do.
> > >
> > > Why are you deleting entries comparing entryUsn to a totally new and
> > > uncomparable lastUSN ?
> > >
> > > What should be done is that you should remove all entryUSN *attributes*,
> > > then do one enumeration to refresh them all, then remove any entry that
> > > has no updated entryUSN.
> > >
> > > If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> > > simply removing *all* entries for no good reason, withy a lot of churn
> > > in the ldb files due to memebrship removals etc, and lost of cached
> > > password for users.
> > >
> > > I guess this is a NACK on the approach unless I grossly misunderstood
> > > something.
> > >
> > > Simo.
> >
> > The patch is completely rewritten to follow the approach mentioned above.
>
> I think you should call sysdb_set_enumerated() for the domain once the
> process finishes. See ldap_id_enumerate_reschedule().
>
> Otherwise looks good to me.

Thanks. The patch is attached.

[Jakub Hrozek]

On Thu, Aug 23, 2012 at 10:12:57AM +0200, Pavel Březina wrote:
> On 08/22/2012 05:37 PM, Jakub Hrozek wrote:
> > On Thu, Aug 16, 2012 at 05:42:45PM +0200, Pavel Březina wrote:
> > > On 08/07/2012 04:04 PM, Simo Sorce wrote:
> > > > On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/734
> > > > >
> > > > > Patches 1 and 2 adds support sysdb functions.
> > > > > Patch 3 uses them to remove those entries.
> > > >
> > > > Sorry but this implementation doesn't seem to do what the ticket says
> > > > you should do.
> > > >
> > > > Why are you deleting entries comparing entryUsn to a totally new and
> > > > uncomparable lastUSN ?
> > > >
> > > > What should be done is that you should remove all entryUSN *attributes*,
> > > > then do one enumeration to refresh them all, then remove any entry that
> > > > has no updated entryUSN.
> > > >
> > > > If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> > > > simply removing *all* entries for no good reason, withy a lot of churn
> > > > in the ldb files due to memebrship removals etc, and lost of cached
> > > > password for users.
> > > >
> > > > I guess this is a NACK on the approach unless I grossly misunderstood
> > > > something.
> > > >
> > > > Simo.
> > >
> > > The patch is completely rewritten to follow the approach mentioned above.
> >
> > I think you should call sysdb_set_enumerated() for the domain once the
> > process finishes. See ldap_id_enumerate_reschedule().
> >
> > Otherwise looks good to me.
>
> Thanks. The patch is attached.

Ack

[Jakub Hrozek]

On Thu, Aug 23, 2012 at 01:08:21PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 23, 2012 at 10:12:57AM +0200, Pavel Březina wrote:
> > On 08/22/2012 05:37 PM, Jakub Hrozek wrote:
> > > On Thu, Aug 16, 2012 at 05:42:45PM +0200, Pavel Březina wrote:
> > > > On 08/07/2012 04:04 PM, Simo Sorce wrote:
> > > > > On Tue, 2012-08-07 at 15:23 +0200, Pavel Březina wrote:
> > > > > > https://fedorahosted.org/sssd/ticket/734
> > > > > >
> > > > > > Patches 1 and 2 adds support sysdb functions.
> > > > > > Patch 3 uses them to remove those entries.
> > > > >
> > > > > Sorry but this implementation doesn't seem to do what the ticket says
> > > > > you should do.
> > > > >
> > > > > Why are you deleting entries comparing entryUsn to a totally new and
> > > > > uncomparable lastUSN ?
> > > > >
> > > > > What should be done is that you should remove all entryUSN *attributes*,
> > > > > then do one enumeration to refresh them all, then remove any entry that
> > > > > has no updated entryUSN.
> > > > >
> > > > > If you remove entries with (old)entryUSN > (new)lastUSN you amy end up
> > > > > simply removing *all* entries for no good reason, withy a lot of churn
> > > > > in the ldb files due to memebrship removals etc, and lost of cached
> > > > > password for users.
> > > > >
> > > > > I guess this is a NACK on the approach unless I grossly misunderstood
> > > > > something.
> > > > >
> > > > > Simo.
> > > >
> > > > The patch is completely rewritten to follow the approach mentioned above.
> > >
> > > I think you should call sysdb_set_enumerated() for the domain once the
> > > process finishes. See ldap_id_enumerate_reschedule().
> > >
> > > Otherwise looks good to me.
> >
> > Thanks. The patch is attached.
>
> Ack

Pushed to master.