Horton

Thread

Commits

  1. Switch function current_schema[s]() to be parallel-unsafe

  1. Shouldn't current_schema() be at least PARALLEL RESTRICTED?

    Michael Paquier <michael@paquier.xyz> — 2019-01-18T02:46:18Z

    Hi all,

While working on the recent issues with 2PC and temporary objects I
have added a test case based on current_schema() for the first time in
history, and the buildfarm complained about it, as mentioned here:
https://www.postgresql.org/message-id/20190118005949.GD1883@paquier.xyz

The has been causing crake and lapwing to complain about
current_schema() failing to create a temporary schema, which can
happen when invoked for the first time of a session:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-01-18%2000%3A34%3A20
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-18%2001%3A20%3A01

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

current_schemas() also has this problem.

For now I have stabilized the test by making sure that non-parallel
plans get used, which makes the buildfarm happy, still that's just a
workaround in my opinion.   One of the reasons why current_schema()
can create temporary schemas is that there are string dependencies
with search_path and the way sessions use it, hence it seems to me
that it would be better to mark the function at least parallel
restricted on HEAD and avoid any unstable behavior?

Thoughts?
--
Michael

  2. Re: Shouldn't current_schema() be at least PARALLEL RESTRICTED?

    Robert Haas <robertmhaas@gmail.com> — 2019-01-18T20:30:16Z

    On Thu, Jan 17, 2019 at 9:46 PM Michael Paquier <michael@paquier.xyz> wrote:
> While working on the recent issues with 2PC and temporary objects I
> have added a test case based on current_schema() for the first time in
> history, and the buildfarm complained about it, as mentioned here:
> https://www.postgresql.org/message-id/20190118005949.GD1883@paquier.xyz
>
> The has been causing crake and lapwing to complain about
> current_schema() failing to create a temporary schema, which can
> happen when invoked for the first time of a session:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-01-18%2000%3A34%3A20
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-18%2001%3A20%3A01
>
> 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
>
> current_schemas() also has this problem.
>
> For now I have stabilized the test by making sure that non-parallel
> plans get used, which makes the buildfarm happy, still that's just a
> workaround in my opinion.   One of the reasons why current_schema()
> can create temporary schemas is that there are string dependencies
> with search_path and the way sessions use it, hence it seems to me
> that it would be better to mark the function at least parallel
> restricted on HEAD and avoid any unstable behavior?

It seems like, as currently implemented, the function is
parallel-unsafe, because any inserts, updates, or deletes are
currently parallel-unsafe, including insertions into catalogs.  But I
am a bit confused why a function that is called current_schema() ends
up creating a temps schema.

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

  3. Re: Shouldn't current_schema() be at least PARALLEL RESTRICTED?

    Michael Paquier <michael@paquier.xyz> — 2019-01-19T01:20:52Z

    On Fri, Jan 18, 2019 at 03:30:16PM -0500, Robert Haas wrote:
> It seems like, as currently implemented, the function is
> parallel-unsafe, because any inserts, updates, or deletes are
> currently parallel-unsafe, including insertions into catalogs.  But I
> am a bit confused why a function that is called current_schema() ends
> up creating a temps schema.

This is documented in namespace.c which needs tricks related to
search_path if referring directly to 'pg_temp', especially if the
namespace creation is marked as pending because its creation cannot
happen outside a transaction context, and the initialization
processing of search_path happens out of that.

Please let me suggest the attached patch then to switch the function
as parallel unsafe, for HEAD.  That still seems like the best way of
course for now after sleeping on it.

Thoughts?
--
Michael

  4. Re: Shouldn't current_schema() be at least PARALLEL RESTRICTED?

    Daniel Gustafsson <daniel@yesql.se> — 2019-03-26T14:16:00Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Reviewing the codepath in question (as well as the commit that changed the behaviour of fetch_search_path()), switching current_schema(s) to parallel-unsafe seems like the correct solution. check-world tests pass cleanly and no documentation patch is required.

Switching to ready for committer.

The new status of this patch is: Ready for Committer

  5. Re: Shouldn't current_schema() be at least PARALLEL RESTRICTED?

    Michael Paquier <michael@paquier.xyz> — 2019-03-27T02:36:50Z

    On Tue, Mar 26, 2019 at 02:16:00PM +0000, Daniel Gustafsson wrote:
> The new status of this patch is: Ready for Committer

Thanks Daniel for the review, committed.
--
Michael