Thread: BUG #15960: ON CONFLICT Trying accessing to variables
The following bug has been logged on the website:
Bug reference: 15960
Logged by: Anton Dutov
Email address: anton.dutov@gmail.com
PostgreSQL version: 11.3
Operating system: Linux
Description:
Using upsert in stored procedure ON CONFLICT(id, ...) trows "ERROR: column
reference "id" is ambiguous" where id used as column name and as variable,
but ON CONFLICT can't use variables
Reproduce
CREATE TABLE IF NOT EXISTS test_table (id INT PRIMARY KEY, note TEXT);
CREATE OR REPLACE FUNCTION test_table_add (
in_id INT
,in_note TEXT
,OUT id INT
) RETURNS INT LANGUAGE plpgsql VOLATILE SECURITY DEFINER AS $$
BEGIN
INSERT INTO test_table (id, note) VALUES (in_id, in_note)
ON CONFLICT (id) DO UPDATE SET note = in_note;
END;
$$;
SELECT test_table_add (1, 'ONE');
On Thu, Aug 15, 2019 at 6:28 AM PG Bug reporting form <noreply@postgresql.org> wrote: > Using upsert in stored procedure ON CONFLICT(id, ...) trows "ERROR: column > reference "id" is ambiguous" where id used as column name and as variable, > but ON CONFLICT can't use variables This is not a bug. You must resolve the ambiguity by using a plpgsql variable whose name isn't exactly the same as a column name. In principle you can have an index on a simple constant, which is occasionally useful (e.g. using a unique index to enforce that you can either have zero or one rows in some particular table). -- Peter Geoghegan
Hi, On August 15, 2019 9:17:36 AM PDT, Peter Geoghegan <pg@bowt.ie> wrote: >On Thu, Aug 15, 2019 at 6:28 AM PG Bug reporting form ><noreply@postgresql.org> wrote: >> Using upsert in stored procedure ON CONFLICT(id, ...) trows "ERROR: >column >> reference "id" is ambiguous" where id used as column name and as >variable, >> but ON CONFLICT can't use variables > >This is not a bug. You must resolve the ambiguity by using a plpgsql >variable whose name isn't exactly the same as a column name. I'm not sure I quite buy that. There is no actual ambiguity here. I don't buy the variable referencing a constant - that'dnot correctly work for subsequent uses of the statement if the variable differs - don't think we'd have replacing etcset up. Nor do I think we would even evaluate The variable/param. Seems like it's more a question of preventing the hook from resolving things at that point? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Thu, Aug 15, 2019 at 10:05 AM Andres Freund <andres@anarazel.de> wrote: > I'm not sure I quite buy that. There is no actual ambiguity here. I don't buy the variable referencing a constant - that'dnot correctly work for subsequent uses of the statement if the variable differs - don't think we'd have replacing etcset up. Nor do I think we would even evaluate The variable/param. If we were going to fix this, then it would probably be because of the issue around it working inconsistently when the variable differs over multiple calls. That's something that we've heard about at least once before. I'm not excited about fixing the ambiguity issue. > Seems like it's more a question of preventing the hook from resolving things at that point? I don't know. -- Peter Geoghegan
Hi, On 2019-08-15 10:09:14 -0700, Peter Geoghegan wrote: > On Thu, Aug 15, 2019 at 10:05 AM Andres Freund <andres@anarazel.de> wrote: > > I'm not sure I quite buy that. There is no actual ambiguity here. I don't buy the variable referencing a constant -that'd not correctly work for subsequent uses of the statement if the variable differs - don't think we'd have replacingetc set up. Nor do I think we would even evaluate The variable/param. > > If we were going to fix this, then it would probably be because of the > issue around it working inconsistently when the variable differs over > multiple calls. That's something that we've heard about at least once > before. I'm not excited about fixing the ambiguity issue. Well, I think it'd currently error out in many cases (presumably once we're over the 5 executions limit or such?), so that'd prevent the error from actually causing problems like that. > > Seems like it's more a question of preventing the hook from resolving things at that point? > > I don't know. Seems like we'd just need a new EXPR_KIND_*, maybe EXPR_KIND_ARBITER_EXPRESSION, which plpgsql's column hook would just ignore. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-08-15 10:09:14 -0700, Peter Geoghegan wrote:
>> On Thu, Aug 15, 2019 at 10:05 AM Andres Freund <andres@anarazel.de> wrote:
>>> Seems like it's more a question of preventing the hook from resolving things at that point?
>> I don't know.
> Seems like we'd just need a new EXPR_KIND_*, maybe
> EXPR_KIND_ARBITER_EXPRESSION, which plpgsql's column hook would just
> ignore.
It's already using EXPR_KIND_INDEX_EXPRESSION, so I doubt you need a
new exprkind; it'd be fine not to resolve plpgsql variables in CREATE
INDEX too. Possibly the same could happen for some other exprkinds.
But I'm not really sure that having the hook discriminate against
particular exprkinds is a good idea, on two grounds:
1. It introduces user-visible inconsistency. Despite the OP's opinion,
I am not convinced that having "id" behave differently here than it
would elsewhere is a good thing.
2. It seems like an inevitable source of future bugs, when somebody
changes something in a way that makes it desirable to resolve plpgsql
variables in a particular exprkind. Yeah, that most likely means they
should have split that exprkind in two, but this is the sort of action
at a distance that's very unlikely to get noticed until too late.
Maybe it'd be a net positive despite these objections, but I do not
think it's a clear-cut win.
regards, tom lane
Hi,
On 2019-08-15 18:06:25 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-08-15 10:09:14 -0700, Peter Geoghegan wrote:
> >> On Thu, Aug 15, 2019 at 10:05 AM Andres Freund <andres@anarazel.de> wrote:
> >>> Seems like it's more a question of preventing the hook from resolving things at that point?
>
> >> I don't know.
>
> > Seems like we'd just need a new EXPR_KIND_*, maybe
> > EXPR_KIND_ARBITER_EXPRESSION, which plpgsql's column hook would just
> > ignore.
>
> It's already using EXPR_KIND_INDEX_EXPRESSION, so I doubt you need a
> new exprkind; it'd be fine not to resolve plpgsql variables in CREATE
> INDEX too. Possibly the same could happen for some other exprkinds.
Right.
> But I'm not really sure that having the hook discriminate against
> particular exprkinds is a good idea, on two grounds:
>
> 1. It introduces user-visible inconsistency. Despite the OP's opinion,
> I am not convinced that having "id" behave differently here than it
> would elsewhere is a good thing.
One counter-argument to that would be that we already don't interpret
expressions to potentially refer to plpgsql variables in some
cases. Given that those largely (all?) are DDL however, I'm not sure
it's a very strong counter argument.
CREATE OR REPLACE FUNCTION blarg() RETURNS VOID LANGUAGE plpgsql AS $$
BEGIN
CREATE TEMPORARY TABLE blargt(id int, CHECK(id < 10));
END;$$;
SELECT blarg();
Greetings,
Andres Freund