Re: FW: 0.82 freebcp: -h option seems to be broken

2008/12/19 Lowden, James K <[email protected]>

> Said Constantin Vasilyev:
> > > freebcp tempdb.dbo.foo in /tmp/foo.bcp -S MSSQLSRV -c -h
> > CHECK_CONSTRAINTS -U xxx -P yyy
> > Msg 20076, Level 7
> > bcp_init must be called before any other bcp routines
> >
> > db-lib: Unable to set hint "CHECK_CONSTRAINTS"
> >
> > I looked into the code and it appears like freebcp is trying to call
> > bcp_options before bcp_init.
> >
> > While being there, I also checked bcp_options function and found that
> > list of valid
> > bcp hints is incomplete (FIRE_TRIGGERS hint is missing).
>
> Oh, crumb.  Actually, the -h option in freebcp is just broken.
>
> True, bcp_options() is called before bcp_init().  But that's technically
> OK, or was, because bcp_options() just records the hint for later use.
> The hint text is appended to the INSERT BULK statement generated by the
> user calling bcp_init().
>
> The error you stumbled on is caused by parameter checking code added
> later to bcp_options.  It's been there for a year at least and you're
> the first to mention it.  :-/
>
> freebcp simply grabs whatever is passed to the -h option and sends it
> verbatim to bcp_options.  That was my original idea: let the user format
> the hint text and let the server accept/reject it.  That was bcp.c
> revision 1.45.  All it did was make sure the option text started with
> something that looked like a hint.  Nothing checked that the hint text
> was completely correct.  It could say "ROW_PER_BATCH IS A DUMB IDEA;
> drop sysobjects" and that's what would go to the server.
>
> ISTR an objection somewhere along the line that we shouldn't pass
> user-provided text unvetted to the server, and the code was changed to
> pass the internal static validation string instead of the user-provided
> one.  Consequently if you pass "ROWS_PER_BATCH = 100", only
> "ROWS_PER_BATCH" goes to the server.  Which won't work.
>
> (Honestly, don't see a problem passing the text.  The freebcp user isn't
> malicious, and a malformed INSERT BULK statement will likely result only
> in the server dropping the connection, hardly a disaster.)
>
> So, as of today:
>
> 1.  No hints work because they're stopped by the parameter checker.
> 2.  If they were passed, only non-valued options would work because the
> text is truncated before the "=" sign.
> 3.  The option list is incomplete, as you said.
>
> And there's more...
>
> The hint syntax is a mini-language of its own.  Consider parsing "ORDER
> (column [ASC | DESC] [,...n])" and btw the "," is also used to separate
> successive hints.  That's a job for regex, but the last time I pulled in
> regex, Frediano rewrote the code to remove the dependency.  I don't want
> to see a whole mini-parser for this written with strtok(3).  I'd rather
> see either "no hints without HAVE_REGEX" or "raw hints without
> HAVE_REGEX".
>
>
I would prefer if FreeTDS does not compile.

And the bcp_options interface should be changed.  Sybase defines one
> measly option, BCPLABELED, that we don't even support.  We should change
> the signature to:
>
> bcp_options( DBPROCESS *, bcp_option_t, char* value, int);
>
> where bcp_option_t is an enumerated type representing the hints, and the
> value is a good, old-fashioned C string.
>
> My proposal:
>
> 1.  Add regex to freebcp. (This might have other benefits, e.g. support
> for CSV files.)
>

I don't remember why I removed regex but probably was a portability issue.
Windows for instance does not support regex. Also the bug "Having two kinds
of REs is a botch." is not that great. I would personally use only extended
syntax or even use PCRE (http://www.pcre.org/) library instead, it's very
portable! And well.. Perl re are even much more powerful too! Using POSIX
regex where available and another library on other systems could be a pain.

I just did a quick search for "posix regex windows" and results are not
very exiting, quite old libraries or mostly links to pcre.


> 2.  freebcp parses the hints and passes an enum-value pair to
> bcp_options.
> 3.  Make sure bcp_init is called first.
> 4.  Support multiple hints.  tds_bcpinfo::hint becomes char**.
>
> That should do it.  Thanks for the problem report.  I'm so glad to have
> something to work on, now that version-autodetection hasn't been
> started.  Feel free to jump in any time.  ;-)
>
> --jkl
>
>
Frediano