MS RFC: Layer Plug-in Architecture

[Jani Averbach]

Hello Frank,

On 8/22/05, Frank Warmerdam wrote: 
> On 8/22/05, Jani Averbach <javerbach at extendthereach.com> wrote:
>
> I am assuming there will still be quite a few generic layer operations
> not going through the vtable, as now, right?  The vtable is only for
> backend-format specific operations.

Yes, I was unclear, I will add this to the proposal.

> I don't think it is a convention to use hungarian prefix in generic
> MapServer code, is it?  I would suggest just calling the functions
> in the vtable something like "LayerIinitItemInfo" instead of prefixing
> with fp.  If we were to use a prefix, I would encourage pfn (pointer
> to function) to match GDAL/OGR and other hungarian usage.

I will use whatever notations are liked.

> I would also like to suggest that the functions like
> msLayerInitItemInfo() that call into the vtable ought to do some
> reasonable default action (like nothing) if the function pointers
> are NULL.  So layers only need to implement (ie. override) those
> functions that make sense.

Yes, another option is to initialize vtable with reasonable dummies,
so we don't have to have an extra check if pointers are null or not. I
would lean towards this because then we don't have to have extra
logic in maplayer.
 
> >    2.2.  New functions and/or fields for vtable
> >
> >          2.2.1. CloseConnection
> >
> >                 This function is used to actually close the
> >                 connection, in case that layer implements some kind of
> >                 connection pooling by its own.  If layer doesn't use
> >                 any connection pooling, this function should be
> >                 implemented as no-op. Caller should first call layer's
> >                 "close" function, and finally at the very end
> >                 ``CloseConnection``.
> >
> >                 The signature of function is
> >                 ::
> >
> >                    int (*fpLayerCloseConnection)(layerObj *layer);
> >
> >
> >                 And the main place where this function will be called,
> >                 is ``mapfile.c: 4822``
> >                 ::
> >
> >                    void msCloseConnections(mapObj *map)
> >
> >                 This function is needed because e.g. ``POSTGIS`` is
> >                 implementing this usage pattern at the moment
> >                 ``maplayer.c:599``
> >                 ::
> >
> >                     void msLayerClose(layerObj *layer)
> >                     ...
> >                    /*
> >                     * Due to connection sharing, we need to close the
> >                     * results and free the cursor, but not close 
> >                     * the connection.
> >                     */
> >                     msPOSTGISLayerResultClose(layer);
> 
> I am confused about what the close connection method is
> for.  Isn't the existing msCloseConnections() logic just left over
> from the "old" connection pooling mechanism?  I think we ought
> to take the old connection pool stuff out as part of this overhaul. 

The problem with this is that it affects the design of vtable
functions.  We have to decide what should happen when LayerClose is
called.  The POSTGIS is standing out of the row now, by calling
''msPOSTGISLayerResultClose'' instead of LayerClose.  So we could
decide LayerClose is the real close and fix POSTGIS, or we could
implement this CloseConnection logic, when the Layer is closed by
LayerClose, whatever this means for specific layer, and any remaining
connections are closed by CloseConnection. If there isn't anything
special to do, then CloseConnection should be no-op for the layer.

This has to be decided before we could implement the proposal.


> >          2.2.2. GetConnectionTypeName
> >
> >                 This function is used to get MAP-file name for this
> >                 layer/connectiontype.  It is used when map file is
> >                 written back (``mapfile.c: 2856``), actual strings will
> >                 be: ``OGR``, ``POSTGIS`` and so on.
> 
> Will we, or will we not still have "connectiontype" entry in
> the layerObj?  I think we would, and that the connection type
> name can be derived from that the same way it is now. 

My reason for this function was an idea that at some point in the
future, we could start using CONNECTIONTYPE "string" type parser, and
derive connection type dynamically as an argument for
CONNECTIONTYPE (I think that was what Steve was thinking). So when
this day comes, we will need CONNECTIONTYPE argument to be provided by
layer.  Also, my personal opinion is that it is clearer this
way. Also, it doesn't cost to us much now, but adding it later will
need the revving of vtable. Either way, if you like to drop this I am fine
with it.


> >          2.2.3. SetTimeFilter
> >
> >                 This function is used to create a time filter for
> >                 layer. At the moment we have three special cases
> >                 (``maplayer.c: 1635``):
> 
> Aren't there other filter translation issues a well?  In an
> ideal world, layers ought to take care of transforming filters
> into their native format but it seems like this is a big piece
> to chew off.  I'm not clear why time is a special case of
> filter translation.
> 
> >    2.3. Things to fix or extra functions to add to the vtable
> >
> >         I would like to see following items fixed, but I don't know
> >         which way to go.
> >
> >         2.3.1. FLTApplyFilterToLayer (mapogcfilter.c: 1084)
> >
> >                Could we create two helper functions, one for "SQL"
> >                case and the another for "non-SQL" case, and set
> >                all layers, except ``POSTGIS`` and
> >                ``ORACLESPATIAL`` to call directly this "non-SQL"
> >                version of this helper function (else-branch of
> >                if).  ``ORACLESPATIAL`` and ``POSTGIS`` could use
> >                "SQL" version of the helper function (actual
> >                if-branch) if the conditions for this are met. This
> >                way we could remove all layer specific logic from
> >                this function, and make this function's logic
> >                actually generic.
> 
> Ahh, this addresses my earlier question I see.  I would note that
> OGR now uses SQL filter translation too. 

Sorry, I don't follow you: are you fine with SetTimerFilter and
changing FLTApplyFilterToLayer to be virtual table based?

> >         2.3.2. layerObj->items allocation
> >
> >                There is layer (SDE) specific code in ``msLayerWhichItems`` 
> >                (``maplayer.c: 880``) when initial items allocation is
> >                done.
> >                ::
> >
> >                   /* TODO: it would be nice to move this into the SDE
> >                    * code itself, feels wrong here...
> >                    */
> >                   if(layer->connectiontype == MS_SDE) {
> >                   ...
> >
> >                One solution would be to provide a layer specific
> >                function
> >                ::
> >
> >                   int (*fpCreateItems)(layerObj *)
> >
> >                which does the allocation and set numitems to correct
> >                value.
> 
> I don't understand what CreateItems does that is distinct from
> the InitItemInfo function.  Could you clarify? 

At the moment the space for layer->items and layer-numites is set
outside of layer specific abstraction level, inside msLayerWhichItems,
(maplayer.c:880). And after about 50 lines of code, msInitItemInfo is
called, with already allocated items.  So my idea was to provide
special allocator/creator function for ''items'' and ''numitems'' so
we don't have to have this special case for SDE, and layers could
allocate as much space for items as they need.  This isn't a road
block, so we could progress if this isn't resolved.

> 
> >         2.3.3. msCheckConnection (``mapfile.c: 4779``)
> >
> >                This api looks to be deprecated. It is called only from
> >                ``msMYGISLayerOpen``.  Is it hard to fix GIS to use
> >                mappool?  If this isn't feasible thing to do, we could
> >                remove all other cases and just leave GIS inside this
> >                function.
> 
> Is the MyGIS interface still in significant use?  My understanding
> is that it is no longer supported by Attila.  Perhaps it is time to
> remove MyGIS support since it seems hard to maintain.

Can't say anything about that.

> 
> > * To find best place to initialize layer's vtable. One possible place
> >   could be ``msLayerOpen``. The problem with that is that if we need
> >   before opening the layer something layer-specific information, it
> >   won't be available.
> 
> I had assumed that the vtable would be initialized at the point
> where the layerObj is established (when parsing the mapfile for
> instance).  However deferring it till the first LayerOpen might work ok.
> Perhaps all the msLayer* functions that can call into the vtable can
> call a function to initialize the vtable if it isn't already setup when
> they are called. 

As architecture wise, I feel that initializing it after parsing map
file is better, but I can't say for sure how well this fit for the
rest of layer's logic.
 
> > * I also like to note that this proposal won't remove all layer
> >   specific code from MapServer e.g. ``WMF``, ``WMS`` and ``GRATICULE``
> >   are used as special cases from place to place.
> 
> Are there any special issues with the raster query layer support
> which is handled via the layer API?

I am sorry but I don't have enough expertise to answer this. I will
add this to the proposal as an open issue if anybody doesn't follow
up.

> I would also like to suggest that as part of this process a "howto"
> document for implementing new layers be prepared.  I would be
> prepared to take a first pass at that.

That would be really helpful.
 
> Finally, you don't seem to have mentioned anything about custom
> layers that are dynamically loaded in this draft.  Are you planning
> to add that?
 
I left it out by purpose. This RFC lays all ground work what is needed
for dynamic layers, so pretty much all what is needed for dynamic
layers is specific for its own implementation. I have planned to
provide a next RFC for it after we have first sorted this one out. I
also liked to keep it out of this one because I felt that this
proposal is complicated enough as it is now. If you feel that it makes
more sense with this RFC, then I can add it.


Thank you for your comments,
Jani

-- 
Jani Averbach