public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 3/5] pg_rewind: Replace the hybrid list+array data structure with simplehash.
5+ messages / 3 participants
[nested] [flat]
* [PATCH v2 3/5] pg_rewind: Replace the hybrid list+array data structure with simplehash.
@ 2020-08-19 12:34 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Heikki Linnakangas @ 2020-08-19 12:34 UTC (permalink / raw)
Now that simplehash can be use in frontend code, let's make use of it.
---
src/bin/pg_rewind/copy_fetch.c | 4 +-
src/bin/pg_rewind/fetch.c | 2 +-
src/bin/pg_rewind/fetch.h | 2 +-
src/bin/pg_rewind/filemap.c | 285 ++++++++++++++------------------
src/bin/pg_rewind/filemap.h | 67 +++-----
src/bin/pg_rewind/libpq_fetch.c | 4 +-
src/bin/pg_rewind/pg_rewind.c | 12 +-
7 files changed, 168 insertions(+), 208 deletions(-)
diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c
index 18fad32600e..61aed8018b6 100644
--- a/src/bin/pg_rewind/copy_fetch.c
+++ b/src/bin/pg_rewind/copy_fetch.c
@@ -207,9 +207,9 @@ copy_executeFileMap(filemap_t *map)
file_entry_t *entry;
int i;
- for (i = 0; i < map->narray; i++)
+ for (i = 0; i < map->nactions; i++)
{
- entry = map->array[i];
+ entry = map->actions[i];
execute_pagemap(&entry->target_modified_pages, entry->path);
switch (entry->action)
diff --git a/src/bin/pg_rewind/fetch.c b/src/bin/pg_rewind/fetch.c
index f18fe5386ed..f41d0f295ea 100644
--- a/src/bin/pg_rewind/fetch.c
+++ b/src/bin/pg_rewind/fetch.c
@@ -37,7 +37,7 @@ fetchSourceFileList(void)
* Fetch all relation data files that are marked in the given data page map.
*/
void
-executeFileMap(void)
+execute_file_actions(filemap_t *filemap)
{
if (datadir_source)
copy_executeFileMap(filemap);
diff --git a/src/bin/pg_rewind/fetch.h b/src/bin/pg_rewind/fetch.h
index 7cf8b6ea090..b20df8b1537 100644
--- a/src/bin/pg_rewind/fetch.h
+++ b/src/bin/pg_rewind/fetch.h
@@ -25,7 +25,7 @@
*/
extern void fetchSourceFileList(void);
extern char *fetchFile(const char *filename, size_t *filesize);
-extern void executeFileMap(void);
+extern void execute_file_actions(filemap_t *filemap);
/* in libpq_fetch.c */
extern void libpqProcessFileList(void);
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 7971daeda5e..e6e037d1cd4 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -3,6 +3,19 @@
* filemap.c
* A data structure for keeping track of files that have changed.
*
+ * This source file contains the logic to decide what to do with different
+ * kinds of files, and the data structure to support it. Before modifying
+ * anything, pg_rewind collects information about all the files and their
+ * attributes in the target and source data directories. It also scans the
+ * WAL log in the target, and collects information about data blocks that
+ * were changed. All this information is stored in a hash table, using the
+ * file path, relative to the root of the data directory, as the key.
+ *
+ * After collecting all the information required, the filemap_finalize()
+ * function scans the hash table and decides what action needs to be taken
+ * for each file. Finally, it sorts the array to the final order that the
+ * actions should be executed in.
+ *
* Copyright (c) 2013-2020, PostgreSQL Global Development Group
*
*-------------------------------------------------------------------------
@@ -14,22 +27,39 @@
#include <unistd.h>
#include "catalog/pg_tablespace_d.h"
+#include "common/hashfn.h"
#include "common/string.h"
#include "datapagemap.h"
#include "filemap.h"
#include "pg_rewind.h"
#include "storage/fd.h"
-filemap_t *filemap = NULL;
+/*
+ * Define a hash table which we can use to store information about the files
+ * mentioned in the backup manifest.
+ */
+static uint32 hash_string_pointer(const char *s);
+#define SH_PREFIX filehash
+#define SH_ELEMENT_TYPE file_entry_t
+#define SH_KEY_TYPE const char *
+#define SH_KEY path
+#define SH_HASH_KEY(tb, key) hash_string_pointer(key)
+#define SH_EQUAL(tb, a, b) (strcmp(a, b) == 0)
+#define SH_SCOPE static inline
+#define SH_RAW_ALLOCATOR pg_malloc0
+#define SH_DECLARE
+#define SH_DEFINE
+#include "lib/simplehash.h"
+
+static filehash_hash *filehash;
static bool isRelDataFile(const char *path);
static char *datasegpath(RelFileNode rnode, ForkNumber forknum,
BlockNumber segno);
-static int path_cmp(const void *a, const void *b);
-static file_entry_t *get_filemap_entry(const char *path, bool create);
+static file_entry_t *insert_filehash_entry(const char *path);
+static file_entry_t *lookup_filehash_entry(const char *path);
static int final_filemap_cmp(const void *a, const void *b);
-static void filemap_list_to_array(filemap_t *map);
static bool check_file_excluded(const char *path, bool is_source);
/*
@@ -131,54 +161,26 @@ static const struct exclude_list_item excludeFiles[] =
};
/*
- * Create a new file map (stored in the global pointer "filemap").
+ * Initialize the hash table for the file map.
*/
void
-filemap_create(void)
+filemap_init(void)
{
- filemap_t *map;
-
- map = pg_malloc(sizeof(filemap_t));
- map->first = map->last = NULL;
- map->nlist = 0;
- map->array = NULL;
- map->narray = 0;
-
- Assert(filemap == NULL);
- filemap = map;
+ filehash = filehash_create(1000, NULL);
}
-/* Look up or create entry for 'path' */
+/* Look up entry for 'path', creating new one if it doesn't exists */
static file_entry_t *
-get_filemap_entry(const char *path, bool create)
+insert_filehash_entry(const char *path)
{
- filemap_t *map = filemap;
file_entry_t *entry;
- file_entry_t **e;
- file_entry_t key;
- file_entry_t *key_ptr;
-
- if (map->array)
- {
- key.path = (char *) path;
- key_ptr = &key;
- e = bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *),
- path_cmp);
- }
- else
- e = NULL;
+ bool found;
- if (e)
- entry = *e;
- else if (!create)
- entry = NULL;
- else
+ entry = filehash_insert(filehash, path, &found);
+ if (!found)
{
- /* Create a new entry for this file */
- entry = pg_malloc(sizeof(file_entry_t));
entry->path = pg_strdup(path);
entry->isrelfile = isRelDataFile(path);
- entry->action = FILE_ACTION_UNDECIDED;
entry->target_exists = false;
entry->target_type = FILE_TYPE_UNDEFINED;
@@ -192,21 +194,18 @@ get_filemap_entry(const char *path, bool create)
entry->source_size = 0;
entry->source_link_target = NULL;
- entry->next = NULL;
-
- if (map->last)
- {
- map->last->next = entry;
- map->last = entry;
- }
- else
- map->first = map->last = entry;
- map->nlist++;
+ entry->action = FILE_ACTION_UNDECIDED;
}
return entry;
}
+static file_entry_t *
+lookup_filehash_entry(const char *path)
+{
+ return filehash_lookup(filehash, path);
+}
+
/*
* Callback for processing source file list.
*
@@ -220,8 +219,6 @@ process_source_file(const char *path, file_type_t type, size_t size,
{
file_entry_t *entry;
- Assert(filemap->array == NULL);
-
/*
* Pretend that pg_wal is a directory, even if it's really a symlink. We
* don't want to mess with the symlink itself, nor complain if it's a
@@ -238,7 +235,9 @@ process_source_file(const char *path, file_type_t type, size_t size,
pg_fatal("data file \"%s\" in source is not a regular file", path);
/* Remember this source file */
- entry = get_filemap_entry(path, true);
+ entry = insert_filehash_entry(path);
+ if (entry->source_exists)
+ pg_fatal("duplicate source file \"%s\"", path);
entry->source_exists = true;
entry->source_type = type;
entry->source_size = size;
@@ -256,7 +255,6 @@ void
process_target_file(const char *path, file_type_t type, size_t size,
const char *link_target)
{
- filemap_t *map = filemap;
file_entry_t *entry;
/*
@@ -264,21 +262,6 @@ process_target_file(const char *path, file_type_t type, size_t size,
* from the target data folder all paths which have been filtered out from
* the source data folder when processing the source files.
*/
- if (map->array == NULL)
- {
- /* on first call, initialize lookup array */
- if (map->nlist == 0)
- {
- /* should not happen */
- pg_fatal("source file list is empty");
- }
-
- filemap_list_to_array(map);
-
- Assert(map->array != NULL);
-
- qsort(map->array, map->narray, sizeof(file_entry_t *), path_cmp);
- }
/*
* Like in process_source_file, pretend that pg_wal is always a directory.
@@ -287,7 +270,9 @@ process_target_file(const char *path, file_type_t type, size_t size,
type = FILE_TYPE_DIRECTORY;
/* Remember this target file */
- entry = get_filemap_entry(path, true);
+ entry = insert_filehash_entry(path);
+ if (entry->target_exists)
+ pg_fatal("duplicate source file \"%s\"", path);
entry->target_exists = true;
entry->target_type = type;
entry->target_size = size;
@@ -300,7 +285,7 @@ process_target_file(const char *path, file_type_t type, size_t size,
* changed blocks in the pagemap of the file.
*
* NOTE: All the files on both systems must have already been added to the
- * file map!
+ * hash table!
*/
void
process_target_wal_block_change(ForkNumber forknum, RelFileNode rnode,
@@ -311,47 +296,45 @@ process_target_wal_block_change(ForkNumber forknum, RelFileNode rnode,
BlockNumber blkno_inseg;
int segno;
- Assert(filemap->array);
-
segno = blkno / RELSEG_SIZE;
blkno_inseg = blkno % RELSEG_SIZE;
path = datasegpath(rnode, forknum, segno);
- entry = get_filemap_entry(path, false);
+ entry = lookup_filehash_entry(path);
pfree(path);
+ /*
+ * If the block still exists in both systems, remember it. Otherwise we
+ * can safely ignore it.
+ *
+ * If the block is beyond the EOF in the source system, or the file doesn't
+ * doesn'exist in the source at all, we're going to truncate/remove it away
+ * from the target anyway. Likewise, if it doesn't exist in the target
+ * anymore, we will copy it over with the "tail" from the source system,
+ * anyway.
+ *
+ * It is possible to find WAL for a file that doesn't exist on either
+ * system anymore. It means that the relation was dropped later in the
+ * target system, and independently on the source system too, or that
+ * it was created and dropped in the target system and it never existed
+ * in the source. Either way, we can safely ignore it.
+ */
if (entry)
{
- int64 end_offset;
-
Assert(entry->isrelfile);
if (entry->target_type != FILE_TYPE_REGULAR)
pg_fatal("unexpected page modification for directory or symbolic link \"%s\"",
entry->path);
- /*
- * If the block beyond the EOF in the source system, no need to
- * remember it now, because we're going to truncate it away from the
- * target anyway. Also no need to remember the block if it's beyond
- * the current EOF in the target system; we will copy it over with the
- * "tail" from the source system, anyway.
- */
- end_offset = (blkno_inseg + 1) * BLCKSZ;
- if (end_offset <= entry->source_size &&
- end_offset <= entry->target_size)
- datapagemap_add(&entry->target_modified_pages, blkno_inseg);
- }
- else
- {
- /*
- * If we don't have any record of this file in the file map, it means
- * that it's a relation that doesn't exist in the source system. It
- * could exist in the target system; we haven't moved the target-only
- * entries from the linked list to the array yet! But in any case, if
- * it doesn't exist in the source it will be removed from the target
- * too, and we can safely ignore it.
- */
+ if (entry->target_exists && entry->source_exists)
+ {
+ off_t end_offset;
+
+ end_offset = (blkno_inseg + 1) * BLCKSZ;
+ if (end_offset <= entry->source_size && end_offset <= entry->target_size)
+ datapagemap_add(&entry->target_modified_pages, blkno_inseg);
+ }
}
}
@@ -413,34 +396,6 @@ check_file_excluded(const char *path, bool is_source)
return false;
}
-/*
- * Convert the linked list of entries in map->first/last to the array,
- * map->array.
- */
-static void
-filemap_list_to_array(filemap_t *map)
-{
- int narray;
- file_entry_t *entry,
- *next;
-
- map->array = (file_entry_t **)
- pg_realloc(map->array,
- (map->nlist + map->narray) * sizeof(file_entry_t *));
-
- narray = map->narray;
- for (entry = map->first; entry != NULL; entry = next)
- {
- map->array[narray++] = entry;
- next = entry->next;
- entry->next = NULL;
- }
- Assert(narray == map->nlist + map->narray);
- map->narray = narray;
- map->nlist = 0;
- map->first = map->last = NULL;
-}
-
static const char *
action_to_str(file_action_t action)
{
@@ -468,32 +423,31 @@ action_to_str(file_action_t action)
* Calculate the totals needed for progress reports.
*/
void
-calculate_totals(void)
+calculate_totals(filemap_t *filemap)
{
file_entry_t *entry;
int i;
- filemap_t *map = filemap;
- map->total_size = 0;
- map->fetch_size = 0;
+ filemap->total_size = 0;
+ filemap->fetch_size = 0;
- for (i = 0; i < map->narray; i++)
+ for (i = 0; i < filemap->nactions; i++)
{
- entry = map->array[i];
+ entry = filemap->actions[i];
if (entry->source_type != FILE_TYPE_REGULAR)
continue;
- map->total_size += entry->source_size;
+ filemap->total_size += entry->source_size;
if (entry->action == FILE_ACTION_COPY)
{
- map->fetch_size += entry->source_size;
+ filemap->fetch_size += entry->source_size;
continue;
}
if (entry->action == FILE_ACTION_COPY_TAIL)
- map->fetch_size += (entry->source_size - entry->target_size);
+ filemap->fetch_size += (entry->source_size - entry->target_size);
if (entry->target_modified_pages.bitmapsize > 0)
{
@@ -502,7 +456,7 @@ calculate_totals(void)
iter = datapagemap_iterate(&entry->target_modified_pages);
while (datapagemap_next(iter, &blk))
- map->fetch_size += BLCKSZ;
+ filemap->fetch_size += BLCKSZ;
pg_free(iter);
}
@@ -510,15 +464,14 @@ calculate_totals(void)
}
void
-print_filemap(void)
+print_filemap(filemap_t *filemap)
{
- filemap_t *map = filemap;
file_entry_t *entry;
int i;
- for (i = 0; i < map->narray; i++)
+ for (i = 0; i < filemap->nactions; i++)
{
- entry = map->array[i];
+ entry = filemap->actions[i];
if (entry->action != FILE_ACTION_NONE ||
entry->target_modified_pages.bitmapsize > 0)
{
@@ -640,15 +593,6 @@ datasegpath(RelFileNode rnode, ForkNumber forknum, BlockNumber segno)
return path;
}
-static int
-path_cmp(const void *a, const void *b)
-{
- file_entry_t *fa = *((file_entry_t **) a);
- file_entry_t *fb = *((file_entry_t **) b);
-
- return strcmp(fa->path, fb->path);
-}
-
/*
* In the final stage, the filemap is sorted so that removals come last.
* From disk space usage point of view, it would be better to do removals
@@ -834,21 +778,48 @@ decide_file_action(file_entry_t *entry)
/*
* Decide what to do with each file.
*/
-void
+filemap_t *
filemap_finalize()
{
int i;
+ filehash_iterator it;
+ file_entry_t *entry;
+ filemap_t *filemap;
- filemap_list_to_array(filemap);
-
- for (i = 0; i < filemap->narray; i++)
+ filehash_start_iterate(filehash, &it);
+ while ((entry = filehash_iterate(filehash, &it)) != NULL)
{
- file_entry_t *entry = filemap->array[i];
-
entry->action = decide_file_action(entry);
}
- /* Sort the actions to the order that they should be performed */
- qsort(filemap->array, filemap->narray, sizeof(file_entry_t *),
+ /*
+ * Turn the hash table into an array, sorted in the order that the actions
+ * should be performed.
+ */
+ filemap = pg_malloc(offsetof(filemap_t, actions) +
+ filehash->members * sizeof(file_entry_t *));
+ filemap->nactions = filehash->members;
+ filehash_start_iterate(filehash, &it);
+ i = 0;
+ while ((entry = filehash_iterate(filehash, &it)) != NULL)
+ {
+ filemap->actions[i++] = entry;
+ }
+
+ qsort(&filemap->actions, filemap->nactions, sizeof(file_entry_t *),
final_filemap_cmp);
+
+ return filemap;
+}
+
+
+/*
+ * Helper function for filemap hash table.
+ */
+static uint32
+hash_string_pointer(const char *s)
+{
+ unsigned char *ss = (unsigned char *) s;
+
+ return hash_bytes(ss, strlen(s));
}
diff --git a/src/bin/pg_rewind/filemap.h b/src/bin/pg_rewind/filemap.h
index a5e8df57f40..3660ffe0990 100644
--- a/src/bin/pg_rewind/filemap.h
+++ b/src/bin/pg_rewind/filemap.h
@@ -12,15 +12,6 @@
#include "storage/block.h"
#include "storage/relfilenode.h"
-/*
- * For every file found in the local or remote system, we have a file entry
- * that contains information about the file on both systems. For relation
- * files, there is also a page map that marks pages in the file that were
- * changed in the target after the last common checkpoint. Each entry also
- * contains an 'action' field, which says what we are going to do with the
- * file.
- */
-
/* these enum values are sorted in the order we want actions to be processed */
typedef enum
{
@@ -44,9 +35,21 @@ typedef enum
FILE_TYPE_SYMLINK
} file_type_t;
+/*
+ * For every file found in the local or remote system, we have a file entry
+ * that contains information about the file on both systems. For relation
+ * files, there is also a page map that marks pages in the file that were
+ * changed in the target after the last common checkpoint.
+ *
+ * When gathering information, these are kept in a hash table, private to
+ * filemap.c. filemap_finalize() fills in the 'action' field, sorts all the
+ * entries, and returns them in an array, ready for executing the actions.
+ */
typedef struct file_entry_t
{
- char *path;
+ uint32 status; /* hash status */
+
+ const char *path;
bool isrelfile; /* is it a relation data file? */
/*
@@ -71,44 +74,25 @@ typedef struct file_entry_t
* What will we do to the file?
*/
file_action_t action;
-
- struct file_entry_t *next;
} file_entry_t;
+/*
+ * This represents the final decisions on what to do with each file.
+ * 'actions' array contains an entry for each file, sorted in the order
+ * that their actions should executed.
+ */
typedef struct filemap_t
{
- /*
- * New entries are accumulated to a linked list, in process_source_file
- * and process_target_file.
- */
- file_entry_t *first;
- file_entry_t *last;
- int nlist; /* number of entries currently in list */
-
- /*
- * After processing all the remote files, the entries in the linked list
- * are moved to this array. After processing local files, too, all the
- * local entries are added to the array by filemap_finalize, and sorted in
- * the final order. After filemap_finalize, all the entries are in the
- * array, and the linked list is empty.
- */
- file_entry_t **array;
- int narray; /* current length of array */
-
- /*
- * Summary information.
- */
+ /* Summary information, filled by calculate_totals() */
uint64 total_size; /* total size of the source cluster */
uint64 fetch_size; /* number of bytes that needs to be copied */
-} filemap_t;
-extern filemap_t *filemap;
-
-extern void filemap_create(void);
-extern void calculate_totals(void);
-extern void print_filemap(void);
+ int nactions; /* size of 'actions' array */
+ file_entry_t *actions[FLEXIBLE_ARRAY_MEMBER];
+} filemap_t;
/* Functions for populating the filemap */
+extern void filemap_init(void);
extern void process_source_file(const char *path, file_type_t type,
size_t size, const char *link_target);
extern void process_target_file(const char *path, file_type_t type,
@@ -116,6 +100,9 @@ extern void process_target_file(const char *path, file_type_t type,
extern void process_target_wal_block_change(ForkNumber forknum,
RelFileNode rnode,
BlockNumber blkno);
-extern void filemap_finalize(void);
+
+extern filemap_t *filemap_finalize(void);
+extern void calculate_totals(filemap_t *filemap);
+extern void print_filemap(filemap_t *filemap);
#endif /* FILEMAP_H */
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index 7fc9161b8c8..9c541bb73d5 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -460,9 +460,9 @@ libpq_executeFileMap(filemap_t *map)
PQresultErrorMessage(res));
PQclear(res);
- for (i = 0; i < map->narray; i++)
+ for (i = 0; i < map->nactions; i++)
{
- entry = map->array[i];
+ entry = map->actions[i];
/* If this is a relation file, copy the modified blocks */
execute_pagemap(&entry->target_modified_pages, entry->path);
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 210984d302b..2bdeed26c53 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -129,6 +129,7 @@ main(int argc, char **argv)
TimeLineID endtli;
ControlFileData ControlFile_new;
bool writerecoveryconf = false;
+ filemap_t *filemap;
pg_logging_init(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_rewind"));
@@ -371,10 +372,11 @@ main(int argc, char **argv)
/*
* Collect information about all files in the target and source systems.
*/
- filemap_create();
if (showprogress)
pg_log_info("reading source file list");
+ filemap_init();
fetchSourceFileList();
+
if (showprogress)
pg_log_info("reading target file list");
traverse_datadir(datadir_target, &process_target_file);
@@ -395,13 +397,13 @@ main(int argc, char **argv)
* We have collected all information we need from both systems. Decide
* what to do with each file.
*/
- filemap_finalize();
+ filemap = filemap_finalize();
if (showprogress)
- calculate_totals();
+ calculate_totals(filemap);
/* this is too verbose even for verbose mode */
if (debug)
- print_filemap();
+ print_filemap(filemap);
/*
* Ok, we're ready to start copying things over.
@@ -421,7 +423,7 @@ main(int argc, char **argv)
* modified the target directory and there is no turning back!
*/
- executeFileMap();
+ execute_file_actions(filemap);
progress_report(true);
--
2.20.1
--------------BF34D0120055DC3839060F92
Content-Type: text/x-patch; charset=UTF-8;
name="v2-0004-pg_rewind-Refactor-the-abstraction-to-fetch-from-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="v2-0004-pg_rewind-Refactor-the-abstraction-to-fetch-from-.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v15 13/14] meson: Remove non-binary targets added to bin_targets.
@ 2022-09-24 18:01 Andres Freund <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Andres Freund @ 2022-09-24 18:01 UTC (permalink / raw)
---
src/bin/psql/meson.build | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/bin/psql/meson.build b/src/bin/psql/meson.build
index 8ec07260d1b..a4c46bf5385 100644
--- a/src/bin/psql/meson.build
+++ b/src/bin/psql/meson.build
@@ -21,7 +21,6 @@ psqlscanslash = custom_target('psqlscanslash',
command: [flex_cmd, '--no-backup', '--fix-warnings', '--', '-Cfe', '-p', '-p'])
generated_sources += psqlscanslash
psql_sources += psqlscanslash
-bin_targets += psqlscanslash
sql_help = custom_target('psql_help',
output: ['sql_help.c', 'sql_help.h'],
@@ -36,7 +35,6 @@ sql_help = custom_target('psql_help',
)
generated_sources += sql_help.to_list()
psql_sources += sql_help
-bin_targets += sql_help
if host_system == 'windows'
psql_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
--
2.37.3.542.gdd3f6c4cae
--ayx3t42hstokjw3l
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v15-0014-meson-wip-headerchecks-cpluspluschecks.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint
@ 2025-03-11 04:17 jian he <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: jian he @ 2025-03-11 04:17 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
hi.
in RememberAllDependentForRebuilding
while (HeapTupleIsValid(depTup = systable_getnext(scan)))
{
if(subtype == AT_SetExpression)
elog(INFO, "foundObject.classId:%d", foundObject.classId);
}
Then do the regress test on generated_stored.sql
I found out only constraints and indexes will be rebuilt
while we are doing ALTER TABLE ALTER COLUMN SET EXPRESSION.
we can also see RememberAllDependentForRebuilding handling of:
case RelationRelationId:
case AttrDefaultRelationId:
case ConstraintRelationId:
RememberAllDependentForRebuilding record
AlteredTableInfo->changedConstraintOids, AlteredTableInfo->changedIndexOids.
ATPostAlterTypeCleanup will construct steps to rebuild these
constraints over the generated column.
and if these constraints are successfully installed,
AlteredTableInfo->constraints will be populated.
then in phase3 ATRewriteTable will do the scan or rewrite.
in summary: ATExecSetExpression, RememberAllDependentForRebuilding
will do all the work to change the generation expression,
whether it's virtual or stored.
we didn't support virtual generated columns over domain with check constraint.
we also didn't support index over virtual generated columns.
to support change generation expressions for virtual generated columns
over check constraints,
the code seems not hard.
Attachments:
[text/x-patch] v1-0001-virtual-generated-column-set-expression-with-check-constr.patch (8.4K, ../../CACJufxH3VETr7orF5rW29GnDk3n1wWbOE3WdkHYd3iPGrQ9E_A@mail.gmail.com/2-v1-0001-virtual-generated-column-set-expression-with-check-constr.patch)
download | inline diff:
From fa81ac3a82bfdb0dea52a985eb534a3866b4af7a Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Tue, 11 Mar 2025 12:17:00 +0800
Subject: [PATCH v1 1/1] virtual generated column set expression with check
constraint
currently, if virtual generated column have check constraints over it
(we currently not supported index on virtual generated column)
then we can not change the generation expression.
for example"
CREATE TABLE gtest20 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL CHECK (b < 50));
INSERT INTO gtest20 (a) VALUES (10);
ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); --error
this patch is to support it.
main gotcha is in ATExecSetExpression,
RememberAllDependentForRebuilding will do all the work.
also add a test for ALTER TABLE SET EXPRESSION for virtual generated column will not
do table rewrite.
discussion: https://postgr.es/m/
---
src/backend/commands/tablecmds.c | 28 +++++++------
.../regress/expected/generated_virtual.out | 39 ++++++++++++-------
src/test/regress/sql/generated_virtual.sql | 19 +++++++--
3 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1f870982559..079080700dc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8464,6 +8464,7 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
Expr *defval;
NewColumnValue *newval;
RawColumnDefault *rawEnt;
+ bool rescan = false;
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
if (!HeapTupleIsValid(tuple))
@@ -8488,17 +8489,7 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
errmsg("column \"%s\" of relation \"%s\" is not a generated column",
colName, RelationGetRelationName(rel))));
- /*
- * TODO: This could be done, just need to recheck any constraints
- * afterwards.
- */
- if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL &&
- rel->rd_att->constr && rel->rd_att->constr->num_check > 0)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints"),
- errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
- colName, RelationGetRelationName(rel))));
+ rescan = (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL);
/*
* We need to prevent this because a change of expression could affect a
@@ -8538,6 +8529,21 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
}
+ if (rescan)
+ {
+ Assert(!rewrite);
+
+ /* make sure we don't conflict with later attribute modifications */
+ CommandCounterIncrement();
+
+ /*
+ * Find everything that depends on the column (constraints, indexes,
+ * etc), and record enough information to let us recreate the objects
+ * after recan.
+ */
+ RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
+ }
+
/*
* Drop the dependency records of the GENERATED expression, in particular
* its INTERNAL dependency on the column, which would otherwise cause
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 7ef05f45be7..8d609c7c66e 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -636,12 +636,22 @@ INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
ERROR: new row for relation "gtest20" violates check constraint "gtest20_b_check"
DETAIL: Failing row contains (30, virtual).
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ERROR: check constraint "gtest20_b_check" of relation "gtest20" is violated by some row
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
+ attnum | attname | attmissingval
+--------+---------+---------------
+ 2 | b |
+ 3 | c | {11}
+(2 rows)
+
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
INSERT INTO gtest20a (a) VALUES (30);
@@ -915,14 +925,15 @@ Partition of: gtest_parent FOR VALUES FROM ('08-01-2016') TO ('09-01-2016')
f3 | bigint | | | generated always as (f2 * 33)
Partition of: gtest_parent FOR VALUES FROM ('09-01-2016') TO ('10-01-2016')
-SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
- tableoid | f1 | f2 | f3
---------------+------------+----+----
- gtest_child | 07-15-2016 | 2 | 8
- gtest_child2 | 08-15-2016 | 3 | 12
- gtest_child3 | 09-13-2016 | 1 | 4
-(3 rows)
-
+--error. check constraint was validated for each partitions's generation expression
+ALTER TABLE gtest_parent ADD CHECK (f3 < 21);
+ERROR: check constraint "gtest_parent_f3_check" of relation "gtest_child2" is violated by some row
+ALTER TABLE gtest_parent ADD CHECK (f3 < 33);
+ERROR: check constraint "gtest_parent_f3_check" of relation "gtest_child2" is violated by some row
+ALTER TABLE gtest_parent ADD CHECK (f3 < 66);
+ERROR: check constraint "gtest_parent_f3_check" of relation "gtest_child2" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index dab8c92ef99..dd4dd4d77e3 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -312,8 +312,15 @@ CREATE TABLE gtest20 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTU
INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
@@ -488,8 +495,14 @@ ALTER TABLE gtest_child ALTER COLUMN f3 SET EXPRESSION AS (f2 * 10);
\d gtest_child
\d gtest_child2
\d gtest_child3
-SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
+--error. check constraint was validated for each partitions's generation expression
+ALTER TABLE gtest_parent ADD CHECK (f3 < 21);
+ALTER TABLE gtest_parent ADD CHECK (f3 < 33);
+ALTER TABLE gtest_parent ADD CHECK (f3 < 66);
+
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
--
2.34.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint
@ 2025-04-24 02:42 jian he <[email protected]>
parent: jian he <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: jian he @ 2025-04-24 02:42 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
On Tue, Mar 11, 2025 at 12:17 PM jian he <[email protected]> wrote:
> hi.
> in summary: ATExecSetExpression, RememberAllDependentForRebuilding
> will do all the work to change the generation expression,
> whether it's virtual or stored.
>
while working on another patch, I found out this can be further simplified.
Thus a new patch is attached.
Attachments:
[text/x-patch] v1-0001-generated-column-set-expression-with-check-constraint.patch (8.1K, ../../CACJufxF9vVYcx53yJpC6Oo7Y7_=20=pmv_t-6SBKAJ63bQXhnQ@mail.gmail.com/2-v1-0001-generated-column-set-expression-with-check-constraint.patch)
download | inline diff:
From 1a5f3da55b625911f6e381f11fe569bbb5cde888 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Thu, 24 Apr 2025 10:41:23 +0800
Subject: [PATCH v1 1/1] generated column set expression with check constraint
currently, if we have check constraints over virtual generated column
then we can not change the generation expression.
for example:
CREATE TABLE gtest20 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL CHECK (b < 50));
INSERT INTO gtest20 (a) VALUES (10);
ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); --error
this patch is to support it.
main gotcha is in ATExecSetExpression,
RememberAllDependentForRebuilding will do all the work.
also add a test for ALTER TABLE SET EXPRESSION for virtual generated column will not
do table rewrite.
discussion: https://postgr.es/m/CACJufxH3VETr7orF5rW29GnDk3n1wWbOE3WdkHYd3iPGrQ9E_A@mail.gmail.com
---
src/backend/commands/tablecmds.c | 31 ++++++-------------
.../regress/expected/generated_virtual.out | 31 +++++++++++++++----
src/test/regress/sql/generated_virtual.sql | 19 ++++++++++--
3 files changed, 51 insertions(+), 30 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2705cf11330..dc4eb160f69 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8601,18 +8601,6 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
errmsg("column \"%s\" of relation \"%s\" is not a generated column",
colName, RelationGetRelationName(rel))));
- /*
- * TODO: This could be done, just need to recheck any constraints
- * afterwards.
- */
- if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL &&
- rel->rd_att->constr && rel->rd_att->constr->num_check > 0)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints"),
- errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
- colName, RelationGetRelationName(rel))));
-
if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL && attTup->attnotnull)
tab->verify_new_notnull = true;
@@ -8642,18 +8630,17 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
* this renders them pointless.
*/
RelationClearMissing(rel);
-
- /* make sure we don't conflict with later attribute modifications */
- CommandCounterIncrement();
-
- /*
- * Find everything that depends on the column (constraints, indexes,
- * etc), and record enough information to let us recreate the objects
- * after rewrite.
- */
- RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
}
+ /* make sure we don't conflict with later attribute modifications */
+ CommandCounterIncrement();
+
+ /*
+ * Find everything that depends on the column (constraints, indexes,
+ * etc), and record enough information to let us recreate the objects.
+ */
+ RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
+
/*
* Drop the dependency records of the GENERATED expression, in particular
* its INTERNAL dependency on the column, which would otherwise cause
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 6300e7c1d96..ae4cf5abf76 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -636,12 +636,22 @@ INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
ERROR: new row for relation "gtest20" violates check constraint "gtest20_b_check"
DETAIL: Failing row contains (30, virtual).
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns on tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ERROR: check constraint "gtest20_b_check" of relation "gtest20" is violated by some row
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
+ attnum | attname | attmissingval
+--------+---------+---------------
+ 2 | b |
+ 3 | c | {11}
+(2 rows)
+
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
INSERT INTO gtest20a (a) VALUES (30);
@@ -973,6 +983,15 @@ SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
gtest_child3 | 09-13-2016 | 1 | 4
(3 rows)
+--check constraint was validated based on each partitions's generation expression
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 19); --error
+ERROR: check constraint "cc1" of relation "gtest_child" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 66); --error
+ERROR: check constraint "cc1" of relation "gtest_child2" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 <> 33); --error
+ERROR: check constraint "cc1" of relation "gtest_child3" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index b4eedeee2fb..b2a8fbc0d71 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -312,8 +312,15 @@ CREATE TABLE gtest20 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTU
INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
@@ -523,6 +530,14 @@ ALTER TABLE gtest_child ALTER COLUMN f3 SET EXPRESSION AS (f2 * 10);
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
+--check constraint was validated based on each partitions's generation expression
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 19); --error
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 66); --error
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 <> 33); --error
+
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
+
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
--
2.34.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint
@ 2025-07-07 02:24 jian he <[email protected]>
parent: jian he <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: jian he @ 2025-07-07 02:24 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
hi.
rebased only.
Attachments:
[text/x-patch] v2-0001-allow-change-generated-expression-while-check-constraint-.patch (8.0K, ../../CACJufxExe7+Gh5MKMFiN5xwwmPXaaZC1jjQOFuovv8Q7b1LgFw@mail.gmail.com/2-v2-0001-allow-change-generated-expression-while-check-constraint-.patch)
download | inline diff:
From 3ff7dd0fd260a6b3a35f8652f0840ab96bc68c70 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Mon, 7 Jul 2025 10:07:25 +0800
Subject: [PATCH v2 1/1] allow change generated expression while check
constraint on it
currently, if we have check constraints over virtual generated column
then we can not change the generation expression.
for example:
CREATE TABLE gtest20 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL CHECK (b < 50));
INSERT INTO gtest20 (a) VALUES (10);
ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); --error
attached patch try to resolve this issue.
main gotcha is in ATExecSetExpression, RememberAllDependentForRebuilding will do
all the work. also add a test for ALTER TABLE SET EXPRESSION for virtual
generated column will not do table rewrite.
discussion: https://postgr.es/m/CACJufxH3VETr7orF5rW29GnDk3n1wWbOE3WdkHYd3iPGrQ9E_A@mail.gmail.com
commitfest: https://commitfest.postgresql.org/patch/5645/
---
src/backend/commands/tablecmds.c | 26 +++++-----------
.../regress/expected/generated_virtual.out | 31 +++++++++++++++----
src/test/regress/sql/generated_virtual.sql | 18 +++++++++--
3 files changed, 48 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb811520c29..d2e91281940 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8600,18 +8600,6 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
errmsg("column \"%s\" of relation \"%s\" is not a generated column",
colName, RelationGetRelationName(rel))));
- /*
- * TODO: This could be done, just need to recheck any constraints
- * afterwards.
- */
- if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL &&
- rel->rd_att->constr && rel->rd_att->constr->num_check > 0)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns in tables with check constraints"),
- errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
- colName, RelationGetRelationName(rel))));
-
if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL && attTup->attnotnull)
tab->verify_new_notnull = true;
@@ -8644,15 +8632,15 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
/* make sure we don't conflict with later attribute modifications */
CommandCounterIncrement();
-
- /*
- * Find everything that depends on the column (constraints, indexes,
- * etc), and record enough information to let us recreate the objects
- * after rewrite.
- */
- RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
}
+ /*
+ * Find everything that depends on the column (constraints, indexes, etc),
+ * and record enough information to let us recreate the objects after
+ * rewrite.
+ */
+ RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName);
+
/*
* Drop the dependency records of the GENERATED expression, in particular
* its INTERNAL dependency on the column, which would otherwise cause
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 3b40e15a95a..2c825fa06d4 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -633,12 +633,22 @@ INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
ERROR: new row for relation "gtest20" violates check constraint "gtest20_b_check"
DETAIL: Failing row contains (30, virtual).
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns in tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
-ERROR: ALTER TABLE / SET EXPRESSION is not supported for virtual generated columns in tables with check constraints
-DETAIL: Column "b" of relation "gtest20" is a virtual generated column.
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ERROR: check constraint "gtest20_b_check" of relation "gtest20" is violated by some row
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
+ attnum | attname | attmissingval
+--------+---------+---------------
+ 2 | b |
+ 3 | c | {11}
+(2 rows)
+
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
INSERT INTO gtest20a (a) VALUES (30);
@@ -982,6 +992,15 @@ SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
gtest_child3 | 09-13-2016 | 1 | 4
(3 rows)
+--check constraint was validated based on each partitions's generation expression
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 19); --error
+ERROR: check constraint "cc1" of relation "gtest_child" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 66); --error
+ERROR: check constraint "cc1" of relation "gtest_child2" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 <> 33); --error
+ERROR: check constraint "cc1" of relation "gtest_child3" is violated by some row
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index e2b31853e01..4bff3eac035 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -313,8 +313,15 @@ CREATE TABLE gtest20 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTU
INSERT INTO gtest20 (a) VALUES (10); -- ok
INSERT INTO gtest20 (a) VALUES (30); -- violates constraint
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint (currently not supported)
-ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok (currently not supported)
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 100); -- violates constraint
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok
+--test no table rewrite happen
+ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 4), ADD COLUMN C int default 11;
+SELECT pa.attnum,pa.attname,attmissingval
+FROM pg_attribute pa
+JOIN pg_attrdef patt ON pa.attrelid = patt.adrelid AND pa.attnum = patt.adnum
+WHERE pa.attrelid = 'gtest20'::regclass
+ORDER BY pa.attnum;
CREATE TABLE gtest20a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest20a (a) VALUES (10);
@@ -532,6 +539,13 @@ ALTER TABLE gtest_child ALTER COLUMN f3 SET EXPRESSION AS (f2 * 10);
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
+--check constraint was validated based on each partitions's generation expression
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 19); --error
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 < 66); --error
+ALTER TABLE gtest_parent ADD CONSTRAINT cc1 CHECK (f3 <> 33); --error
+ALTER TABLE gtest_parent ADD CONSTRAINT cc CHECK (f3 < 67); --ok
+ALTER TABLE gtest_parent DROP CONSTRAINT cc;
+
-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
--
2.34.1
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-07-07 02:24 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19 12:34 [PATCH v2 3/5] pg_rewind: Replace the hybrid list+array data structure with simplehash. Heikki Linnakangas <[email protected]>
2022-09-24 18:01 [PATCH v15 13/14] meson: Remove non-binary targets added to bin_targets. Andres Freund <[email protected]>
2025-03-11 04:17 support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint jian he <[email protected]>
2025-04-24 02:42 ` Re: support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint jian he <[email protected]>
2025-07-07 02:24 ` Re: support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint jian he <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox