Re: Cleaning up array_in()

From: Nikhil Benesch <nikhil(dot)benesch(at)gmail(dot)com>
To: Alexander Lakhin <exclusion(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Cleaning up array_in()
Date: 2023-06-05 01:48:38
Message-ID: CAPWqQZQf3eV1UKw0x-uSML3yuvP_MYzvsxj_pYRy2+07fJ1soQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I took a look at 0002 because I attempted a similar but more surgical
fix in [0].

I spotted a few opportunities for further reducing state tracked by
`ArrayCount`. You may not find all of these suggestions to be
worthwhile.

1) `in_quotes` appears to be wholly redundant with `parse_state ==
ARRAY_QUOTED_ELEM_STARTED`.

2) The `empty_array` special case does not seem to be important to
ArrayCount's callers, which don't even special case `ndims == 0` but
rather `ArrayGetNItemsSafe(..) == 0`. Perhaps this is a philosophical
question as to whether `ArrayCount('{{}, {}}')` should return
(ndims=2, dims=[2, 0]) or (ndims=0). Obviously someone needs to do
that normalization, but `ArrayCount` could leave that normalization to
`ReadArrayStr`.

3) `eoArray` could be replaced with a new `ArrayParseState` of
`ARRAY_END`. Just a matter of taste, but "end of array" feels like a
parser state to me.

I also have a sense that `ndims_frozen` made the distinction between
`ARRAY_ELEM_DELIMITED` and `ARRAY_LEVEL_DELIMITED` unnecessary, and
the two states could be merged into a single `ARRAY_DELIMITED` state,
but I've not pulled on this thread hard enough to say so confidently.

Thanks for doing the serious overhaul. As you say, the element
counting logic is much easier to follow now. I'm much more confident
that your patch is correct than mine.

Cheers,
Nikhil

[0]: https://www.postgresql.org/message-id/CAPWqQZRHsFuvWJj%3DczXuKEB03LF4ctPpDE1k3CoexweEFicBKQ%40mail.gmail.com

On Tue, May 9, 2023 at 7:00 AM Alexander Lakhin <exclusion(at)gmail(dot)com> wrote:
>
> 09.05.2023 06:06, Tom Lane wrote:
> > Alexander Lakhin <exclusion(at)gmail(dot)com> writes:
> >> The only thing that confused me, is the error message (it's not new, too):
> >> select '{{{{{{{{{{1}}}}}}}}}}'::int[];
> >> or even:
> >> select '{{{{{{{{{{'::int[];
> >> ERROR: number of array dimensions (7) exceeds the maximum allowed (6)
> > Yeah, I didn't touch that, but it's pretty bogus because the first
> > number will always be "7" even if you wrote more than 7 left braces,
> > since the code errors out immediately upon finding that it's seen
> > too many braces.
> >
> > The equivalent message in the PLs just says "number of array dimensions
> > exceeds the maximum allowed (6)". I'm inclined to do likewise in
> > array_in, but didn't touch it here.
>
> I think that, strictly speaking, we have no array dimensions in the string
> '{{{{{{{{{{'; there are only characters (braces) during the string parsing.
> While in the PLs we definitely deal with real arrays, which have dimensions.
>
> >> Beside that, I would like to note the following error text changes
> >> (all of these are feasible, I think):
> > I'll look into whether we can improve those, unless you had a patch
> > in mind already?
>
> Those messages looked more or less correct to me, I just wanted to note how they are
> changing (and haven't highlighted messages, that are not), but if you see here room
> for improvement, please look into it (I have no good formulations yet).
>
> Best regards,
> Alexander
>
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2023-06-05 02:30:00 Re: WAL Insertion Lock Improvements
Previous Message Amit Kapila 2023-06-05 00:02:59 Re: Reload configuration more frequently in apply worker.