From 05f0668cdc7df1c6ae0388e652ca3da576485c60 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Fri, 13 Sep 2019 14:04:23 +0200 Subject: [PATCH] Fix: schema leak onto create index statement cache (#2964) DESCRIPTION: Fix schema leak on CREATE INDEX statement When a CREATE INDEX is cached between execution we might leak the schema name onto the cached statement of an earlier execution preventing the right index to be created. Even though the cache is cleared when the search_path changes we can trigger this behaviour by having the schema already on the search path before a colliding table is created in a schema earlier on the `search_path`. When calling an unqualified create index via a function (used to trigger the caching behaviour) we see that the index is created on the wrong table after the schema leaked onto the statement. By copying the complete `PlannedStmt` and `utilityStmt` during our planning phase for distributed ddls we make sure we are not leaking the schema name onto a cached data structure. Caveat; COPY statements already have a lot of parsestree copying ongoing without directly putting it back on the `pstmt`. We should verify that copies modify the statement and potentially copy the complete `pstmt` there already. --- .../distributed/commands/utility_hook.c | 20 +++++---- .../regress/expected/multi_prepare_plsql.out | 44 +++++++++++++++++++ src/test/regress/sql/multi_prepare_plsql.sql | 31 +++++++++++++ 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 4acee388b..de7758032 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -290,6 +290,14 @@ multi_ProcessUtility(PlannedStmt *pstmt, { return; } + + /* + * we need to set the parsetree here already as we copy and replace the original + * parsetree during ddl propagation. In reality we need to refactor the code above + * to not juggle copy the parsetree and leak it to a potential cache above the + * utillity hook. + */ + pstmt->utilityStmt = parsetree; } /* we're mostly in DDL (and VACUUM/TRUNCATE) territory at this point... */ @@ -312,16 +320,12 @@ multi_ProcessUtility(PlannedStmt *pstmt, /* only generate worker DDLJobs if propagation is enabled */ if (EnableDDLPropagation) { + /* copy planned statement since we might scribble on it or its utilityStmt */ + pstmt = copyObject(pstmt); + parsetree = pstmt->utilityStmt; + if (IsA(parsetree, IndexStmt)) { - MemoryContext oldContext = MemoryContextSwitchTo(GetMemoryChunkContext( - parsetree)); - - /* copy parse tree since we might scribble on it to fix the schema name */ - parsetree = copyObject(parsetree); - - MemoryContextSwitchTo(oldContext); - ddlJobs = PlanIndexStmt((IndexStmt *) parsetree, queryString); } diff --git a/src/test/regress/expected/multi_prepare_plsql.out b/src/test/regress/expected/multi_prepare_plsql.out index 1bac37702..1901ab389 100644 --- a/src/test/regress/expected/multi_prepare_plsql.out +++ b/src/test/regress/expected/multi_prepare_plsql.out @@ -1212,6 +1212,50 @@ SELECT ddl_in_plpgsql(); (1 row) +-- test prepared ddl with multi search path to make sure the schema name doesn't leak on +-- to the cached statement +CREATE OR REPLACE FUNCTION ddl_in_plpgsql() +RETURNS VOID AS +$BODY$ +BEGIN + CREATE INDEX prepared_index ON prepare_ddl(x); +END; +$BODY$ LANGUAGE plpgsql; +CREATE SCHEMA otherschema; +SET search_path TO otherschema, public; +SELECT ddl_in_plpgsql(); + ddl_in_plpgsql +---------------- + +(1 row) + +DROP INDEX prepared_index; +-- this creates the same table it 'otherschema'. If there is a leak the index will not be +-- created on this table, but instead on the table in the public schema +CREATE TABLE prepare_ddl (x int, y int); +SELECT create_distributed_table('prepare_ddl', 'x'); + create_distributed_table +-------------------------- + +(1 row) + +SELECT ddl_in_plpgsql(); + ddl_in_plpgsql +---------------- + +(1 row) + +-- verify the index is created in the correct schema +SELECT schemaname, indexrelname FROM pg_stat_all_indexes WHERE indexrelname = 'prepared_index'; + schemaname | indexrelname +-------------+---------------- + otherschema | prepared_index +(1 row) + +-- cleanup +DROP TABLE prepare_ddl; +DROP SCHEMA otherschema; +RESET search_path; -- test prepared COPY CREATE OR REPLACE FUNCTION copy_in_plpgsql() RETURNS VOID AS diff --git a/src/test/regress/sql/multi_prepare_plsql.sql b/src/test/regress/sql/multi_prepare_plsql.sql index 03ec9b7c4..09c23ed31 100644 --- a/src/test/regress/sql/multi_prepare_plsql.sql +++ b/src/test/regress/sql/multi_prepare_plsql.sql @@ -555,6 +555,37 @@ $BODY$ LANGUAGE plpgsql; SELECT ddl_in_plpgsql(); SELECT ddl_in_plpgsql(); +-- test prepared ddl with multi search path to make sure the schema name doesn't leak on +-- to the cached statement +CREATE OR REPLACE FUNCTION ddl_in_plpgsql() +RETURNS VOID AS +$BODY$ +BEGIN + CREATE INDEX prepared_index ON prepare_ddl(x); +END; +$BODY$ LANGUAGE plpgsql; + +CREATE SCHEMA otherschema; +SET search_path TO otherschema, public; + +SELECT ddl_in_plpgsql(); +DROP INDEX prepared_index; + +-- this creates the same table it 'otherschema'. If there is a leak the index will not be +-- created on this table, but instead on the table in the public schema +CREATE TABLE prepare_ddl (x int, y int); +SELECT create_distributed_table('prepare_ddl', 'x'); + +SELECT ddl_in_plpgsql(); +-- verify the index is created in the correct schema +SELECT schemaname, indexrelname FROM pg_stat_all_indexes WHERE indexrelname = 'prepared_index'; + +-- cleanup +DROP TABLE prepare_ddl; +DROP SCHEMA otherschema; + +RESET search_path; + -- test prepared COPY CREATE OR REPLACE FUNCTION copy_in_plpgsql() RETURNS VOID AS