mirror of https://github.com/citusdata/citus.git
Refactor the deparsing of a CREATE EXTENSION to prevent NULL POINTER dereferences (#3518)
DESCRIPTION: satisfy static analysis tool for a nullptr dereference During the static analysis project on the codebase this code has been flagged as having the potential for a null pointer dereference. Funnily enough the author had already made a comment of it in the code this was not possible due to us setting the schema name before we pass in the statement. If we want to reuse this code in a later setting this comment might not always apply and we could actually run into null pointer dereference. This patch changes a bit of the code around to first of all make sure there is no NULL pointer dereference in this code anymore. Secondly we allow for better deparsing by setting and adhering to the `if_not_exists` flag on the statement. And finally add support for all syntax described in the documentation of postgres (FROM was missing).pull/3572/head
Nils Dijk
GitHub
parent
9096c650f6
commit
268ad741a9
|
|
@ -12,6 +12,7 @@
|
|||
|
||||
#include "citus_version.h"
|
||||
#include "catalog/pg_extension_d.h"
|
||||
#include "commands/defrem.h"
|
||||
#include "commands/extension.h"
|
||||
#include "distributed/citus_ruleutils.h"
|
||||
#include "distributed/commands.h"
|
||||
|
|
@ -101,13 +102,12 @@ ExtractNewExtensionVersion(Node *parseTree)
|
|||
Assert(false);
|
||||
}
|
||||
|
||||
Value *newVersionValue = GetExtensionOption(optionsList, "new_version");
|
||||
DefElem *newVersionValue = GetExtensionOption(optionsList, "new_version");
|
||||
|
||||
/* return target string safely */
|
||||
if (newVersionValue)
|
||||
{
|
||||
const char *newVersion = strVal(newVersionValue);
|
||||
|
||||
const char *newVersion = defGetString(newVersionValue);
|
||||
return pstrdup(newVersion);
|
||||
}
|
||||
else
|
||||
|
|
@ -171,6 +171,9 @@ PostprocessCreateExtensionStmt(Node *node, const char *queryString)
|
|||
*/
|
||||
AddSchemaFieldIfMissing(stmt);
|
||||
|
||||
/* always send commands with IF NOT EXISTS */
|
||||
stmt->if_not_exists = true;
|
||||
|
||||
const char *createExtensionStmtSql = DeparseTreeNode(node);
|
||||
|
||||
/*
|
||||
|
|
@ -201,7 +204,7 @@ AddSchemaFieldIfMissing(CreateExtensionStmt *createExtensionStmt)
|
|||
{
|
||||
List *optionsList = createExtensionStmt->options;
|
||||
|
||||
Value *schemaNameValue = GetExtensionOption(optionsList, "schema");
|
||||
DefElem *schemaNameValue = GetExtensionOption(optionsList, "schema");
|
||||
|
||||
if (!schemaNameValue)
|
||||
{
|
||||
|
|
|
|||
|
|
@ -13,14 +13,17 @@
|
|||
#include "postgres.h"
|
||||
|
||||
#include "catalog/namespace.h"
|
||||
#include "commands/defrem.h"
|
||||
#include "distributed/deparser.h"
|
||||
#include "distributed/listutils.h"
|
||||
#include "lib/stringinfo.h"
|
||||
#include "nodes/pg_list.h"
|
||||
#include "nodes/parsenodes.h"
|
||||
#include "nodes/pg_list.h"
|
||||
#include "utils/builtins.h"
|
||||
|
||||
/* Local functions forward declarations for helper functions */
|
||||
static void AppendCreateExtensionStmt(StringInfo buf, CreateExtensionStmt *stmt);
|
||||
static void AppendCreateExtensionStmtOptions(StringInfo buf, List *options);
|
||||
static void AppendDropExtensionStmt(StringInfo buf, DropStmt *stmt);
|
||||
static void AppendExtensionNameList(StringInfo buf, List *objects);
|
||||
static void AppendAlterExtensionSchemaStmt(StringInfo buf,
|
||||
|
|
@ -30,36 +33,22 @@ static void AppendAlterExtensionStmt(StringInfo buf,
|
|||
|
||||
|
||||
/*
|
||||
* GetExtensionOption returns Value* of DefElem node with "defname" from "options" list
|
||||
* GetExtensionOption returns DefElem * node with "defname" from "options" list
|
||||
*/
|
||||
Value *
|
||||
DefElem *
|
||||
GetExtensionOption(List *extensionOptions, const char *defname)
|
||||
{
|
||||
Value *targetValue = NULL;
|
||||
|
||||
ListCell *defElemCell = NULL;
|
||||
|
||||
foreach(defElemCell, extensionOptions)
|
||||
DefElem *defElement = NULL;
|
||||
foreach_ptr(defElement, extensionOptions)
|
||||
{
|
||||
DefElem *defElement = (DefElem *) lfirst(defElemCell);
|
||||
|
||||
if (IsA(defElement, DefElem) && strncmp(defElement->defname, defname,
|
||||
NAMEDATALEN) == 0)
|
||||
if (IsA(defElement, DefElem) &&
|
||||
strncmp(defElement->defname, defname, NAMEDATALEN) == 0)
|
||||
{
|
||||
targetValue = (Value *) defElement->arg;
|
||||
break;
|
||||
return defElement;
|
||||
}
|
||||
}
|
||||
|
||||
/* return target string safely */
|
||||
if (targetValue)
|
||||
{
|
||||
return targetValue;
|
||||
}
|
||||
else
|
||||
{
|
||||
return NULL;
|
||||
}
|
||||
return NULL;
|
||||
}
|
||||
|
||||
|
||||
|
|
@ -86,54 +75,72 @@ DeparseCreateExtensionStmt(Node *node)
|
|||
static void
|
||||
AppendCreateExtensionStmt(StringInfo buf, CreateExtensionStmt *createExtensionStmt)
|
||||
{
|
||||
List *optionsList = createExtensionStmt->options;
|
||||
appendStringInfoString(buf, "CREATE EXTENSION ");
|
||||
|
||||
const char *extensionName = createExtensionStmt->extname;
|
||||
extensionName = quote_identifier(extensionName);
|
||||
|
||||
/*
|
||||
* We fetch "new_version", "schema" and "cascade" options from
|
||||
* optionList as we will append "IF NOT EXISTS" clause regardless of
|
||||
* statement's content before propagating it to worker nodes.
|
||||
* We also do not care old_version for now.
|
||||
*/
|
||||
Value *schemaNameValue = GetExtensionOption(optionsList, "schema");
|
||||
|
||||
/* these can be NULL hence check before fetching the stored value */
|
||||
Value *newVersionValue = GetExtensionOption(optionsList, "new_version");
|
||||
Value *cascadeValue = GetExtensionOption(optionsList, "cascade");
|
||||
|
||||
/*
|
||||
* We do not check for if schemaName is NULL as we append it in deparse
|
||||
* logic if it is not specified.
|
||||
*/
|
||||
const char *schemaName = strVal(schemaNameValue);
|
||||
schemaName = quote_identifier(schemaName);
|
||||
|
||||
appendStringInfo(buf, "CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s",
|
||||
extensionName, schemaName);
|
||||
|
||||
/* "new_version" may not be specified in CreateExtensionStmt */
|
||||
if (newVersionValue)
|
||||
if (createExtensionStmt->if_not_exists)
|
||||
{
|
||||
const char *newVersion = strVal(newVersionValue);
|
||||
newVersion = quote_identifier(newVersion);
|
||||
|
||||
appendStringInfo(buf, " VERSION %s", newVersion);
|
||||
appendStringInfoString(buf, "IF NOT EXISTS ");
|
||||
}
|
||||
|
||||
/* "cascade" may not be specified in CreateExtensionStmt */
|
||||
if (cascadeValue)
|
||||
{
|
||||
bool cascade = intVal(cascadeValue);
|
||||
/*
|
||||
* Up until here we have been ending the statement in a space, which makes it possible
|
||||
* to just append the quoted extname. From here onwards we will not have the string
|
||||
* ending in a space so appends should begin with a whitespace.
|
||||
*/
|
||||
appendStringInfoString(buf, quote_identifier(createExtensionStmt->extname));
|
||||
AppendCreateExtensionStmtOptions(buf, createExtensionStmt->options);
|
||||
appendStringInfoString(buf, ";");
|
||||
}
|
||||
|
||||
if (cascade)
|
||||
|
||||
/*
|
||||
* AppendCreateExtensionStmtOptions takes the option list of a CreateExtensionStmt and
|
||||
* loops over the options to add them to the statement we are building.
|
||||
*
|
||||
* An error will be thrown if we run into an unsupported option, comparable to how
|
||||
* postgres gives an error when parsing this list.
|
||||
*/
|
||||
static void
|
||||
AppendCreateExtensionStmtOptions(StringInfo buf, List *options)
|
||||
{
|
||||
if (list_length(options) > 0)
|
||||
{
|
||||
/* only append WITH if we actual will add options to the statement */
|
||||
appendStringInfoString(buf, " WITH");
|
||||
}
|
||||
|
||||
/* Add the options to the statement */
|
||||
DefElem *defElem = NULL;
|
||||
foreach_ptr(defElem, options)
|
||||
{
|
||||
if (strcmp(defElem->defname, "schema") == 0)
|
||||
{
|
||||
appendStringInfoString(buf, " CASCADE");
|
||||
const char *schemaName = defGetString(defElem);
|
||||
appendStringInfo(buf, " SCHEMA %s", quote_identifier(schemaName));
|
||||
}
|
||||
else if (strcmp(defElem->defname, "new_version") == 0)
|
||||
{
|
||||
const char *newVersion = defGetString(defElem);
|
||||
appendStringInfo(buf, " VERSION %s", quote_identifier(newVersion));
|
||||
}
|
||||
else if (strcmp(defElem->defname, "old_version") == 0)
|
||||
{
|
||||
const char *oldVersion = defGetString(defElem);
|
||||
appendStringInfo(buf, " FROM %s", quote_identifier(oldVersion));
|
||||
}
|
||||
else if (strcmp(defElem->defname, "cascade") == 0)
|
||||
{
|
||||
bool cascade = defGetBoolean(defElem);
|
||||
if (cascade)
|
||||
{
|
||||
appendStringInfoString(buf, " CASCADE");
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
elog(ERROR, "unrecognized option: %s", defElem->defname);
|
||||
}
|
||||
}
|
||||
|
||||
appendStringInfoString(buf, " ;");
|
||||
}
|
||||
|
||||
|
||||
|
|
@ -165,17 +172,25 @@ AppendAlterExtensionStmt(StringInfo buf, AlterExtensionStmt *alterExtensionStmt)
|
|||
const char *extensionName = alterExtensionStmt->extname;
|
||||
extensionName = quote_identifier(extensionName);
|
||||
|
||||
Value *newVersionValue = GetExtensionOption(optionsList, "new_version");
|
||||
appendStringInfo(buf, "ALTER EXTENSION %s UPDATE", extensionName);
|
||||
|
||||
appendStringInfo(buf, "ALTER EXTENSION %s UPDATE ", extensionName);
|
||||
|
||||
/* "new_version" may not be specified in AlterExtensionStmt */
|
||||
if (newVersionValue)
|
||||
/*
|
||||
* Append the options for ALTER EXTENSION ... UPDATE
|
||||
* Currently there is only 1 option, but this structure follows how postgres parses
|
||||
* the options.
|
||||
*/
|
||||
DefElem *option = NULL;
|
||||
foreach_ptr(option, optionsList)
|
||||
{
|
||||
const char *newVersion = strVal(newVersionValue);
|
||||
newVersion = quote_identifier(newVersion);
|
||||
|
||||
appendStringInfo(buf, " TO %s", newVersion);
|
||||
if (strcmp(option->defname, "new_version") == 0)
|
||||
{
|
||||
const char *newVersion = defGetString(option);
|
||||
appendStringInfo(buf, " TO %s", quote_identifier(newVersion));
|
||||
}
|
||||
else
|
||||
{
|
||||
elog(ERROR, "unrecognized option: %s", option->defname);
|
||||
}
|
||||
}
|
||||
|
||||
appendStringInfoString(buf, ";");
|
||||
|
|
|
|||
|
|
@ -97,8 +97,8 @@ extern void QualifyAlterFunctionDependsStmt(Node *stmt);
|
|||
extern char * DeparseAlterRoleStmt(Node *stmt);
|
||||
|
||||
/* forward declarations for deparse_extension_stmts.c */
|
||||
extern Value * GetExtensionOption(List *extensionOptions,
|
||||
const char *defname);
|
||||
extern DefElem * GetExtensionOption(List *extensionOptions,
|
||||
const char *defname);
|
||||
extern char * DeparseCreateExtensionStmt(Node *stmt);
|
||||
extern char * DeparseDropExtensionStmt(Node *stmt);
|
||||
extern char * DeparseAlterExtensionSchemaStmt(Node *stmt);
|
||||
|
|
|
|||
|
|
@ -182,6 +182,69 @@ SELECT create_reference_table('ref_table_2');
|
|||
|
||||
(1 row)
|
||||
|
||||
-- we also add an old style extension from before extensions which we upgrade to an extension
|
||||
-- by exercising it before the add node we verify it will create the extension (without upgrading)
|
||||
-- it on the new worker as well. For this we use the dict_int extension which is in contrib,
|
||||
-- supports FROM unpackaged, and is relatively small
|
||||
-- create objects for dict_int manually so we can upgrade from unpacked
|
||||
CREATE FUNCTION dintdict_init(internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
CREATE FUNCTION dintdict_lexize(internal, internal, internal, internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
CREATE TEXT SEARCH TEMPLATE intdict_template (LEXIZE = dintdict_lexize, INIT = dintdict_init );
|
||||
CREATE TEXT SEARCH DICTIONARY intdict (TEMPLATE = intdict_template);
|
||||
COMMENT ON TEXT SEARCH DICTIONARY intdict IS 'dictionary for integers';
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE FUNCTION dintdict_init(internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,"CREATE FUNCTION")
|
||||
(1 row)
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE FUNCTION dintdict_lexize(internal, internal, internal, internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,"CREATE FUNCTION")
|
||||
(1 row)
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE TEXT SEARCH TEMPLATE intdict_template (LEXIZE = dintdict_lexize, INIT = dintdict_init );
|
||||
$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,"CREATE TEXT SEARCH TEMPLATE")
|
||||
(1 row)
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE TEXT SEARCH DICTIONARY intdict (TEMPLATE = intdict_template);
|
||||
$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,"CREATE TEXT SEARCH DICTIONARY")
|
||||
(1 row)
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
COMMENT ON TEXT SEARCH DICTIONARY intdict IS 'dictionary for integers';
|
||||
$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,COMMENT)
|
||||
(1 row)
|
||||
|
||||
CREATE EXTENSION dict_int FROM unpackaged;
|
||||
SELECT run_command_on_workers($$SELECT count(extnamespace) FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,1)
|
||||
(1 row)
|
||||
|
||||
SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,1.0)
|
||||
(1 row)
|
||||
|
||||
-- and add the other node
|
||||
SELECT 1 from master_add_node('localhost', :worker_2_port);
|
||||
NOTICE: Replicating reference table "ref_table_2" to the node localhost:xxxxx
|
||||
|
|
@ -205,6 +268,21 @@ SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extnam
|
|||
(localhost,57638,t,1.3)
|
||||
(2 rows)
|
||||
|
||||
-- check for the unpackaged extension to be created correctly
|
||||
SELECT run_command_on_workers($$SELECT count(extnamespace) FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,1)
|
||||
(localhost,57638,t,1)
|
||||
(2 rows)
|
||||
|
||||
SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
run_command_on_workers
|
||||
---------------------------------------------------------------------
|
||||
(localhost,57637,t,1.0)
|
||||
(localhost,57638,t,1.0)
|
||||
(2 rows)
|
||||
|
||||
-- and similarly check for the reference table
|
||||
select count(*) from pg_dist_partition where partmethod='n' and logicalrelid='ref_table_2'::regclass;
|
||||
count
|
||||
|
|
|
|||
|
|
@ -110,6 +110,41 @@ SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extnam
|
|||
CREATE TABLE ref_table_2 (x seg);
|
||||
SELECT create_reference_table('ref_table_2');
|
||||
|
||||
-- we also add an old style extension from before extensions which we upgrade to an extension
|
||||
-- by exercising it before the add node we verify it will create the extension (without upgrading)
|
||||
-- it on the new worker as well. For this we use the dict_int extension which is in contrib,
|
||||
-- supports FROM unpackaged, and is relatively small
|
||||
|
||||
-- create objects for dict_int manually so we can upgrade from unpacked
|
||||
CREATE FUNCTION dintdict_init(internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
CREATE FUNCTION dintdict_lexize(internal, internal, internal, internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
CREATE TEXT SEARCH TEMPLATE intdict_template (LEXIZE = dintdict_lexize, INIT = dintdict_init );
|
||||
CREATE TEXT SEARCH DICTIONARY intdict (TEMPLATE = intdict_template);
|
||||
COMMENT ON TEXT SEARCH DICTIONARY intdict IS 'dictionary for integers';
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE FUNCTION dintdict_init(internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
$$);
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE FUNCTION dintdict_lexize(internal, internal, internal, internal) RETURNS internal AS 'dict_int.so' LANGUAGE C STRICT;
|
||||
$$);
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE TEXT SEARCH TEMPLATE intdict_template (LEXIZE = dintdict_lexize, INIT = dintdict_init );
|
||||
$$);
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
CREATE TEXT SEARCH DICTIONARY intdict (TEMPLATE = intdict_template);
|
||||
$$);
|
||||
|
||||
SELECT run_command_on_workers($$
|
||||
COMMENT ON TEXT SEARCH DICTIONARY intdict IS 'dictionary for integers';
|
||||
$$);
|
||||
|
||||
CREATE EXTENSION dict_int FROM unpackaged;
|
||||
SELECT run_command_on_workers($$SELECT count(extnamespace) FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
|
||||
-- and add the other node
|
||||
SELECT 1 from master_add_node('localhost', :worker_2_port);
|
||||
|
||||
|
|
@ -117,6 +152,10 @@ SELECT 1 from master_add_node('localhost', :worker_2_port);
|
|||
SELECT run_command_on_workers($$SELECT count(extnamespace) FROM pg_extension WHERE extname = 'seg'$$);
|
||||
SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extname = 'seg'$$);
|
||||
|
||||
-- check for the unpackaged extension to be created correctly
|
||||
SELECT run_command_on_workers($$SELECT count(extnamespace) FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
SELECT run_command_on_workers($$SELECT extversion FROM pg_extension WHERE extname = 'dict_int'$$);
|
||||
|
||||
-- and similarly check for the reference table
|
||||
select count(*) from pg_dist_partition where partmethod='n' and logicalrelid='ref_table_2'::regclass;
|
||||
SELECT count(*) FROM pg_dist_shard WHERE logicalrelid='ref_table_2'::regclass;
|
||||
|
|
|
|||
Loading…
Reference in New Issue