[Qgis-developer] [Python] QgsFeatureIterator for geometryless PostgreSQL-Layer

[Martin Dobias]

On Fri, Apr 12, 2013 at 10:33 AM, Jürgen E. <jef at norbit.de> wrote:
>> Recently I have been wondering whether it would not be better to
>> actually stop using flags and replace them by usual getter/setter
>> functions. My main concern is the setSubsetOfAttributes() function: it
>> also sets the flag, so the users may end up with a weird behavior,
>> e.g.:
>> QgsFeatureRequest().setSubsetOfAttributes( ["name","type"], fields
>> ).setFlags( NoGeometry )
>> That would fetch all attributes... because the setFlags overrides the
>> previously set SubsetOfAttributes flag.
>
> Yes, I've also thought about that, but somehow missed to speak up - but IIRC
> (;)) I migrated everything accordingly.
>
> Given that the flags need qualification it's even shorter to use the
> getter/setters API-wise.

That's right.
Thinking about it again, we should really fix the API before 2.0. It
seems you have ported that correctly (btw. many thanks for that!), but
ordinary users that do not read c++ code will sooner or later struggle
with it. I'm adding that to my todo list :-)


>> Also, the way how things are done right now, providers need to
>> implement fetching of attributes in this way (pseudo-code):
>> if using subset of attributes:
>>   for each attribute from subset:
>>     fetch attribute
>> else:
>>   for each attribute from provider:
>>     fetch attribute
>
>> This is quite annoying, we should probably have a simple array where
>> each value would indicate whether to fetch attribute with that index
>> or not (by default all values would be true). The fetching would be
>> simplified to:
>> for each attribute from provider:
>>   if attribute should be fetched:
>>     fetch attribute
>
>> What do you think? :-)
>
> Don't they just all do:
>
> subsetOfAttributes ? mRequest.subsetOfAttributes() : P->attributeIndexes()

Not really. Usually there's the kind of code I have mentioned above.
In theory we could change it to use the attributeIndexes() call.

> Can't we move that to a requestedAttributes() in QgsAbstractFeatureIterator and
> just iterate over the result?

Yes we can :-)


> BTW what about parallel iterators?  I suppose there were problems and thats why
> you added the "active iterators" - do you recall which providers were affected
> and how?

I have modified providers to have pointers to active iterators because
of two reasons:
1. if the user asks for a second iterator and the provider does not
support multiple iterators, the first one has to be closed - they
would not behave correctly if used together
2. if the layer is deleted for whatever reason, we must ensure that
any iterators that may still exist will be closed - they would cause
crashes otherwise

As of now, all providers support only one iterator per provider
instance. For parallel iterators (N iterators, one thread) we could
have a provider capability indicating whether they are supported or
not. Providers supporting parallel iterators would not close the
previously active iterators - instead they would only keep them in the
list of active iterators.

When we add multi-threaded rendering, things will get a bit more
complex. Providers allowing parallel iterators should be fine - they
will just need some locking of the list of active iterators. There
will be a slight problem with providers allowing just one iterator: in
a different thread we cannot simply close the currently active
iterator (from a different thread) as that would be a run condition. I
guess we will have to define the behavior in the following way - when
the provider is asked for a new iterator, it will:
a. wait for the active iterator to get closed - if it comes from a
different thread
b. close the old iterator - in case it comes from the same thread
This should ensure that the rendering (or processing) thread will wait
until GUI thread finishes what it wants (and vice versa), while still
keeping the current behavior when dealing with only one thread. We
will just need to explicitly assign iterators with the threads that
asks for them.
Btw. the case b is really just a sanity check for invalid code - it
should not happen in production environment - if it does, there is
something wrong with the application logic.

Maybe during the porting of providers to support multi-threaded
environment we will find out that it will be easier to simply update
all providers to allow multiple iterators instead of adding the
support for the case of N threads and one iterator - that is yet
something that needs investigation.

Regards
Martin