pgsql: Restrict the use of temporary namespace in two-phase transaction

On Fri, Jan 18, 2019 at 12:22:52AM +0000, Michael Paquier wrote:

Restrict the use of temporary namespace in two-phase transactions

Attempting to use a temporary table within a two-phase transaction is
forbidden for ages. However, there have been uncovered grounds for
a couple of other object types and commands which work on temporary
objects with two-phase commit. In short, trying to create, lock or drop
an object on a temporary schema should not be authorized within a
two-phase transaction, as it would cause its state to create
dependencies with other sessions, causing all sorts of side effects with
the existing session or other sessions spawned later on trying to use
the same temporary schema name.

I have been monitoring the buildfarm and crake is complaining:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=crake&br=HEAD

Here is the problem:
SET search_path TO 'pg_temp';
BEGIN;
SELECT current_schema() ~ 'pg_temp' AS is_temp_schema;
- is_temp_schema
-----------------
- t
-(1 row)
-
+ERROR:  cannot create temporary tables during a parallel operation
PREPARE TRANSACTION 'twophase_search';
-ERROR:  cannot PREPARE a transaction that has operated on temporary namespace

I am actually amazed to see the planner choose a parallel plan for
that, and the test can be fixed by enforcing those parameters I think:
SET max_parallel_workers = 0;
SET max_parallel_workers_per_gather = 0;
Could somebody confirm my assumption here by the way? This enforces a
non-parallel plan, right?

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?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

I have been monitoring the buildfarm and crake is complaining:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=crake&amp;br=HEAD

I am actually amazed to see the planner choose a parallel plan for
that,

That's due to force_parallel_mode = regress, I imagine.

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?

regards, tom lane

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

On 18/01/2019 01:22, Michael Paquier wrote:

Restrict the use of temporary namespace in two-phase transactions

We usually don't use "namespace" in user-facing error messages. Can you
change it to say "temporary schema"?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Feb 08, 2019 at 10:41:59AM +0100, Peter Eisentraut wrote:

We usually don't use "namespace" in user-facing error messages. Can you
change it to say "temporary schema"?

Or just switch to "temporary objects" like it's done on HEAD for the
second message?

Please note that I have kept the error message for temporary tables
for compatibility reasons on stable branches, and I would rather not
touch that. The second one is new though.
--
Michael

On 08/02/2019 11:00, Michael Paquier wrote:

On Fri, Feb 08, 2019 at 10:41:59AM +0100, Peter Eisentraut wrote:

We usually don't use "namespace" in user-facing error messages. Can you
change it to say "temporary schema"?

Or just switch to "temporary objects" like it's done on HEAD for the
second message?

Yeah, even better.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 09/02/2019 16:06, Peter Eisentraut wrote:

On 08/02/2019 11:00, Michael Paquier wrote:

On Fri, Feb 08, 2019 at 10:41:59AM +0100, Peter Eisentraut wrote:

We usually don't use "namespace" in user-facing error messages. Can you
change it to say "temporary schema"?

Or just switch to "temporary objects" like it's done on HEAD for the
second message?

Yeah, even better.

Committed that way.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Feb 11, 2019 at 10:59:20AM +0100, Peter Eisentraut wrote:

Committed that way.

Thanks Peter for adjusting the message, I was just going to do it.
There was a long weekend here and I had zero access to my laptop,
explaining the delay in replying.
--
Michael