FreeTDS Development Group

[Constantin Vasilyev <vasilyev AT ncbi.nlm.nih.gov>]

Hi James,

Thanks for looking into this. 
Unfortunatelly (and as always) I have no time for carefull C programming :(,
so I just made a quick fix by removing all hints-related error-checking from 
bcp.c
and passing whatever values submitted with -h option directly to the server.
Here's what the MS SQL says if option(s) is incorrect:

Starting copy...
Msg 170, Level 15, State 1
Server 'MSSQLSRV', Line 1
        Line 1: Incorrect syntax near 'CHECK_CONSTR'.
Msg 170, Level 15
General SQL Server error: Check messages from the SQL Server


That looks like a descent error handling to me, so I'm going to stick with 
this solution for now.

Here's the code (sorry, noting fancy):

        switch (option) {
        case BCPLABELED:
                tdsdump_log(TDS_DBG_FUNC, "UNIMPLEMENTED bcp option: 
BCPLABELED\n");
                break;
        case BCPHINTS:
                if (!value || valuelen <= 0)
                        break;
                dbproc->bcpinfo->hint = value;
                return SUCCEED;
        default:
                tdsdump_log(TDS_DBG_FUNC, "UNIMPLEMENTED bcp option: %u\n", 
option);
                break;
        }


I'm sure you can make something better. Will watch this thread and accept
whatever you have to offer.

Thanks again!

Constantin Vasilyev
NCBI  Contractor, DBA

On Fri, Dec 19, 2008 at 05:09:37PM -0500, Lowden, James K wrote:
> 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".  
> 
> 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.) 
> 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
> 
> 
> 
> 
> 
> _______________________________________________
> FreeTDS mailing list
> FreeTDS AT lists.ibiblio.org
> http://lists.ibiblio.org/mailman/listinfo/freetds