Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codegen / logic issue for "GIXSQLCursorDeclareParams" #88

Closed
GitMensch opened this issue Jul 6, 2022 · 61 comments
Closed

codegen / logic issue for "GIXSQLCursorDeclareParams" #88

GitMensch opened this issue Jul 6, 2022 · 61 comments
Labels
bug Something isn't working

Comments

@GitMensch
Copy link
Contributor

A program using cursors get the following generated directly before the first section (I hope it isn't before / in DECLARATIVES if those exist);

*   ESQL STARTUP DECLARATIONS (START)
....
"GIXSQLStartSQL"
"GIXSQLSetSQLParams"   *> N times
"GIXSQLCursorDeclareParams"
"GIXSQLEndSQL"

... for each cursor

*   ESQL STARTUP DECLARATIONS (END) 

The issues here are:

  • this happens for all cursors - while the program may not use any of those for nearly all calls
  • this happens for each invocation of the program and after the first iteration the cursors are setup leading the generated _gixsqlCursorDeclare always ending in DBERR_CURSOR_EXISTS and SQLCA being updated with error codes.

Suggestion to solve this:

  • generated a variable with the cursor-name + "initialized" for each cursor in the program
  • before each use of the cursor in the program: check if that variable is space - when yes set it to "X" and do the initializations only for this cursor

If this takes "too long for the next version" (I personally see it as big drawback in GixSQL and therefore suggest to fix that "soon") then generate a MOVE SQLCA TO SQLCA-GIXSQL-BACKUP directly after ESQL STARTUP DECLARATIONS (START) and a MOVE SQLCA-GIXSQL-BACKUP TO SQLCA-GIXSQL-BACKUP directly before ESQL STARTUP DECLARATIONS (END).

@mridoni mridoni added the enhancement New feature or request label Jul 6, 2022
@GitMensch
Copy link
Contributor Author

re enhancement: as long as the SQLCA is not back-up'd and restored around this it is an actual error because the values "magically change" between invocations and the programs run as if the last statement was an error.

Readjusting the generation to do the preparation only once and only directly before the cursor is used the first time would definitely be both more work an enhancement improving performance and the ability to debug actual errors.

@mridoni mridoni added bug Something isn't working and removed enhancement New feature or request labels Jul 7, 2022
@mridoni
Copy link
Owner

mridoni commented Jul 7, 2022

You are right, the current behaviour is definitely a “logic” bug. I also guess that “selective/lazy” initialization should only apply to cursors declared in WORKING-STORAGE (those are the ones referenced in the “startup declarations”.

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

I have implemented what follows, let me know if it might work. It is all activated with a new preprocessor flag (--smart-cursor-init).

  • This only applies to cursors declared in WORKING-STORAGE. All the other cursors have no special treatment
  • A series of flags is defined in WORKING-STORAGE to keep track of cursor initialization
  • Cursor initialization statements are still output (almost) just after PROCEDURE DIVISION, but they are delimited by their own paragraphs
  • A paragraph definition is output just after the last cursor initialization.
  • A GO TO statement is output between PROCEDURE DIVISION and the first cursor initialization, that skips all the cursor initializations, going to the paragraph above
  • Before each OPEN, FETCH or CLOSE SQL statement the initialization flag is checked. If it is not set, cursor initialization is performed with a PERFORM THRU to the appropriate paragraph, the flag is set and control resumes.

You can look at the attached (preprocessed) program, it is the same code as in #81, only preprocessed with the --smart-cursor-init flag, it compiles and runs successfully.

TSQL027A.cbsql.txt

@GitMensch
Copy link
Contributor Author

GitMensch commented Jul 8, 2022

That seems very reasonable - I'd drop the -EX paragraphs and the -ST suffix, then just PERFORM GIXSQL-CI-P-TSQL027A-VM3.

I further suggest to move the complete block to the end of the processed program - this way you do not have that GO TO executed each time (visible by -ftrace-all and when using the debugger). This also simplifies the codegen as you do not have to look out for DECLARATIVES at all (at least not for gixsql) and you'd comply with the COBOL standard rule "if any paragraph is in a section, every paragraph must" by implicit inserting them into the last section if there is one.

@GitMensch
Copy link
Contributor Author

GitMensch commented Jul 8, 2022

You may want to insert the call twice as follows:

GIXSQL*    EXEC SQL AT :DBS
GIXSQL*        OPEN VM2 
GIXSQL*    END-EXEC.
GIXSQL     IF GIXSQL-CI-F-TSQL027A-VM2 = ' ' THEN
GIXSQL         PERFORM GIXSQL-CI-P-TSQL027A-VM2
GIXSQL         IF SQLCODE = 0
GIXSQL           MOVE 'X' TO GIXSQL-CI-F-TSQL027A-VM2
GIXSQL           CALL STATIC "GIXSQLCursorOpen" USING
GIXSQL               BY REFERENCE SQLCA
GIXSQL               BY REFERENCE "TSQL027A_VM2" & x"00"
GIXSQL           END-CALL
GIXSQL        END-IF
GIXSQL     ELSE
GIXSQL        CALL STATIC "GIXSQLCursorOpen" USING
GIXSQL            BY REFERENCE SQLCA
GIXSQL            BY REFERENCE "TSQL027A_VM2" & x"00"
GIXSQL        END-CALL
GIXSQL     END-IF

... likely better: less code one minor comparision generated more:

GIXSQL*    EXEC SQL AT :DBS
GIXSQL*        OPEN VM2 
GIXSQL*    END-EXEC.
GIXSQL     IF GIXSQL-CI-F-TSQL027A-VM2 = ' ' THEN
GIXSQL         PERFORM GIXSQL-CI-P-TSQL027A-VM2
GIXSQL         IF SQLCODE = 0
GIXSQL           MOVE 'X' TO GIXSQL-CI-F-TSQL027A-VM2
GIXSQL        END-IF
GIXSQL     END-IF
GIXSQL     IF GIXSQL-CI-F-TSQL027A-VM2 = 'X' THEN
GIXSQL        CALL STATIC "GIXSQLCursorOpen" USING
GIXSQL            BY REFERENCE SQLCA
GIXSQL            BY REFERENCE "TSQL027A_VM2" & x"00"
GIXSQL        END-CALL
GIXSQL     END-IF

--> marker is only set if the initialization worked and the cursor access is only done if it did, too.

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

That seems very reasonable - I'd drop the -EX paragraphs and the -ST suffix, then just PERFORM GIXSQL-CI-P-TSQL027A-VM3.

I agree, it is simpler and more readable.

And for some strange reason I'd place the MOVE 'X' TO GIXSQL-CI-F-TSQL027A-VM3 into GIXSQL-CI-P-TSQL027A-VM3 itself

In this case I wanted to avoid a jump, it might be more costly than checking the variable before (if needed) jumping, but I guess a lot depends on compiler optimizations.

I further suggest to move the complete block to the end of the processed programm

I am not sure about this : if there is no STOP RUN, EXIT PROGRAM or GOBACK before this initialization section, it will be executed, or am I missing something obvious?

@GitMensch
Copy link
Contributor Author

And for some strange reason I'd place the MOVE 'X' TO GIXSQL-CI-F-TSQL027A-VM3 into GIXSQL-CI-P-TSQL027A-VM3 itself

That's already edited out :-) The check with the sqlcode and then checking both ' ' and 'X' seems better in any case.

I further suggest to move the complete block to the end of the processed program

I am not sure about this : if there is no STOP RUN, EXIT PROGRAM or GOBACK before this initialization section, it will be executed, or am I missing something obvious?

only my intent to still leave the GO TO in as is. When the block is moved to the end the GO TO GIX-SKIP-CRSR-INIT will only be executed if / when the program "falls through", not on every call of the program as it is now.

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

Done, this is the new preprocessed output.

TSQL027A.cbsql.txt

@GitMensch
Copy link
Contributor Author

GitMensch commented Jul 8, 2022

Rechecked with 2 PCs - the new preprocessed output file is identical to the old one, maybe GH needs another name for it?

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

No, "I" need a check, because I uploaded the wrong file, sorry :-)

TSQL027A-2.cbsql.txt

@GitMensch
Copy link
Contributor Author

GIXSQL-CI-P-TSQL027A-VM2-ST is generated before OPEN, FETCH and CLOSE - shouldn't it be just before the OPEN (not sure, so asking)?

You may want to indent the lines within IF GIXSQL-CI-F-TSQL027A-VM2 = 'X' THEN and may want to add at least one generated comment line before GIXSQL* ESQL CURSOR DECLARATIONS (START) and GIXSQL* ESQL CURSOR INIT FLAGS (START), but other than that it looks good to me.

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

GIXSQL-CI-P-TSQL027A-VM2-ST is generated before OPEN, FETCH and CLOSE - shouldn't it be just before the OPEN (not sure, so asking)?

In theory, yes, but this would (probably) lead to a hard crash if the program flow is wrong and it - erroneously - performs a FETCH or a CLOSE before opening the cursor. This way, the cursor is always initialized, the FETCH fails, SQLCODE is set and hopefully intercepted, so the error in the program flow can be corrected. But I have doubts about it too, since it could slow down FETCH operations too much (e.g. in a tight loop) and a badly coded program can be expected to crash anyway. Since this is the first implementation of this feature, for now I will comment the checks in FETCH and CLOSE generation, then will see if there is a need for them to keep things "cleaner".

You may want to indent the lines within IF GIXSQL-CI-F-TSQL027A-VM2 = 'X' THEN and may want to add at least one generated comment line before GIXSQL* ESQL CURSOR DECLARATIONS (START) and GIXSQL* ESQL CURSOR INIT FLAGS (START), but other than that it looks good to me.

Ok, done

@GitMensch
Copy link
Contributor Author

n theory, yes, but this would (probably) lead to a hard crash if the program flow is wrong and it - erroneously - performs a FETCH or a CLOSE before opening the cursor.

There should be a test case ensuring this does not happen, and from a quick glance I see no reason why it should:

Cursor *cursor = cursor_manager.get(cname);
if (cursor == NULL) {
spdlog::error("cursor {} not registered", cname);
setStatus(st, NULL, DBERR_NO_SUCH_CURSOR);
return RESULT_FAILED;
}

It sets SQLCA as expected.

since it could slow down FETCH operations too much (e.g. in a tight loop)

Yes, that's the point.

Looking forward to 1.0.17 with upcoming fixes and features .I guess that may land somewhere next week?

@mridoni
Copy link
Owner

mridoni commented Jul 8, 2022

Looking forward to 1.0.17 with upcoming fixes and features .I guess that may land somewhere next week?

Yes, that's the release date I am more or less planning.

mridoni added a commit that referenced this issue Jul 13, 2022
- Added support for "smart" cursor initialization (#88)
- Added support for EXECUTE prepared-statement INTO #(87)
- Fixed a logging problem (#84)
- Fixed "wrong generated COBOL in 1.0.16" (#83)
- Fixed "missing "close" for spdlog?" (#82)
- Added support for using prepared statements in cursors (#81)
- Variable length fields indicators are now 32-bit long by default (#80)
- Added support for using variable length fields with prepared statements (#79)
- Added upport for using group fields in INSERT and SELECT..INTO statements (#6)
- Added support for more connection string formats (including ocesql compatibility) (#16)
- Added Support for DISCONNECT ALL (#89)
- Performed some refactoring to improve code size
- Fixed a few memory leaks
@GitMensch
Copy link
Contributor Author

Looks like the very good changes you already had for this did not get into 1.0.17?

I see ESQL CURSOR DECLARATIONS (START) still generated at start of PROCEDURE DIVISION, so SQLCODE is likely also still wrong on follow-up CALLs.

@mridoni
Copy link
Owner

mridoni commented Jul 22, 2022

Looks like the very good changes you already had for this did not get into 1.0.17?

Have you run gixpp with -L/smart-cursor-init? This should be present and working.

@GitMensch
Copy link
Contributor Author

GitMensch commented Jul 22, 2022

No, of course not, as it isn't mentioned in https://github.com/mridoni/gixsql/releases/tag/v1.0.17 - is there a reason for making this optional (even more: not default)?

I'm just executing the gixsql wrapper script (so as a workaround I'll add it there).

@GitMensch
Copy link
Contributor Author

Note: the flag changes the codegen, but does not work, the compiler complains about a bunch of not existing variables and sections:

TEST.cob:647: error: 'GIXSQL-CI-F-TEST-CSKEY01N' is not defined
TEST.cob:752: error: 'GIXSQL-CI-F-TEST-CSKEY01NG' is not defined
TEST.cob:912: error: 'GIXSQL-CI-F-TEST-CSKEY01B' is not defined
TEST.cob:1017: error: 'GIXSQL-CI-F-TEST-CSKEY01BG' is not defined
TEST.cob:648: error: 'GIXSQL-CI-P-TEST-CSKEY01N' is not defined
TEST.cob:753: error: 'GIXSQL-CI-P-TEST-CSKEY01NG' is not defined
TEST.cob:913: error: 'GIXSQL-CI-P-TEST-CSKEY01B' is not defined
TEST.cob:1018: error: 'GIXSQL-CI-P-TEST-CSKEY01BG' is not defined

@mridoni
Copy link
Owner

mridoni commented Jul 22, 2022

Ok, I will look into it again (and set "smart cursor init" as default). As you know my test passes, so there is probably something in the code you are testing with that exposes a bug not present in my own test code. I'll try to dig deeper.

Thanks

@GitMensch
Copy link
Contributor Author

Ok, I will look into it again (and set "smart cursor init" as default)

That has exposed bugs for the VARLENGTH32 issue, too, so maybe you get a reproducer "for free".

@GitMensch
Copy link
Contributor Author

I couldn't reproduce a broken -L on the first try (will try more), but found #94 during debugging (where I've found -L to generate reasonable working code).

@GitMensch
Copy link
Contributor Author

Think I've found it: it happens if you have the cursor declarations within the program, not within SQL EXEC DECLARE (before the PROCEDURE DIVISION). Shouldn't your tests catch this as soon you change the default to be -L?

@mridoni
Copy link
Owner

mridoni commented Jul 26, 2022

Yes, they should, but they didn't... 😞 I will look into it, thanks for your investigation.

@GitMensch
Copy link
Contributor Author

Another issue seen with cursor declarations in WORKING-STORAGE:

The variable GIXSQL-CI-F-PROG-CSKEY01NG was added directly before PROCEDURE DIVISION - but it must be placed at the end of WORKING-STORAGE (not in LINKAGE as it is currently...).

mridoni added a commit that referenced this issue Aug 23, 2022
- Added new Oracle driver, based on ODPI
- Added new SQLite driver
- All the drivers have been updated and now implement the complete set of supported features
- Solution for "PG: issue with prepared statements" (#99)
- Solution for "PCursors cannot be re-opened after close" (#98)
- Solution for "libgixpp: setStatus is called for errors without DBI parm passed - sets SQLERRM" (#94)
- Solution for "error handling (especially for 07001)" (#92)
- Solution for "show-stopper bug in pgsql_prepare" (#91)
- Solution for "PREPARE does not work with VARLENGTH groups (ocesql compat)" (#79)
- Partial solution for "PREPARE does not work with VARLENGTH groups (ocesql compat)" (#68)
- Solution for "The PostgreSQL driver needs START TRANSACTION before using cursors" (#14)
- Solution for "FR: support EXEC SQL VAR" (#21)
- Fixed a bug in "problems with "codegen / logic issue for "GIXSQLCursorDeclareParams" (#88)
- Solution for "FR: allow mapping of "NoRecCode"' (#95) - added --no-rec-code parameter to gixpp
- Tokens in the parser have been labeled to improve diagnostics (pulled PR #96 by @GitMensch)
- Fixed COMP-3 handling in drivers other than PostgreSQL
- Rewrote the test suite (still MSTest-based) to dynamically generate a matrix of test to be run on the various platforms/database drivers
- Added options for parameter generation in gixpp (-a was removed)
- Added new GIXSQL_FIXUP_PARAMS option for runtime, to automatically convert parameter format in prepared statments
- "Native" cursors are now the default for the PostgreSQL driver
- "Smart" cursor initialization is now the default for all cursors, including those declared in WORKING-STORAGE (-L was removed from gixpp), should fix #101
- Removed dynamic cursor emulation from the ODBC driver when using PostgreSQL
@GitMensch
Copy link
Contributor Author

Something is still amiss here: GIXSQLCursorDeclareParams is now only called once, which is possibly total correct, but GIXSQLSetSQLParams which is called before seems to be necessary to called each time on CURSOR OPEN to get the updated data, no? at least if the used parameters are in LINKAGE this is a must as their address change, it may or may not be necessary to get the new data when the cursor is re-opened after the fields change when they are defined in WORKING-STORAGE.

@mridoni
Copy link
Owner

mridoni commented Aug 24, 2022

Ok, I will look into it: it passed all the test cases, but it is really possible that this particular program flow eluded my tests.

Thanks

@GitMensch
Copy link
Contributor Author

I'm still looking for the error at hand, the parameters seem correct as GIXSQLCursorOpen calls DbInterfacePGSQL::cursor_open which reads the parameters; that would likely error though if the parameters were in LINKAGE because of the missing GIXSQLSetSQLParams (*only for parameters in LINKAGE!) the address is unchanged. I assume the other DB interfaces' cursor_open also read the parameters.

Minor thing I've seen during debugging: cursor->setOpened() should not be called from GIXSQLCursorOpen after it called cursor_open - as this already set the cursor state correctly.

@mridoni
Copy link
Owner

mridoni commented Aug 24, 2022

This could be solved with a bit of refactoring anyway, if necessary: for each cursor there are two paragraphs emitted in the initialization code at the end of the module:

  • "paragraph 1" is generated, named (as it is at the moment) GIXSQL-CI-P-MYCRSR. This would only contain the actual "declare" code
  • "paragraph 2" is generated, named - something like GIXSQL-CI-P-PAR-MYCRSR. This would only contain the StartSQL/SetSQLParams/EndSQL calls
  • when first opening a cursor, paragraph 2 is called, followed by paragraph 1
  • during the subsequent "open" calls, the current check for paragraph 1 is performed, but paragraph 2 is always called

@GitMensch
Copy link
Contributor Author

Sounds good. Note that if this is about the field information (type, length, address) only, then this is only necessary for items in LINKAGE (or defined with the BASED clause).

@mridoni
Copy link
Owner

mridoni commented Aug 24, 2022

Sounds good. Note that if this is about the field information (type, length, address) only, then this is only necessary for items in LINKAGE (or defined with the BASED clause).

Yes, but at that point it would be more linear to behave like this in every case: the cost of doing it while opening a cursor, compared with the actual "open" on the DBMS side, is almost non-existent.

@GitMensch
Copy link
Contributor Author

Retested: The codegen error with missing sections for cursor reads when the DECLARE is part of PROCEDURE DIVISION from 1.0.18,dev2 (or 1) is still there in 1.0.18 final. Can you reproduce this or do you need a reproducer (I could create one tomorrow)?

@mridoni
Copy link
Owner

mridoni commented Aug 24, 2022

Yes, a test case would be useful, since in all evidence the one I am using is not catching this.

Thanks

@GitMensch
Copy link
Contributor Author

GitMensch commented Aug 26, 2022

It always feels a bit strange if you take a big program and reduce it to the max for a minimal reproducer...

Here it is:

       IDENTIFICATION DIVISION.
       PROGRAM-ID.    TE609.
       DATA DIVISION.
       WORKING-STORAGE SECTION.
      *-----------------------------------------------------------*
           EXEC SQL BEGIN DECLARE SECTION END-EXEC.
      *
       01 DBS                 PIC  X(010).
       01 TABID               PIC S9(018).
       01 MODTIME             PIC  X(026).

       77 KEY01-PART          PIC  9(008) VALUE 42.
      *
           EXEC SQL END DECLARE SECTION END-EXEC.
           EXEC SQL INCLUDE SQLCA END-EXEC.
      *-----------------------------------------------------------*
      *-----------------------------------------------------------*
       PROCEDURE DIVISION.
       TMAIN SECTION.
       P030-00.
      *
           EXEC SQL AT :DBS
              DECLARE CSKEY01N CURSOR FOR
                 SELECT TABID, MODTIME FROM TAB
                 WHERE
                    KEY01 >= (
                        LPAD(:KEY01-PART , 008, '0')
                             )
                 ORDER BY KEY01 ASC
           END-EXEC.
      *
           IF SQLCODE NOT = 0
              GO TO P031-99.
      *
           EXEC SQL
              OPEN CSKEY01N
           END-EXEC.
      *
           IF SQLCODE NOT = 0
              GO TO P031-99.
      *
       P031-20.
      *
           EXEC SQL
               FETCH CSKEY01N
               INTO
                 :TABID,
                 :MODTIME
           END-EXEC.

           EXEC SQL
              CLOSE CSKEY01N
            END-EXEC.
      *
       P031-99.
           GOBACK.

Producing a preparsed TE609.cob and compiling that leads to:

TE609.cob: in section 'TMAIN':
TE609.cob: in paragraph 'P030-00':
TE609.cob:51: error: 'GIXSQL-CI-P-TE609-CSKEY01N' is not defined

which is correct as the section was not inserted.

@mridoni
Copy link
Owner

mridoni commented Aug 27, 2022

Ok, thanks, I'll look into it as soon as I can.

@mridoni
Copy link
Owner

mridoni commented Aug 29, 2022

This was actually quite easy: I forgot to remove this test:

if (!startup_items.size())
return true;

but fixing this exposed another problem: it seems that this declaration is not properly parsed:

77 KEY01-PART          PIC  9(008) VALUE 42.

and preprocessing fails. If I change it to (only modifying the level):

01 KEY01-PART          PIC  9(008) VALUE 42.

it works (even though I obviously cannot run it). I will try to see if I can solve this too and release it with 1.0.18a

@mridoni
Copy link
Owner

mridoni commented Aug 29, 2022

Commenting out this line in the lexer makes it work, but even if it shouldn't be a problem, I will have (at least) to re-run all the tests for regressions.

("66"|"77"|"78"|"88")[^\.]*"." {}

@GitMensch
Copy link
Contributor Author

77 is definitely wrong here, as it is a plain variable with own storage.
78 is a constant - so would be useful to be usable as input , but I don't know how other preparsers handle this and if gixsql can handle "read only" already (actually should, because one can use "normal" literals already).
88 items should not be used at all for exec sql, so dropping these seems reasonable (otherwise you could only error on use).
66 items are "kind of redefines" with pre-fixed type (either the same, when there is only one entry, with THROUGH always as PIC X of appropriate size). I guess that the parser may not be able to handle those, in this case skipping them in the lexer seems reasonable (I don't think another sql preprocessor handles these).

@mridoni
Copy link
Owner

mridoni commented Aug 29, 2022

Ok, thanks. I will try removing 77 and 78 and leave the other two in then I will re-run all the tests.

@GitMensch
Copy link
Contributor Author

Ok, thanks. I will try removing 77 and 78 and leave the other two in then I will re-run all the tests.

... and best include a new test with both.

@mridoni
Copy link
Owner

mridoni commented Aug 29, 2022

... and best include a new test with both.

It would be nice to intercept the 78s and display an error if they are (erronously) used as result parameters. I had a look at it but this requires modifying some sensitive stuff where the field tree is built (i.e. gix_esql_driver::cb_build_field_tree) so it is better to leave this for another time. A separate issue is probably needed to track this.

@GitMensch
Copy link
Contributor Author

The separate issue is created, I suggest to let the scanner pass the level 78 if it is already in the parser and error out there, or just leave it as "skipped" (then the parser will error correctly with "unknown parameter"). It is fine to go with level 77 "as soon as possible", which seems (as no work other than the test case is missing) to be 1.0.18.a; if it is more work you may want to error in the parser for that, too.

@mridoni
Copy link
Owner

mridoni commented Aug 29, 2022

It is fine to go with level 77 "as soon as possible", which seems (as no work other than the test case is missing) to be 1.0.18.a; if it is more work you may want to error in the parser for that, too.

The packages are being built, tomorrow I will run some more tests.

mridoni added a commit that referenced this issue Aug 30, 2022
- Changed logging system initializationto avoid possible crashes
- Fixed some spelling problems in the documentation
- Fixed some memory leaks in the runtime
- Solution for "sqlstate by driver may set 00000 for errors (in GixSQL?)" (#114)
- Additional fix for "codegen / logic issue for "GIXSQLCursorDeclareParams" (#88)
- Allowing variable with level 77/78 in the parser
@GitMensch
Copy link
Contributor Author

GitMensch commented Aug 30, 2022

During testing with a patched 1.0.18 (including the possibly bad workaround for #119) I got "strange errors".
Debugging showed this may happen with code similar to the new testcase

EXEC SQL AT :DBS
DECLARE CRSR017D CURSOR FOR
SELECT TABID, MTIME FROM TAB
WHERE
KEY01 >= (
MYFUNC(:KEY-ID-1 , :KEY-ID-2 , 099, '1')
)
ORDER BY KEY01 ASC
END-EXEC.
*
IF SQLCODE NOT = 0
GO TO P003-C.
*
EXEC SQL
OPEN CRSR017D
END-EXEC.
*
IF SQLCODE NOT = 0
GO TO P003-C.

Currently the codegen is somewhat like

GIXSQL*    EXEC SQL AT :DBS
GIXSQL*       DECLARE CRSR017D CURSOR FOR
GIXSQL*          SELECT TABID, MTIME FROM TAB
GIXSQL*          WHERE
GIXSQL*             KEY01 >= (
GIXSQL*                 MYFUNC(:KEY-ID-1 , :KEY-ID-2 , 099, '1')
GIXSQL*                      )
GIXSQL*          ORDER BY KEY01 ASC
GIXSQL*    END-EXEC.
      *
           IF SQLCODE NOT = 0
              GO TO P003-C.
      *
GIXSQL*    EXEC SQL
GIXSQL*       OPEN CRSR017D
GIXSQL*    END-EXEC.
GIXSQL     IF GIXSQL-CI-F-TSQL017E-CRSR017D = ' ' THEN
GIXSQL         PERFORM GIXSQL-CI-P-TSQL017E-CRSR017D
GIXSQL         IF SQLCODE = 0
GIXSQL           MOVE 'X' TO GIXSQL-CI-F-TSQL017E-CRSR017D
GIXSQL        END-IF
GIXSQL     END-IF
GIXSQL     IF GIXSQL-CI-F-TSQL017E-CRSR017D = 'X' THEN
GIXSQL         CALL "GIXSQLCursorOpen" USING
GIXSQL             BY REFERENCE SQLCA
GIXSQL             BY REFERENCE "TSQL017E_CRSR017D" & x"00"
GIXSQL         END-CALL
GIXSQL     END-IF.
      *
           IF SQLCODE NOT = 0
              GO TO P003-C.

It could be solved by adjusting the codegen for the "DECLARE in PROCEDURE DIVISION" case as follows (duplicating a the declare part)

GIXSQL*    EXEC SQL AT :DBS
GIXSQL*       DECLARE CRSR017D CURSOR FOR
GIXSQL*          SELECT TABID, MTIME FROM TAB
GIXSQL*          WHERE
GIXSQL*             KEY01 >= (
GIXSQL*                 MYFUNC(:KEY-ID-1 , :KEY-ID-2 , 099, '1')
GIXSQL*                      )
GIXSQL*          ORDER BY KEY01 ASC
GIXSQL*    END-EXEC.
GIXSQL     IF GIXSQL-CI-F-TSQL017E-CRSR017D = ' ' THEN
GIXSQL         PERFORM GIXSQL-CI-P-TSQL017E-CRSR017D
GIXSQL         IF SQLCODE = 0
GIXSQL           MOVE 'X' TO GIXSQL-CI-F-TSQL017E-CRSR017D
GIXSQL        END-IF
GIXSQL     ELSE
GIXSQL        MOVE 0 TO SQLCODE
GIXSQL     END-IF
      *
           IF SQLCODE NOT = 0
              GO TO P003-C.
      *
GIXSQL*    EXEC SQL
GIXSQL*       OPEN CRSR017D
GIXSQL*    END-EXEC.
GIXSQL     IF GIXSQL-CI-F-TSQL017E-CRSR017D = ' ' THEN
GIXSQL         PERFORM GIXSQL-CI-P-TSQL017E-CRSR017D
GIXSQL         IF SQLCODE = 0
GIXSQL           MOVE 'X' TO GIXSQL-CI-F-TSQL017E-CRSR017D
GIXSQL        END-IF
GIXSQL     END-IF
GIXSQL     IF GIXSQL-CI-F-TSQL017E-CRSR017D = 'X' THEN
GIXSQL         CALL "GIXSQLCursorOpen" USING
GIXSQL             BY REFERENCE SQLCA
GIXSQL             BY REFERENCE "TSQL017E_CRSR017D" & x"00"
GIXSQL         END-CALL
GIXSQL     END-IF.
      *
           IF SQLCODE NOT = 0
              GO TO P003-C.

This is important for current code that checks the SQLCODE after the DECLARE and has an appropriate error handling there; it is even more important when the program is called the second time and the last time the program was called SQLCODE left with 100 for "no record found" - and now "seem to fail" on the DECLARE - with a status 100...

@mridoni
Copy link
Owner

mridoni commented Aug 30, 2022

Ok, I will look into it, but it might be a week or so until I find the time to work on it again, this is why I released v1.0.18a now. (I am also trying to release v1.0.18a for Gix-IDE, but today uploads on GitHub seem to be really really slow)

@GitMensch
Copy link
Contributor Author

Sounds fair.

@GitMensch
Copy link
Contributor Author

GitMensch commented Oct 17, 2022

I'm aware that there's a lot ongoing from reading your dev-blog (mostly related to Gix Debugger and to packaging of Gix IDE)...
still I'd like to leave a ping asking about generation adjustments of SQLCODE.

Could you come up with a change for that (as it is still the reason the codebase in question - previously using ocesql - cannot be switched over to GixSQL) and release 1.0.18b, possibly with including #127?

Edit: While this seems like an easier issue to solve it would not make the codebase working because of #119, which, as I've understood has a target of GixSQL 1.0.19.

@mridoni
Copy link
Owner

mridoni commented Oct 20, 2022

This patch modifies the code generation as indicated above. During the next few days (possibly the beginning of next week) I will apply the three patches (this one, the one for #119 and PR #127) and release v1.0.18b as a pre-release.

issue88.patch.txt

mridoni added a commit that referenced this issue Oct 20, 2022
- Added support for generating preprocessed source block metadata
- Modified cursor initialization (#88)
- Integrated PR #127
- Allow quoted identifiers in COPY statements (#102)
- Fixed option handling in connection strings
- Fixed a MinGW build issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants