Thread: Two small patches for the isolationtester lexer
When writing an isolation testcase recently I bumped into the 1024 line buffer size limit in the lexer for my setup block. Adding some stored procedures to the test makes it quite easy to break 1024 characters, and while these could be added as steps it, it’s not a good workaround since the permutation order becomes trickier (and more set in stone). As far as I can see in the history, this limit is chosen as a decent sized buffer and not rooted in a specific requirement, so I propose to bump it slightly to 2048 instead (an equally arbitrarily chosen number). Is there a reason to keep it at 1024 that I’m missing? I also (again) forgot about the # comments not being allowed inside setup and teardown blocks, so patch 0002 proposes adding support for these as the documentation implies. Since SQL comments will be counted towards the line buffer, and sent with the command, supporting both kinds of comments seems reasonable and consistent. make check passes with these patches applies, but I’m quite rusty in this area so I might be missing something. cheers ./daniel
Attachment
Daniel Gustafsson <daniel@yesql.se> writes: > When writing an isolation testcase recently I bumped into the 1024 line buffer > size limit in the lexer for my setup block. Adding some stored procedures to > the test makes it quite easy to break 1024 characters, and while these could be > added as steps it, it’s not a good workaround since the permutation order > becomes trickier (and more set in stone). As far as I can see in the history, > this limit is chosen as a decent sized buffer and not rooted in a specific > requirement, so I propose to bump it slightly to 2048 instead (an equally > arbitrarily chosen number). Is there a reason to keep it at 1024 that I’m > missing? I can't think of one; but I wonder if it's worth working a bit harder and removing the fixed limit altogether, probably by using a PQExpBuffer. If you've hit 1024 today, somebody will bump up against 2048 tomorrow. > I also (again) forgot about the # comments not being allowed inside setup and > teardown blocks, so patch 0002 proposes adding support for these as the > documentation implies. Since SQL comments will be counted towards the line > buffer, and sent with the command, supporting both kinds of comments seems > reasonable and consistent. Hmm, not sure this is a good idea. # is a valid SQL operator name, so doing this would create some risk of breaking legal queries. Admittedly, those operators are rare enough that maybe nobody would ever need them in isolationtester scripts, but I'm not sure that providing an additional way to spell "comment" is worth that. regards, tom lane
> On 21 Feb 2018, at 21:41, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> When writing an isolation testcase recently I bumped into the 1024 line buffer >> size limit in the lexer for my setup block. Adding some stored procedures to >> the test makes it quite easy to break 1024 characters, and while these could be >> added as steps it, it’s not a good workaround since the permutation order >> becomes trickier (and more set in stone). As far as I can see in the history, >> this limit is chosen as a decent sized buffer and not rooted in a specific >> requirement, so I propose to bump it slightly to 2048 instead (an equally >> arbitrarily chosen number). Is there a reason to keep it at 1024 that I’m >> missing? > > I can't think of one; but I wonder if it's worth working a bit harder and > removing the fixed limit altogether, probably by using a PQExpBuffer. > If you've hit 1024 today, somebody will bump up against 2048 tomorrow. The thought did cross my mind, but I opted for the simple hack first. I can take a stab at using a PQExpBuffer to see where that leads. >> I also (again) forgot about the # comments not being allowed inside setup and >> teardown blocks, so patch 0002 proposes adding support for these as the >> documentation implies. Since SQL comments will be counted towards the line >> buffer, and sent with the command, supporting both kinds of comments seems >> reasonable and consistent. > > Hmm, not sure this is a good idea. # is a valid SQL operator name, so > doing this would create some risk of breaking legal queries. Admittedly, > those operators are rare enough that maybe nobody would ever need them in > isolationtester scripts, but I'm not sure that providing an additional > way to spell "comment" is worth that. Good point, didn’t think about that. cheers ./daniel
I wrote; > Daniel Gustafsson <daniel@yesql.se> writes: >> I also (again) forgot about the # comments not being allowed inside setup and >> teardown blocks, so patch 0002 proposes adding support for these as the >> documentation implies. > Hmm, not sure this is a good idea. # is a valid SQL operator name, so > doing this would create some risk of breaking legal queries. Actually, looking closer, this would also trigger on '#' used inside a SQL literal, which seems to move the problem cases into the "pretty likely" category instead of the "far-fetched" one. So I'd only be OK with it if we made the lexer smart enough to distinguish inside-a-SQL- literal from not. That might be a good thing anyway, since it would allow us to not choke on literals containing '}', but it'd be a great deal more work. You might be able to steal code from the psql lexer though. regards, tom lane
Daniel Gustafsson <daniel@yesql.se> writes: > On 21 Feb 2018, at 21:41, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I can't think of one; but I wonder if it's worth working a bit harder and >> removing the fixed limit altogether, probably by using a PQExpBuffer. >> If you've hit 1024 today, somebody will bump up against 2048 tomorrow. > The thought did cross my mind, but I opted for the simple hack first. I can > take a stab at using a PQExpBuffer to see where that leads. Another idea is just to teach addlitchar to realloc the buffer bigger when necessary. > I also (again) forgot about the # comments not being allowed inside setup and > teardown blocks, so patch 0002 proposes adding support for these as the > documentation implies. Since SQL comments will be counted towards the line > buffer, and sent with the command, supporting both kinds of comments seems > reasonable and consistent. >> >> Hmm, not sure this is a good idea. # is a valid SQL operator name, so >> doing this would create some risk of breaking legal queries. Admittedly, >> those operators are rare enough that maybe nobody would ever need them in >> isolationtester scripts, but I'm not sure that providing an additional >> way to spell "comment" is worth that. > Good point, didn’t think about that. > cheers ./daniel
> On 22 Feb 2018, at 05:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> On 21 Feb 2018, at 21:41, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I can't think of one; but I wonder if it's worth working a bit harder and >>> removing the fixed limit altogether, probably by using a PQExpBuffer. >>> If you've hit 1024 today, somebody will bump up against 2048 tomorrow. > >> The thought did cross my mind, but I opted for the simple hack first. I can >> take a stab at using a PQExpBuffer to see where that leads. > > Another idea is just to teach addlitchar to realloc the buffer bigger > when necessary. I think this is the best approach for the task, the attached patch changes the static allocation to instead realloc when required. Having an upper limit on the allocation seemed like a good idea to me, but perhaps it’s overthinking and complicating things for no good reason. cheers ./daniel
Attachment
> On 22 Feb 2018, at 05:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote; >> Daniel Gustafsson <daniel@yesql.se> writes: >>> I also (again) forgot about the # comments not being allowed inside setup and >>> teardown blocks, so patch 0002 proposes adding support for these as the >>> documentation implies. > >> Hmm, not sure this is a good idea. # is a valid SQL operator name, so >> doing this would create some risk of breaking legal queries. > > Actually, looking closer, this would also trigger on '#' used inside a > SQL literal, which seems to move the problem cases into the "pretty > likely" category instead of the "far-fetched" one. So I'd only be OK > with it if we made the lexer smart enough to distinguish inside-a-SQL- > literal from not. That might be a good thing anyway, since it would > allow us to not choke on literals containing '}', but it'd be a great > deal more work. You might be able to steal code from the psql lexer > though. I agree, patch 0002 was broken and the correct fix is a much bigger project - one too big for me to tackle right now (but hopefully at some point in the near future). Thanks for the review of it though! cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes: >> On 22 Feb 2018, at 05:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Another idea is just to teach addlitchar to realloc the buffer bigger >> when necessary. > I think this is the best approach for the task, the attached patch changes the > static allocation to instead realloc when required. Having an upper limit on > the allocation seemed like a good idea to me, but perhaps it’s overthinking and > complicating things for no good reason. Yeah, it doesn't seem to me that we need any fixed limit on the length, so I deleted that part of the logic, and pushed it with one or two other cosmetic adjustments. regards, tom lane
Daniel Gustafsson <daniel@yesql.se> writes: > On 22 Feb 2018, at 05:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Actually, looking closer, this would also trigger on '#' used inside a >> SQL literal, which seems to move the problem cases into the "pretty >> likely" category instead of the "far-fetched" one. So I'd only be OK >> with it if we made the lexer smart enough to distinguish inside-a-SQL- >> literal from not. That might be a good thing anyway, since it would >> allow us to not choke on literals containing '}', but it'd be a great >> deal more work. You might be able to steal code from the psql lexer >> though. > I agree, patch 0002 was broken and the correct fix is a much bigger project - > one too big for me to tackle right now (but hopefully at some point in the near > future). Thanks for the review of it though! OK. I'm going to mark this commitfest entry closed, since the other patch is in and this one needs a good bit more work. Please start a new thread, or at least a new CF entry, if you do more work in this area. regards, tom lane
> On 01 Mar 2018, at 06:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Daniel Gustafsson <daniel@yesql.se> writes: >> I agree, patch 0002 was broken and the correct fix is a much bigger project - >> one too big for me to tackle right now (but hopefully at some point in the near >> future). Thanks for the review of it though! > > OK. I'm going to mark this commitfest entry closed, since the other patch > is in and this one needs a good bit more work. Please start a new thread, > or at least a new CF entry, if you do more work in this area. I absolutely agree, thanks! cheers ./daniel