Обсуждение: 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

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

> 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