Обсуждение: Re: pgsql: Restrict the use of temporary namespace in two-phase transaction

On Thu, Jan 17, 2019 at 8:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Anyway, it seems to me that this is pointing out to another issue:
> > current_schema() can trigger a namespace creation, hence shouldn't we
> > mark it as PARALLEL UNSAFE and make sure that we never run into this
> > problem?
>
> That seems a bit annoying, but maybe we have little choice?

The only other option I see is to make current_schema() not trigger a
namespace creation.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jan 17, 2019 at 8:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Anyway, it seems to me that this is pointing out to another issue:
>>> current_schema() can trigger a namespace creation, hence shouldn't we
>>> mark it as PARALLEL UNSAFE and make sure that we never run into this
>>> problem?

>> That seems a bit annoying, but maybe we have little choice?

> The only other option I see is to make current_schema() not trigger a
> namespace creation.

Seems hard to avoid.  We could conceivably make it return "pg_temp"
for the temp schema instead of the schema's actual name, but it's
not very hard to think of ways whereby that would make use of the
result fail in contexts where it previously worked.

Another idea is to force creation of the temp namespace as soon as
we see that search_path references it.  I'm not very sure exactly
where would be a convenient place to make that happen, though.
There are paths whereby the GUC's value will change outside a
transaction, so we couldn't tie it directly to the GUC update.

            regards, tom lane

On Fri, Jan 18, 2019 at 03:34:30PM -0500, Tom Lane wrote:
> Seems hard to avoid.  We could conceivably make it return "pg_temp"
> for the temp schema instead of the schema's actual name, but it's
> not very hard to think of ways whereby that would make use of the
> result fail in contexts where it previously worked.

CREATE EXTENSION is one such case.  It would not work if referring to
the synonym pg_temp, but it can work if using directly the temporary
namespace of the session.  So I feel that changing such things is
prone to break more things than to actually fix things.

> Another idea is to force creation of the temp namespace as soon as
> we see that search_path references it.  I'm not very sure exactly
> where would be a convenient place to make that happen, though.
> There are paths whereby the GUC's value will change outside a
> transaction, so we couldn't tie it directly to the GUC update.

This is documented at the top of namespace.c: "initial GUC processing
of search_path happens outside a transaction".
--
Michael

On Sat, Jan 19, 2019 at 09:08:27AM +0900, Michael Paquier wrote:
> On Fri, Jan 18, 2019 at 03:34:30PM -0500, Tom Lane wrote:
>> Seems hard to avoid.  We could conceivably make it return "pg_temp"
>> for the temp schema instead of the schema's actual name, but it's
>> not very hard to think of ways whereby that would make use of the
>> result fail in contexts where it previously worked.
>
> CREATE EXTENSION is one such case.  It would not work if referring to
> the synonym pg_temp, but it can work if using directly the temporary
> namespace of the session.  So I feel that changing such things is
> prone to break more things than to actually fix things.

As long as I don't forget about it..  current_schema() is classified
as stable, so it's not like we can make it return pg_temp and then the
real temporary schema name within the same transaction...
--
Michael

On Sat, Jan 19, 2019 at 5:05 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 17, 2019 at 8:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Anyway, it seems to me that this is pointing out to another issue:
> > > current_schema() can trigger a namespace creation, hence shouldn't we
> > > mark it as PARALLEL UNSAFE and make sure that we never run into this
> > > problem?
> >
> > That seems a bit annoying, but maybe we have little choice?
>
> The only other option I see is to make current_schema() not trigger a
> namespace creation.
>

Or can we make the test script set force_parallel_mode = off? Since
the failure case is a very rare in real world I think that it might be
better to change the test scripts rather than changing properly of
current_schema().

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Jan 22, 2019 at 01:47:05PM +0900, Masahiko Sawada wrote:
> Or can we make the test script set force_parallel_mode = off? Since
> the failure case is a very rare in real world I think that it might be
> better to change the test scripts rather than changing properly of
> current_schema().

Please see 396676b, which is in my opinion a quick workaround to the
problem.  Even if that's a rare case, it would be confusing to the
user to see it :(
--
Michael

On Tue, Jan 22, 2019 at 2:17 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jan 22, 2019 at 01:47:05PM +0900, Masahiko Sawada wrote:
> > Or can we make the test script set force_parallel_mode = off? Since
> > the failure case is a very rare in real world I think that it might be
> > better to change the test scripts rather than changing properly of
> > current_schema().
>
> Please see 396676b, which is in my opinion a quick workaround to the
> problem.

Oops, sorry for the too late response. Thank you.

> Even if that's a rare case, it would be confusing to the
> user to see it :(

Indeed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center