Re: Rethinking plpgsql's assignment implementation

ne 13. 12. 2020 v 22:41 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> I wrote:
> > So my idea here is to add a parsing-mode option to raw_parser(),
> > which would be an enum with values like "normal SQL statement",
> > "expression only", "type name", "plpgsql assignment statement".
>
> Here's a fleshed-out patch series that attacks things that way.
> I'm a lot better pleased with this than with my original approach.
>
> 0001 creates the basic infrastructure for "raw parse modes", and as
> proof of concept simplifies typeStringToTypeName(). There's a minor
> functional improvement there, which is that we can now use the core
> parser's error cursor position, so instead of
>
> regression=# do $$ declare x int[23/] ; begin end $$;
> ERROR: syntax error at or near "/"
> LINE 1: do $$ declare x int[23/] ; begin end $$;
> ^
> CONTEXT: invalid type name "int[23/] "
>
> you get
>
> regression=# do $$ declare x int[23/] ; begin end $$;
> ERROR: syntax error at or near "/"
> LINE 1: do $$ declare x int[23/] ; begin end $$;
> ^
> CONTEXT: invalid type name "int[23/] "
>
> It's possible we could dispense with the error context callback
> in typeStringToTypeName altogether, but I've not experimented much.
>
>
> 0002 tackles the next problem, which is to make this feature accessible
> through SPI. There are a couple of possibly-controversial choices here.
>
> Following the principle that we should avoid changing documented SPI
> interfaces, we need a new version of SPI_prepare to pass RawParseMode
> through. This'll be the fourth one :-(, so I decided it was time to
> try to make a definition that can stay API-compatible through future
> changes. So it takes a struct of options, and I added a promise that
> zeroing the struct is enough to guarantee forward compatibility
> through future additions.
>
> This leaves both of the previous iterations, SPI_prepare_cursor
> and SPI_prepare_params, unused anywhere in the core code.
> I suppose we can't kill them (codesearch.debian.net knows of some
> external uses) but I propose to mark them deprecated, with an eye
> to at least removing their documentation someday.
>
> I did not want to add a RawParseMode parameter to pg_parse_query(),
> because that would have affected a larger number of unrelated modules,
> and it would not have been great from a header-inclusion footprint
> standpoint either. So I chose to pass down the mode from SPI by
> having it just call raw_parser() directly instead of going through
> pg_parse_query(). Perhaps this is a modularity violation, or perhaps
> there's somebody who really wants the extra tracing overhead in
> pg_parse_query() to apply to SPI queries. I'm open to discussing
> whether this should be done differently.
>
> (However, having made these two patches, I'm now wondering whether
> there is any rhyme or reason to the existing state of affairs
> with some callers going through pg_parse_query() while others use
> raw_parser() directly. It's hard to knock making a different
> choice in spi.c unless we have a coherent policy about which to
> use where.)
>
>
> Next, 0003 invents a raw parse mode for plpgsql expressions (which,
> in some contexts, can be pretty nearly whole SELECT statements),
> and uses that to get plpgsql out of the business of prefixing
> "SELECT " to user-written text. I would not have bothered with this
> as a standalone fix, but I think it does make for less-confusing
> error messages --- we've definitely had novices ask "where'd this
> SELECT come from?" in the past. (I cheated a bit on PERFORM, though.
> Unlike other places, it needs to allow UNION, so it can't use the
> same restricted syntax.)
>
> 0004 then reimplements plpgsql assignment. This is essentially the same
> patch I submitted before, but redesigned to work with the infrastructure
> from 0001-0003.
>
> 0005 adds documentation and test cases. It also fixes a couple
> of pre-existing problems that the plpgsql parser had with assigning
> to sub-fields of record fields, which I discovered while making the
> tests.
>
> Finally, 0006 removes plpgsql's ARRAYELEM datum type, on the grounds
> that we don't need it anymore. This might be a little controversial
> too, because there was still one way to reach the code: GET DIAGNOSTICS
> with an array element as target would do so. However, that seems like
> a pretty weird corner case. Reviewing the git history, I find that
> I added support for that in commit 55caaaeba; but a check of the
> associated discussion shows that there was no actual user request for
> that, I'd just done it because it was easy and seemed more symmetric.
> The amount of code involved here seems way more than is justified by
> that one case, so I think we should just take it out and lose the
> "feature". (I did think about whether GET DIAGNOSTICS could be
> reimplemented on top of the new infrastructure, but it wouldn't be
> easy because we don't have a SQL-expression representation of the
> GET DIAGNOSTICS values. Moreover, going in that direction would add
> an expression evaluation, making GET DIAGNOSTICS slower. So I think
> we should just drop it.)
>
>
It is a really great patch. I did fast check and I didn't find any
functionality issue

--
-- Name: footype; Type: TYPE; Schema: public; Owner: pavel
--

CREATE TYPE public.footype AS (
a integer,
b integer
);

ALTER TYPE public.footype OWNER TO pavel;

--
-- Name: bootype; Type: TYPE; Schema: public; Owner: pavel
--

CREATE TYPE public.bootype AS (
a integer,
f public.footype
);

ALTER TYPE public.bootype OWNER TO pavel;

--
-- Name: cootype; Type: TYPE; Schema: public; Owner: pavel
--

CREATE TYPE public.cootype AS (
a integer,
b integer[]
);

ALTER TYPE public.cootype OWNER TO pavel;

--
-- Name: dootype; Type: TYPE; Schema: public; Owner: pavel
--

CREATE TYPE public.dootype AS (
a integer,
b public.footype,
c public.footype[]
);

ALTER TYPE public.dootype OWNER TO pavel;

--
-- PostgreSQL database dump complete
--

postgres=# do $$
<<lab>>
declare
a footype[];
b bootype;
ba bootype[];
c cootype[];
d dootype[];
x int default 1;
begin
a[10] := row(10,20);
a[11] := (30,40);
a[3] := (0,0);
a[3].a := 100;
raise notice '%', a;
b.a := 100;
b.f.a := 1000;
raise notice '%', b;
ba[0] := b;

ba[0].a = 33; ba[0].f := row(33,33);
lab.ba[0].f.a := 1000000;
raise notice '%', ba;
c[0].a := 10000;
c[0].b := ARRAY[1,2,4];
lab.c[0].b[1] := 10000;
raise notice '% %', c, c[0].b[x];

d[0].a := 100;
d[0].b.a := 101;
d[0].c[x+1].a := 102;
raise notice '%', d;
end;
$$;
NOTICE:
[3:11]={"(100,0)",NULL,NULL,NULL,NULL,NULL,NULL,"(10,20)","(30,40)"}
NOTICE: (100,"(1000,)")
NOTICE: [0:0]={"(33,\"(1000000,33)\")"}
NOTICE: [0:0]={"(10000,\"{10000,2,4}\")"} 10000
NOTICE: [0:0]={"(100,\"(101,)\",\"[2:2]={\"\"(102,)\"\"}\")"}
DO

Regards

Pavel

> regards, tom lane
>
>