Date: Thu, 20 Jan 2022 06:03:40 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
	Joel Jacobson <joel@compiler.org>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Schema variables - new implementation for Postgres 15
Message-ID: <20220119220340.m5xtlwuinywcncvd@jrouhaud>
References: <b846763e-1f9a-40a8-ac67-a3a4585826ef@www.fastmail.com>
 <CAFj8pRBT-bRQJBqkzon7tHcoFZ1byng06peZfZa0G72z46YFxg@mail.gmail.com>
 <1ebeaf57-da83-42e1-9302-fb126281814d@www.fastmail.com>
 <CAFj8pRD9iEpPN=qX1FKtNxYha9B48sZVqAjFgck7Ah0DpFckOg@mail.gmail.com>
 <CAEZATCUdkmVBKCy8WcLinMP9rjFD8hHrZ0Qy4tMEor_P_GHv2g@mail.gmail.com>
 <CAFj8pRDT4RW_ZiAbsbRGjTDZ+7on3y09dV04KVAEKya_HTo=1A@mail.gmail.com>
 <20220114024402.btyn7vrpkhnso2bq@jrouhaud>
 <CAFj8pRC9dWjbiP2d4naacU4FYgE9-NSTwGtq+M71GnaYL+odog@mail.gmail.com>
 <20220119080117.4c2fjzs5p7vtdxhk@jrouhaud>
 <CAFj8pRC24VHS0YujMr42T12o15gcABhpuarhDku7_nngtsxUnA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAFj8pRC24VHS0YujMr42T12o15gcABhpuarhDku7_nngtsxUnA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220119220340.m5xtlwuinywcncvd%40jrouhaud>
Precedence: bulk

Hi,

On Wed, Jan 19, 2022 at 09:09:41PM +0100, Pavel Stehule wrote:
> st 19. 1. 2022 v 9:01 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
> 
> RAISE NOTICE should use local variables, and in this case is a question if we
> should raise a warning. There are two possible analogies - we can see session
> variables like global variables, and then the warning should not be raised,
> or we can see relation between session variables and plpgsql variables
> similar like session variables and some with higher priority, and then
> warning should be raised. If we want to ensure that the new session variable
> doesn't break code, then session variables should have lower priority than
> plpgsql variables too. And because the plpgsql protection against collision
> cannot  be used, then I prefer raising the warning.

Ah that's indeed a good point.  I agree, they're from a different part of the
system so they should be treated as different things, and thus raising a
warning.  It's consistent with the chosen conservative approach anyway.

> PLpgSQL assignment should not be in collision with session variables ever

Agreed.

> 
> >
> > =# DO $$
> > BEGIN
> > v := 'test';
> > RAISE NOTICE 'v: %', v;
> > END;
> > $$ LANGUAGE plpgsql;
> > ERROR:  42601: "v" is not a known variable
> > LINE 3: v := 'test';
> >
> > But the RAISE NOTICE does see the session variable (which should be the
> > correct
> > behavior I think), so the warning should have been raised for this
> > instruction
> > (and in that case the message is incorrect, as it's not shadowing a
> > column).
> >
> 
> In this case I can detect node type, and I can identify external param
> node, but I cannot to detect if this code was executed from PLpgSQL or from
> some other
> 
> So I can to modify warning text to some

Yes, that's what I had in mind too.

> DETAIL:  The identifier can be column reference or query parameter or
> session variable reference.
> HINT:  The column reference and query parameter is preferred against
> session variable reference.
> 
> I cannot to use term "plpgsql variable" becase I cannot to ensure validity
> of this message
> 
> Maybe is better to don't talk about source of this issue, and just talk
> about result - so the warning text should be just
> 
> MESSAGE: "session variable \"xxxx\" is shadowed
> DETAIL: "session variables can be shadowed by columns, routine's variables
> and routine's arguments with same name"
> 
> Is it better?

I clearly prefer the 2nd version.