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

lib: Increase max size of SQL statements #1124

Merged
merged 4 commits into from Jul 25, 2022

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented Nov 28, 2020

Increases length of SQL statements in db.execute and elsewhere (DB_SQL_MAX)
from 1024 * pow(2, 3) to 1024 * pow(2, 6). Longer SQL statements are needed
for long attribute values, but also for tables with a lot of columns,
esp. when this table needs to be dropped and re-created during v.db.dropcolumn
(which is the original motivation for this change).

The new value is much higher than the current one and the SQL statement I had problem with in the v.db.dropcolumn case above. However, since this is likely to be bigger than a path or other things we have fixed sizes for, it seems it deserves more.

One char array would now take 0.0625 MiB which may be a problem if rest of the program allocates a lot of other things on stack, but likely fine because there is usually only few instances DB_SQL_MAX array in the program. If every occurrence of DB_SQL_MAX in the code (77) would be allocated at the same time (which is not the case), it would be under 5 MiB which still fits into glibc stack size.

The real, ideal fix would be no limit and dynamically allocated arrays for the SQL statement, but that's a bigger change than this PR.

Increases length of SQL statements in db.execute and elsewhere (DB_SQL_MAX)
from 1024 * pow(2, 3) to 1024 * pow(2, 6). Longer SQL statements are needed
for long attribute values, but also for tables with a lot of columns,
esp. when this table needs to be dropped and re-created during v.db.dropcolumn
(which is the original motivation for this change).

The new value is much higher than the current one and the SQL in the
v.db.dropcolumn case above. If every occurence of DB_SQL_MAX in the
code (77) would be allocated at the same time, it would be still
under 5 MiB.
@wenzeslaus wenzeslaus added the enhancement New feature or request label Nov 28, 2020
@neteler
Copy link
Member

neteler commented Nov 28, 2020

Out of curiosity: Which error message appears at time without this charge in case of a table with many columns and buffer being exceeded?

@veroandreo
Copy link
Contributor

I had these issues last year when trying to drop columns from a table with many many columns after using i.segment.stats. Here's the ticket I had opened back then: https://trac.osgeo.org/grass/ticket/3919

@wenzeslaus
Copy link
Member Author

Which error message appears at time without this charge in case of a table with many columns and buffer being exceeded?

The one I got was what @veroandreo reported, so something like:

<a messy and duplicated SQL backend error here>
ERROR: Error while executing: 'CREATE TEMPORARY TABLE
   <long, but truncated table definition here>'
ERROR: Deleting column failed

Obviously, this suboptimal, mostly unhelpful, and it can still can occur even when db.execute is used directly. It is only far less likely with this change. Ideally, there would be no limit (or only the backend's limit if any).

@wenzeslaus
Copy link
Member Author

I have updated the description to be clear about the new size and that the ideal solution is something else.

@wenzeslaus
Copy link
Member Author

From Trac Ticket #3919:

I tried changing both to 16384 and 32768, and compiling again, but still the same problem. It reaches the same place as above in the table. So, it seems the change does not have any effect. Any other place I should change?

This PR changes that to 65536, but it is possible it was not length issue, but rather compilation issue at that point. I struggled with that myself (whether I need to re-compile db.execute, lib or both).

@wenzeslaus
Copy link
Member Author

The OSGeo4W tests passed before and the Ubuntu tests are passing, so I intent to merge this, although 65536 bytes goes over some of the low limits discussed in bug-coreutils: thread stack size (but for that even 16384 would be too much since it is the whole stack size on some systems which would make even our current value 8192 barely acceptable).

@wenzeslaus wenzeslaus added this to the 8.2.0 milestone Dec 2, 2021
@wenzeslaus wenzeslaus modified the milestones: 8.2.0, 8.4.0 Feb 27, 2022
@metzm
Copy link
Contributor

metzm commented Mar 18, 2022

A rather old PR, I suggest to rebase and wait for the GH checks.

I approve this PR, but there could be a new issue for dynamic allocation for the size of the SQL statement passed.

@wenzeslaus wenzeslaus merged commit 2b85ab5 into OSGeo:main Jul 25, 2022
@wenzeslaus wenzeslaus deleted the increase-sql-max-len branch July 25, 2022 15:37
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
Increases length of SQL statements in db.execute and elsewhere (DB_SQL_MAX)
from 1024 * pow(2, 3) to 1024 * pow(2, 6). Longer SQL statements are needed
for long attribute values, but also for tables with a lot of columns,
esp. when this table needs to be dropped and re-created during v.db.dropcolumn
(which is the original motivation for this change).

The new value is much higher than the current one and the SQL statement I had problem with in the v.db.dropcolumn case above. However, since this is likely to be bigger than a path or other things we have fixed sizes for, it seems it deserves more.

One char array would now take 0.0625 MiB which may be a problem if rest of the program allocates a lot of other things on stack, but likely fine because there is usually only few instances DB_SQL_MAX array in the program. If every occurrence of DB_SQL_MAX in the code (77) would be allocated at the same time (which is not the case), it would be under 5 MiB which still fits into glibc stack size (7.4 MB).

The stack size issue may appear on some special platforms where 65536 bytes is a significant part of the stack size limit, but in general, for these platforms even 16384 can be too much since 65536 bytes might be the whole stack size or more on some systems (HP-UX 11 has 16 KB). This makes even our current value of 8192 bytes barely acceptable there, so increasing it is probably not an issue because the code doesn't work there already.

A real, ideal fix would be having no limit and using dynamically allocated arrays for all SQL statements.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Increases length of SQL statements in db.execute and elsewhere (DB_SQL_MAX)
from 1024 * pow(2, 3) to 1024 * pow(2, 6). Longer SQL statements are needed
for long attribute values, but also for tables with a lot of columns,
esp. when this table needs to be dropped and re-created during v.db.dropcolumn
(which is the original motivation for this change).

The new value is much higher than the current one and the SQL statement I had problem with in the v.db.dropcolumn case above. However, since this is likely to be bigger than a path or other things we have fixed sizes for, it seems it deserves more.

One char array would now take 0.0625 MiB which may be a problem if rest of the program allocates a lot of other things on stack, but likely fine because there is usually only few instances DB_SQL_MAX array in the program. If every occurrence of DB_SQL_MAX in the code (77) would be allocated at the same time (which is not the case), it would be under 5 MiB which still fits into glibc stack size (7.4 MB).

The stack size issue may appear on some special platforms where 65536 bytes is a significant part of the stack size limit, but in general, for these platforms even 16384 can be too much since 65536 bytes might be the whole stack size or more on some systems (HP-UX 11 has 16 KB). This makes even our current value of 8192 bytes barely acceptable there, so increasing it is probably not an issue because the code doesn't work there already.

A real, ideal fix would be having no limit and using dynamically allocated arrays for all SQL statements.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Increases length of SQL statements in db.execute and elsewhere (DB_SQL_MAX)
from 1024 * pow(2, 3) to 1024 * pow(2, 6). Longer SQL statements are needed
for long attribute values, but also for tables with a lot of columns,
esp. when this table needs to be dropped and re-created during v.db.dropcolumn
(which is the original motivation for this change).

The new value is much higher than the current one and the SQL statement I had problem with in the v.db.dropcolumn case above. However, since this is likely to be bigger than a path or other things we have fixed sizes for, it seems it deserves more.

One char array would now take 0.0625 MiB which may be a problem if rest of the program allocates a lot of other things on stack, but likely fine because there is usually only few instances DB_SQL_MAX array in the program. If every occurrence of DB_SQL_MAX in the code (77) would be allocated at the same time (which is not the case), it would be under 5 MiB which still fits into glibc stack size (7.4 MB).

The stack size issue may appear on some special platforms where 65536 bytes is a significant part of the stack size limit, but in general, for these platforms even 16384 can be too much since 65536 bytes might be the whole stack size or more on some systems (HP-UX 11 has 16 KB). This makes even our current value of 8192 bytes barely acceptable there, so increasing it is probably not an issue because the code doesn't work there already.

A real, ideal fix would be having no limit and using dynamically allocated arrays for all SQL statements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants