FreeTDS / freetds

Official FreeTDS repository
http://www.freetds.org/
GNU General Public License v2.0
460 stars 158 forks source link

Change to SYBASE CS_DATAFMT 15.0 #431

Open yairlenga opened 2 years ago

yairlenga commented 2 years ago

In Sybase 15, the CS_DATAFMT structure was modified. The attribute name used to be char[CS_MAX_NAME] (CS_MAX_NAME=132), and was modified to char[CS_MAX_CHAR] (CS_MAX_CHAR=256).

In FREETDS (I'm using 1.3.3) cs_types.h, continue to define the structure with CS_MAX_NAME. This make it possible to compile, and run code that does not use the "large identifier name", which in most cases work - as very few real world databases will have column names larger than 132.

The limitation is with binary compatibility. The FREETDS libct.so library provides binary compatibility with Sybase libct.so for many programs - changing the LD_LIBRARY_PATH to point to freetds allow existing programs to run. This is important, as we are using FREETDS ability to make the program run against SQL Servers. Because of the size changes, code that uses the CS_DATAFMT (ct_bind, ct_describe) is NOT binary compatible with Sybase.

My Request: Please update cs_types.h, so that CS_DATAFMT will define 'char name[CS_MAX_NAME]', matching CS_MAX_NAME with Sybase. Either as the default option, or as a config option ("long ids"). This solution will allow large number of binaries compiled against Sybase code to run without re-linking against freetds.

Important: Recently, AWS announce Babelfish (PostgreSQL connectivity via TDS). With FREETDS, programs compiled against Sybase libraries can run against AWS PostgreSQL instance. The binary compatibility is esstential.

Requested Change - 2 lines - in cstypes.h

define CS_MAX_CHAR 256

and then in cs_datafmt: char name[CS_MAX_CHAR], instead of char name[132]

fziglio commented 2 years ago

But this change would break other programs expecting that length to be 132. Yes, an option (configure I suppose) could help, or we could provide 2 libraries (one with 132 and the other with 256). I though the 2 libraries (Sybase and FreeTDS) were not binary compatible!

fziglio commented 2 years ago

ok, the way Sybase managed to change the structure is using CS_VERSION_xxx constants. These constants are different if the the structures are using large arrays, so they can keep the ABI

yairlenga commented 2 years ago

ok, the way Sybase managed to change the structure is using CS_VERSION_xxx constants. These constants are different if the the structures are using large arrays, so they can keep the ABI

The comment about other programs expecting the old size (132) is a fair. I would like to argue that as of today, the default build should the long one (256), and not the short one, based on the following:

It will be nice to have, as you suggested, an option to use the 132 byte size, but to have the 256 as the out of the box default.

I believe same applies to few other structure (CS_OBJNAME, ...). Will be nice to fix them all with the same conditional #ifdef (CS_NO_LARGE_IDENTIFIERS).

Yair

fziglio commented 2 years ago

Working on it, trying to implement both, as Sybase libraries are doing. Missing CS_SERVERMSG at the moment.

fziglio commented 2 years ago

Can you try https://github.com/FreeTDS/freetds/commit/0bcb077c8b94c4e6bf211471143c7feadb420387 ? It would be nice also a review, I'm not really satisfy of it.

yairlenga commented 2 years ago

Hi, Thanks for implementing. I'm on vacation, I will test next week, when I get back to the office, Yair.

On Sun, Dec 12, 2021 at 7:27 PM Frediano Ziglio @.***> wrote:

Can you try 0bcb077 https://github.com/FreeTDS/freetds/commit/0bcb077c8b94c4e6bf211471143c7feadb420387 ? It would be nice also a review, I'm not really satisfy of it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FreeTDS/freetds/issues/431#issuecomment-991937757, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFDOJWX3U7YALFWUKRUOT3UQTLRNANCNFSM5ISN2EQA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.