From 61ea0bd12a80a5237d06be7d8dfac36f8e3a929b Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 30 Jan 2019 16:46:20 +0300
Subject: [PATCH 01/37] Clarify the README

---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 87867e8..d7d897f 100644
--- a/README.md
+++ b/README.md
@@ -56,7 +56,7 @@ Typical installation procedure may look like this:
 ## Module functions
 
 The functions provided by the **pg_variables** module are shown in the tables
-below. The module supports the following scalar and record types.
+below.
 
 To use **pgv_get()** function required package and variable must exists. It is
 necessary to set variable with **pgv_set()** function to use **pgv_get()**

From 68edd02456eb298d302104ba143b83bb1e0a8bcd Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Thu, 31 Jan 2019 15:19:54 +0300
Subject: [PATCH 02/37] Fix for 8766f08eae: Use xact callback instead of the
 hook, it allowes to clean up after rollback

---
 pg_variables.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 74d0341..7a299e8 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -80,9 +80,6 @@ static void makePackHTAB(Package *package, bool is_trans);
 static inline ChangedObject *makeChangedObject(TransObject *object,
 											   MemoryContext ctx);
 
-/* Hook functions */
-static void variable_ExecutorEnd(QueryDesc *queryDesc);
-
 #define CHECK_ARGS_FOR_NULL() \
 do { \
 	if (fcinfo->argnull[0]) \
@@ -113,9 +110,6 @@ static Oid LastTypeId = InvalidOid;
  */
 static HASH_SEQ_STATUS *LastHSeqStatus = NULL;
 
-/* Saved hook values for recall */
-static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
-
 /* This stack contains lists of changed variables and packages per each subxact level */
 static dlist_head *changesStack = NULL;
 static MemoryContext changesStackContext = NULL;
@@ -2120,23 +2114,15 @@ pgvTransCallback(XactEvent event, void *arg)
 				break;
 		}
 	}
-}
 
-/*
- * ExecutorEnd hook: clean up hash table sequential scan status
- */
-static void
-variable_ExecutorEnd(QueryDesc *queryDesc)
-{
-	if (LastHSeqStatus)
-	{
-		hash_seq_term(LastHSeqStatus);
-		LastHSeqStatus = NULL;
-	}
-	if (prev_ExecutorEnd)
-		prev_ExecutorEnd(queryDesc);
-	else
-		standard_ExecutorEnd(queryDesc);
+	if (event == XACT_EVENT_PARALLEL_COMMIT || event == XACT_EVENT_COMMIT ||
+		event == XACT_EVENT_PREPARE ||
+		event == XACT_EVENT_PARALLEL_ABORT || event == XACT_EVENT_ABORT)
+		if (LastHSeqStatus)
+		{
+			hash_seq_term(LastHSeqStatus);
+			LastHSeqStatus = NULL;
+		}
 }
 
 /*
@@ -2147,10 +2133,6 @@ _PG_init(void)
 {
 	RegisterXactCallback(pgvTransCallback, NULL);
 	RegisterSubXactCallback(pgvSubTransCallback, NULL);
-
-	/* Install hooks. */
-	prev_ExecutorEnd = ExecutorEnd_hook;
-	ExecutorEnd_hook = variable_ExecutorEnd;
 }
 
 /*
@@ -2161,5 +2143,4 @@ _PG_fini(void)
 {
 	UnregisterXactCallback(pgvTransCallback, NULL);
 	UnregisterSubXactCallback(pgvSubTransCallback, NULL);
-	ExecutorEnd_hook = prev_ExecutorEnd;
 }

From e5af4d33bcc16f026086f55c078af97df3c59d89 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Thu, 31 Jan 2019 15:28:02 +0300
Subject: [PATCH 03/37] Fix for 68edd02456eb: We need the hook anyway if we use
 long transactions

---
 pg_variables.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/pg_variables.c b/pg_variables.c
index 7a299e8..85a90d8 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -80,6 +80,9 @@ static void makePackHTAB(Package *package, bool is_trans);
 static inline ChangedObject *makeChangedObject(TransObject *object,
 											   MemoryContext ctx);
 
+/* Hook functions */
+static void variable_ExecutorEnd(QueryDesc *queryDesc);
+
 #define CHECK_ARGS_FOR_NULL() \
 do { \
 	if (fcinfo->argnull[0]) \
@@ -110,6 +113,9 @@ static Oid LastTypeId = InvalidOid;
  */
 static HASH_SEQ_STATUS *LastHSeqStatus = NULL;
 
+/* Saved hook values for recall */
+static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
+
 /* This stack contains lists of changed variables and packages per each subxact level */
 static dlist_head *changesStack = NULL;
 static MemoryContext changesStackContext = NULL;
@@ -2125,6 +2131,23 @@ pgvTransCallback(XactEvent event, void *arg)
 		}
 }
 
+/*
+ * ExecutorEnd hook: clean up hash table sequential scan status
+ */
+static void
+variable_ExecutorEnd(QueryDesc *queryDesc)
+{
+	if (LastHSeqStatus)
+	{
+		hash_seq_term(LastHSeqStatus);
+		LastHSeqStatus = NULL;
+	}
+	if (prev_ExecutorEnd)
+		prev_ExecutorEnd(queryDesc);
+	else
+		standard_ExecutorEnd(queryDesc);
+}
+
 /*
  * Register callback function when module starts
  */
@@ -2133,6 +2156,10 @@ _PG_init(void)
 {
 	RegisterXactCallback(pgvTransCallback, NULL);
 	RegisterSubXactCallback(pgvSubTransCallback, NULL);
+
+	/* Install hooks. */
+	prev_ExecutorEnd = ExecutorEnd_hook;
+	ExecutorEnd_hook = variable_ExecutorEnd;
 }
 
 /*
@@ -2143,4 +2170,5 @@ _PG_fini(void)
 {
 	UnregisterXactCallback(pgvTransCallback, NULL);
 	UnregisterSubXactCallback(pgvSubTransCallback, NULL);
+	ExecutorEnd_hook = prev_ExecutorEnd;
 }

From 1b0b3bec601a177ebf8947e51382925c11901d07 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Thu, 31 Jan 2019 15:44:28 +0300
Subject: [PATCH 04/37] Fix for 68edd02456eb: Do not call hash_seq_term(), we
 can't trust to its pointer in the callback function

---
 expected/pg_variables.out | 43 ++++++++++++++++++++++++++-------------
 pg_variables.c            |  7 +------
 sql/pg_variables.sql      | 14 ++++++++++---
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 2bc5520..0afdb44 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -553,20 +553,6 @@ SELECT pgv_insert('vars3', 'r1', row(1, 1));
 ERROR:  new record structure differs from variable "r1" structure
 SELECT pgv_insert('vars3', 'r1', row('str1', 'str1'));
 ERROR:  new record structure differs from variable "r1" structure
-SELECT pgv_select('vars3', 'r1') LIMIT 2;
- pgv_select 
-------------
- (,strNULL)
- (1,str11)
-(2 rows)
-
-SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
- pgv_select 
-------------
- (2,)
- (0,str00)
-(2 rows)
-
 SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
@@ -657,6 +643,35 @@ SELECT pgv_exists('vars3', 'r1');
 
 SELECT pgv_select('vars2', 'j1');
 ERROR:  variable "j1" requires "jsonb" value
+-- Tests for SRF's sequential scan of an internal hash table
+DO
+$$BEGIN
+    PERFORM pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
+    PERFORM pgv_select('vars3', 'r3');
+END$$;
+ERROR:  unrecognized variable "r3"
+CONTEXT:  SQL statement "SELECT pgv_select('vars3', 'r3')"
+PL/pgSQL function inline_code_block line 3 at PERFORM
+-- Check that the hash table was cleaned up after rollback
+SELECT pgv_select('vars3', 'r1', 1);
+ pgv_select 
+------------
+ 
+(1 row)
+
+SELECT pgv_select('vars3', 'r1') LIMIT 2;
+ pgv_select 
+------------
+ (,strNULL)
+ (2,)
+(2 rows)
+
+SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
+ pgv_select 
+------------
+ (0,str00)
+(1 row)
+
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
  package |   name   | is_transactional 
diff --git a/pg_variables.c b/pg_variables.c
index 85a90d8..33626b3 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -2121,14 +2121,9 @@ pgvTransCallback(XactEvent event, void *arg)
 		}
 	}
 
-	if (event == XACT_EVENT_PARALLEL_COMMIT || event == XACT_EVENT_COMMIT ||
-		event == XACT_EVENT_PREPARE ||
-		event == XACT_EVENT_PARALLEL_ABORT || event == XACT_EVENT_ABORT)
+	if (event == XACT_EVENT_PARALLEL_ABORT || event == XACT_EVENT_ABORT)
 		if (LastHSeqStatus)
-		{
-			hash_seq_term(LastHSeqStatus);
 			LastHSeqStatus = NULL;
-		}
 }
 
 /*
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 9d5fcf3..9838fb5 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -153,9 +153,6 @@ SELECT pgv_insert('vars3', 'r1', row(1, 'str1', 'str2'));
 SELECT pgv_insert('vars3', 'r1', row(1, 1));
 SELECT pgv_insert('vars3', 'r1', row('str1', 'str1'));
 
-SELECT pgv_select('vars3', 'r1') LIMIT 2;
-SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
-
 SELECT pgv_select('vars3', 'r1');
 SELECT pgv_select('vars3', 'r1', 1);
 SELECT pgv_select('vars3', 'r1', 0);
@@ -175,6 +172,17 @@ SELECT pgv_exists('vars3', 'r3');
 SELECT pgv_exists('vars3', 'r1');
 SELECT pgv_select('vars2', 'j1');
 
+-- Tests for SRF's sequential scan of an internal hash table
+DO
+$$BEGIN
+    PERFORM pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
+    PERFORM pgv_select('vars3', 'r3');
+END$$;
+-- Check that the hash table was cleaned up after rollback
+SELECT pgv_select('vars3', 'r1', 1);
+SELECT pgv_select('vars3', 'r1') LIMIT 2;
+SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
+
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
 SELECT package FROM pgv_stats() order by package;

From e9d74c452f07941e63dbf1cf46348130c40feb49 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Fri, 1 Feb 2019 16:04:46 +0300
Subject: [PATCH 05/37] Remove package automatically when it becomes empty

---
 README.md                       |  4 +++
 expected/pg_variables_trans.out | 10 +++----
 pg_variables.c                  | 53 +++++++++++++++++++++++++++++----
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/README.md b/README.md
index d7d897f..0c584c0 100644
--- a/README.md
+++ b/README.md
@@ -38,6 +38,10 @@ SELECT pgv_get('vars', 'trans_int', NULL::int);
      101
 ```
 
+You can aggregate variables into packages. This is done to be able to have
+variables with different names or to quickly remove the whole batch of
+variables. If the package becomes empty, it is automatically deleted.
+
 ## License
 
 This module available under the [license](LICENSE) similar to
diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index d5b8fb5..397afac 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1035,7 +1035,7 @@ SELECT pgv_get('vars2', 'any1',NULL::text);
 ROLLBACK TO sp1;
 COMMIT;
 SELECT pgv_get('vars2', 'any1',NULL::text);
-ERROR:  unrecognized variable "any1"
+ERROR:  unrecognized package "vars2"
 SELECT pgv_free();
  pgv_free 
 ----------
@@ -1385,7 +1385,7 @@ SELECT pgv_get('vars2', 'any1',NULL::text);
 
 ROLLBACK;
 SELECT pgv_get('vars2', 'any1',NULL::text);
-ERROR:  unrecognized variable "any1"
+ERROR:  unrecognized package "vars2"
 SELECT pgv_free();
  pgv_free 
 ----------
@@ -1652,7 +1652,7 @@ SELECT pgv_exists('vars', 'any1');
 (1 row)
 
 SELECT pgv_get('vars', 'any1',NULL::text);
-ERROR:  unrecognized variable "any1"
+ERROR:  unrecognized package "vars"
 SELECT * FROM pgv_list() ORDER BY package, name;
  package | name | is_transactional 
 ---------+------+------------------
@@ -1858,7 +1858,7 @@ SELECT pgv_insert('package', 'errs',row(1), true);
 SELECT pgv_insert('vars4', 'r1', row('str1', 'str1'));
 ERROR:  could not identify a hash function for type unknown
 SELECT pgv_select('vars4', 'r1', 0);
-ERROR:  unrecognized variable "r1"
+ERROR:  unrecognized package "vars4"
 -- If variable created and removed in same transaction level,
 -- it should be totally removed and should not be present
 -- in changes list and cache.
@@ -1878,7 +1878,7 @@ SELECT pgv_remove('vars', 'any1');
 
 RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
-ERROR:  unrecognized variable "any1"
+ERROR:  unrecognized package "vars"
 COMMIT;
 SELECT pgv_free();
  pgv_free 
diff --git a/pg_variables.c b/pg_variables.c
index 33626b3..1c6318e 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -955,6 +955,7 @@ removePackageInternal(Package *package)
 	{
 		MemoryContextDelete(package->hctxRegular);
 		package->hctxRegular = NULL;
+		package->varHashRegular = NULL;
 	}
 
 	/* Add to changes list */
@@ -967,6 +968,17 @@ removePackageInternal(Package *package)
 	GetActualState(package)->is_valid = false;
 }
 
+static bool
+isPackageEmpty(Package *package)
+{
+	int var_num = hash_get_num_entries(package->varHashTransact);
+
+	if (package->varHashRegular)
+		var_num += hash_get_num_entries(package->varHashRegular);
+
+	return var_num == 0;
+}
+
 /*
  * Reset cache variables to their default values. It is necessary to do in case
  * of some changes: removing, rollbacking, etc.
@@ -1657,6 +1669,7 @@ removeObject(TransObject *object, TransObjectType type)
 {
 	bool		found;
 	HTAB	   *hash;
+	Package	   *package = NULL;
 
 	/*
 	 * Delete an object from the change history of the overlying
@@ -1666,16 +1679,20 @@ removeObject(TransObject *object, TransObjectType type)
 			removeFromChangesStack(object, type);
 	if (type == TRANS_PACKAGE)
 	{
-		Package    *package = (Package *) object;
+		package = (Package *) object;
 
 		/* Regular variables had already removed */
 		MemoryContextDelete(package->hctxTransact);
 		hash = packagesHash;
 	}
 	else
-		hash = ((Variable *) object)->is_transactional ?
-			   ((Variable *) object)->package->varHashTransact :
-			   ((Variable *) object)->package->varHashRegular;
+	{
+		Variable *var = (Variable *) object;
+		package = var->package;
+		hash = var->is_transactional ?
+			   var->package->varHashTransact :
+			   var->package->varHashRegular;
+	}
 
 	/* Remove all object's states */
 	while (!dlist_is_empty(&object->states))
@@ -1684,6 +1701,12 @@ removeObject(TransObject *object, TransObjectType type)
 	/* Remove object from hash table */
 	hash_search(hash, object->name, HASH_REMOVE, &found);
 
+	/* Remove package if it is became empty */
+	if (type == TRANS_VARIABLE &&
+		isObjectChangedInCurrentTrans(&package->transObject) &&
+		isPackageEmpty(package))
+		GetActualState(&package->transObject)->is_valid = false;
+
 	resetVariablesCache(true);
 }
 
@@ -1725,8 +1748,19 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 	{
 		if (!state->is_valid)
 		{
-			dlist_pop_head_node(&object->states);
-			pfree(state);
+			if (isPackageEmpty((Package *)object))
+			{
+				removeObject(object, TRANS_PACKAGE);
+				return;
+			}
+
+			if (dlist_has_next(&object->states, &state->node))
+			{
+				dlist_pop_head_node(&object->states);
+				pfree(state);
+			}
+			else
+				state->is_valid = true;
 			/* Restore regular vars HTAB */
 			makePackHTAB((Package *) object, false);
 		}
@@ -1797,6 +1831,13 @@ releaseSavepoint(TransObject *object, TransObjectType type)
 			!dlist_has_next(states, dlist_head_node(states)))
 		{
 			removeObject(object, type);
+			/* Remove package if it becomes empty */
+			if (type == TRANS_VARIABLE)
+			{
+				Package *pack = ((Variable *) object)->package;
+				if (isPackageEmpty(pack))
+					(GetActualState(&pack->transObject))->is_valid = false;
+			}
 			return;
 		}
 	}

From 51f3ace266e381a85ceae82ddd3a77f7a29ae848 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Fri, 1 Feb 2019 22:49:43 +0300
Subject: [PATCH 06/37] Fix adding package to changes list after last variable
 removal

---
 expected/pg_variables.out | 26 +++++++++++++++
 pg_variables.c            | 68 ++++++++++++++++++---------------------
 sql/pg_variables.sql      |  9 ++++++
 3 files changed, 67 insertions(+), 36 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 0afdb44..3ef1026 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -672,6 +672,32 @@ SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
  (0,str00)
 (1 row)
 
+-- Clean memory after unsuccessful creation of a variable
+SELECT pgv_insert('vars4', 'r1', row('str1', 'str1'));
+ERROR:  could not identify a hash function for type unknown
+SELECT package FROM pgv_stats() WHERE package = 'vars4';
+ package 
+---------
+(0 rows)
+
+-- Remove package if it is empty
+SELECT pgv_insert('vars4', 'r2', row(1, 'str1', 'str2'));
+ pgv_insert 
+------------
+ 
+(1 row)
+
+SELECT pgv_remove('vars4', 'r2');
+ pgv_remove 
+------------
+ 
+(1 row)
+
+SELECT package FROM pgv_stats() WHERE package = 'vars4';
+ package 
+---------
+(0 rows)
+
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
  package |   name   | is_transactional 
diff --git a/pg_variables.c b/pg_variables.c
index 1c6318e..f970435 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -52,7 +52,7 @@ static void getKeyFromName(text *name, char *key);
 static Package *getPackageByName(text *name, bool create, bool strict);
 static Variable *getVariableInternal(Package *package,
 									 text *name, Oid typid,
-									 bool strict);
+									 bool strict, bool type_strict);
 static Variable *createVariableInternal(Package *package,
 										text *name, Oid typid,
 										bool is_transactional);
@@ -197,7 +197,7 @@ variable_get(text *package_name, text *var_name,
 		return 0;
 	}
 
-	variable = getVariableInternal(package, var_name, typid, strict);
+	variable = getVariableInternal(package, var_name, typid, strict, true);
 
 	if (variable == NULL)
 	{
@@ -455,7 +455,7 @@ variable_update(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 		LastVariable = variable;
 	}
 	else
@@ -543,7 +543,7 @@ variable_delete(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 		LastVariable = variable;
 	}
 	else
@@ -592,7 +592,7 @@ variable_select(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 
 		record = &(GetActualValue(variable).record);
 
@@ -667,7 +667,7 @@ variable_select_by_value(PG_FUNCTION_ARGS)
 	}
 
 	package = getPackageByName(package_name, false, true);
-	variable = getVariableInternal(package, var_name, RECORDOID, true);
+	variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 
 	if (!value_is_null)
 		check_record_key(variable, value_type);
@@ -736,7 +736,7 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 
 		check_record_key(variable, ARR_ELEMTYPE(values));
 
@@ -858,12 +858,11 @@ package_exists(PG_FUNCTION_ARGS)
 Datum
 remove_variable(PG_FUNCTION_ARGS)
 {
-	text	   *package_name;
-	text	   *var_name;
-	Package	   *package;
-	Variable   *variable;
-	bool		found;
-	char		key[NAMEDATALEN];
+	text		   *package_name;
+	text		   *var_name;
+	Package		   *package;
+	Variable	   *variable;
+	TransObject	   *transObject;
 
 	CHECK_ARGS_FOR_NULL();
 
@@ -871,30 +870,18 @@ remove_variable(PG_FUNCTION_ARGS)
 	var_name = PG_GETARG_TEXT_PP(1);
 
 	package = getPackageByName(package_name, false, true);
-	getKeyFromName(var_name, key);
+	variable = getVariableInternal(package, var_name, 0, true, false);
 
-	variable = (Variable *) hash_search(package->varHashRegular,
-										key, HASH_REMOVE, &found);
-	if (found)
+	/* Add package to changes list, so we can remove it if it  */
+	if (!isObjectChangedInCurrentTrans(&package->transObject))
 	{
-		/* Regular variable */
-		removeState(&variable->transObject, TRANS_VARIABLE,
-					GetActualState(variable));
+		createSavepoint(&package->transObject, TRANS_PACKAGE);
+		addToChangesStack(&package->transObject, TRANS_PACKAGE);
 	}
-	else
-	{
-		TransObject *transObject;
 
-		variable = (Variable *) hash_search(package->varHashTransact,
-											key, HASH_FIND, &found);
-		/* Variable doesn't exist in both HTAB */
-		if (!found)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unrecognized variable \"%s\"", key)));
-
-		/* Transactional variable */
-		transObject = &variable->transObject;
+	transObject = &variable->transObject;
+	if (variable->is_transactional)
+	{
 		if (!isObjectChangedInCurrentTrans(transObject))
 		{
 			createSavepoint(transObject, TRANS_VARIABLE);
@@ -902,6 +889,8 @@ remove_variable(PG_FUNCTION_ARGS)
 		}
 		GetActualState(variable)->is_valid = false;
 	}
+	else
+		removeObject(&variable->transObject, TRANS_VARIABLE);
 
 	resetVariablesCache(false);
 
@@ -1449,7 +1438,8 @@ getPackageByName(text *name, bool create, bool strict)
  * flag 'is_transactional' of this variable is unknown.
  */
 static Variable *
-getVariableInternal(Package *package, text *name, Oid typid, bool strict)
+getVariableInternal(Package *package, text *name, Oid typid, bool strict,
+					bool type_strict)
 {
 	Variable   *variable;
 	char		key[NAMEDATALEN];
@@ -1466,7 +1456,7 @@ getVariableInternal(Package *package, text *name, Oid typid, bool strict)
 	/* Check variable type */
 	if (found)
 	{
-		if (variable->typid != typid)
+		if (type_strict && variable->typid != typid)
 		{
 			char	   *var_type = DatumGetCString(DirectFunctionCall1(regtypeout,
 																	   ObjectIdGetDatum(variable->typid)));
@@ -1574,6 +1564,12 @@ createVariableInternal(Package *package, text *name, Oid typid,
 							&scalar->typbyval);
 			varState->value.scalar.is_null = true;
 		}
+
+		if (!isObjectChangedInCurrentTrans(&package->transObject))
+		{
+			createSavepoint(&package->transObject, TRANS_PACKAGE);
+			addToChangesStack(&package->transObject, TRANS_PACKAGE);
+		}
 	}
 
 	GetActualState(variable)->is_valid = true;
@@ -1675,7 +1671,7 @@ removeObject(TransObject *object, TransObjectType type)
 	 * Delete an object from the change history of the overlying
 	 * transaction level (head of 'changesStack' at this point).
 	 */
-	if (!dlist_is_empty(changesStack))
+	if (changesStack && !dlist_is_empty(changesStack))
 			removeFromChangesStack(object, type);
 	if (type == TRANS_PACKAGE)
 	{
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 9838fb5..b4d7ce3 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -183,6 +183,15 @@ SELECT pgv_select('vars3', 'r1', 1);
 SELECT pgv_select('vars3', 'r1') LIMIT 2;
 SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
 
+-- Clean memory after unsuccessful creation of a variable
+SELECT pgv_insert('vars4', 'r1', row('str1', 'str1'));
+SELECT package FROM pgv_stats() WHERE package = 'vars4';
+
+-- Remove package if it is empty
+SELECT pgv_insert('vars4', 'r2', row(1, 'str1', 'str2'));
+SELECT pgv_remove('vars4', 'r2');
+SELECT package FROM pgv_stats() WHERE package = 'vars4';
+
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
 SELECT package FROM pgv_stats() order by package;

From 4dd3fd56f07d63692c42b70150f69f08ca5c83d0 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Sat, 9 Feb 2019 17:38:01 +0300
Subject: [PATCH 07/37] Remove redundant argument from getVariableInternal()

---
 pg_variables.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index f970435..3e811b0 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -52,7 +52,7 @@ static void getKeyFromName(text *name, char *key);
 static Package *getPackageByName(text *name, bool create, bool strict);
 static Variable *getVariableInternal(Package *package,
 									 text *name, Oid typid,
-									 bool strict, bool type_strict);
+									 bool strict);
 static Variable *createVariableInternal(Package *package,
 										text *name, Oid typid,
 										bool is_transactional);
@@ -197,7 +197,7 @@ variable_get(text *package_name, text *var_name,
 		return 0;
 	}
 
-	variable = getVariableInternal(package, var_name, typid, strict, true);
+	variable = getVariableInternal(package, var_name, typid, strict);
 
 	if (variable == NULL)
 	{
@@ -455,7 +455,7 @@ variable_update(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true);
 		LastVariable = variable;
 	}
 	else
@@ -543,7 +543,7 @@ variable_delete(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true);
 		LastVariable = variable;
 	}
 	else
@@ -592,7 +592,7 @@ variable_select(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true);
 
 		record = &(GetActualValue(variable).record);
 
@@ -667,7 +667,7 @@ variable_select_by_value(PG_FUNCTION_ARGS)
 	}
 
 	package = getPackageByName(package_name, false, true);
-	variable = getVariableInternal(package, var_name, RECORDOID, true, true);
+	variable = getVariableInternal(package, var_name, RECORDOID, true);
 
 	if (!value_is_null)
 		check_record_key(variable, value_type);
@@ -736,7 +736,7 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true);
 
 		check_record_key(variable, ARR_ELEMTYPE(values));
 
@@ -870,9 +870,9 @@ remove_variable(PG_FUNCTION_ARGS)
 	var_name = PG_GETARG_TEXT_PP(1);
 
 	package = getPackageByName(package_name, false, true);
-	variable = getVariableInternal(package, var_name, 0, true, false);
+	variable = getVariableInternal(package, var_name, InvalidOid, true);
 
-	/* Add package to changes list, so we can remove it if it  */
+	/* Add package to changes list, so we can remove it if it is empty */
 	if (!isObjectChangedInCurrentTrans(&package->transObject))
 	{
 		createSavepoint(&package->transObject, TRANS_PACKAGE);
@@ -1438,8 +1438,7 @@ getPackageByName(text *name, bool create, bool strict)
  * flag 'is_transactional' of this variable is unknown.
  */
 static Variable *
-getVariableInternal(Package *package, text *name, Oid typid, bool strict,
-					bool type_strict)
+getVariableInternal(Package *package, text *name, Oid typid, bool strict)
 {
 	Variable   *variable;
 	char		key[NAMEDATALEN];
@@ -1456,7 +1455,7 @@ getVariableInternal(Package *package, text *name, Oid typid, bool strict,
 	/* Check variable type */
 	if (found)
 	{
-		if (type_strict && variable->typid != typid)
+		if (typid != InvalidOid && variable->typid != typid)
 		{
 			char	   *var_type = DatumGetCString(DirectFunctionCall1(regtypeout,
 																	   ObjectIdGetDatum(variable->typid)));

From 2f955d4962dc6cd96636ec6fb7e2782a96e96743 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Sat, 9 Feb 2019 18:16:44 +0300
Subject: [PATCH 08/37] Fix comment

---
 pg_variables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 3e811b0..deb085e 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -1696,7 +1696,7 @@ removeObject(TransObject *object, TransObjectType type)
 	/* Remove object from hash table */
 	hash_search(hash, object->name, HASH_REMOVE, &found);
 
-	/* Remove package if it is became empty */
+	/* Remove package if it became empty */
 	if (type == TRANS_VARIABLE &&
 		isObjectChangedInCurrentTrans(&package->transObject) &&
 		isPackageEmpty(package))
@@ -1826,7 +1826,7 @@ releaseSavepoint(TransObject *object, TransObjectType type)
 			!dlist_has_next(states, dlist_head_node(states)))
 		{
 			removeObject(object, type);
-			/* Remove package if it becomes empty */
+			/* Remove package if it became empty */
 			if (type == TRANS_VARIABLE)
 			{
 				Package *pack = ((Variable *) object)->package;

From 93162b5abcd868dd25b8d75a45c1b5d1fe1859d6 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 13 Feb 2019 16:25:03 +0300
Subject: [PATCH 09/37] PGPRO-2440: Use destination's context to add into the
 hash table

---
 pg_variables.c        | 26 +++++++-------------------
 pg_variables.h        |  2 ++
 pg_variables_record.c | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index deb085e..4833523 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -1582,36 +1582,24 @@ createVariableInternal(Package *package, text *name, Oid typid,
 static void
 copyValue(VarState *src, VarState *dest, Variable *destVar)
 {
-	MemoryContext oldcxt,
-				destctx;
+	MemoryContext oldcxt;
 
-	destctx = destVar->package->hctxTransact;
-	oldcxt = MemoryContextSwitchTo(destctx);
+	oldcxt = MemoryContextSwitchTo(destVar->package->hctxTransact);
 
 	if (destVar->typid == RECORDOID)
 		/* copy record value */
 	{
-		bool		found;
-		HASH_SEQ_STATUS *rstat;
-		HashRecordEntry *item_prev,
-				   *item_new;
+		HASH_SEQ_STATUS rstat;
+		HashRecordEntry *item_src;
 		RecordVar  *record_src = &src->value.record;
 		RecordVar  *record_dest = &dest->value.record;
 
 		init_record(record_dest, record_src->tupdesc, destVar);
 
 		/* Copy previous history entry into the new one */
-		rstat = (HASH_SEQ_STATUS *) palloc0(sizeof(HASH_SEQ_STATUS));
-		hash_seq_init(rstat, record_src->rhash);
-		while ((item_prev = (HashRecordEntry *) hash_seq_search(rstat)) != NULL)
-		{
-			HashRecordKey k;
-
-			k = item_prev->key;
-			item_new = (HashRecordEntry *) hash_search(record_dest->rhash, &k,
-													   HASH_ENTER, &found);
-			item_new->tuple = heap_copytuple(item_prev->tuple);
-		}
+		hash_seq_init(&rstat, record_src->rhash);
+		while ((item_src = (HashRecordEntry *) hash_seq_search(&rstat)) != NULL)
+			copy_record(record_dest, item_src->tuple, destVar);
 	}
 	else
 		/* copy scalar value */
diff --git a/pg_variables.h b/pg_variables.h
index a26eac6..96689c2 100755
--- a/pg_variables.h
+++ b/pg_variables.h
@@ -151,6 +151,8 @@ typedef struct ChangesStackNode
 extern void init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable);
 extern void check_attributes(Variable *variable, TupleDesc tupdesc);
 extern void check_record_key(Variable *variable, Oid typid);
+extern void copy_record(RecordVar *dest_record, HeapTuple src_tuple,
+						Variable *variable);
 
 extern void insert_record(Variable *variable, HeapTupleHeader tupleHeader);
 extern bool update_record(Variable *variable, HeapTupleHeader tupleHeader);
diff --git a/pg_variables_record.c b/pg_variables_record.c
index 2bb40b5..341dca2 100644
--- a/pg_variables_record.c
+++ b/pg_variables_record.c
@@ -153,6 +153,47 @@ init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable)
 	MemoryContextSwitchTo(oldcxt);
 }
 
+/*
+ * Copy record using src_tuple.
+ */
+void
+copy_record(RecordVar *dest_record, HeapTuple src_tuple, Variable *variable)
+{
+	HeapTuple	tuple;
+	Datum		value;
+	bool		isnull;
+	HashRecordKey k;
+	HashRecordEntry *item;
+	bool		found;
+	MemoryContext oldcxt;
+
+	oldcxt = MemoryContextSwitchTo(dest_record->hctx);
+
+	/* Inserting a new record into dest_record */
+	tuple = heap_copytuple(src_tuple);
+	value = fastgetattr(tuple, 1, dest_record->tupdesc, &isnull);
+
+	k.value = value;
+	k.is_null = isnull;
+	k.hash_proc = &dest_record->hash_proc;
+	k.cmp_proc = &dest_record->cmp_proc;
+
+	item = (HashRecordEntry *) hash_search(dest_record->rhash, &k,
+										   HASH_ENTER, &found);
+	if (found)
+	{
+		heap_freetuple(tuple);
+		MemoryContextSwitchTo(oldcxt);
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("there is a record in the variable \"%s\" with same "
+						"key", GetName(variable))));
+	}
+	item->tuple = tuple;
+
+	MemoryContextSwitchTo(oldcxt);
+}
+
 /*
  * New record structure should be the same as the first record.
  */

From c163b97cd04f3112e0c4342a2dba65eff723c8bf Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 13 Feb 2019 17:26:40 +0300
Subject: [PATCH 10/37] Improve tests

---
 expected/pg_variables.out       | 49 +++++++++++++++++++++++++++++++--
 expected/pg_variables_trans.out | 28 +++++++++++++++++++
 sql/pg_variables.sql            | 18 ++++++++++++
 sql/pg_variables_trans.sql      |  9 ++++++
 4 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 3ef1026..3e067ee 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -1,4 +1,19 @@
 CREATE EXTENSION pg_variables;
+-- Test packages - sanity checks
+SELECT pgv_free();
+ pgv_free 
+----------
+ 
+(1 row)
+
+SELECT pgv_exists(NULL); -- fail
+ERROR:  package name can not be NULL
+SELECT pgv_remove(NULL); -- fail
+ERROR:  package name can not be NULL
+SELECT pgv_remove('vars'); -- fail
+ERROR:  unrecognized package "vars"
+SELECT pgv_exists('vars111111111111111111111111111111111111111111111111111111111111'); -- fail
+ERROR:  name "vars111111111111111111111111111111111111111111111111111111111111" is too long
 -- Integer variables
 SELECT pgv_get_int('vars', 'int1');
 ERROR:  unrecognized package "vars"
@@ -553,6 +568,34 @@ SELECT pgv_insert('vars3', 'r1', row(1, 1));
 ERROR:  new record structure differs from variable "r1" structure
 SELECT pgv_insert('vars3', 'r1', row('str1', 'str1'));
 ERROR:  new record structure differs from variable "r1" structure
+SELECT pgv_select('vars3', 'r1', ARRAY[[1,2]]); -- fail
+ERROR:  searching for elements in multidimensional arrays is not supported
+-- Test variables caching
+SELECT pgv_insert('vars3', 'r2', row(1, 'str1', 'str2'));
+ pgv_insert 
+------------
+ 
+(1 row)
+
+SELECT pgv_update('vars3', 'r2', row(1, 'str2', 'str1'));
+ pgv_update 
+------------
+ t
+(1 row)
+
+-- Test NULL values
+SELECT pgv_insert('vars3', 'r2', NULL); -- fail
+ERROR:  record argument can not be NULL
+SELECT pgv_update('vars3', 'r2', NULL); -- fail
+ERROR:  record argument can not be NULL
+select pgv_delete('vars3', 'r2', NULL::int);
+ pgv_delete 
+------------
+ f
+(1 row)
+
+SELECT pgv_select('vars3', 'r1', NULL::int[]); -- fail
+ERROR:  array argument can not be NULL
 SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
@@ -724,7 +767,8 @@ SELECT * FROM pgv_list() order by package, name;
  vars2   | j1       | f
  vars2   | j2       | f
  vars3   | r1       | f
-(22 rows)
+ vars3   | r2       | f
+(23 rows)
 
 SELECT package FROM pgv_stats() order by package;
  package 
@@ -786,7 +830,8 @@ SELECT * FROM pgv_list() order by package, name;
  vars    | tstz2    | f
  vars    | tstzNULL | f
  vars3   | r1       | f
-(19 rows)
+ vars3   | r2       | f
+(20 rows)
 
 SELECT pgv_free();
  pgv_free 
diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index 397afac..c4c2495 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1880,6 +1880,34 @@ RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
 ERROR:  unrecognized package "vars"
 COMMIT;
+-- Test for PGPRO-2440
+SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
+ pgv_insert 
+------------
+ 
+(1 row)
+
+BEGIN;
+SELECT pgv_insert('vars3', 'r3', row(2 :: integer, NULL::varchar), true);
+ pgv_insert 
+------------
+ 
+(1 row)
+
+SAVEPOINT comm;
+SELECT pgv_insert('vars3', 'r3', row(3 :: integer, NULL::varchar), true);
+ pgv_insert 
+------------
+ 
+(1 row)
+
+COMMIT;
+SELECT pgv_delete('vars3', 'r3', 3);
+ pgv_delete 
+------------
+ t
+(1 row)
+
 SELECT pgv_free();
  pgv_free 
 ----------
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index b4d7ce3..a262a21 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -1,5 +1,12 @@
 CREATE EXTENSION pg_variables;
 
+-- Test packages - sanity checks
+SELECT pgv_free();
+SELECT pgv_exists(NULL); -- fail
+SELECT pgv_remove(NULL); -- fail
+SELECT pgv_remove('vars'); -- fail
+SELECT pgv_exists('vars111111111111111111111111111111111111111111111111111111111111'); -- fail
+
 -- Integer variables
 SELECT pgv_get_int('vars', 'int1');
 SELECT pgv_get_int('vars', 'int1', false);
@@ -152,6 +159,17 @@ SELECT pgv_insert('vars3', 'r1', tab) FROM tab;
 SELECT pgv_insert('vars3', 'r1', row(1, 'str1', 'str2'));
 SELECT pgv_insert('vars3', 'r1', row(1, 1));
 SELECT pgv_insert('vars3', 'r1', row('str1', 'str1'));
+SELECT pgv_select('vars3', 'r1', ARRAY[[1,2]]); -- fail
+
+-- Test variables caching
+SELECT pgv_insert('vars3', 'r2', row(1, 'str1', 'str2'));
+SELECT pgv_update('vars3', 'r2', row(1, 'str2', 'str1'));
+
+-- Test NULL values
+SELECT pgv_insert('vars3', 'r2', NULL); -- fail
+SELECT pgv_update('vars3', 'r2', NULL); -- fail
+select pgv_delete('vars3', 'r2', NULL::int);
+SELECT pgv_select('vars3', 'r1', NULL::int[]); -- fail
 
 SELECT pgv_select('vars3', 'r1');
 SELECT pgv_select('vars3', 'r1', 1);
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index fc0b876..a5af1b2 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -482,4 +482,13 @@ RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
 COMMIT;
 
+-- Test for PGPRO-2440
+SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
+BEGIN;
+SELECT pgv_insert('vars3', 'r3', row(2 :: integer, NULL::varchar), true);
+SAVEPOINT comm;
+SELECT pgv_insert('vars3', 'r3', row(3 :: integer, NULL::varchar), true);
+COMMIT;
+SELECT pgv_delete('vars3', 'r3', 3);
+
 SELECT pgv_free();

From 660968a6be46693fff510fd8e7b2c3c81d4c5ccd Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 13 Feb 2019 17:48:08 +0300
Subject: [PATCH 11/37] Improve tests

---
 expected/pg_variables.out | 18 ++++++++++++++++--
 pg_variables.c            | 10 +---------
 sql/pg_variables.sql      |  5 ++++-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 3e067ee..393bf06 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -577,10 +577,16 @@ SELECT pgv_insert('vars3', 'r2', row(1, 'str1', 'str2'));
  
 (1 row)
 
-SELECT pgv_update('vars3', 'r2', row(1, 'str2', 'str1'));
+SELECT pgv_update('vars3', 'r1', row(3, 'str22'::varchar));
  pgv_update 
 ------------
- t
+ f
+(1 row)
+
+select pgv_delete('vars3', 'r2', NULL::int);
+ pgv_delete 
+------------
+ f
 (1 row)
 
 -- Test NULL values
@@ -611,6 +617,8 @@ SELECT pgv_select('vars3', 'r1', 1);
  (1,str11)
 (1 row)
 
+SELECT pgv_select('vars3', 'r1', 1::float); -- fail
+ERROR:  requested value type differs from variable "r1" key type
 SELECT pgv_select('vars3', 'r1', 0);
  pgv_select 
 ------------
@@ -641,6 +649,12 @@ SELECT pgv_update('vars3', 'r1', tab) FROM tab;
  t
 (4 rows)
 
+SELECT pgv_update('vars3', 'r1', row(4, 'str44'::varchar));
+ pgv_update 
+------------
+ f
+(1 row)
+
 SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
diff --git a/pg_variables.c b/pg_variables.c
index 4833523..d28aa92 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -918,15 +918,7 @@ remove_package(PG_FUNCTION_ARGS)
 	package_name = PG_GETARG_TEXT_PP(0);
 
 	package = getPackageByName(package_name, false, true);
-	if (package)
-		removePackageInternal(package);
-	else
-	{
-		getKeyFromName(package_name, key);
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("unrecognized package \"%s\"", key)));
-	}
+	removePackageInternal(package);
 
 	resetVariablesCache(true);
 
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index a262a21..76d1f9e 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -163,7 +163,8 @@ SELECT pgv_select('vars3', 'r1', ARRAY[[1,2]]); -- fail
 
 -- Test variables caching
 SELECT pgv_insert('vars3', 'r2', row(1, 'str1', 'str2'));
-SELECT pgv_update('vars3', 'r2', row(1, 'str2', 'str1'));
+SELECT pgv_update('vars3', 'r1', row(3, 'str22'::varchar));
+select pgv_delete('vars3', 'r2', NULL::int);
 
 -- Test NULL values
 SELECT pgv_insert('vars3', 'r2', NULL); -- fail
@@ -173,12 +174,14 @@ SELECT pgv_select('vars3', 'r1', NULL::int[]); -- fail
 
 SELECT pgv_select('vars3', 'r1');
 SELECT pgv_select('vars3', 'r1', 1);
+SELECT pgv_select('vars3', 'r1', 1::float); -- fail
 SELECT pgv_select('vars3', 'r1', 0);
 SELECT pgv_select('vars3', 'r1', NULL::int);
 SELECT pgv_select('vars3', 'r1', ARRAY[1, 0, NULL]);
 
 UPDATE tab SET t = 'str33' WHERE id = 1;
 SELECT pgv_update('vars3', 'r1', tab) FROM tab;
+SELECT pgv_update('vars3', 'r1', row(4, 'str44'::varchar));
 SELECT pgv_select('vars3', 'r1');
 
 SELECT pgv_delete('vars3', 'r1', 1);

From ac709b4109bff6e4f29cb2fe363fe8c6558c4369 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 13 Feb 2019 18:14:57 +0300
Subject: [PATCH 12/37] Improve tests

---
 regression.diffs     | 24 ++++++++++++++++++++++++
 regression.out       |  3 +++
 sql/pg_variables.sql |  2 ++
 3 files changed, 29 insertions(+)
 create mode 100644 regression.diffs
 create mode 100644 regression.out

diff --git a/regression.diffs b/regression.diffs
new file mode 100644
index 0000000..d45b1a0
--- /dev/null
+++ b/regression.diffs
@@ -0,0 +1,24 @@
+*** /home/artur/source/pg/pg_variables/expected/pg_variables.out	2019-02-13 17:46:10.430008139 +0300
+--- /home/artur/source/pg/pg_variables/results/pg_variables.out	2019-02-13 18:14:44.927663951 +0300
+***************
+*** 583,594 ****
+--- 583,598 ----
+   f
+  (1 row)
+  
++ SELECT pgv_update('vars4', 'r1', row(3, 'str22'::varchar)); -- fail
++ ERROR:  unrecognized package "vars4"
+  select pgv_delete('vars3', 'r2', NULL::int);
+   pgv_delete 
+  ------------
+   f
+  (1 row)
+  
++ select pgv_delete('vars4', 'r2', NULL::int); -- fail
++ ERROR:  unrecognized package "vars4"
+  -- Test NULL values
+  SELECT pgv_insert('vars3', 'r2', NULL); -- fail
+  ERROR:  record argument can not be NULL
+
+======================================================================
+
diff --git a/regression.out b/regression.out
new file mode 100644
index 0000000..d1a58f3
--- /dev/null
+++ b/regression.out
@@ -0,0 +1,3 @@
+test pg_variables                 ... FAILED
+test pg_variables_any             ... ok
+test pg_variables_trans           ... ok
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 76d1f9e..64158cd 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -164,7 +164,9 @@ SELECT pgv_select('vars3', 'r1', ARRAY[[1,2]]); -- fail
 -- Test variables caching
 SELECT pgv_insert('vars3', 'r2', row(1, 'str1', 'str2'));
 SELECT pgv_update('vars3', 'r1', row(3, 'str22'::varchar));
+SELECT pgv_update('vars4', 'r1', row(3, 'str22'::varchar)); -- fail
 select pgv_delete('vars3', 'r2', NULL::int);
+select pgv_delete('vars4', 'r2', NULL::int); -- fail
 
 -- Test NULL values
 SELECT pgv_insert('vars3', 'r2', NULL); -- fail

From d00692b87677c6f3e2b47ab9c1a8c5529a8556d0 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Wed, 13 Feb 2019 18:22:04 +0300
Subject: [PATCH 13/37] Improve tests

---
 expected/pg_variables.out |  4 ++++
 regression.diffs          | 24 ------------------------
 regression.out            |  3 ---
 3 files changed, 4 insertions(+), 27 deletions(-)
 delete mode 100644 regression.diffs
 delete mode 100644 regression.out

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 393bf06..725810f 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -583,12 +583,16 @@ SELECT pgv_update('vars3', 'r1', row(3, 'str22'::varchar));
  f
 (1 row)
 
+SELECT pgv_update('vars4', 'r1', row(3, 'str22'::varchar)); -- fail
+ERROR:  unrecognized package "vars4"
 select pgv_delete('vars3', 'r2', NULL::int);
  pgv_delete 
 ------------
  f
 (1 row)
 
+select pgv_delete('vars4', 'r2', NULL::int); -- fail
+ERROR:  unrecognized package "vars4"
 -- Test NULL values
 SELECT pgv_insert('vars3', 'r2', NULL); -- fail
 ERROR:  record argument can not be NULL
diff --git a/regression.diffs b/regression.diffs
deleted file mode 100644
index d45b1a0..0000000
--- a/regression.diffs
+++ /dev/null
@@ -1,24 +0,0 @@
-*** /home/artur/source/pg/pg_variables/expected/pg_variables.out	2019-02-13 17:46:10.430008139 +0300
---- /home/artur/source/pg/pg_variables/results/pg_variables.out	2019-02-13 18:14:44.927663951 +0300
-***************
-*** 583,594 ****
---- 583,598 ----
-   f
-  (1 row)
-  
-+ SELECT pgv_update('vars4', 'r1', row(3, 'str22'::varchar)); -- fail
-+ ERROR:  unrecognized package "vars4"
-  select pgv_delete('vars3', 'r2', NULL::int);
-   pgv_delete 
-  ------------
-   f
-  (1 row)
-  
-+ select pgv_delete('vars4', 'r2', NULL::int); -- fail
-+ ERROR:  unrecognized package "vars4"
-  -- Test NULL values
-  SELECT pgv_insert('vars3', 'r2', NULL); -- fail
-  ERROR:  record argument can not be NULL
-
-======================================================================
-
diff --git a/regression.out b/regression.out
deleted file mode 100644
index d1a58f3..0000000
--- a/regression.out
+++ /dev/null
@@ -1,3 +0,0 @@
-test pg_variables                 ... FAILED
-test pg_variables_any             ... ok
-test pg_variables_trans           ... ok

From dfe6ac2f0f77898d1ef6595e9e5549cd3256afec Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Thu, 14 Feb 2019 13:26:04 +0300
Subject: [PATCH 14/37] Allow to store record type as a scalar variable

---
 expected/pg_variables.out | 30 +++++++++++--
 pg_variables.c            | 89 +++++++++++++++++++++++----------------
 pg_variables.h            |  5 +++
 sql/pg_variables.sql      | 10 +++++
 4 files changed, 95 insertions(+), 39 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 725810f..592c401 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -759,6 +759,27 @@ SELECT package FROM pgv_stats() WHERE package = 'vars4';
 ---------
 (0 rows)
 
+-- Record variables as scalar
+SELECT pgv_set('vars5', 'r1', row(1, 'str11'));
+ pgv_set 
+---------
+ 
+(1 row)
+
+SELECT pgv_get('vars5', 'r1', NULL::record);
+  pgv_get  
+-----------
+ (1,str11)
+(1 row)
+
+SELECT pgv_set('vars5', 'r1', row(1, 'str11'), true); -- fail
+ERROR:  variable "r1" already created as NOT TRANSACTIONAL
+SELECT pgv_insert('vars5', 'r1', row(1, 'str11')); -- fail
+ERROR:  "r1" isn't a record variable
+SELECT pgv_select('vars5', 'r1'); -- fail
+ERROR:  "r1" isn't a record variable
+SELECT pgv_get('vars3', 'r1', NULL::record); -- fail
+ERROR:  "r1" isn't a scalar variable
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
  package |   name   | is_transactional 
@@ -786,7 +807,8 @@ SELECT * FROM pgv_list() order by package, name;
  vars2   | j2       | f
  vars3   | r1       | f
  vars3   | r2       | f
-(23 rows)
+ vars5   | r1       | f
+(24 rows)
 
 SELECT package FROM pgv_stats() order by package;
  package 
@@ -794,7 +816,8 @@ SELECT package FROM pgv_stats() order by package;
  vars
  vars2
  vars3
-(3 rows)
+ vars5
+(4 rows)
 
 SELECT pgv_remove('vars', 'int3');
 ERROR:  unrecognized variable "int3"
@@ -849,7 +872,8 @@ SELECT * FROM pgv_list() order by package, name;
  vars    | tstzNULL | f
  vars3   | r1       | f
  vars3   | r2       | f
-(20 rows)
+ vars5   | r1       | f
+(21 rows)
 
 SELECT pgv_free();
  pgv_free 
diff --git a/pg_variables.c b/pg_variables.c
index d28aa92..d561817 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -50,12 +50,10 @@ static void ensurePackagesHashExists(void);
 static void getKeyFromName(text *name, char *key);
 
 static Package *getPackageByName(text *name, bool create, bool strict);
-static Variable *getVariableInternal(Package *package,
-									 text *name, Oid typid,
-									 bool strict);
-static Variable *createVariableInternal(Package *package,
-										text *name, Oid typid,
-										bool is_transactional);
+static Variable *getVariableInternal(Package *package, text *name,
+									 Oid typid, bool is_record, bool strict);
+static Variable *createVariableInternal(Package *package, text *name, Oid typid,
+										bool is_record, bool is_transactional);
 static void removePackageInternal(Package *package);
 static void resetVariablesCache(bool with_package);
 
@@ -65,7 +63,7 @@ static void releaseSavepoint(TransObject *object, TransObjectType type);
 static void rollbackSavepoint(TransObject *object, TransObjectType type);
 
 static void copyValue(VarState *src, VarState *dest, Variable *destVar);
-static void freeValue(VarState *varstate, Oid typid);
+static void freeValue(VarState *varstate, bool is_record);
 static void removeState(TransObject *object, TransObjectType type,
 						TransState *stateToDelete);
 static bool isObjectChangedInCurrentTrans(TransObject *object);
@@ -160,7 +158,7 @@ variable_set(text *package_name, text *var_name,
 	ScalarVar  *scalar;
 
 	package = getPackageByName(package_name, true, false);
-	variable = createVariableInternal(package, var_name, typid,
+	variable = createVariableInternal(package, var_name, typid, false,
 									  is_transactional);
 
 	scalar = &(GetActualValue(variable).scalar);
@@ -197,7 +195,7 @@ variable_get(text *package_name, text *var_name,
 		return 0;
 	}
 
-	variable = getVariableInternal(package, var_name, typid, strict);
+	variable = getVariableInternal(package, var_name, typid, false, strict);
 
 	if (variable == NULL)
 	{
@@ -343,7 +341,7 @@ variable_insert(PG_FUNCTION_ARGS)
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
 		variable = createVariableInternal(package, var_name, RECORDOID,
-										  is_transactional);
+										  true, is_transactional);
 		LastVariable = variable;
 	}
 	else
@@ -455,7 +453,8 @@ variable_update(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true,
+									   true);
 		LastVariable = variable;
 	}
 	else
@@ -543,7 +542,8 @@ variable_delete(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(var_name), GetName(LastVariable),
 				VARSIZE_ANY_EXHDR(var_name)) != 0)
 	{
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true,
+									   true);
 		LastVariable = variable;
 	}
 	else
@@ -592,7 +592,8 @@ variable_select(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true,
+									   true);
 
 		record = &(GetActualValue(variable).record);
 
@@ -667,7 +668,7 @@ variable_select_by_value(PG_FUNCTION_ARGS)
 	}
 
 	package = getPackageByName(package_name, false, true);
-	variable = getVariableInternal(package, var_name, RECORDOID, true);
+	variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 
 	if (!value_is_null)
 		check_record_key(variable, value_type);
@@ -736,7 +737,8 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 		var_name = PG_GETARG_TEXT_PP(1);
 
 		package = getPackageByName(package_name, false, true);
-		variable = getVariableInternal(package, var_name, RECORDOID, true);
+		variable = getVariableInternal(package, var_name, RECORDOID, true,
+									   true);
 
 		check_record_key(variable, ARR_ELEMTYPE(values));
 
@@ -870,7 +872,7 @@ remove_variable(PG_FUNCTION_ARGS)
 	var_name = PG_GETARG_TEXT_PP(1);
 
 	package = getPackageByName(package_name, false, true);
-	variable = getVariableInternal(package, var_name, InvalidOid, true);
+	variable = getVariableInternal(package, var_name, InvalidOid, false, true);
 
 	/* Add package to changes list, so we can remove it if it is empty */
 	if (!isObjectChangedInCurrentTrans(&package->transObject))
@@ -908,7 +910,6 @@ remove_package(PG_FUNCTION_ARGS)
 {
 	Package	   *package;
 	text	   *package_name;
-	char		key[NAMEDATALEN];
 
 	if (PG_ARGISNULL(0))
 		ereport(ERROR,
@@ -1430,7 +1431,8 @@ getPackageByName(text *name, bool create, bool strict)
  * flag 'is_transactional' of this variable is unknown.
  */
 static Variable *
-getVariableInternal(Package *package, text *name, Oid typid, bool strict)
+getVariableInternal(Package *package, text *name, Oid typid, bool is_record,
+					bool strict)
 {
 	Variable   *variable;
 	char		key[NAMEDATALEN];
@@ -1447,15 +1449,25 @@ getVariableInternal(Package *package, text *name, Oid typid, bool strict)
 	/* Check variable type */
 	if (found)
 	{
-		if (typid != InvalidOid && variable->typid != typid)
+		if (typid != InvalidOid)
 		{
-			char	   *var_type = DatumGetCString(DirectFunctionCall1(regtypeout,
-																	   ObjectIdGetDatum(variable->typid)));
+			if (variable->typid != typid)
+			{
+				char	   *var_type = DatumGetCString(
+										DirectFunctionCall1(regtypeout,
+											ObjectIdGetDatum(variable->typid)));
 
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("variable \"%s\" requires \"%s\" value",
-							key, var_type)));
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						errmsg("variable \"%s\" requires \"%s\" value",
+								key, var_type)));
+			}
+
+			if (variable->is_record != is_record)
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						errmsg("\"%s\" isn't a %s variable",
+								key, is_record ? "record" : "scalar")));
 		}
 		if (!GetActualState(variable)->is_valid && strict)
 			ereport(ERROR,
@@ -1475,11 +1487,11 @@ getVariableInternal(Package *package, text *name, Oid typid, bool strict)
 
 /*
  * Create a variable or return a pointer to existing one.
- * Function is useful to set new value to variable and
- * flag 'is_transactional' is known.
+ * Function is useful to set new value to variable and flag 'is_transactional'
+ * is known.
  */
 static Variable *
-createVariableInternal(Package *package, text *name, Oid typid,
+createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 					   bool is_transactional)
 {
 	Variable   *variable;
@@ -1521,6 +1533,12 @@ createVariableInternal(Package *package, text *name, Oid typid,
 							key, var_type)));
 		}
 
+		if (variable->is_record != is_record)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("\"%s\" isn't a %s variable",
+					 		key, is_record ? "record" : "scalar")));
+
 		/*
 		 * Savepoint must be created when variable changed in current
 		 * transaction. For each transaction level there should be a
@@ -1540,6 +1558,7 @@ createVariableInternal(Package *package, text *name, Oid typid,
 		/* Variable entry was created, so initialize new variable. */
 		variable->typid = typid;
 		variable->package = package;
+		variable->is_record = is_record;
 		variable->is_transactional = is_transactional;
 
 		dlist_init(GetStateStorage(variable));
@@ -1547,7 +1566,7 @@ createVariableInternal(Package *package, text *name, Oid typid,
 										  sizeof(VarState));
 
 		dlist_push_head(GetStateStorage(variable), &varState->state.node);
-		if (typid != RECORDOID)
+		if (!variable->is_record)
 		{
 			ScalarVar  *scalar = &(varState->value.scalar);
 
@@ -1578,7 +1597,7 @@ copyValue(VarState *src, VarState *dest, Variable *destVar)
 
 	oldcxt = MemoryContextSwitchTo(destVar->package->hctxTransact);
 
-	if (destVar->typid == RECORDOID)
+	if (destVar->is_record)
 		/* copy record value */
 	{
 		HASH_SEQ_STATUS rstat;
@@ -1610,19 +1629,17 @@ copyValue(VarState *src, VarState *dest, Variable *destVar)
 }
 
 static void
-freeValue(VarState *varstate, Oid typid)
+freeValue(VarState *varstate, bool is_record)
 {
-	if (typid == RECORDOID && varstate->value.record.hctx)
+	if (is_record && varstate->value.record.hctx)
 	{
 		/* All records will be freed */
 		MemoryContextDelete(varstate->value.record.hctx);
 	}
-	else if (varstate->value.scalar.typbyval == false &&
+	else if (!is_record && varstate->value.scalar.typbyval == false &&
 			 varstate->value.scalar.is_null == false &&
 			 varstate->value.scalar.value)
-	{
 		pfree(DatumGetPointer(varstate->value.scalar.value));
-	}
 }
 
 static void
@@ -1632,7 +1649,7 @@ removeState(TransObject *object, TransObjectType type, TransState *stateToDelete
 	{
 		Variable   *var = (Variable *) object;
 
-		freeValue((VarState *) stateToDelete, var->typid);
+		freeValue((VarState *) stateToDelete, var->is_record);
 	}
 	dlist_delete(&stateToDelete->node);
 	pfree(stateToDelete);
diff --git a/pg_variables.h b/pg_variables.h
index 96689c2..ce61df1 100755
--- a/pg_variables.h
+++ b/pg_variables.h
@@ -101,6 +101,11 @@ typedef struct Variable
 	TransObject transObject;
 	Package    *package;
 	Oid			typid;
+	/*
+	 * We need an additional flag to determine variable's type since we can
+	 * store record type DATUM within scalar variable
+	 */
+	bool		is_record;
 
 	/*
 	 * The flag determines the further behavior of the variable. Can be
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 64158cd..9e32468 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -215,6 +215,16 @@ SELECT pgv_insert('vars4', 'r2', row(1, 'str1', 'str2'));
 SELECT pgv_remove('vars4', 'r2');
 SELECT package FROM pgv_stats() WHERE package = 'vars4';
 
+-- Record variables as scalar
+SELECT pgv_set('vars5', 'r1', row(1, 'str11'));
+SELECT pgv_get('vars5', 'r1', NULL::record);
+SELECT pgv_set('vars5', 'r1', row(1, 'str11'), true); -- fail
+
+SELECT pgv_insert('vars5', 'r1', row(1, 'str11')); -- fail
+SELECT pgv_select('vars5', 'r1'); -- fail
+
+SELECT pgv_get('vars3', 'r1', NULL::record); -- fail
+
 -- Manipulate variables
 SELECT * FROM pgv_list() order by package, name;
 SELECT package FROM pgv_stats() order by package;

From 1a7be6a3273f9e0c91b29876ae28ce62b7631dca Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Thu, 21 Feb 2019 17:21:05 +0300
Subject: [PATCH 15/37] Don't create empty HTAB in package, if it is not
 necessary

---
 expected/pg_variables_trans.out |   3 +-
 pg_variables.c                  | 210 +++++++++++++++++---------------
 2 files changed, 113 insertions(+), 100 deletions(-)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index c4c2495..a709f6b 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1794,7 +1794,8 @@ SELECT package FROM pgv_stats() ORDER BY package;
  package 
 ---------
  vars
-(1 row)
+ vars2
+(2 rows)
 
 SELECT * FROM pgv_list() ORDER BY package, name;
  package | name | is_transactional 
diff --git a/pg_variables.c b/pg_variables.c
index d561817..f7e34e7 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -49,7 +49,8 @@ extern void _PG_fini(void);
 static void ensurePackagesHashExists(void);
 static void getKeyFromName(text *name, char *key);
 
-static Package *getPackageByName(text *name, bool create, bool strict);
+static Package *getPackage(text *name, bool strict);
+static Package *createPackage(text *name, bool is_trans);
 static Variable *getVariableInternal(Package *package, text *name,
 									 Oid typid, bool is_record, bool strict);
 static Variable *createVariableInternal(Package *package, text *name, Oid typid,
@@ -157,7 +158,7 @@ variable_set(text *package_name, text *var_name,
 	Variable   *variable;
 	ScalarVar  *scalar;
 
-	package = getPackageByName(package_name, true, false);
+	package = createPackage(package_name, is_transactional);
 	variable = createVariableInternal(package, var_name, typid, false,
 									  is_transactional);
 
@@ -188,7 +189,7 @@ variable_get(text *package_name, text *var_name,
 	Variable   *variable;
 	ScalarVar  *scalar;
 
-	package = getPackageByName(package_name, false, strict);
+	package = getPackage(package_name, strict);
 	if (package == NULL)
 	{
 		*is_null = true;
@@ -325,9 +326,10 @@ variable_insert(PG_FUNCTION_ARGS)
 	if (LastPackage == NULL ||
 		VARSIZE_ANY_EXHDR(package_name) != strlen(GetName(LastPackage)) ||
 		strncmp(VARDATA_ANY(package_name), GetName(LastPackage),
-				VARSIZE_ANY_EXHDR(package_name)) != 0)
+				VARSIZE_ANY_EXHDR(package_name)) != 0 ||
+		!pack_htab(LastPackage, is_transactional))
 	{
-		package = getPackageByName(package_name, true, false);
+		package = createPackage(package_name, is_transactional);
 		LastPackage = package;
 		LastVariable = NULL;
 	}
@@ -440,7 +442,7 @@ variable_update(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(package_name), GetName(LastPackage),
 				VARSIZE_ANY_EXHDR(package_name)) != 0)
 	{
-		package = getPackageByName(package_name, false, true);
+		package = getPackage(package_name, true);
 		LastPackage = package;
 		LastVariable = NULL;
 	}
@@ -529,7 +531,7 @@ variable_delete(PG_FUNCTION_ARGS)
 		strncmp(VARDATA_ANY(package_name), GetName(LastPackage),
 				VARSIZE_ANY_EXHDR(package_name)) != 0)
 	{
-		package = getPackageByName(package_name, false, true);
+		package = getPackage(package_name, true);
 		LastPackage = package;
 		LastVariable = NULL;
 	}
@@ -591,7 +593,7 @@ variable_select(PG_FUNCTION_ARGS)
 		package_name = PG_GETARG_TEXT_PP(0);
 		var_name = PG_GETARG_TEXT_PP(1);
 
-		package = getPackageByName(package_name, false, true);
+		package = getPackage(package_name, true);
 		variable = getVariableInternal(package, var_name, RECORDOID, true,
 									   true);
 
@@ -667,7 +669,7 @@ variable_select_by_value(PG_FUNCTION_ARGS)
 		value = 0;
 	}
 
-	package = getPackageByName(package_name, false, true);
+	package = getPackage(package_name, true);
 	variable = getVariableInternal(package, var_name, RECORDOID, true, true);
 
 	if (!value_is_null)
@@ -736,7 +738,7 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 		package_name = PG_GETARG_TEXT_PP(0);
 		var_name = PG_GETARG_TEXT_PP(1);
 
-		package = getPackageByName(package_name, false, true);
+		package = getPackage(package_name, true);
 		variable = getVariableInternal(package, var_name, RECORDOID, true,
 									   true);
 
@@ -802,14 +804,14 @@ variable_exists(PG_FUNCTION_ARGS)
 	Package	   *package;
 	Variable   *variable;
 	char		key[NAMEDATALEN];
-	bool		found;
+	bool		found = false;
 
 	CHECK_ARGS_FOR_NULL();
 
 	package_name = PG_GETARG_TEXT_PP(0);
 	var_name = PG_GETARG_TEXT_PP(1);
 
-	package = getPackageByName(package_name, false, false);
+	package = getPackage(package_name, false);
 	if (package == NULL)
 	{
 		PG_FREE_IF_COPY(package_name, 0);
@@ -820,9 +822,10 @@ variable_exists(PG_FUNCTION_ARGS)
 
 	getKeyFromName(var_name, key);
 
-	variable = (Variable *) hash_search(package->varHashRegular,
-										key, HASH_FIND, &found);
-	if (!found)
+	if (package->varHashRegular)
+		variable = (Variable *) hash_search(package->varHashRegular,
+											key, HASH_FIND, &found);
+	if (!found && package->varHashTransact)
 		variable = (Variable *) hash_search(package->varHashTransact,
 											key, HASH_FIND, &found);
 
@@ -848,7 +851,7 @@ package_exists(PG_FUNCTION_ARGS)
 
 	package_name = PG_GETARG_TEXT_PP(0);
 
-	res = getPackageByName(package_name, false, false) != NULL;
+	res = getPackage(package_name, false) != NULL;
 
 	PG_FREE_IF_COPY(package_name, 0);
 	PG_RETURN_BOOL(res);
@@ -871,7 +874,7 @@ remove_variable(PG_FUNCTION_ARGS)
 	package_name = PG_GETARG_TEXT_PP(0);
 	var_name = PG_GETARG_TEXT_PP(1);
 
-	package = getPackageByName(package_name, false, true);
+	package = getPackage(package_name, true);
 	variable = getVariableInternal(package, var_name, InvalidOid, false, true);
 
 	/* Add package to changes list, so we can remove it if it is empty */
@@ -918,7 +921,7 @@ remove_package(PG_FUNCTION_ARGS)
 
 	package_name = PG_GETARG_TEXT_PP(0);
 
-	package = getPackageByName(package_name, false, true);
+	package = getPackage(package_name, true);
 	removePackageInternal(package);
 
 	resetVariablesCache(true);
@@ -953,7 +956,10 @@ removePackageInternal(Package *package)
 static bool
 isPackageEmpty(Package *package)
 {
-	int var_num = hash_get_num_entries(package->varHashTransact);
+	int var_num = 0;
+	
+	if (package->varHashTransact)
+		var_num += hash_get_num_entries(package->varHashTransact);
 
 	if (package->varHashRegular)
 		var_num += hash_get_num_entries(package->varHashRegular);
@@ -1065,8 +1071,10 @@ get_packages_and_variables(PG_FUNCTION_ARGS)
 				/* Get variables list for package */
 				for (i = 0; i < 2; i++)
 				{
-					hash_seq_init(&vstat, i ? package->varHashTransact :
-								  package->varHashRegular);
+					HTAB *htab = pack_htab(package, i);
+					if (!htab)
+						continue;
+					hash_seq_init(&vstat, htab);
 					while ((variable =
 							(Variable *) hash_seq_search(&vstat)) != NULL)
 					{
@@ -1228,9 +1236,10 @@ get_packages_stats(PG_FUNCTION_ARGS)
 		/* Fill data */
 		values[0] = PointerGetDatum(cstring_to_text(GetName(package)));
 
-		if (GetActualState(package)->is_valid)
+		if (package->hctxRegular)
 			getMemoryTotalSpace(package->hctxRegular, 0, &regularSpace);
-		getMemoryTotalSpace(package->hctxTransact, 0, &transactSpace);
+		if (package->hctxTransact)
+			getMemoryTotalSpace(package->hctxTransact, 0, &transactSpace);
 
 		totalSpace = regularSpace + transactSpace;
 		values[1] = Int64GetDatum(totalSpace);
@@ -1300,6 +1309,7 @@ makePackHTAB(Package *package, bool is_trans)
 	HTAB		  **htab;
 	MemoryContext  *context;
 
+	// maybe we should use macro pack_hctx?
 	htab = is_trans ? &package->varHashTransact : &package->varHashRegular;
 	context = is_trans ? &package->hctxTransact : &package->hctxRegular;
 
@@ -1317,40 +1327,48 @@ makePackHTAB(Package *package, bool is_trans)
 }
 
 static Package *
-getPackageByName(text *name, bool create, bool strict)
+getPackage(text *name, bool strict)
 {
 	Package	   *package;
-	PackState  *packState;
 	char		key[NAMEDATALEN];
 	bool		found;
 
 	getKeyFromName(name, key);
 
-	if (create)
-		ensurePackagesHashExists();
-	else
+	/* Find a package entry */
+	if (packagesHash)
 	{
-		if (!packagesHash)
-		{
-			if (strict)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("unrecognized package \"%s\"", key)));
+		package = (Package *) hash_search(packagesHash, key, HASH_FIND, &found);
 
-			return NULL;
-		}
+		if (found && GetActualState(package)->is_valid)
+			return package;
 	}
+	/* Package not found or it's current state is "invalid" */
+	if (strict)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					errmsg("unrecognized package \"%s\"", key)));
+
+	return NULL;
+}
+
+static Package *
+createPackage(text *name, bool is_trans)
+{
+	Package	   *package;
+	char		key[NAMEDATALEN];
+	bool		found;
+
+	getKeyFromName(name, key);
+	ensurePackagesHashExists();
 
 	/* Find or create a package entry */
-	package = (Package *) hash_search(packagesHash, key,
-									  create ? HASH_ENTER : HASH_FIND,
-									  &found);
+	package = (Package *) hash_search(packagesHash, key, HASH_ENTER, &found);
 
 	if (found)
 	{
-		if (GetActualState(package)->is_valid)
-			return package;
-		else if (create)
+		if (!GetActualState(package)->is_valid)
+		/* Make package valid again */
 		{
 			HASH_SEQ_STATUS vstat;
 			Variable   *variable;
@@ -1358,71 +1376,57 @@ getPackageByName(text *name, bool create, bool strict)
 
 			/* Make new history entry of package */
 			if (!isObjectChangedInCurrentTrans(transObj))
-			{
 				createSavepoint(transObj, TRANS_PACKAGE);
-				addToChangesStack(transObj, TRANS_PACKAGE);
-			}
 
 			GetActualState(package)->is_valid = true;
 
-			/* XXX Check is this necessary */
-
-			/* Restore previously removed HTAB for regular variables */
-			makePackHTAB(package, false);
-
 			/* Mark all transactional variables in package as removed */
-			hash_seq_init(&vstat, package->varHashTransact);
-			while ((variable =
-					(Variable *) hash_seq_search(&vstat)) != NULL)
+			if (package->varHashTransact)
 			{
-				transObj = &variable->transObject;
-
-				if (!isObjectChangedInCurrentTrans(transObj))
+				hash_seq_init(&vstat, package->varHashTransact);
+				while ((variable =
+						(Variable *) hash_seq_search(&vstat)) != NULL)
 				{
-					createSavepoint(transObj, TRANS_VARIABLE);
-					addToChangesStack(transObj, TRANS_VARIABLE);
+					transObj = &variable->transObject;
+
+					if (!isObjectChangedInCurrentTrans(transObj))
+					{
+						createSavepoint(transObj, TRANS_VARIABLE);
+						addToChangesStack(transObj, TRANS_VARIABLE);
+					}
+					GetActualState(variable)->is_valid = false;
 				}
-				GetActualState(variable)->is_valid = false;
 			}
-
-			return package;
 		}
-		else if (strict)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unrecognized package \"%s\"", key)));
-		else
-			return NULL;
-	}
-	else if (!create)
-	{
-		if (strict)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unrecognized package \"%s\"", key)));
-		else
-			return package;
 	}
 	else
 	{
-		/*
-		 * Package entry was created, so we need create hash table for
-		 * variables.
-		 */
-		makePackHTAB(package, false);
-		makePackHTAB(package, true);
+		PackState  *packState;
 
+		//memset(package, 0, sizeof(Package));
+		package->varHashRegular = NULL;
+		package->varHashTransact = NULL;
+		package->hctxRegular = NULL;
+		package->hctxTransact = NULL;
 		/* Initialize history */
 		dlist_init(GetStateStorage(package));
 		packState = MemoryContextAllocZero(ModuleContext, sizeof(PackState));
 		dlist_push_head(GetStateStorage(package), &(packState->state.node));
 		packState->state.is_valid = true;
+	}
 
-		/* Add to changes list */
+	/* Create corresponding HTAB if not exists */
+	if (!pack_htab(package, is_trans))
+		makePackHTAB(package, is_trans);
+	/* Add to changes list */
+	//addToChangesStack(&package->transObject, TRANS_PACKAGE);
+	if (!isObjectChangedInCurrentTrans(&package->transObject))
+	{
+		createSavepoint(&package->transObject, TRANS_PACKAGE);
 		addToChangesStack(&package->transObject, TRANS_PACKAGE);
-
-		return package;
 	}
+
+	return package;
 }
 
 /*
@@ -1434,15 +1438,16 @@ static Variable *
 getVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 					bool strict)
 {
-	Variable   *variable;
+	Variable   *variable = NULL;
 	char		key[NAMEDATALEN];
-	bool		found;
+	bool		found = false;
 
 	getKeyFromName(name, key);
 
-	variable = (Variable *) hash_search(package->varHashRegular,
-										key, HASH_FIND, &found);
-	if (!found)
+	if (package->varHashRegular)
+		variable = (Variable *) hash_search(package->varHashRegular,
+											key, HASH_FIND, &found);
+	if (!found && package->varHashTransact)
 		variable = (Variable *) hash_search(package->varHashTransact,
 											key, HASH_FIND, &found);
 
@@ -1496,6 +1501,7 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 {
 	Variable   *variable;
 	TransObject *transObject;
+	HTAB	   *htab;
 	char		key[NAMEDATALEN];
 	bool		found;
 
@@ -1505,14 +1511,16 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 	 * Reverse check: for non-transactional variable search in regular table
 	 * and vice versa.
 	 */
-	hash_search(is_transactional ?
-				package->varHashRegular : package->varHashTransact,
-				key, HASH_FIND, &found);
-	if (found)
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("variable \"%s\" already created as %sTRANSACTIONAL",
-						key, is_transactional ? "NOT " : "")));
+	htab = pack_htab(package, !is_transactional);
+	if (htab)
+	{
+		hash_search(htab, key, HASH_FIND, &found);
+		if (found)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					errmsg("variable \"%s\" already created as %sTRANSACTIONAL",
+							key, is_transactional ? "NOT " : "")));
+	}
 
 	variable = (Variable *) hash_search(pack_htab(package, is_transactional),
 										key, HASH_ENTER, &found);
@@ -1674,7 +1682,11 @@ removeObject(TransObject *object, TransObjectType type)
 		package = (Package *) object;
 
 		/* Regular variables had already removed */
-		MemoryContextDelete(package->hctxTransact);
+		//Here we should think, when regular HTAB should be removed
+		if (package->hctxRegular)
+			MemoryContextDelete(package->hctxRegular);
+		if (package->hctxTransact)
+			MemoryContextDelete(package->hctxTransact);
 		hash = packagesHash;
 	}
 	else

From b53645f3c1777fedf2b6bc8600bc5567a924ba9d Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Mon, 25 Feb 2019 12:03:28 +0300
Subject: [PATCH 16/37] Optimize objects removal in subtransaction commit

---
 expected/pg_variables_trans.out |   2 +-
 pg_variables.c                  | 124 +++++++-------------------------
 2 files changed, 28 insertions(+), 98 deletions(-)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index a709f6b..a13e338 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1879,7 +1879,7 @@ SELECT pgv_remove('vars', 'any1');
 
 RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
-ERROR:  unrecognized package "vars"
+ERROR:  unrecognized variable "any1"
 COMMIT;
 -- Test for PGPRO-2440
 SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
diff --git a/pg_variables.c b/pg_variables.c
index f7e34e7..e309922 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -72,7 +72,6 @@ static bool isObjectChangedInUpperTrans(TransObject *object);
 
 static void addToChangesStack(TransObject *object, TransObjectType type);
 static void pushChangesStack(void);
-static void removeFromChangesStack(TransObject *object, TransObjectType type);
 
 /* Constructors */
 static void makePackHTAB(Package *package, bool is_trans);
@@ -1309,7 +1308,6 @@ makePackHTAB(Package *package, bool is_trans)
 	HTAB		  **htab;
 	MemoryContext  *context;
 
-	// maybe we should use macro pack_hctx?
 	htab = is_trans ? &package->varHashTransact : &package->varHashRegular;
 	context = is_trans ? &package->hctxTransact : &package->hctxRegular;
 
@@ -1403,7 +1401,6 @@ createPackage(text *name, bool is_trans)
 	{
 		PackState  *packState;
 
-		//memset(package, 0, sizeof(Package));
 		package->varHashRegular = NULL;
 		package->varHashTransact = NULL;
 		package->hctxRegular = NULL;
@@ -1419,7 +1416,6 @@ createPackage(text *name, bool is_trans)
 	if (!pack_htab(package, is_trans))
 		makePackHTAB(package, is_trans);
 	/* Add to changes list */
-	//addToChangesStack(&package->transObject, TRANS_PACKAGE);
 	if (!isObjectChangedInCurrentTrans(&package->transObject))
 	{
 		createSavepoint(&package->transObject, TRANS_PACKAGE);
@@ -1671,12 +1667,6 @@ removeObject(TransObject *object, TransObjectType type)
 	HTAB	   *hash;
 	Package	   *package = NULL;
 
-	/*
-	 * Delete an object from the change history of the overlying
-	 * transaction level (head of 'changesStack' at this point).
-	 */
-	if (changesStack && !dlist_is_empty(changesStack))
-			removeFromChangesStack(object, type);
 	if (type == TRANS_PACKAGE)
 	{
 		package = (Package *) object;
@@ -1789,17 +1779,38 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 static void
 releaseSavepoint(TransObject *object, TransObjectType type)
 {
-	dlist_head *states;
+	dlist_head *states = &object->states;
 	Assert(GetActualState(object)->level == GetCurrentTransactionNestLevel());
 
-	/* Mark object as changed in parent transaction... */
-	if (!dlist_is_empty(changesStack) /* ...if there is an upper level... */
-		/* ...and object is not yet in list of that level changes. */
-		&& !isObjectChangedInUpperTrans(object))
+	/*
+	 * If the object is not valid and does not exist at a higher level
+	 * (or if we complete the transaction) - remove object.
+	 */
+	if (!GetActualState(object)->is_valid &&
+			(!dlist_has_next(states, dlist_head_node(states)) ||
+			dlist_is_empty(changesStack))
+		)
+	{
+		removeObject(object, type);
+		return;
+	}
+
+ 	/* If object has been changed in upper level -
+	 * replace state of that level with the current one. */
+	if (isObjectChangedInUpperTrans(object))
+	{
+		TransState *stateToDelete;
+		dlist_node *nodeToDelete;
+		nodeToDelete = dlist_next_node(states, dlist_head_node(states));
+		stateToDelete = dlist_container(TransState, node, nodeToDelete);
+		removeState(object, type, stateToDelete);
+	}
+	/* If the object does not yet have a record in previous level changesStack,
+	 * create it. */
+	else if (!dlist_is_empty(changesStack))
 	{
 		ChangedObject *co_new;
 		ChangesStackNode *csn;
-
 		/*
 		 * Impossible to push in upper list existing node
 		 * because it was created in another context
@@ -1809,43 +1820,7 @@ releaseSavepoint(TransObject *object, TransObjectType type)
 		dlist_push_head(type == TRANS_PACKAGE ? csn->changedPacksList :
 												csn->changedVarsList,
 												&co_new->node);
-
 	}
-	else
-	{
-		states = &object->states;
-
-		/* If object existed in parent transaction... */
-		if (dlist_has_next(states, dlist_head_node(states)))
-		{
-			TransState *stateToDelete;
-			dlist_node *nodeToDelete;
-
-			/* ...remove its previous state */
-			nodeToDelete = dlist_next_node(states, dlist_head_node(states));
-			stateToDelete = dlist_container(TransState, node, nodeToDelete);
-			removeState(object, type, stateToDelete);
-		}
-
-		/*
-		 * Object has no more previous states and can be completely removed if
-		 * necessary
-		 */
-		if (!GetActualState(object)->is_valid &&
-			!dlist_has_next(states, dlist_head_node(states)))
-		{
-			removeObject(object, type);
-			/* Remove package if it became empty */
-			if (type == TRANS_VARIABLE)
-			{
-				Package *pack = ((Variable *) object)->package;
-				if (isPackageEmpty(pack))
-					(GetActualState(&pack->transObject))->is_valid = false;
-			}
-			return;
-		}
-	}
-
 	/* Change subxact level due to release */
 	GetActualState(object)->level--;
 }
@@ -1980,51 +1955,6 @@ addToChangesStack(TransObject *object, TransObjectType type)
 	}
 }
 
-/*
- * Remove from the changes list a deleted package
- */
-static void
-removeFromChangesStack(TransObject *object, TransObjectType type)
-{
-	dlist_mutable_iter var_miter,
-					   pack_miter;
-	dlist_head		  *changesList;
-	ChangesStackNode  *csn = get_actual_changes_list();
-
-	/*
-	 * If we remove package, we should remove corresponding variables
-	 * from changedVarsList first.
-	 */
-	if (type == TRANS_PACKAGE)
-	{
-		changesList = csn->changedVarsList;
-		dlist_foreach_modify(var_miter, changesList)
-		{
-			ChangedObject *co_cur = dlist_container(ChangedObject, node,
-													var_miter.cur);
-			Variable   *var = (Variable *) co_cur->object;
-
-			if (var->package == (Package *)object)
-				dlist_delete(&co_cur->node);
-		}
-	}
-	/* Now remove object itself from changes list */
-	changesList = (type == TRANS_PACKAGE ? csn->changedPacksList :
-										   csn->changedVarsList);
-	dlist_foreach_modify(pack_miter, changesList)
-	{
-		ChangedObject *co_cur = dlist_container(ChangedObject, node,
-												pack_miter.cur);
-		TransObject *obj = co_cur->object;
-
-		if (obj == object)
-		{
-			dlist_delete(&co_cur->node);
-			break;
-		}
-	}
-}
-
 /*
  * Possible actions on variables.
  * Savepoints are created in setters so we don't need a CREATE_SAVEPOINT action.

From 21df5c367aa1f9bd1e381b6967de00f38965412a Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Mon, 25 Feb 2019 15:03:29 +0300
Subject: [PATCH 17/37] Optimize objects removal in subtransaction rollback

---
 expected/pg_variables_trans.out | 28 +++++++++++++++++++++++++++-
 pg_variables.c                  | 16 +++++-----------
 sql/pg_variables_trans.sql      | 11 ++++++++++-
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index a13e338..b352c33 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1881,7 +1881,7 @@ RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
 ERROR:  unrecognized variable "any1"
 COMMIT;
--- Test for PGPRO-2440
+-- Tests for PGPRO-2440
 SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
  pgv_insert 
 ------------
@@ -1909,6 +1909,32 @@ SELECT pgv_delete('vars3', 'r3', 3);
  t
 (1 row)
 
+BEGIN;
+SELECT pgv_set('vars1', 't1', ''::text);
+ pgv_set 
+---------
+ 
+(1 row)
+
+SELECT pgv_set('vars2', 't2', ''::text, true);
+ pgv_set 
+---------
+ 
+(1 row)
+
+SAVEPOINT sp1;
+SAVEPOINT sp2;
+SELECT pgv_free();
+ pgv_free 
+----------
+ 
+(1 row)
+
+ERROR;
+ERROR:  syntax error at or near "ERROR"
+LINE 1: ERROR;
+        ^
+COMMIT;
 SELECT pgv_free();
  pgv_free 
 ----------
diff --git a/pg_variables.c b/pg_variables.c
index e309922..03dd7b4 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -1672,7 +1672,6 @@ removeObject(TransObject *object, TransObjectType type)
 		package = (Package *) object;
 
 		/* Regular variables had already removed */
-		//Here we should think, when regular HTAB should be removed
 		if (package->hctxRegular)
 			MemoryContextDelete(package->hctxRegular);
 		if (package->hctxTransact)
@@ -1696,10 +1695,11 @@ removeObject(TransObject *object, TransObjectType type)
 	hash_search(hash, object->name, HASH_REMOVE, &found);
 
 	/* Remove package if it became empty */
-	if (type == TRANS_VARIABLE &&
-		isObjectChangedInCurrentTrans(&package->transObject) &&
-		isPackageEmpty(package))
+	if (type == TRANS_VARIABLE && isPackageEmpty(package))
+	{
+		Assert(isObjectChangedInCurrentTrans(&package->transObject));
 		GetActualState(&package->transObject)->is_valid = false;
+	}
 
 	resetVariablesCache(true);
 }
@@ -1740,14 +1740,8 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 	state = GetActualState(object);
 	if (type == TRANS_PACKAGE)
 	{
-		if (!state->is_valid)
+		if (!state->is_valid && !isPackageEmpty((Package *)object))
 		{
-			if (isPackageEmpty((Package *)object))
-			{
-				removeObject(object, TRANS_PACKAGE);
-				return;
-			}
-
 			if (dlist_has_next(&object->states, &state->node))
 			{
 				dlist_pop_head_node(&object->states);
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index a5af1b2..ed4a69f 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -482,7 +482,7 @@ RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
 COMMIT;
 
--- Test for PGPRO-2440
+-- Tests for PGPRO-2440
 SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
 BEGIN;
 SELECT pgv_insert('vars3', 'r3', row(2 :: integer, NULL::varchar), true);
@@ -491,4 +491,13 @@ SELECT pgv_insert('vars3', 'r3', row(3 :: integer, NULL::varchar), true);
 COMMIT;
 SELECT pgv_delete('vars3', 'r3', 3);
 
+BEGIN;
+SELECT pgv_set('vars1', 't1', ''::text);
+SELECT pgv_set('vars2', 't2', ''::text, true);
+SAVEPOINT sp1;
+SAVEPOINT sp2;
+SELECT pgv_free();
+ERROR;
+COMMIT;
+
 SELECT pgv_free();

From ab951025db7a89f4b080862ffba71436db1e40f0 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Mon, 4 Mar 2019 11:03:20 +0300
Subject: [PATCH 18/37] Fix gcc warning in variable_exists()

---
 pg_variables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 03dd7b4..b89d213 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -801,7 +801,7 @@ variable_exists(PG_FUNCTION_ARGS)
 	text	   *package_name;
 	text	   *var_name;
 	Package	   *package;
-	Variable   *variable;
+	Variable   *variable = NULL;
 	char		key[NAMEDATALEN];
 	bool		found = false;
 
@@ -831,7 +831,7 @@ variable_exists(PG_FUNCTION_ARGS)
 	PG_FREE_IF_COPY(package_name, 0);
 	PG_FREE_IF_COPY(var_name, 1);
 
-	PG_RETURN_BOOL(found ? GetActualState(variable)->is_valid : found);
+	PG_RETURN_BOOL(variable ? GetActualState(variable)->is_valid : false);
 }
 
 /*

From a682cc3e161bf6ecaaa819e16fc4e054d0629294 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Tue, 26 Feb 2019 20:50:41 +0300
Subject: [PATCH 19/37] Count valid variables to display package does not
 exists if there is no valid variables

---
 expected/pg_variables_trans.out |   2 +-
 pg_variables.c                  | 134 ++++++++++++++++++++++----------
 pg_variables.h                  |   6 +-
 3 files changed, 100 insertions(+), 42 deletions(-)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index b352c33..41aeab0 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1879,7 +1879,7 @@ SELECT pgv_remove('vars', 'any1');
 
 RELEASE comm;
 SELECT pgv_get('vars', 'any1',NULL::text);
-ERROR:  unrecognized variable "any1"
+ERROR:  unrecognized package "vars"
 COMMIT;
 -- Tests for PGPRO-2440
 SELECT pgv_insert('vars3', 'r3', row(1 :: integer, NULL::varchar), true);
diff --git a/pg_variables.c b/pg_variables.c
index b89d213..3b93bdb 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -71,8 +71,14 @@ static bool isObjectChangedInCurrentTrans(TransObject *object);
 static bool isObjectChangedInUpperTrans(TransObject *object);
 
 static void addToChangesStack(TransObject *object, TransObjectType type);
+static void addToChangesStackUpperLevel(TransObject *object,
+										TransObjectType type);
 static void pushChangesStack(void);
 
+static int numOfRegVars(Package *package);
+/* Debug function */
+static int _numOfTransVars(Package *package);
+
 /* Constructors */
 static void makePackHTAB(Package *package, bool is_trans);
 static inline ChangedObject *makeChangedObject(TransObject *object,
@@ -892,10 +898,13 @@ remove_variable(PG_FUNCTION_ARGS)
 			addToChangesStack(transObject, TRANS_VARIABLE);
 		}
 		GetActualState(variable)->is_valid = false;
+		numOfTransVars(package)--;
 	}
 	else
 		removeObject(&variable->transObject, TRANS_VARIABLE);
 
+	Assert (numOfTransVars(package) == _numOfTransVars(package));
+
 	resetVariablesCache(false);
 
 	PG_FREE_IF_COPY(package_name, 0);
@@ -950,6 +959,7 @@ removePackageInternal(Package *package)
 		addToChangesStack(transObject, TRANS_PACKAGE);
 	}
 	GetActualState(package)->is_valid = false;
+	numOfTransVars(package) = 0;
 }
 
 static bool
@@ -1338,7 +1348,8 @@ getPackage(text *name, bool strict)
 	{
 		package = (Package *) hash_search(packagesHash, key, HASH_FIND, &found);
 
-		if (found && GetActualState(package)->is_valid)
+		if (found && GetActualState(package)->is_valid && 
+			numOfTransVars(package) + numOfRegVars(package))
 			return package;
 	}
 	/* Package not found or it's current state is "invalid" */
@@ -1410,17 +1421,15 @@ createPackage(text *name, bool is_trans)
 		packState = MemoryContextAllocZero(ModuleContext, sizeof(PackState));
 		dlist_push_head(GetStateStorage(package), &(packState->state.node));
 		packState->state.is_valid = true;
+		packState->trans_var_num = 0;
+		/* Add to changes list */
+		if (!isObjectChangedInCurrentTrans(&package->transObject))
+			addToChangesStack(&package->transObject, TRANS_PACKAGE);
 	}
 
 	/* Create corresponding HTAB if not exists */
 	if (!pack_htab(package, is_trans))
 		makePackHTAB(package, is_trans);
-	/* Add to changes list */
-	if (!isObjectChangedInCurrentTrans(&package->transObject))
-	{
-		createSavepoint(&package->transObject, TRANS_PACKAGE);
-		addToChangesStack(&package->transObject, TRANS_PACKAGE);
-	}
 
 	return package;
 }
@@ -1586,7 +1595,13 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 		}
 	}
 
+	if (is_transactional &&
+		(!found || !GetActualState(variable)->is_valid))
+		numOfTransVars(package)++;
 	GetActualState(variable)->is_valid = true;
+	
+	Assert (numOfTransVars(package) == _numOfTransVars(package));
+	
 	/* If it is necessary, put variable to changedVars */
 	if (is_transactional)
 		addToChangesStack(transObject, TRANS_VARIABLE);
@@ -1715,8 +1730,11 @@ createSavepoint(TransObject *object, TransObjectType type)
 
 	prevState = GetActualState(object);
 	if (type == TRANS_PACKAGE)
+	{
 		newState = (TransState *) MemoryContextAllocZero(ModuleContext,
 														 sizeof(PackState));
+		((PackState *)newState)->trans_var_num = ((PackState *)prevState)->trans_var_num;
+	}
 	else
 	{
 		Variable   *var = (Variable *) object;
@@ -1729,6 +1747,15 @@ createSavepoint(TransObject *object, TransObjectType type)
 	newState->is_valid = prevState->is_valid;
 }
 
+static int
+numOfRegVars(Package *package)
+{
+	if (package->varHashRegular)
+		return hash_get_num_entries(package->varHashRegular);
+	else
+		return 0;
+}
+
 /*
  * Rollback object to its previous state
  */
@@ -1738,32 +1765,27 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 	TransState *state;
 
 	state = GetActualState(object);
-	if (type == TRANS_PACKAGE)
+	removeState(object, type, state);
+
+	if (dlist_is_empty(&object->states))
 	{
-		if (!state->is_valid && !isPackageEmpty((Package *)object))
+		if (type == TRANS_PACKAGE && numOfRegVars((Package *)object))
 		{
-			if (dlist_has_next(&object->states, &state->node))
+			PackState		 *packState;
+
+			packState = MemoryContextAllocZero(ModuleContext, sizeof(PackState));
+			dlist_push_head(&object->states, &(packState->state.node));
+			packState->state.is_valid = true;
+			packState->state.level = GetCurrentTransactionNestLevel() - 1;
+			packState->trans_var_num = 0;
+
+			if (!dlist_is_empty(changesStack))
 			{
-				dlist_pop_head_node(&object->states);
-				pfree(state);
+				addToChangesStackUpperLevel(object, type);
 			}
-			else
-				state->is_valid = true;
-			/* Restore regular vars HTAB */
-			makePackHTAB((Package *) object, false);
 		}
 		else
-			/* Pass current state to parent level */
-			releaseSavepoint(object, TRANS_PACKAGE);
-	}
-	else
-	{
-		/* Remove current state */
-		removeState(object, TRANS_VARIABLE, state);
-
-		/* Remove variable if it was created in rolled back transaction */
-		if (dlist_is_empty(&object->states))
-			removeObject(object, TRANS_VARIABLE);
+			removeObject(object, type);
 	}
 }
 
@@ -1802,21 +1824,31 @@ releaseSavepoint(TransObject *object, TransObjectType type)
 	/* If the object does not yet have a record in previous level changesStack,
 	 * create it. */
 	else if (!dlist_is_empty(changesStack))
-	{
-		ChangedObject *co_new;
-		ChangesStackNode *csn;
-		/*
-		 * Impossible to push in upper list existing node
-		 * because it was created in another context
-		 */
-		csn = dlist_head_element(ChangesStackNode, node, changesStack);
-		co_new = makeChangedObject(object, csn->ctx);
-		dlist_push_head(type == TRANS_PACKAGE ? csn->changedPacksList :
-												csn->changedVarsList,
-												&co_new->node);
-	}
+		addToChangesStackUpperLevel(object, type);
+
 	/* Change subxact level due to release */
 	GetActualState(object)->level--;
+	if (type == TRANS_PACKAGE)
+	{
+		Package *package = (Package *)object;
+		Assert (numOfTransVars(package) == _numOfTransVars(package));
+	}
+}
+
+static void
+addToChangesStackUpperLevel(TransObject *object, TransObjectType type)
+{
+	ChangedObject *co_new;
+	ChangesStackNode *csn;
+	/*
+		* Impossible to push in upper list existing node
+		* because it was created in another context
+		*/
+	csn = dlist_head_element(ChangesStackNode, node, changesStack);
+	co_new = makeChangedObject(object, csn->ctx);
+	dlist_push_head(type == TRANS_PACKAGE ? csn->changedPacksList :
+											csn->changedVarsList,
+											&co_new->node);
 }
 
 /*
@@ -2136,3 +2168,25 @@ _PG_fini(void)
 	UnregisterSubXactCallback(pgvSubTransCallback, NULL);
 	ExecutorEnd_hook = prev_ExecutorEnd;
 }
+
+/* Get exact count of valid variables in package. For debug only. */
+static int
+_numOfTransVars(Package *package)
+{
+	HASH_SEQ_STATUS vstat;
+	Variable	   *variable;
+	unsigned long res = 0;
+
+	if (package->varHashTransact)
+	{
+		hash_seq_init(&vstat, package->varHashTransact);
+		while ((variable = (Variable *) hash_seq_search(&vstat)) != NULL)
+		{
+			if (GetActualState(variable)->is_valid &&
+				GetActualState(package)->is_valid)
+				res++;
+		}
+	}
+
+	return res;
+}
diff --git a/pg_variables.h b/pg_variables.h
index ce61df1..7c6129b 100755
--- a/pg_variables.h
+++ b/pg_variables.h
@@ -63,7 +63,8 @@ typedef struct TransState
 /* List node that stores one of the package's states */
 typedef struct PackState
 {
-	TransState	state;
+	TransState		state;
+	unsigned long	trans_var_num; /* Number of valid transactional variables */
 }			PackState;
 
 /* List node that stores one of the variable's states */
@@ -170,6 +171,9 @@ extern void removeObject(TransObject *object, TransObjectType type);
 #define GetActualValue(variable) \
 	(((VarState *) GetActualState(variable))->value)
 
+#define numOfTransVars(package) \
+	(((PackState *) GetActualState(package))->trans_var_num)
+
 #define GetName(object) \
 	(AssertVariableIsOfTypeMacro(object->transObject, TransObject), \
 	 object->transObject.name)

From a57056aa6f18d959cf26df6873587a26ff6a9848 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 27 Feb 2019 11:44:41 +0300
Subject: [PATCH 20/37] Refactoring of object's history initialization

---
 pg_variables.c | 71 ++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 3b93bdb..594aa32 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -83,6 +83,7 @@ static int _numOfTransVars(Package *package);
 static void makePackHTAB(Package *package, bool is_trans);
 static inline ChangedObject *makeChangedObject(TransObject *object,
 											   MemoryContext ctx);
+static void initObjectHistory(TransObject *object, TransObjectType type);
 
 /* Hook functions */
 static void variable_ExecutorEnd(QueryDesc *queryDesc);
@@ -1334,6 +1335,37 @@ makePackHTAB(Package *package, bool is_trans)
 						HASH_ELEM | HASH_CONTEXT);
 }
 
+static void
+initObjectHistory(TransObject *object, TransObjectType type)
+{
+	/* Initialize history */
+	TransState *state;
+	int			size;
+
+	size = (type == TRANS_PACKAGE ? sizeof(PackState) : sizeof(VarState));
+	dlist_init(&object->states);
+	state = MemoryContextAllocZero(ModuleContext, size);
+	dlist_push_head(&object->states, &(state->node));
+
+	/* Initialize state */
+	state->is_valid = true;
+	if (type == TRANS_PACKAGE)
+		((PackState *)state)->trans_var_num = 0;
+	else
+	{
+		Variable *variable = (Variable *) object;
+		if (!variable->is_record)
+		{
+			VarState * varState = (VarState *) state;
+			ScalarVar  *scalar = &(varState->value.scalar);
+
+			get_typlenbyval(variable->typid, &scalar->typlen, 
+							&scalar->typbyval);
+			varState->value.scalar.is_null = true;
+		}
+	}
+}
+
 static Package *
 getPackage(text *name, bool strict)
 {
@@ -1410,18 +1442,12 @@ createPackage(text *name, bool is_trans)
 	}
 	else
 	{
-		PackState  *packState;
-
+		/* Package entry was created, so initialize it. */
 		package->varHashRegular = NULL;
 		package->varHashTransact = NULL;
 		package->hctxRegular = NULL;
 		package->hctxTransact = NULL;
-		/* Initialize history */
-		dlist_init(GetStateStorage(package));
-		packState = MemoryContextAllocZero(ModuleContext, sizeof(PackState));
-		dlist_push_head(GetStateStorage(package), &(packState->state.node));
-		packState->state.is_valid = true;
-		packState->trans_var_num = 0;
+		initObjectHistory(&package->transObject, TRANS_PACKAGE);
 		/* Add to changes list */
 		if (!isObjectChangedInCurrentTrans(&package->transObject))
 			addToChangesStack(&package->transObject, TRANS_PACKAGE);
@@ -1566,27 +1592,12 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 	}
 	else
 	{
-		VarState   *varState;
-
 		/* Variable entry was created, so initialize new variable. */
 		variable->typid = typid;
 		variable->package = package;
 		variable->is_record = is_record;
 		variable->is_transactional = is_transactional;
-
-		dlist_init(GetStateStorage(variable));
-		varState = MemoryContextAllocZero(pack_hctx(package, is_transactional),
-										  sizeof(VarState));
-
-		dlist_push_head(GetStateStorage(variable), &varState->state.node);
-		if (!variable->is_record)
-		{
-			ScalarVar  *scalar = &(varState->value.scalar);
-
-			get_typlenbyval(variable->typid, &scalar->typlen,
-							&scalar->typbyval);
-			varState->value.scalar.is_null = true;
-		}
+		initObjectHistory(transObject, TRANS_VARIABLE);
 
 		if (!isObjectChangedInCurrentTrans(&package->transObject))
 		{
@@ -1771,18 +1782,10 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 	{
 		if (type == TRANS_PACKAGE && numOfRegVars((Package *)object))
 		{
-			PackState		 *packState;
-
-			packState = MemoryContextAllocZero(ModuleContext, sizeof(PackState));
-			dlist_push_head(&object->states, &(packState->state.node));
-			packState->state.is_valid = true;
-			packState->state.level = GetCurrentTransactionNestLevel() - 1;
-			packState->trans_var_num = 0;
-
+			initObjectHistory(object, type);
+			GetActualState(object)->level = GetCurrentTransactionNestLevel() - 1;
 			if (!dlist_is_empty(changesStack))
-			{
 				addToChangesStackUpperLevel(object, type);
-			}
 		}
 		else
 			removeObject(object, type);

From cc5ec13024905d37db9a2ad04834b61fa29f023a Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 27 Feb 2019 17:36:09 +0300
Subject: [PATCH 21/37] Fix createPackage

---
 pg_variables.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 594aa32..5bb6274 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -1408,19 +1408,17 @@ createPackage(text *name, bool is_trans)
 
 	if (found)
 	{
+		TransObject *transObj = &package->transObject;
+
+		if (!isObjectChangedInCurrentTrans(transObj))
+			createSavepoint(transObj, TRANS_PACKAGE);
+
 		if (!GetActualState(package)->is_valid)
-		/* Make package valid again */
 		{
 			HASH_SEQ_STATUS vstat;
 			Variable   *variable;
-			TransObject *transObj = &package->transObject;
-
-			/* Make new history entry of package */
-			if (!isObjectChangedInCurrentTrans(transObj))
-				createSavepoint(transObj, TRANS_PACKAGE);
 
 			GetActualState(package)->is_valid = true;
-
 			/* Mark all transactional variables in package as removed */
 			if (package->varHashTransact)
 			{
@@ -1448,14 +1446,14 @@ createPackage(text *name, bool is_trans)
 		package->hctxRegular = NULL;
 		package->hctxTransact = NULL;
 		initObjectHistory(&package->transObject, TRANS_PACKAGE);
-		/* Add to changes list */
-		if (!isObjectChangedInCurrentTrans(&package->transObject))
-			addToChangesStack(&package->transObject, TRANS_PACKAGE);
 	}
 
 	/* Create corresponding HTAB if not exists */
 	if (!pack_htab(package, is_trans))
 		makePackHTAB(package, is_trans);
+	/* Add to changes list */
+	if (!isObjectChangedInCurrentTrans(&package->transObject))
+		addToChangesStack(&package->transObject, TRANS_PACKAGE);
 
 	return package;
 }

From a289ae2b3ec47a5009052d291b1803675f82ffb0 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 27 Feb 2019 18:47:32 +0300
Subject: [PATCH 22/37] Replace package emptiness check to remove_variable()

---
 pg_variables.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 5bb6274..1c3df0c 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -900,6 +900,8 @@ remove_variable(PG_FUNCTION_ARGS)
 		}
 		GetActualState(variable)->is_valid = false;
 		numOfTransVars(package)--;
+		if ((numOfTransVars(package) + numOfRegVars(package)) == 0)
+			GetActualState(package)->is_valid = false;
 	}
 	else
 		removeObject(&variable->transObject, TRANS_VARIABLE);
@@ -1380,9 +1382,11 @@ getPackage(text *name, bool strict)
 	{
 		package = (Package *) hash_search(packagesHash, key, HASH_FIND, &found);
 
-		if (found && GetActualState(package)->is_valid && 
-			numOfTransVars(package) + numOfRegVars(package))
+		if (found && GetActualState(package)->is_valid)
+		{
+			Assert (numOfTransVars(package) + numOfRegVars(package) > 0);
 			return package;
+		}
 	}
 	/* Package not found or it's current state is "invalid" */
 	if (strict)
@@ -1778,7 +1782,7 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 
 	if (dlist_is_empty(&object->states))
 	{
-		if (type == TRANS_PACKAGE && numOfRegVars((Package *)object))
+		if (type == TRANS_PACKAGE && numOfRegVars((Package *)object) > 0)
 		{
 			initObjectHistory(object, type);
 			GetActualState(object)->level = GetCurrentTransactionNestLevel() - 1;

From 53e6b3f2c2769f1f2a1658895112c40c3adfc49d Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 27 Feb 2019 18:59:18 +0300
Subject: [PATCH 23/37] Rename macro numOfTransVars()

---
 pg_variables.c | 11 ++++++-----
 pg_variables.h |  8 ++------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 1c3df0c..d7cc333 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -899,8 +899,8 @@ remove_variable(PG_FUNCTION_ARGS)
 			addToChangesStack(transObject, TRANS_VARIABLE);
 		}
 		GetActualState(variable)->is_valid = false;
-		numOfTransVars(package)--;
-		if ((numOfTransVars(package) + numOfRegVars(package)) == 0)
+		GetPackState(package)->trans_var_num--;
+		if ((GetPackState(package)->trans_var_num + numOfRegVars(package)) == 0)
 			GetActualState(package)->is_valid = false;
 	}
 	else
@@ -962,7 +962,7 @@ removePackageInternal(Package *package)
 		addToChangesStack(transObject, TRANS_PACKAGE);
 	}
 	GetActualState(package)->is_valid = false;
-	numOfTransVars(package) = 0;
+	GetPackState(package)->trans_var_num = 0;
 }
 
 static bool
@@ -1384,7 +1384,8 @@ getPackage(text *name, bool strict)
 
 		if (found && GetActualState(package)->is_valid)
 		{
-			Assert (numOfTransVars(package) + numOfRegVars(package) > 0);
+			Assert (GetPackState(package)->trans_var_num +
+					numOfRegVars(package) > 0);
 			return package;
 		}
 	}
@@ -1610,7 +1611,7 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 
 	if (is_transactional &&
 		(!found || !GetActualState(variable)->is_valid))
-		numOfTransVars(package)++;
+		GetPackState(package)->trans_var_num++;
 	GetActualState(variable)->is_valid = true;
 	
 	Assert (numOfTransVars(package) == _numOfTransVars(package));
diff --git a/pg_variables.h b/pg_variables.h
index 7c6129b..fd54315 100755
--- a/pg_variables.h
+++ b/pg_variables.h
@@ -171,15 +171,11 @@ extern void removeObject(TransObject *object, TransObjectType type);
 #define GetActualValue(variable) \
 	(((VarState *) GetActualState(variable))->value)
 
-#define numOfTransVars(package) \
-	(((PackState *) GetActualState(package))->trans_var_num)
+#define GetPackState(package) \
+	(((PackState *) GetActualState(package)))
 
 #define GetName(object) \
 	(AssertVariableIsOfTypeMacro(object->transObject, TransObject), \
 	 object->transObject.name)
 
-#define GetStateStorage(object) \
-	(AssertVariableIsOfTypeMacro(object->transObject, TransObject), \
-	 &(object->transObject.states))
-
 #endif							/* __PG_VARIABLES_H__ */

From ead49524a0f1bcd1770f948cad3241051f510a0b Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 27 Feb 2019 18:59:46 +0300
Subject: [PATCH 24/37] Remove debug function

---
 pg_variables.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index d7cc333..47c22c2 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -76,8 +76,6 @@ static void addToChangesStackUpperLevel(TransObject *object,
 static void pushChangesStack(void);
 
 static int numOfRegVars(Package *package);
-/* Debug function */
-static int _numOfTransVars(Package *package);
 
 /* Constructors */
 static void makePackHTAB(Package *package, bool is_trans);
@@ -906,8 +904,6 @@ remove_variable(PG_FUNCTION_ARGS)
 	else
 		removeObject(&variable->transObject, TRANS_VARIABLE);
 
-	Assert (numOfTransVars(package) == _numOfTransVars(package));
-
 	resetVariablesCache(false);
 
 	PG_FREE_IF_COPY(package_name, 0);
@@ -1614,8 +1610,6 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 		GetPackState(package)->trans_var_num++;
 	GetActualState(variable)->is_valid = true;
 	
-	Assert (numOfTransVars(package) == _numOfTransVars(package));
-	
 	/* If it is necessary, put variable to changedVars */
 	if (is_transactional)
 		addToChangesStack(transObject, TRANS_VARIABLE);
@@ -1834,11 +1828,6 @@ releaseSavepoint(TransObject *object, TransObjectType type)
 
 	/* Change subxact level due to release */
 	GetActualState(object)->level--;
-	if (type == TRANS_PACKAGE)
-	{
-		Package *package = (Package *)object;
-		Assert (numOfTransVars(package) == _numOfTransVars(package));
-	}
 }
 
 static void
@@ -2174,25 +2163,3 @@ _PG_fini(void)
 	UnregisterSubXactCallback(pgvSubTransCallback, NULL);
 	ExecutorEnd_hook = prev_ExecutorEnd;
 }
-
-/* Get exact count of valid variables in package. For debug only. */
-static int
-_numOfTransVars(Package *package)
-{
-	HASH_SEQ_STATUS vstat;
-	Variable	   *variable;
-	unsigned long res = 0;
-
-	if (package->varHashTransact)
-	{
-		hash_seq_init(&vstat, package->varHashTransact);
-		while ((variable = (Variable *) hash_seq_search(&vstat)) != NULL)
-		{
-			if (GetActualState(variable)->is_valid &&
-				GetActualState(package)->is_valid)
-				res++;
-		}
-	}
-
-	return res;
-}

From 56076bea1da2fa93c3bfdb76289010aaa0f1dcf5 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Tue, 5 Mar 2019 14:52:03 +0300
Subject: [PATCH 25/37] Add test for proper package state level assignment

---
 expected/pg_variables_trans.out | 27 +++++++++++++++++++++++++++
 sql/pg_variables_trans.sql      |  9 +++++++++
 2 files changed, 36 insertions(+)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index 41aeab0..0d0fd14 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1935,6 +1935,33 @@ ERROR:  syntax error at or near "ERROR"
 LINE 1: ERROR;
         ^
 COMMIT;
+BEGIN;
+SELECT pgv_set('vars', 'any1', 'some value'::text, true);
+ pgv_set 
+---------
+ 
+(1 row)
+
+SELECT pgv_free();
+ pgv_free 
+----------
+ 
+(1 row)
+
+SAVEPOINT sp_to_rollback;
+SELECT pgv_set('vars', 'any1', 'some value'::text, true);
+ pgv_set 
+---------
+ 
+(1 row)
+
+ROLLBACK TO sp_to_rollback;
+COMMIT;
+SELECT package FROM pgv_stats() ORDER BY package;
+ package 
+---------
+(0 rows)
+
 SELECT pgv_free();
  pgv_free 
 ----------
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index ed4a69f..aa3ebcc 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -500,4 +500,13 @@ SELECT pgv_free();
 ERROR;
 COMMIT;
 
+BEGIN;
+SELECT pgv_set('vars', 'any1', 'some value'::text, true);
+SELECT pgv_free();
+SAVEPOINT sp_to_rollback;
+SELECT pgv_set('vars', 'any1', 'some value'::text, true);
+ROLLBACK TO sp_to_rollback;
+COMMIT;
+SELECT package FROM pgv_stats() ORDER BY package;
+
 SELECT pgv_free();

From 89ac2eecfd093ad559ebda07d4dd2e1852d8e53e Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Tue, 5 Mar 2019 15:20:48 +0300
Subject: [PATCH 26/37] Add missing comment

---
 pg_variables.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pg_variables.c b/pg_variables.c
index 47c22c2..dcf3606 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -1605,6 +1605,10 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 		}
 	}
 
+	/*
+	 * If the variable has been created or has just become valid,
+	 * increment the counter of valid transactional variables.
+	 */
 	if (is_transactional &&
 		(!found || !GetActualState(variable)->is_valid))
 		GetPackState(package)->trans_var_num++;

From 745ea14f6fb17220438594f26a8d6d5525d41805 Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Wed, 6 Mar 2019 18:29:20 +0300
Subject: [PATCH 27/37] Fix package removal in rollback

---
 expected/pg_variables_trans.out | 43 +++++++++++++++++++++
 pg_variables.c                  | 68 ++++++++++++++++++++++++---------
 sql/pg_variables_trans.sql      | 16 ++++++++
 3 files changed, 109 insertions(+), 18 deletions(-)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index 0d0fd14..83d7893 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -1962,6 +1962,49 @@ SELECT package FROM pgv_stats() ORDER BY package;
 ---------
 (0 rows)
 
+-- Package should exist after rollback if it contains regular variable
+BEGIN;
+SELECT pgv_set('vars', 'any1', 'some value'::text);
+ pgv_set 
+---------
+ 
+(1 row)
+
+ROLLBACK;
+SELECT package FROM pgv_stats() ORDER BY package;
+ package 
+---------
+ vars
+(1 row)
+
+-- Package should not exist if it becomes empty in rolled back transaction
+BEGIN;
+SAVEPOINT comm2;
+SELECT pgv_remove('vars');
+ pgv_remove 
+------------
+ 
+(1 row)
+
+ROLLBACK TO comm2;
+SELECT pgv_exists('vars');
+ pgv_exists 
+------------
+ f
+(1 row)
+
+SELECT package FROM pgv_stats() ORDER BY package;
+ package 
+---------
+ vars
+(1 row)
+
+COMMIT;
+SELECT package FROM pgv_stats() ORDER BY package;
+ package 
+---------
+(0 rows)
+
 SELECT pgv_free();
  pgv_free 
 ----------
diff --git a/pg_variables.c b/pg_variables.c
index dcf3606..db01921 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -961,13 +961,11 @@ removePackageInternal(Package *package)
 	GetPackState(package)->trans_var_num = 0;
 }
 
+/* Check if package has any valid variables */
 static bool
 isPackageEmpty(Package *package)
 {
-	int var_num = 0;
-	
-	if (package->varHashTransact)
-		var_num += hash_get_num_entries(package->varHashTransact);
+	int var_num = GetPackState(package)->trans_var_num;
 
 	if (package->varHashRegular)
 		var_num += hash_get_num_entries(package->varHashRegular);
@@ -1357,7 +1355,7 @@ initObjectHistory(TransObject *object, TransObjectType type)
 			VarState * varState = (VarState *) state;
 			ScalarVar  *scalar = &(varState->value.scalar);
 
-			get_typlenbyval(variable->typid, &scalar->typlen, 
+			get_typlenbyval(variable->typid, &scalar->typlen,
 							&scalar->typbyval);
 			varState->value.scalar.is_null = true;
 		}
@@ -1613,7 +1611,7 @@ createVariableInternal(Package *package, text *name, Oid typid, bool is_record,
 		(!found || !GetActualState(variable)->is_valid))
 		GetPackState(package)->trans_var_num++;
 	GetActualState(variable)->is_valid = true;
-	
+
 	/* If it is necessary, put variable to changedVars */
 	if (is_transactional)
 		addToChangesStack(transObject, TRANS_VARIABLE);
@@ -1779,16 +1777,49 @@ rollbackSavepoint(TransObject *object, TransObjectType type)
 	state = GetActualState(object);
 	removeState(object, type, state);
 
-	if (dlist_is_empty(&object->states))
+	if (type == TRANS_PACKAGE)
 	{
-		if (type == TRANS_PACKAGE && numOfRegVars((Package *)object) > 0)
+		/* If there is no more states... */
+		if (dlist_is_empty(&object->states))
+		{
+			/* ...but object is a package and has some regular variables... */
+			if (numOfRegVars((Package *)object) > 0)
+			{
+				/* ...create a new state to make package valid. */
+				initObjectHistory(object, type);
+				GetActualState(object)->level = GetCurrentTransactionNestLevel() - 1;
+				if (!dlist_is_empty(changesStack))
+					addToChangesStackUpperLevel(object, type);
+			}
+			else
+				/* ...or remove an object if it is no longer needed. */
+				removeObject(object, type);
+		}
+		/*
+		* But if a package has more states, but hasn't valid variables,
+		* mark it as not valid or remove at top level transaction.
+		*/
+		else if (isPackageEmpty((Package *)object))
 		{
-			initObjectHistory(object, type);
-			GetActualState(object)->level = GetCurrentTransactionNestLevel() - 1;
-			if (!dlist_is_empty(changesStack))
+			if (dlist_is_empty(changesStack))
+			{
+				removeObject(object, type);
+				return;
+			}
+			else if (!isObjectChangedInUpperTrans(object) &&
+					 !dlist_is_empty(changesStack))
+			{
+				createSavepoint(object, type);
 				addToChangesStackUpperLevel(object, type);
+				GetActualState(object)->level = GetCurrentTransactionNestLevel() - 1;
+			}
+			GetActualState(object)->is_valid = false;
 		}
-		else
+	}
+	else
+	{
+		if (dlist_is_empty(&object->states))
+			/* Remove a variable if it is no longer needed. */
 			removeObject(object, type);
 	}
 }
@@ -1840,9 +1871,9 @@ addToChangesStackUpperLevel(TransObject *object, TransObjectType type)
 	ChangedObject *co_new;
 	ChangesStackNode *csn;
 	/*
-		* Impossible to push in upper list existing node
-		* because it was created in another context
-		*/
+	 * Impossible to push in upper list existing node
+	 * because it was created in another context
+	 */
 	csn = dlist_head_element(ChangesStackNode, node, changesStack);
 	co_new = makeChangedObject(object, csn->ctx);
 	dlist_push_head(type == TRANS_PACKAGE ? csn->changedPacksList :
@@ -1875,13 +1906,14 @@ isObjectChangedInUpperTrans(TransObject *object)
 			   *prev_state;
 
 	cur_state = GetActualState(object);
-	if (dlist_has_next(&object->states, &cur_state->node))
+	if (dlist_has_next(&object->states, &cur_state->node) &&
+		cur_state->level == GetCurrentTransactionNestLevel())
 	{
 		prev_state = dlist_container(TransState, node, cur_state->node.next);
 		return prev_state->level == GetCurrentTransactionNestLevel() - 1;
 	}
-
-	return false;
+	else
+		return cur_state->level == GetCurrentTransactionNestLevel() - 1;
 }
 
 /*
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index aa3ebcc..41ea22b 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -509,4 +509,20 @@ ROLLBACK TO sp_to_rollback;
 COMMIT;
 SELECT package FROM pgv_stats() ORDER BY package;
 
+-- Package should exist after rollback if it contains regular variable
+BEGIN;
+SELECT pgv_set('vars', 'any1', 'some value'::text);
+ROLLBACK;
+SELECT package FROM pgv_stats() ORDER BY package;
+
+-- Package should not exist if it becomes empty in rolled back transaction
+BEGIN;
+SAVEPOINT comm2;
+SELECT pgv_remove('vars');
+ROLLBACK TO comm2;
+SELECT pgv_exists('vars');
+SELECT package FROM pgv_stats() ORDER BY package;
+COMMIT;
+SELECT package FROM pgv_stats() ORDER BY package;
+
 SELECT pgv_free();

From a6948c3bb18369bbe06ead585d43775a438a609e Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Thu, 7 Mar 2019 16:08:14 +0300
Subject: [PATCH 28/37] Add test

---
 expected/pg_variables_trans.out | 19 +++++++++++++++++++
 sql/pg_variables_trans.sql      |  6 ++++++
 2 files changed, 25 insertions(+)

diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index 83d7893..b3576fd 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -2005,6 +2005,25 @@ SELECT package FROM pgv_stats() ORDER BY package;
 ---------
 (0 rows)
 
+SELECT pgv_set('vars', 'any1', 'some value'::text);
+ pgv_set 
+---------
+ 
+(1 row)
+
+BEGIN;
+SELECT pgv_remove('vars');
+ pgv_remove 
+------------
+ 
+(1 row)
+
+ROLLBACK;
+SELECT package FROM pgv_stats() ORDER BY package;
+ package 
+---------
+(0 rows)
+
 SELECT pgv_free();
  pgv_free 
 ----------
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index 41ea22b..a3b6319 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -525,4 +525,10 @@ SELECT package FROM pgv_stats() ORDER BY package;
 COMMIT;
 SELECT package FROM pgv_stats() ORDER BY package;
 
+SELECT pgv_set('vars', 'any1', 'some value'::text);
+BEGIN;
+SELECT pgv_remove('vars');
+ROLLBACK;
+SELECT package FROM pgv_stats() ORDER BY package;
+
 SELECT pgv_free();

From 931961185d2f79490e8217ee06ebe6c67882e74a Mon Sep 17 00:00:00 2001
From: Sergey Cherkashin <4erkashin@list.ru>
Date: Mon, 1 Apr 2019 17:53:14 +0300
Subject: [PATCH 29/37] Clear LastHSeqStatus when subtransaction ends

---
 pg_variables.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index db01921..1d8ce8f 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -2124,6 +2124,7 @@ pgvSubTransCallback(SubXactEvent event, SubTransactionId mySubid,
 				break;
 		}
 	}
+	LastHSeqStatus = NULL;
 }
 
 /*
@@ -2152,10 +2153,7 @@ pgvTransCallback(XactEvent event, void *arg)
 				break;
 		}
 	}
-
-	if (event == XACT_EVENT_PARALLEL_ABORT || event == XACT_EVENT_ABORT)
-		if (LastHSeqStatus)
-			LastHSeqStatus = NULL;
+	LastHSeqStatus = NULL;
 }
 
 /*

From 6fa76381631bb69b3eea4306cdfc2e7e7f310793 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
Date: Tue, 2 Apr 2019 18:12:46 +0300
Subject: [PATCH 30/37] PGPRO-2601: Copy flattened HeapTuple

---
 pg_variables.c        |  28 +++---
 pg_variables.h        |   6 +-
 pg_variables_record.c | 200 ++++++++++++++++++++++++++----------------
 3 files changed, 142 insertions(+), 92 deletions(-)

diff --git a/pg_variables.c b/pg_variables.c
index 1d8ce8f..be0c7bc 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -606,7 +606,7 @@ variable_select(PG_FUNCTION_ARGS)
 		funcctx = SRF_FIRSTCALL_INIT();
 		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
 
-		funcctx->tuple_desc = CreateTupleDescCopy(record->tupdesc);
+		funcctx->tuple_desc = record->tupdesc;
 
 		rstat = (HASH_SEQ_STATUS *) palloc0(sizeof(HASH_SEQ_STATUS));
 		hash_seq_init(rstat, record->rhash);
@@ -626,11 +626,10 @@ variable_select(PG_FUNCTION_ARGS)
 	item = (HashRecordEntry *) hash_seq_search(rstat);
 	if (item != NULL)
 	{
-		Datum		result;
-
-		result = HeapTupleGetDatum(item->tuple);
+		Assert(!HeapTupleHeaderHasExternal(
+							(HeapTupleHeader) DatumGetPointer(item->tuple)));
 
-		SRF_RETURN_NEXT(funcctx, result);
+		SRF_RETURN_NEXT(funcctx, item->tuple);
 	}
 	else
 	{
@@ -694,7 +693,12 @@ variable_select_by_value(PG_FUNCTION_ARGS)
 	PG_FREE_IF_COPY(var_name, 1);
 
 	if (found)
-		PG_RETURN_DATUM(HeapTupleGetDatum(item->tuple));
+	{
+		Assert(!HeapTupleHeaderHasExternal(
+							(HeapTupleHeader) DatumGetPointer(item->tuple)));
+
+		PG_RETURN_DATUM(item->tuple);
+	}
 	else
 		PG_RETURN_NULL();
 }
@@ -751,7 +755,7 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 		funcctx = SRF_FIRSTCALL_INIT();
 		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
 
-		funcctx->tuple_desc = CreateTupleDescCopy(GetActualValue(variable).record.tupdesc);
+		funcctx->tuple_desc = GetActualValue(variable).record.tupdesc;
 
 		var = (VariableIteratorRec *) palloc(sizeof(VariableIteratorRec));
 		var->iterator = array_create_iterator(values, 0, NULL);
@@ -784,11 +788,9 @@ variable_select_by_values(PG_FUNCTION_ARGS)
 											   HASH_FIND, &found);
 		if (found)
 		{
-			Datum		result;
-
-			result = HeapTupleGetDatum(item->tuple);
-
-			SRF_RETURN_NEXT(funcctx, result);
+			Assert(!HeapTupleHeaderHasExternal(
+							(HeapTupleHeader) DatumGetPointer(item->tuple)));
+			SRF_RETURN_NEXT(funcctx, item->tuple);
 		}
 	}
 
@@ -1639,7 +1641,7 @@ copyValue(VarState *src, VarState *dest, Variable *destVar)
 		/* Copy previous history entry into the new one */
 		hash_seq_init(&rstat, record_src->rhash);
 		while ((item_src = (HashRecordEntry *) hash_seq_search(&rstat)) != NULL)
-			copy_record(record_dest, item_src->tuple, destVar);
+			insert_record_copy(record_dest, item_src->tuple, destVar);
 	}
 	else
 		/* copy scalar value */
diff --git a/pg_variables.h b/pg_variables.h
index fd54315..3e233bd 100755
--- a/pg_variables.h
+++ b/pg_variables.h
@@ -128,7 +128,7 @@ typedef struct HashRecordKey
 typedef struct HashRecordEntry
 {
 	HashRecordKey key;
-	HeapTuple	tuple;
+	Datum		tuple;
 }			HashRecordEntry;
 
 /* Element of list with objects created, changed or removed within transaction */
@@ -157,12 +157,12 @@ typedef struct ChangesStackNode
 extern void init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable);
 extern void check_attributes(Variable *variable, TupleDesc tupdesc);
 extern void check_record_key(Variable *variable, Oid typid);
-extern void copy_record(RecordVar *dest_record, HeapTuple src_tuple,
-						Variable *variable);
 
 extern void insert_record(Variable *variable, HeapTupleHeader tupleHeader);
 extern bool update_record(Variable *variable, HeapTupleHeader tupleHeader);
 extern bool delete_record(Variable *variable, Datum value, bool is_null);
+extern void insert_record_copy(RecordVar *dest_record, Datum src_tuple,
+							   Variable *variable);
 extern void removeObject(TransObject *object, TransObjectType type);
 
 #define GetActualState(object) \
diff --git a/pg_variables_record.c b/pg_variables_record.c
index 341dca2..1ae1279 100644
--- a/pg_variables_record.c
+++ b/pg_variables_record.c
@@ -8,8 +8,10 @@
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
+#include "funcapi.h"
 
 #include "access/htup_details.h"
+#include "access/tuptoaster.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
 #include "utils/builtins.h"
@@ -134,7 +136,11 @@ init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable)
 #endif
 
 	oldcxt = MemoryContextSwitchTo(record->hctx);
-	record->tupdesc = CreateTupleDescCopyConstr(tupdesc);
+	record->tupdesc = CreateTupleDescCopy(tupdesc);
+	record->tupdesc->tdhasoid = false;
+	record->tupdesc->tdtypeid = RECORDOID;
+	record->tupdesc->tdtypmod = -1;
+	record->tupdesc = BlessTupleDesc(record->tupdesc);
 
 	/* Initialize hash table. */
 	ctl.keysize = sizeof(HashRecordKey);
@@ -153,47 +159,6 @@ init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable)
 	MemoryContextSwitchTo(oldcxt);
 }
 
-/*
- * Copy record using src_tuple.
- */
-void
-copy_record(RecordVar *dest_record, HeapTuple src_tuple, Variable *variable)
-{
-	HeapTuple	tuple;
-	Datum		value;
-	bool		isnull;
-	HashRecordKey k;
-	HashRecordEntry *item;
-	bool		found;
-	MemoryContext oldcxt;
-
-	oldcxt = MemoryContextSwitchTo(dest_record->hctx);
-
-	/* Inserting a new record into dest_record */
-	tuple = heap_copytuple(src_tuple);
-	value = fastgetattr(tuple, 1, dest_record->tupdesc, &isnull);
-
-	k.value = value;
-	k.is_null = isnull;
-	k.hash_proc = &dest_record->hash_proc;
-	k.cmp_proc = &dest_record->cmp_proc;
-
-	item = (HashRecordEntry *) hash_search(dest_record->rhash, &k,
-										   HASH_ENTER, &found);
-	if (found)
-	{
-		heap_freetuple(tuple);
-		MemoryContextSwitchTo(oldcxt);
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("there is a record in the variable \"%s\" with same "
-						"key", GetName(variable))));
-	}
-	item->tuple = tuple;
-
-	MemoryContextSwitchTo(oldcxt);
-}
-
 /*
  * New record structure should be the same as the first record.
  */
@@ -247,15 +212,78 @@ check_record_key(Variable *variable, Oid typid)
 						"key type", GetName(variable))));
 }
 
+static Datum
+copy_record_tuple(RecordVar *record, HeapTupleHeader tupleHeader)
+{
+	TupleDesc	tupdesc;
+	HeapTupleHeader result;
+	int			tuple_len;
+
+	tupdesc = record->tupdesc;
+
+	/*
+	 * If the tuple contains any external TOAST pointers, we have to inline
+	 * those fields to meet the conventions for composite-type Datums.
+	 */
+	if (HeapTupleHeaderHasExternal(tupleHeader))
+		return toast_flatten_tuple_to_datum(tupleHeader,
+									HeapTupleHeaderGetDatumLength(tupleHeader),
+											tupdesc);
+
+	/*
+	 * Fast path for easy case: just make a palloc'd copy and insert the
+	 * correct composite-Datum header fields (since those may not be set if
+	 * the given tuple came from disk, rather than from heap_form_tuple).
+	 */
+	tuple_len = HeapTupleHeaderGetDatumLength(tupleHeader);
+	result = (HeapTupleHeader) palloc(tuple_len);
+	memcpy((char *) result, (char *) tupleHeader, tuple_len);
+
+	HeapTupleHeaderSetDatumLength(result, tuple_len);
+	HeapTupleHeaderSetTypeId(result, tupdesc->tdtypeid);
+	HeapTupleHeaderSetTypMod(result, tupdesc->tdtypmod);
+
+	return PointerGetDatum(result);
+}
+
+static Datum
+get_record_key(Datum tuple, TupleDesc tupdesc, bool *isnull)
+{
+	HeapTupleHeader th = (HeapTupleHeader) DatumGetPointer(tuple);
+	bool		hasnulls = th->t_infomask & HEAP_HASNULL;
+	bits8	   *bp = th->t_bits;		/* ptr to null bitmap in tuple */
+	char	   *tp;						/* ptr to tuple data */
+	long		off;					/* offset in tuple data */
+	int			keyatt = 0;
+	Form_pg_attribute attr = GetTupleDescAttr(tupdesc, keyatt);
+
+	if (hasnulls && att_isnull(keyatt, bp))
+	{
+		*isnull = true;
+		return (Datum) NULL;
+	}
+
+	tp = (char *) th + th->t_hoff;
+	off = 0;
+	if (attr->attlen == -1)
+		off = att_align_pointer(off, attr->attalign, -1, tp + off);
+	else
+	{
+		/* not varlena, so safe to use att_align_nominal */
+		off = att_align_nominal(off, attr->attalign);
+	}
+
+	*isnull = false;
+	return fetchatt(attr, tp + off);
+}
+
 /*
  * Insert a new record. New record key should be unique in the variable.
  */
 void
 insert_record(Variable *variable, HeapTupleHeader tupleHeader)
 {
-	TupleDesc	tupdesc;
-	HeapTuple	tuple;
-	int			tuple_len;
+	Datum		tuple;
 	Datum		value;
 	bool		isnull;
 	RecordVar  *record;
@@ -270,20 +298,10 @@ insert_record(Variable *variable, HeapTupleHeader tupleHeader)
 
 	oldcxt = MemoryContextSwitchTo(record->hctx);
 
-	tupdesc = record->tupdesc;
-
-	/* Build a HeapTuple control structure */
-	tuple_len = HeapTupleHeaderGetDatumLength(tupleHeader);
-
-	tuple = (HeapTuple) palloc(HEAPTUPLESIZE + tuple_len);
-	tuple->t_len = tuple_len;
-	ItemPointerSetInvalid(&(tuple->t_self));
-	tuple->t_tableOid = InvalidOid;
-	tuple->t_data = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
-	memcpy((char *) tuple->t_data, (char *) tupleHeader, tuple_len);
+	tuple = copy_record_tuple(record, tupleHeader);
 
 	/* Inserting a new record */
-	value = fastgetattr(tuple, 1, tupdesc, &isnull);
+	value = get_record_key(tuple, record->tupdesc, &isnull);
 	/* First, check if there is a record with same key */
 	k.value = value;
 	k.is_null = isnull;
@@ -294,7 +312,7 @@ insert_record(Variable *variable, HeapTupleHeader tupleHeader)
 										   HASH_ENTER, &found);
 	if (found)
 	{
-		heap_freetuple(tuple);
+		pfree(DatumGetPointer(tuple));
 		MemoryContextSwitchTo(oldcxt);
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -313,9 +331,7 @@ insert_record(Variable *variable, HeapTupleHeader tupleHeader)
 bool
 update_record(Variable *variable, HeapTupleHeader tupleHeader)
 {
-	TupleDesc	tupdesc;
-	HeapTuple	tuple;
-	int			tuple_len;
+	Datum		tuple;
 	Datum		value;
 	bool		isnull;
 	RecordVar  *record;
@@ -330,20 +346,10 @@ update_record(Variable *variable, HeapTupleHeader tupleHeader)
 
 	oldcxt = MemoryContextSwitchTo(record->hctx);
 
-	tupdesc = record->tupdesc;
-
-	/* Build a HeapTuple control structure */
-	tuple_len = HeapTupleHeaderGetDatumLength(tupleHeader);
-
-	tuple = (HeapTuple) palloc(HEAPTUPLESIZE + tuple_len);
-	tuple->t_len = tuple_len;
-	ItemPointerSetInvalid(&(tuple->t_self));
-	tuple->t_tableOid = InvalidOid;
-	tuple->t_data = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
-	memcpy((char *) tuple->t_data, (char *) tupleHeader, tuple_len);
+	tuple = copy_record_tuple(record, tupleHeader);
 
 	/* Update a record */
-	value = fastgetattr(tuple, 1, tupdesc, &isnull);
+	value = get_record_key(tuple, record->tupdesc, &isnull);
 	k.value = value;
 	k.is_null = isnull;
 	k.hash_proc = &record->hash_proc;
@@ -353,13 +359,13 @@ update_record(Variable *variable, HeapTupleHeader tupleHeader)
 										   HASH_FIND, &found);
 	if (!found)
 	{
-		heap_freetuple(tuple);
+		pfree(DatumGetPointer(tuple));
 		MemoryContextSwitchTo(oldcxt);
 		return false;
 	}
 
 	/* Release old tuple */
-	heap_freetuple(item->tuple);
+	pfree(DatumGetPointer(item->tuple));
 	item->tuple = tuple;
 
 	MemoryContextSwitchTo(oldcxt);
@@ -387,7 +393,49 @@ delete_record(Variable *variable, Datum value, bool is_null)
 	item = (HashRecordEntry *) hash_search(record->rhash, &k,
 										   HASH_REMOVE, &found);
 	if (found)
-		heap_freetuple(item->tuple);
+		pfree(DatumGetPointer(item->tuple));
 
 	return found;
 }
+
+/*
+ * Copy record using src_tuple.
+ */
+void
+insert_record_copy(RecordVar *dest_record, Datum src_tuple, Variable *variable)
+{
+	Datum		tuple;
+	Datum		value;
+	bool		isnull;
+	HashRecordKey k;
+	HashRecordEntry *item;
+	bool		found;
+	MemoryContext oldcxt;
+
+	oldcxt = MemoryContextSwitchTo(dest_record->hctx);
+
+	/* Inserting a new record into dest_record */
+	tuple = copy_record_tuple(dest_record,
+							  (HeapTupleHeader) DatumGetPointer(src_tuple));
+	value = get_record_key(tuple, dest_record->tupdesc, &isnull);
+
+	k.value = value;
+	k.is_null = isnull;
+	k.hash_proc = &dest_record->hash_proc;
+	k.cmp_proc = &dest_record->cmp_proc;
+
+	item = (HashRecordEntry *) hash_search(dest_record->rhash, &k,
+										   HASH_ENTER, &found);
+	if (found)
+	{
+		pfree(DatumGetPointer(tuple));
+		MemoryContextSwitchTo(oldcxt);
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("there is a record in the variable \"%s\" with same "
+						"key", GetName(variable))));
+	}
+	item->tuple = tuple;
+
+	MemoryContextSwitchTo(oldcxt);
+}

From 68989c24b009890461aadee7764fad8d7f679f81 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
Date: Tue, 2 Apr 2019 20:48:36 +0300
Subject: [PATCH 31/37] PGPRO-2601: Test pgv_select() on TupleDesc of dropped
 table

---
 expected/pg_variables.out       | 10 +++++++
 expected/pg_variables_trans.out | 48 +++++++++++++++++----------------
 sql/pg_variables.sql            |  4 +++
 sql/pg_variables_trans.sql      |  2 ++
 4 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 592c401..c456d60 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -704,6 +704,16 @@ SELECT pgv_exists('vars3', 'r1');
 
 SELECT pgv_select('vars2', 'j1');
 ERROR:  variable "j1" requires "jsonb" value
+-- PGPRO-2601 - Test pgv_select() on TupleDesc of dropped table
+DROP TABLE tab;
+SELECT pgv_select('vars3', 'r1');
+ pgv_select 
+------------
+ (,strNULL)
+ (2,)
+ (0,str00)
+(3 rows)
+
 -- Tests for SRF's sequential scan of an internal hash table
 DO
 $$BEGIN
diff --git a/expected/pg_variables_trans.out b/expected/pg_variables_trans.out
index b3576fd..025cc77 100644
--- a/expected/pg_variables_trans.out
+++ b/expected/pg_variables_trans.out
@@ -416,6 +416,8 @@ SELECT pgv_get_jsonb('vars2', 'j2');
 (1 row)
 
 COMMIT;
+CREATE TABLE tab (id int, t varchar);
+INSERT INTO tab VALUES (0, 'str00'), (1, 'str33'), (2, NULL), (NULL, 'strNULL');
 BEGIN;
 SELECT pgv_insert('vars3', 'r1', tab, true) FROM tab;
  pgv_insert 
@@ -452,8 +454,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -462,8 +464,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -473,8 +475,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -483,8 +485,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -824,8 +826,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -833,8 +835,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -843,8 +845,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -853,8 +855,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -1132,8 +1134,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -1142,8 +1144,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -1153,8 +1155,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -1163,8 +1165,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -1230,8 +1232,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -1239,8 +1241,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -1249,8 +1251,8 @@ SELECT pgv_select('vars3', 'r1');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,str55)
  (0,str00)
 (5 rows)
@@ -1259,8 +1261,8 @@ SELECT pgv_select('vars3', 'r2');
  pgv_select 
 ------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (0,str00)
 (4 rows)
 
@@ -1436,8 +1438,8 @@ SELECT pgv_select('vars3', 'r1');
         pgv_select         
 ---------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"after savepoint sp1")
  (0,str00)
 (5 rows)
@@ -1447,8 +1449,8 @@ SELECT pgv_select('vars3', 'r1');
         pgv_select         
 ---------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"after savepoint sp1")
  (0,str00)
 (5 rows)
@@ -1458,8 +1460,8 @@ SELECT pgv_select('vars3', 'r1');
         pgv_select         
 ---------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"after savepoint sp1")
  (0,str00)
 (5 rows)
@@ -1469,8 +1471,8 @@ SELECT pgv_select('vars3', 'r1');
               pgv_select              
 --------------------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"after savepoint sp1")
  (0,str00)
  (7,"row after sp2 to remove in sp4")
@@ -1481,8 +1483,8 @@ SELECT pgv_select('vars3', 'r1');
               pgv_select              
 --------------------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"after savepoint sp1")
  (0,str00)
  (7,"row after sp2 to remove in sp4")
@@ -1493,8 +1495,8 @@ SELECT pgv_select('vars3', 'r1');
          pgv_select         
 ----------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"before savepoint sp1")
  (0,str00)
 (5 rows)
@@ -1504,8 +1506,8 @@ SELECT pgv_select('vars3', 'r1');
          pgv_select         
 ----------------------------
  (,strNULL)
- (2,)
  (1,str33)
+ (2,)
  (5,"before savepoint sp1")
  (0,str00)
 (5 rows)
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 9e32468..3bafdbd 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -195,6 +195,10 @@ SELECT pgv_exists('vars3', 'r3');
 SELECT pgv_exists('vars3', 'r1');
 SELECT pgv_select('vars2', 'j1');
 
+-- PGPRO-2601 - Test pgv_select() on TupleDesc of dropped table
+DROP TABLE tab;
+SELECT pgv_select('vars3', 'r1');
+
 -- Tests for SRF's sequential scan of an internal hash table
 DO
 $$BEGIN
diff --git a/sql/pg_variables_trans.sql b/sql/pg_variables_trans.sql
index a3b6319..c4effd9 100644
--- a/sql/pg_variables_trans.sql
+++ b/sql/pg_variables_trans.sql
@@ -80,6 +80,8 @@ SELECT pgv_get_jsonb('vars2', 'j1');
 SELECT pgv_get_jsonb('vars2', 'j2');
 COMMIT;
 
+CREATE TABLE tab (id int, t varchar);
+INSERT INTO tab VALUES (0, 'str00'), (1, 'str33'), (2, NULL), (NULL, 'strNULL');
 
 BEGIN;
 SELECT pgv_insert('vars3', 'r1', tab, true) FROM tab;

From 2a4bba31f679992afb54aa40b4e16b25715f5b91 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
Date: Wed, 3 Apr 2019 11:09:03 +0300
Subject: [PATCH 32/37] PGPRO-2601: Remove LastHSeqStatus and add test a cursor
 with the hash table

---
 expected/pg_variables.out | 31 +++++++++++++++++++++++++++++--
 pg_variables.c            | 21 ---------------------
 sql/pg_variables.sql      | 13 +++++++++++--
 3 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index c456d60..6ded8c8 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -730,7 +730,8 @@ SELECT pgv_select('vars3', 'r1', 1);
  
 (1 row)
 
-SELECT pgv_select('vars3', 'r1') LIMIT 2;
+SELECT pgv_select('vars3', 'r1') LIMIT 2; -- warning
+WARNING:  leaked hash_seq_search scan for hash table 0x561738129110
  pgv_select 
 ------------
  (,strNULL)
@@ -743,8 +744,34 @@ SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
  (0,str00)
 (1 row)
 
+-- PGPRO-2601 - Test a cursor with the hash table
+BEGIN;
+DECLARE r1_cur CURSOR FOR SELECT pgv_select('vars3', 'r1');
+FETCH 1 in r1_cur;
+ pgv_select 
+------------
+ (,strNULL)
+(1 row)
+
+SELECT pgv_select('vars3', 'r1');
+ pgv_select 
+------------
+ (,strNULL)
+ (2,)
+ (0,str00)
+(3 rows)
+
+FETCH 1 in r1_cur;
+ pgv_select 
+------------
+ (2,)
+(1 row)
+
+CLOSE r1_cur;
+COMMIT; -- warning
+WARNING:  leaked hash_seq_search scan for hash table 0x561738129110
 -- Clean memory after unsuccessful creation of a variable
-SELECT pgv_insert('vars4', 'r1', row('str1', 'str1'));
+SELECT pgv_insert('vars4', 'r1', row('str1', 'str1')); -- fail
 ERROR:  could not identify a hash function for type unknown
 SELECT package FROM pgv_stats() WHERE package = 'vars4';
  package 
diff --git a/pg_variables.c b/pg_variables.c
index be0c7bc..a43c3c9 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -108,14 +108,6 @@ static Variable *LastVariable = NULL;
 /* Recent row type id */
 static Oid LastTypeId = InvalidOid;
 
-/*
- * Cache sequentially search through hash table status.  It is necessary for
- * clean up if hash_seq_term() wasn't called or if we didn't scan the whole
- * table. In this case we need to manually call hash_seq_term() within
- * variable_ExecutorEnd().
- */
-static HASH_SEQ_STATUS *LastHSeqStatus = NULL;
-
 /* Saved hook values for recall */
 static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
 
@@ -615,8 +607,6 @@ variable_select(PG_FUNCTION_ARGS)
 		MemoryContextSwitchTo(oldcontext);
 		PG_FREE_IF_COPY(package_name, 0);
 		PG_FREE_IF_COPY(var_name, 1);
-
-		LastHSeqStatus = rstat;
 	}
 
 	funcctx = SRF_PERCALL_SETUP();
@@ -633,7 +623,6 @@ variable_select(PG_FUNCTION_ARGS)
 	}
 	else
 	{
-		LastHSeqStatus = NULL;
 		pfree(rstat);
 		SRF_RETURN_DONE(funcctx);
 	}
@@ -1212,8 +1201,6 @@ get_packages_stats(PG_FUNCTION_ARGS)
 			hash_seq_init(pstat, packagesHash);
 
 			funcctx->user_fctx = pstat;
-
-			LastHSeqStatus = pstat;
 		}
 		else
 			funcctx->user_fctx = NULL;
@@ -1260,7 +1247,6 @@ get_packages_stats(PG_FUNCTION_ARGS)
 	}
 	else
 	{
-		LastHSeqStatus = NULL;
 		pfree(pstat);
 		SRF_RETURN_DONE(funcctx);
 	}
@@ -2126,7 +2112,6 @@ pgvSubTransCallback(SubXactEvent event, SubTransactionId mySubid,
 				break;
 		}
 	}
-	LastHSeqStatus = NULL;
 }
 
 /*
@@ -2155,7 +2140,6 @@ pgvTransCallback(XactEvent event, void *arg)
 				break;
 		}
 	}
-	LastHSeqStatus = NULL;
 }
 
 /*
@@ -2164,11 +2148,6 @@ pgvTransCallback(XactEvent event, void *arg)
 static void
 variable_ExecutorEnd(QueryDesc *queryDesc)
 {
-	if (LastHSeqStatus)
-	{
-		hash_seq_term(LastHSeqStatus);
-		LastHSeqStatus = NULL;
-	}
 	if (prev_ExecutorEnd)
 		prev_ExecutorEnd(queryDesc);
 	else
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index 3bafdbd..a373e93 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -207,11 +207,20 @@ $$BEGIN
 END$$;
 -- Check that the hash table was cleaned up after rollback
 SELECT pgv_select('vars3', 'r1', 1);
-SELECT pgv_select('vars3', 'r1') LIMIT 2;
+SELECT pgv_select('vars3', 'r1') LIMIT 2; -- warning
 SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
 
+-- PGPRO-2601 - Test a cursor with the hash table
+BEGIN;
+DECLARE r1_cur CURSOR FOR SELECT pgv_select('vars3', 'r1');
+FETCH 1 in r1_cur;
+SELECT pgv_select('vars3', 'r1');
+FETCH 1 in r1_cur;
+CLOSE r1_cur;
+COMMIT; -- warning
+
 -- Clean memory after unsuccessful creation of a variable
-SELECT pgv_insert('vars4', 'r1', row('str1', 'str1'));
+SELECT pgv_insert('vars4', 'r1', row('str1', 'str1')); -- fail
 SELECT package FROM pgv_stats() WHERE package = 'vars4';
 
 -- Remove package if it is empty

From 97e236540a4d8abf924c8e3369817f2d7d2858c1 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
Date: Wed, 3 Apr 2019 11:29:46 +0300
Subject: [PATCH 33/37] PGPRO-2601: Fix tests

---
 expected/pg_variables.out | 4 ++--
 sql/pg_variables.sql      | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/expected/pg_variables.out b/expected/pg_variables.out
index 6ded8c8..ba0b64a 100644
--- a/expected/pg_variables.out
+++ b/expected/pg_variables.out
@@ -724,6 +724,7 @@ ERROR:  unrecognized variable "r3"
 CONTEXT:  SQL statement "SELECT pgv_select('vars3', 'r3')"
 PL/pgSQL function inline_code_block line 3 at PERFORM
 -- Check that the hash table was cleaned up after rollback
+SET client_min_messages to 'ERROR';
 SELECT pgv_select('vars3', 'r1', 1);
  pgv_select 
 ------------
@@ -731,7 +732,6 @@ SELECT pgv_select('vars3', 'r1', 1);
 (1 row)
 
 SELECT pgv_select('vars3', 'r1') LIMIT 2; -- warning
-WARNING:  leaked hash_seq_search scan for hash table 0x561738129110
  pgv_select 
 ------------
  (,strNULL)
@@ -769,7 +769,7 @@ FETCH 1 in r1_cur;
 
 CLOSE r1_cur;
 COMMIT; -- warning
-WARNING:  leaked hash_seq_search scan for hash table 0x561738129110
+RESET client_min_messages;
 -- Clean memory after unsuccessful creation of a variable
 SELECT pgv_insert('vars4', 'r1', row('str1', 'str1')); -- fail
 ERROR:  could not identify a hash function for type unknown
diff --git a/sql/pg_variables.sql b/sql/pg_variables.sql
index a373e93..a9fdbbd 100644
--- a/sql/pg_variables.sql
+++ b/sql/pg_variables.sql
@@ -206,6 +206,7 @@ $$BEGIN
     PERFORM pgv_select('vars3', 'r3');
 END$$;
 -- Check that the hash table was cleaned up after rollback
+SET client_min_messages to 'ERROR';
 SELECT pgv_select('vars3', 'r1', 1);
 SELECT pgv_select('vars3', 'r1') LIMIT 2; -- warning
 SELECT pgv_select('vars3', 'r1') LIMIT 2 OFFSET 2;
@@ -218,6 +219,7 @@ SELECT pgv_select('vars3', 'r1');
 FETCH 1 in r1_cur;
 CLOSE r1_cur;
 COMMIT; -- warning
+RESET client_min_messages;
 
 -- Clean memory after unsuccessful creation of a variable
 SELECT pgv_insert('vars4', 'r1', row('str1', 'str1')); -- fail

From 152cc15da2f6b503511397f33d7c5f21216918f5 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Tue, 9 Apr 2019 13:55:06 +0300
Subject: [PATCH 34/37] PGPRO-2564: Add ATX compatibility check

---
 pg_variables.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/pg_variables.c b/pg_variables.c
index a43c3c9..ad3f67e 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -2088,6 +2088,15 @@ processChanges(Action action)
 	}
 }
 
+static void
+compatibility_check(void)
+{
+#ifdef PGPRO_EE
+	if (getNestLevelATX() != 0)
+		elog(ERROR, "pg_variable extension is not compatible with autonomous transactions and connection pooling");
+#endif
+}
+
 /*
  * Intercept execution during subtransaction processing
  */
@@ -2101,6 +2110,7 @@ pgvSubTransCallback(SubXactEvent event, SubTransactionId mySubid,
 		{
 			case SUBXACT_EVENT_START_SUB:
 				pushChangesStack();
+				compatibility_check();
 				break;
 			case SUBXACT_EVENT_COMMIT_SUB:
 				processChanges(RELEASE_SAVEPOINT);
@@ -2125,6 +2135,7 @@ pgvTransCallback(XactEvent event, void *arg)
 		switch (event)
 		{
 			case XACT_EVENT_PRE_COMMIT:
+				compatibility_check();
 				processChanges(RELEASE_SAVEPOINT);
 				break;
 			case XACT_EVENT_ABORT:

From 8f62247c94d0dc8645e67608ebd8ef4581b25c98 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Tue, 25 Jun 2019 17:36:23 +0300
Subject: [PATCH 35/37] Add /log/ to .gitignore

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index ee52e69..03329b5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -38,6 +38,7 @@ lib*.pc
 /pgsql.sln.cache
 /Debug/
 /Release/
+/log/
 /tmp_install/
 Dockerfile
 pg_variables--1.1.sql

From 0e25dd1b2b20fc6df62236a342575844b4e3c6a5 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Tue, 30 Jul 2019 16:38:42 +0300
Subject: [PATCH 36/37] Improve support of PostgreSQL 12

---
 pg_variables.c        | 14 ++++++++++++++
 pg_variables_record.c |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/pg_variables.c b/pg_variables.c
index ad3f67e..781b460 100755
--- a/pg_variables.c
+++ b/pg_variables.c
@@ -86,6 +86,19 @@ static void initObjectHistory(TransObject *object, TransObjectType type);
 /* Hook functions */
 static void variable_ExecutorEnd(QueryDesc *queryDesc);
 
+#if PG_VERSION_NUM >= 120000
+#define CHECK_ARGS_FOR_NULL() \
+do { \
+	if (fcinfo->args[0].isnull) \
+		ereport(ERROR, \
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
+				 errmsg("package name can not be NULL"))); \
+	if (fcinfo->args[1].isnull) \
+		ereport(ERROR, \
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
+				 errmsg("variable name can not be NULL"))); \
+} while(0)
+#else			/* PG_VERSION_NUM < 120000 */
 #define CHECK_ARGS_FOR_NULL() \
 do { \
 	if (fcinfo->argnull[0]) \
@@ -97,6 +110,7 @@ do { \
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
 				 errmsg("variable name can not be NULL"))); \
 } while(0)
+#endif			/* PG_VERSION_NUM */
 
 static HTAB *packagesHash = NULL;
 static MemoryContext ModuleContext = NULL;
diff --git a/pg_variables_record.c b/pg_variables_record.c
index 1ae1279..beba0d6 100644
--- a/pg_variables_record.c
+++ b/pg_variables_record.c
@@ -137,7 +137,9 @@ init_record(RecordVar *record, TupleDesc tupdesc, Variable *variable)
 
 	oldcxt = MemoryContextSwitchTo(record->hctx);
 	record->tupdesc = CreateTupleDescCopy(tupdesc);
+#if PG_VERSION_NUM < 120000
 	record->tupdesc->tdhasoid = false;
+#endif
 	record->tupdesc->tdtypeid = RECORDOID;
 	record->tupdesc->tdtypmod = -1;
 	record->tupdesc = BlessTupleDesc(record->tupdesc);

From b01b0f9e75a9e7d43e60cf5545651a6186deafc8 Mon Sep 17 00:00:00 2001
From: Arthur Zakirov <z-arthur@yandex.ru>
Date: Fri, 9 Aug 2019 13:41:57 +0300
Subject: [PATCH 37/37] Issue #26: Add pgv_set() and pgv_get() arguments
 description

---
 README.md | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/README.md b/README.md
index 0c584c0..01be495 100644
--- a/README.md
+++ b/README.md
@@ -102,6 +102,28 @@ Function | Returns
 `pgv_set(package text, name text, value anyarray, is_transactional bool default false)` | `void`
 `pgv_get(package text, name text, var_type anyarray, strict bool default true)` | `anyarray`
 
+`pgv_set` arguments:
+- `package` - name of the package, it will be created if it doesn't exist.
+- `name` - name of the variable, it will be created if it doesn't exist.
+`pgv_set` fails if the variable already exists and its transactionality doesn't
+match `is_transactional` argument.
+- `value` - new value for the variable. `pgv_set` fails if the variable already
+exists and its type doesn't match new value's type.
+- `is_transactional` - transactionality of the newly created variable, by
+default it is false.
+
+`pgv_get` arguments:
+- `package` - name of the existing package. If the package doesn't exist result
+depends on `strict` argument: if it is false then `pgv_get` returns NULL
+otherwise it fails.
+- `name` - name of the the existing variable. If the variable doesn't exist
+result depends on `strict` argument: if it is false then `pgv_get` returns NULL
+otherwise it fails.
+- `var_type` - type of the existing variable. It is necessary to pass it to get
+correct return type.
+- `strict` - pass false if `pgv_get` shouldn't raise an error if a variable or a
+package didn't created before, by default it is true.
+
 ## **Deprecated** scalar variables functions
 
 ### Integer variables