[PATCH] ecpg: fix progname memory leak
Hi,
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
Best regards,
Maksim Kita
>From 2f730117cbff1207c6b0bffc3097831fe5028fec Mon Sep 17 00:00:00 2001
From: Maksim Kita
Date: Wed, 7 Oct 2020 14:15:52 +0300
Subject: [PATCH] ecpg: fix progname memory leak
Added goto on error cleanup for progname in ecpg main.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
---
src/interfaces/ecpg/preproc/ecpg.c | 33 ++--
src/interfaces/ecpg/preproc/preproc_extern.h | 1 +
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 44a6d5119b..1be9483d88 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -127,7 +127,7 @@ main(int argc, char *const argv[])
bool verbose = false,
header_mode = false;
struct _include_path *ip;
- const char *progname;
+ const char *progname = NULL;
char my_exec_path[MAXPGPATH];
char include_path[MAXPGPATH];
@@ -138,7 +138,8 @@ main(int argc, char *const argv[])
if (find_my_exec(argv[0], my_exec_path) < 0)
{
fprintf(stderr, _("%s: could not locate my own executable path\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
if (argc > 1)
@@ -146,12 +147,14 @@ main(int argc, char *const argv[])
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
help(progname);
- exit(0);
+ ret_value = EXIT_OK;
+ goto err;
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
{
printf("ecpg (PostgreSQL) %s\n", PG_VERSION);
- exit(0);
+ ret_value = EXIT_OK;
+ goto err;
}
}
@@ -216,7 +219,8 @@ main(int argc, char *const argv[])
else
{
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
break;
case 'r':
@@ -229,7 +233,8 @@ main(int argc, char *const argv[])
else
{
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
break;
case 'D':
@@ -245,7 +250,8 @@ main(int argc, char *const argv[])
break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
-return ILLEGAL_OPTION;
+ret_value = ILLEGAL_OPTION;
+goto err;
}
}
@@ -264,14 +270,16 @@ main(int argc, char *const argv[])
for (ip = include_paths; ip != NULL; ip = ip->next)
fprintf(stderr, " %s\n", ip->path);
fprintf(stderr, _("end of search list\n"));
- return 0;
+ ret_value = EXIT_OK;
+ goto err;
}
if (optind >= argc) /* no files specified */
{
fprintf(stderr, _("%s: no input files specified\n"), progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
else
{
@@ -473,7 +481,7 @@ main(int argc, char *const argv[])
/*
* If there was an error, delete the output file.
*/
-if (ret_value != 0)
+if (ret_value != EXIT_OK)
{
if (strcmp(output_filename, "-") != 0 && unlink(output_filename) != 0)
fprintf(stderr, _("could not remove output file \"%s\"\n"), output_filename);
@@ -489,5 +497,10 @@ main(int argc, char *const argv[])
free(input_filename);
}
}
+
+err:
+ if (progname)
+ free((void*)(progname));
+
return ret_value;
}
diff --git a/src/interfaces/ecpg/preproc/preproc_extern.h b/src/interfaces/ecpg/preproc/preproc_extern.h
index 51d5f94f07..5728dd8a96 100644
--- a/src/interfaces/ecpg/preproc/preproc_extern.h
+++ b/src/interfaces/ecpg/preproc/preproc_extern.h
@@ -106,6 +106,7 @@ extern int filtered_base_yylex(void);
/* return codes */
+#define EXIT_OK0
#define ILLEGAL_OPTION 1
#define NO_INCLUDE_FILE 2
#define PARSE_ERROR 3
--
2.25.1
Allow deleting enum value
Hi,
There is a question related to TODO task with name "Allow deleting enumerated
values from an existing enumerated data type".
I made simple changes in parser and implement RemoveEnumLabel function, but as
I understand if enum value that we want to
delete is in use by some tables, we have to prevent deletion to avoid
inconsistency.
Could you please provide some references where similar functionality was
implemented ? Thanks.
Attached basic patch without handling dependencies.
Example of problem that I mention using basic implementation:
postgres=# CREATE TYPE enum_test as ENUM ('1', '2', '3');
CREATE TYPE
postgres=# CREATE TYPE test_enum as ENUM ('1', '2', '3');
CREATE TYPE
postgres=# CREATE TABLE test_table (value test_enum);
CREATE TABLE
postgres=# INSERT INTO test_table VALUES ('1'), ('2');
INSERT 0 2
postgres=# ALTER TYPE test_enum DELETE VALUE '2';
ALTER TYPE
postgres=# SELECT enum_range(NULL::test_enum);
enum_range
{1,3}
(1 row)
postgres=# SELECT * FROM test_table;
ERROR: invalid internal value for enum: 16396
Best regards,
Maksim Kita
>From 2ae09254584340b7700e31680d752822a782be75 Mon Sep 17 00:00:00 2001
From: Maksim Kita
Date: Wed, 7 Oct 2020 17:18:01 +0300
Subject: [PATCH] Allow alter type delete value in enum
---
src/backend/catalog/pg_enum.c | 60 +
src/backend/commands/typecmds.c | 8 +++--
src/backend/parser/gram.y | 11 ++
src/include/catalog/pg_enum.h | 1 +
4 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 27e4100a6f..c547dfd22c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -583,6 +583,66 @@ RenameEnumLabel(Oid enumTypeOid,
table_close(pg_enum, RowExclusiveLock);
}
+extern void RemoveEnumLabel(Oid enumTypeOid, const char *val)
+{
+ Relation pg_enum;
+ HeapTuple enum_tup;
+ Form_pg_enum en;
+ CatCList *list;
+ int nelems;
+ HeapTuple old_tup;
+ int i;
+
+ /* check length of new label is ok */
+ if (strlen(val) > (NAMEDATALEN - 1))
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", val),
+ errdetail("Labels must be %d characters or less.",
+ NAMEDATALEN - 1)));
+
+ /*
+ * Acquire a lock on the enum type, which we won't release until commit.
+ * This ensures that two backends aren't concurrently modifying the same
+ * enum type. Since we are not changing the type's sort order, this is
+ * probably not really necessary, but there seems no reason not to take
+ * the lock to be sure.
+ */
+ LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+ pg_enum = table_open(EnumRelationId, RowExclusiveLock);
+
+ /* Get the list of existing members of the enum */
+ list = SearchSysCacheList1(ENUMTYPOIDNAME,
+ ObjectIdGetDatum(enumTypeOid));
+ nelems = list->n_members;
+
+ /*
+ * Check if the label is in use. (The unique index on pg_enum would catch that anyway, but we
+ * prefer a friendlier error message.)
+ */
+ for (i = 0; i < nelems; i++)
+ {
+ enum_tup = &(list->members[i]->tuple);
+ en = (Form_pg_enum) GETSTRUCT(enum_tup);
+ if (strcmp(NameStr(en->enumlabel), val) == 0)
+ {
+ old_tup = enum_tup;
+ break;
+ }
+ }
+ if (!old_tup)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+ val)));
+
+ ReleaseCatCacheList(list);
+
+ CatalogTupleDelete(pg_enum, &old_tup->t_self);
+
+ table_close(pg_enum, RowExclusiveLock);
+}
/*
* Test if the given enum value is on the blacklist
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 483bb65ddc..b9b3e18a94 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1232,13 +1232,17 @@ AlterEnum(AlterEnumStmt *stmt)
ReleaseSysCache(tup);
- if (stmt->oldVal)
+ if (stmt->oldVal && stmt->newVal)
{
/* Rename an existing label */
RenameEnumLabel(enum_type_oid, stmt->oldVal, stmt->newVal);
}
- else
+ else if (stmt->oldVal)
{
+ /* Delete an existing label */
+ RemoveEnumLabel(enum_type_oid, stmt->oldVal);
+ }
+ else {
/* Add a new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
stmt->newValNeighbor, stmt->newValIsAfter,
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0d101d8171..82dd5147fa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5792,6 +5792,17 @@ AlterEnumStmt:
n->skipIfNewValExists = false;
$$ = (Node *) n;
}
+ | ALTER TYPE_P any_name DELETE_P VALUE_P Sconst
+ {
+AlterEnumStmt *n = makeNode(AlterEnumStmt);
+n->typeName = $3;
+n->oldVal = $6;
+n->newVal =
Re: Allow deleting enum value
I like the idea, during prototype I added additional column and modified
enum_in method. But enum_in is called in contexts that can be important
for user (like comparisons).
Example:
postgres=# CREATE TYPE test_enum AS enum ('1', '2', '3');
CREATE TYPE
postgres=# CREATE TABLE test_table ( value test_enum );
postgres=# INSERT INTO test_table VALUES ('1'), ('2'), ('3');
INSERT 0 3
postgres=# ALTER TYPE test_enum DELETE VALUE '2';
ALTER TYPE
postgres=# INSERT INTO test_table VALUES ('2');
ERROR: enum value is dropped test_enum: "2"
LINE 1: INSERT INTO test_table VALUES ('2');
postgres=# SELECT * FROM test_table WHERE value = '2';
ERROR: enum value is dropped test_enum: "2"
LINE 1: SELECT * FROM test_table WHERE value = '2';
postgres=# UPDATE test_table SET value = '3' WHERE value = '2';
ERROR: enum value is dropped test_enum: "2"
LINE 1: UPDATE test_table SET value = '3' WHERE value = '2';
Probably we need to make more specific change for enum type to prevent
using of dropped column in context of insert or update (where we
creating dropped enum value), but not in others.
Is that possible ? What places should I look into ? Thanks.
Best regards,
Maksim Kita
>From f829ae210fc0f6a010cba9c9e4e7772a9b59ae37 Mon Sep 17 00:00:00 2001
From: Maksim Kita
Date: Wed, 7 Oct 2020 23:35:56 +0300
Subject: [PATCH] Allow alter type delete value in enum
---
src/backend/catalog/pg_enum.c | 69 +
src/backend/commands/typecmds.c | 8 +++-
src/backend/parser/gram.y | 11 ++
src/backend/utils/adt/enum.c| 10 +
src/include/catalog/pg_enum.h | 2 +
5 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 27e4100a6f..907db93cf4 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -133,6 +133,7 @@ EnumValuesCreate(Oid enumTypeOid, List *vals)
values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(elemno + 1);
namestrcpy(&enumlabel, lab);
values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
+ values[Anum_pg_enum_isdropped - 1] = false;
tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
@@ -485,6 +486,7 @@ restart:
values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(newelemorder);
namestrcpy(&enumlabel, newVal);
values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
+ values[Anum_pg_enum_isdropped - 1] = false;
enum_tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
CatalogTupleInsert(pg_enum, enum_tup);
heap_freetuple(enum_tup);
@@ -583,6 +585,73 @@ RenameEnumLabel(Oid enumTypeOid,
table_close(pg_enum, RowExclusiveLock);
}
+extern void RemoveEnumLabel(Oid enumTypeOid, const char *val)
+{
+ Relation pg_enum;
+ HeapTuple enum_tup;
+ Form_pg_enum en;
+ CatCList *list;
+ int nelems;
+ HeapTuple old_tup;
+ int i;
+
+ /* check length of new label is ok */
+ if (strlen(val) > (NAMEDATALEN - 1))
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", val),
+ errdetail("Labels must be %d characters or less.",
+ NAMEDATALEN - 1)));
+
+ /*
+ * Acquire a lock on the enum type, which we won't release until commit.
+ * This ensures that two backends aren't concurrently modifying the same
+ * enum type. Since we are not changing the type's sort order, this is
+ * probably not really necessary, but there seems no reason not to take
+ * the lock to be sure.
+ */
+ LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+ pg_enum = table_open(EnumRelationId, RowExclusiveLock);
+
+ /* Get the list of existing members of the enum */
+ list = SearchSysCacheList1(ENUMTYPOIDNAME,
+ ObjectIdGetDatum(enumTypeOid));
+ nelems = list->n_members;
+
+ /*
+ * Check if the label is in use. (The unique index on pg_enum would catch that anyway, but we
+ * prefer a friendlier error message.)
+ */
+ for (i = 0; i < nelems; i++)
+ {
+ enum_tup = &(list->members[i]->tuple);
+ en = (Form_pg_enum) GETSTRUCT(enum_tup);
+ if (strcmp(NameStr(en->enumlabel), val) == 0)
+ {
+ old_tup = enum_tup;
+ break;
+ }
+ }
+ if (!old_tup)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+ val)));
+
+ /* OK, make a writable copy of old tuple */
+ enum_tup = heap_copytuple(old_tup);
+ en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+ ReleaseCatCacheList(list);
+
+ /* Update the pg_enum entry */
+ en->isdropped = true;
+ CatalogTupleUpdate(pg_enum, &enum_tup->t_self, enum_tup);
+ heap_freetuple(enum_tup);
+
+ table_close(pg_enum, RowExclusiveLock);
+}
/*
* Test if the given
