[Oledb-dev] A major rewrite of the Postgres OLE DB Provider.

[Shachar Shemesh]

Jeremy Lea wrote:
> The security differences are really minor...  If you're letting user's
> inject SQL, then you better be handling it., no matter if they can run
> multiple statements.
>   
As are the performance. If you are using so many statements in a row
that parsing and RTT becomes an issue, you are obviously way past the
point where you should have used stored procedures.
> Earlier versions are not going to work,
They are not something I'm concerned about.
> The current design doesn't do DBTYPE to pg_type oid mapping properly,
>   
Define "properly". As a developer, you have full control over what oid a
certain DBTYPE will map to, assuming it's handled. Not all reverse
mappings are handled.
> doesn't handle a number of corner cases as required by the OLE DB docs
> (DBTYPE_NUMERIC colinfo comes to mind).
These are missing and partial implementations, not design problems at
all. The design is up to, no sweat. It was, ha ha, designed for it.
>   The code is really messy in
> it's handling of bytea's BYREF stuff,
No. bytea at large are so partially implemented in the current code that
it was not defined working at all. Again, this is an IMPLEMENTATION
deficiency, not a design one.
>  and the "_text" type is completely
> wrong.
Same as above.
>   It doesn't pass out a SAFEARRAY.  There were a lot more things,
>   
Implementation or design?
> besides the weird C callback design.
See, there was a reason for that:
1. I needed to support non builtin types (varcharci and utinyint).
2. I wanted to avoid, at all costs, code duplication or having the same
functionality implemented all over the case (i.e. - a switch at the
parent, code at the child).
3. It is much much much more run-time efficient (O(log n) for std::map,
O(1) for std::hashmap, O(n) with your implementation).
>  but I forgotten a lot of them -
> when I started looking at why my parameter passing wasn't working, I
> quickly realised that the code would have to be redone from scratch... 
>   
Sounds to me like a heavy case of NIH. It's not like you sent an email
to the mailing list saying "hey, let's make this code better". In fact,
if I had ever heard your name prior to sending the above email, it may
have been different. Had Konstantin sent the exact same email, I would
have understood. He made some effort to communicate over the existing
driver, and he is not an unknown face. An unknown face coming over and
saying "what you did is a mess, here's my better solution" is, at best,
insensitive.

There's nothing wrong with you going your own development effort in
private. After all, that's why I released the code as LGPL. It's just a
bit offensive of you to sling mud over the existing implementation.

And don't get me wrong. Your provider ISN'T better. I'm sure it
scratches your specific itch better than the official one, but for
general consumption I doubt it is. You removed debugged code and
replaced it with new one. There is no way that code that you wrote for
self consumption will be better debugged than code that has been exposed
to hundreds of users over the span of almost two years. It just doesn't
work that way. At best this means that you have wasted who know how much
time over code that only you will ever use. At worst, this is a split of
public resources that are already scarce.
>>>  - Much cleaner code (IMHO), no more STL or exceptions.
>>>       
>> A matter of taste. I think this design breaks every OOD instruction out
>> there (read - no switch-case on child classes).
>>     
>
> Better than throwing exceptions in functions which aren't declared to
> throw...
Get your C++ straight. There is a difference between throwing from
methods not declared to throw and throwing from methods declared to not
throw. There is plenty reasons to do the first (for example - inability
to commit to subfunction's actions). I have not done the second.
>   Dropping the STL in favour of ATL classes reduced the code
> size by about half and having fewer classes really makes it easier to
> understand.
>   
You have more, not less, classes. In any case, the type system was
documented in the readme, precisely because I thought it would be insane
to just dive in. A system of callbacks is the standard way of
implementing a type system. I deemed the method in which the function
pointers are initialized during construction preferable to flooding the
namespace with singleton classes. STL allowed me to use efficient (O(log
n) for the standard STL, O(1) for the enhanced one) lookup of types.

If you don't agree with any of the above reasoning, that's fine. You are
master of your own driver, after all. Do not, however, hint that it's
arbitrary. This is not the only place that your lack of regard for
driver performance is evident.
>>>  - Parsing of parameters on stored procedures.  This was a hack,
>>>       
>> Yes, but the hack is in the OLE-DB's specs. Quite a number of programs
>> don't work without it.
>>     
>>>  which
>>>    was being done at the wrong place in the code.
>>>   
>>>       
>> I'd love it if you elaborated on that.
>>     
>
> The spec says it must be done in GetParameterInfo, you're also doing it
> in Execute.  Once the user has set up an Accessor, you can't play with
> it.
>   
Truth be told, I don't remember the reasoning any longer. I think it had
something to do with "GetParameterInfo" not being guaranteed to even be
called, but I may be misremebering. Also, the specs per-se did not even
mention the function callback support method. It was only that when you
got into the actual practice, that you found out that ADO allows itself
to assume that passing '{ call functionanme }' and expecting the driver
to understand.

Either way, I'm not ruling out the possibility that things could be done
better. I'm just wondering whether throwing it all away and starting
over, in private, was really the best way to go about it.
> No, that's what the current code does - it converts from GMT to local
> time and doesn't convert back.  Well sort of - I don't think it supports
> writing timestamps?
No, the implementation doesn't. The DESIGN, however, does. You were
mud-slinging the design.
>  (I can't see at the moment how this works - I
> presume ADO just sends these as text stings?
No. It just doesn't work. An exception is thrown if you try. Again, this
is by design.
> As for the future of the project...  I needed code to work.  It now
> works.  The provider, unpatched does not for what I need. 
>   
May you enjoy it. Again, it's not the fact that you re-wrote it that
annoys me. It's the fact that you assume, publicly and in a somewhat
offending manner, that if it's not perfect for you, then your driver
must be much superior.

I do believe that had you passed your criticism through the mailing
list, we would now have a driver that does what you needed it to do, did
it without sacrificing things that are important to others, and most
importantly, did not result in you spending who knows how long on a
driver that you are now legally forbidden from passing on to anybody
else in either object or source form.

I'm hoping to have the time sometime to try and salvage the things that
are usable from your driver and enhance the public one with them.
Current time allocations being what they are, this will probably take a
while.

If you do decide to retry for a publicly releasable driver, I hope, for
everyone's sake, that you at least try to do it my way first.

If you don't, I will ask you one thing. Don't call your resulting file
"pgoledb.dll", and don't use the same GUID as my driver. Postgresql
would not be the first DB to have more than one driver. Let's make sure
that people can tell the difference between the two.
> Regards,
>   -Jeremy
>   
Shachar