public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/5] pg_rewind: Replace the hybrid list+array data structure with simplehash. 2+ messages / 2 participants [nested] [flat]
* [PATCH 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; 2+ 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 | 284 ++++++++++++++------------------ 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(+), 207 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 431e8e760e0..6d48b6076e3 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; + bool found; - if (map->array) + entry = filehash_insert(filehash, path, &found); + if (!found) { - key.path = (char *) path; - key_ptr = &key; - e = bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *), - path_cmp); - } - else - e = NULL; - - if (e) - entry = *e; - else if (!create) - entry = NULL; - else - { - /* 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); - /* * Skip the control file. It is handled specially, after copying all the * other files. @@ -262,7 +259,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; @@ -280,7 +279,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; /* @@ -295,21 +293,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. @@ -318,7 +301,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; @@ -331,7 +316,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, @@ -342,47 +327,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); + } } } @@ -444,34 +427,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) { @@ -499,32 +454,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) { @@ -533,7 +487,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); } @@ -541,15 +495,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) { @@ -671,15 +624,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 @@ -710,16 +654,17 @@ final_filemap_cmp(const void *a, const void *b) /* * 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]; file_action_t action; if (!entry->target_exists && entry->source_exists) @@ -825,7 +770,34 @@ filemap_finalize() entry->action = action; } - /* 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 --------------D93EDEBFB124D563B723F4BD Content-Type: text/x-patch; charset=UTF-8; name="0004-pg_rewind-Refactor-the-abstraction-to-fetch-from-loc.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0004-pg_rewind-Refactor-the-abstraction-to-fetch-from-loc.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* [PATCH] Add UPDATE WHERE OFFSET IN clause @ 2022-02-07 05:59 Böszörményi Zoltán <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Böszörményi Zoltán @ 2022-02-07 05:59 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] Hi! A couple of years ago I was working on PostgreSQL features and extensions. Some of them was in ECPG, mainly in the area of improving Informix compatibility. One of the patchsets I invented was the cursor readahead support to improve performance in ECPG which used FETCH N. This work was published at https://github.com/zboszor/ecpg-readahead/commits However, there was one use case for this patchset actually decreased performance, namely if the cursor was used to modify rows via UPDATE WHERE CURRENT OF. It was because the cursor position on the server side was different from the application so the ECPG readahead code had to use MOVE then UPDATE WHERE CURRENT OF. I brewed this idea for a while and yesterday I decided to do something about it and here's the (admittedly, limited) result. I added a new PostgreSQL extension to the UPDATE syntax: UPDATE ... WHERE OFFSET n IN cursor; This new syntax changes the feature disparity between FETCH N and UPDATE WHERE CURRENT OF that existed in PostgreSQL forever. I only implemented this for cursors that use SELECT FOR UPDATE. This part was quite straightforward to implement. The behaviour of this syntax is as follows: * the offset value may be 0 or negative, since it is a virtual index into the rows returned by the last FETCH statement and negative indexes are felt natural relative to the cursor position and the cursor direction * for offset value 0, the behaviour is identical to WHERE CURRENT OF * negative indexes allow UPDATEs on previous rows even if the cursor reached the end of the result set I need clues for how to extend this for cursors that aren't using FOR UPDATE queries, if it's possible at all. Please, review. Best regards, Zoltán Böszörményi Attachments: [text/x-patch] 0001-Add-UPDATE-.-WHERE-OFFSET-IN-clause.patch (16.4K, ../../[email protected]/2-0001-Add-UPDATE-.-WHERE-OFFSET-IN-clause.patch) download | inline diff: From 26967c7510c09fe60d5fe54bd17bae919654c75d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20B=C3=B6sz=C3=B6rm=C3=A9nyi?= <[email protected]> Date: Sun, 6 Feb 2022 19:47:47 +0100 Subject: [PATCH] Add UPDATE ... WHERE OFFSET IN clause PostgreSQL has an extension for the cursor FETCH statement to retrieve multiple rows at once but it only supports updating the last fetched row via a cursor. This is a feature disparity. To rectify this, add the UPDATE ... WHERE OFFSET IN clause to support updating any of the rows retrieved in the last FETCH statement. --- doc/src/sgml/ref/update.sgml | 30 +++++++-- src/backend/executor/execCurrent.c | 21 ++++-- src/backend/executor/execMain.c | 2 +- src/backend/executor/nodeLockRows.c | 8 ++- src/backend/nodes/copyfuncs.c | 2 + src/backend/nodes/equalfuncs.c | 2 + src/backend/nodes/outfuncs.c | 2 + src/backend/nodes/readfuncs.c | 2 + src/backend/parser/gram.y | 12 ++++ src/include/nodes/execnodes.h | 10 +-- src/include/nodes/primnodes.h | 2 + src/test/regress/expected/portals.out | 93 +++++++++++++++++++++++++++ src/test/regress/sql/portals.sql | 47 ++++++++++++++ 13 files changed, 213 insertions(+), 20 deletions(-) diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml index 3fa54e5f70..5c08eae200 100644 --- a/doc/src/sgml/ref/update.sgml +++ b/doc/src/sgml/ref/update.sgml @@ -28,7 +28,7 @@ UPDATE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [ ( <replaceable class="parameter">column_name</replaceable> [, ...] ) = ( <replaceable class="parameter">sub-SELECT</replaceable> ) } [, ...] [ FROM <replaceable class="parameter">from_item</replaceable> [, ...] ] - [ WHERE <replaceable class="parameter">condition</replaceable> | WHERE CURRENT OF <replaceable class="parameter">cursor_name</replaceable> ] + [ WHERE <replaceable class="parameter">condition</replaceable> | WHERE CURRENT OF <replaceable class="parameter">cursor_name</replaceable> | WHERE OFFSET <replaceable class="parameter">offset</replaceable> OF <replaceable class="parameter">cursor_name</replaceable> ] [ RETURNING * | <replaceable class="parameter">output_expression</replaceable> [ [ AS ] <replaceable class="parameter">output_name</replaceable> ] [, ...] ] </synopsis> </refsynopsisdiv> @@ -199,11 +199,26 @@ UPDATE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [ <listitem> <para> The name of the cursor to use in a <literal>WHERE CURRENT OF</literal> - condition. The row to be updated is the one most recently fetched - from this cursor. The cursor must be a non-grouping - query on the <command>UPDATE</command>'s target table. - Note that <literal>WHERE CURRENT OF</literal> cannot be - specified together with a Boolean condition. See + or in a <literal>WHERE OFFSET IN</literal> condition. + The cursor must be a non-grouping query on the + <command>UPDATE</command>'s target table and a + <literal>FOR UPDATE</literal> query on the same table to use the + <literal>WHERE OFFSET IN</literal> condition + </para> + <para> + The row to be updated for <literal>WHERE CURRENT OF</literal> + is the one most recently fetched from this cursor. + </para> + <para> + If multiple rows were fetched in the last <command>FETCH</command> + statement, <literal>WHERE OFFSET IN</literal> may be used with a + negative or zero offset value relative to the cursor's current position and + the direction of the last <command>FETCH</command> statement. + Zero offset is identical to <literal>WHERE CURRENT OF</literal>. + </para> + <para> + Note that <literal>WHERE CURRENT OF</literal> and <literal>WHERE OFFSET IN</literal> + cannot be specified together with a Boolean condition. See <xref linkend="sql-declare"/> for more information about using cursors with <literal>WHERE CURRENT OF</literal>. @@ -442,7 +457,8 @@ UPDATE films SET kind = 'Dramatic' WHERE CURRENT OF c_films; <para> This command conforms to the <acronym>SQL</acronym> standard, except - that the <literal>FROM</literal> and <literal>RETURNING</literal> clauses + that the <literal>FROM</literal>, <literal>RETURNING</literal> and + <literal>WHERE OFFSET IN</literal> clauses are <productname>PostgreSQL</productname> extensions, as is the ability to use <literal>WITH</literal> with <command>UPDATE</command>. </para> diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c index b34b180bc4..6912bb2a25 100644 --- a/src/backend/executor/execCurrent.c +++ b/src/backend/executor/execCurrent.c @@ -131,17 +131,22 @@ execCurrentOf(CurrentOfExpr *cexpr, * The cursor must have a current result row: per the SQL spec, it's * an error if not. */ - if (portal->atStart || portal->atEnd) + if ((portal->atStart || portal->atEnd) && cexpr->cursor_offset == 0) ereport(ERROR, (errcode(ERRCODE_INVALID_CURSOR_STATE), errmsg("cursor \"%s\" is not positioned on a row", cursor_name))); /* Return the currently scanned TID, if there is one */ - if (ItemPointerIsValid(&(erm->curCtid))) - { - *current_tid = erm->curCtid; - return true; + if (erm->curCtidList) { + int cursor_offset = list_length(erm->curCtidList) - 1 + cexpr->cursor_offset + ((portal->atStart || portal->atEnd) ? 1 : 0); + ListCell *ctidCell = NULL; + if (cursor_offset >= 0 && cursor_offset < list_length(erm->curCtidList)) + ctidCell = list_nth_cell(erm->curCtidList, cursor_offset); + if (ctidCell && ItemPointerIsValid((ItemPointer)(ctidCell->ptr_value))) { + *current_tid = *(ItemPointer)(ctidCell->ptr_value); + return true; + } } /* @@ -161,6 +166,12 @@ execCurrentOf(CurrentOfExpr *cexpr, ScanState *scanstate; bool pending_rescan = false; + if (cexpr->cursor_is_offsetof) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" must use FOR UPDATE to use WHERE OFFSET IN", + cursor_name))); + scanstate = search_plan_tree(queryDesc->planstate, table_oid, &pending_rescan); if (!scanstate) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 549d9eb696..58c4da0cdc 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -879,7 +879,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) erm->strength = rc->strength; erm->waitPolicy = rc->waitPolicy; erm->ermActive = false; - ItemPointerSetInvalid(&(erm->curCtid)); + erm->curCtidList = NIL; erm->ermExtra = NULL; Assert(erm->rti > 0 && erm->rti <= estate->es_range_table_size && diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..a25c193d33 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -79,6 +79,7 @@ lnext: Datum datum; bool isNull; ItemPointerData tid; + ItemPointer *tid_ptr; TM_FailureData tmfd; LockTupleMode lockmode; int lockflags = 0; @@ -107,7 +108,8 @@ lnext: { /* this child is inactive right now */ erm->ermActive = false; - ItemPointerSetInvalid(&(erm->curCtid)); + list_free_deep(erm->curCtidList); + erm->curCtidList = NIL; ExecClearTuple(markSlot); continue; } @@ -249,7 +251,9 @@ lnext: } /* Remember locked tuple's TID for EPQ testing and WHERE CURRENT OF */ - erm->curCtid = tid; + tid_ptr = palloc(sizeof(ItemPointerData)); + memcpy(tid_ptr, &tid, sizeof(ItemPointerData)); + erm->curCtidList = lappend(erm->curCtidList, tid_ptr); } /* diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 6bd95bbce2..e800a1c3ba 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2178,6 +2178,8 @@ _copyCurrentOfExpr(const CurrentOfExpr *from) COPY_SCALAR_FIELD(cvarno); COPY_STRING_FIELD(cursor_name); COPY_SCALAR_FIELD(cursor_param); + COPY_SCALAR_FIELD(cursor_offset); + COPY_SCALAR_FIELD(cursor_is_offsetof); return newnode; } diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 4126516222..1c9aa6ae27 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -756,6 +756,8 @@ _equalCurrentOfExpr(const CurrentOfExpr *a, const CurrentOfExpr *b) COMPARE_SCALAR_FIELD(cvarno); COMPARE_STRING_FIELD(cursor_name); COMPARE_SCALAR_FIELD(cursor_param); + COMPARE_SCALAR_FIELD(cursor_offset); + COMPARE_SCALAR_FIELD(cursor_is_offsetof); return true; } diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 6bdad462c7..41c6d10ba4 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -1668,6 +1668,8 @@ _outCurrentOfExpr(StringInfo str, const CurrentOfExpr *node) WRITE_UINT_FIELD(cvarno); WRITE_STRING_FIELD(cursor_name); WRITE_INT_FIELD(cursor_param); + WRITE_INT_FIELD(cursor_offset); + WRITE_BOOL_FIELD(cursor_is_offsetof); } static void diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 3f68f7c18d..f953f406d5 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1269,6 +1269,8 @@ _readCurrentOfExpr(void) READ_UINT_FIELD(cvarno); READ_STRING_FIELD(cursor_name); READ_INT_FIELD(cursor_param); + READ_INT_FIELD(cursor_offset); + READ_BOOL_FIELD(cursor_is_offsetof); READ_DONE(); } diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index c4f3242506..ad14e62f78 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -12754,6 +12754,18 @@ where_or_current_clause: /* cvarno is filled in by parse analysis */ n->cursor_name = $4; n->cursor_param = 0; + n->cursor_is_offsetof = false; + n->cursor_offset = 0; + $$ = (Node *) n; + } + | WHERE OFFSET SignedIconst IN_P cursor_name + { + CurrentOfExpr *n = makeNode(CurrentOfExpr); + /* cvarno is filled in by parse analysis */ + n->cursor_name = $5; + n->cursor_param = 0; + n->cursor_is_offsetof = true; + n->cursor_offset = $3; $$ = (Node *) n; } | /*EMPTY*/ { $$ = NULL; } diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index dd95dc40c7..65ffc149cd 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -664,10 +664,10 @@ typedef struct EState * subqueries-in-FROM will have an ExecRowMark with relation == NULL. See * PlanRowMark for details about most of the fields. In addition to fields * directly derived from PlanRowMark, we store an activity flag (to denote - * inactive children of inheritance trees), curCtid, which is used by the - * WHERE CURRENT OF code, and ermExtra, which is available for use by the plan - * node that sources the relation (e.g., for a foreign table the FDW can use - * ermExtra to hold information). + * inactive children of inheritance trees), curCtidList, which is used by the + * WHERE CURRENT OF/WHERE OFFSET IN code, and ermExtra, which is available + * for use by the plan node that sources the relation (e.g., for a foreign + * table the FDW can use ermExtra to hold information). * * EState->es_rowmarks is an array of these structs, indexed by RT index, * with NULLs for irrelevant RT indexes. es_rowmarks itself is NULL if @@ -684,7 +684,7 @@ typedef struct ExecRowMark LockClauseStrength strength; /* LockingClause's strength, or LCS_NONE */ LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED */ bool ermActive; /* is this mark relevant for current tuple? */ - ItemPointerData curCtid; /* ctid of currently locked tuple, if any */ + List *curCtidList; /* list of ItemPointerData, ctid of currently locked tuple(s), if any */ void *ermExtra; /* available for use by relation source node */ } ExecRowMark; diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index dab5c4ff5d..399b127e5f 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -1363,6 +1363,8 @@ typedef struct CurrentOfExpr Index cvarno; /* RT index of target relation */ char *cursor_name; /* name of referenced cursor, or NULL */ int cursor_param; /* refcursor parameter number, or 0 */ + int cursor_offset; + bool cursor_is_offsetof; } CurrentOfExpr; /* diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out index 9da74876e1..8ce430cd47 100644 --- a/src/test/regress/expected/portals.out +++ b/src/test/regress/expected/portals.out @@ -1536,3 +1536,96 @@ fetch backward all in c2; (3 rows) rollback; +-- Check WHERE OFFSET IN +-- The cursor ends with no current row +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch all from c1; + id | t +----+--- + 1 | a + 2 | b + 3 | c +(3 rows) + +update cursor set t='cc' where offset -1 in c1; +update cursor set t='bb' where offset -2 in c1; +update cursor set t='aa' where offset -3 in c1; +select * from cursor order by id; + id | t +----+---- + 1 | aa + 2 | bb + 3 | cc +(3 rows) + +update cursor set t='xx' where offset 0 in c1; +ERROR: cursor "c1" is not positioned on a row +rollback; +-- The cursor ends with current row +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch 3 from c1; + id | t +----+--- + 1 | a + 2 | b + 3 | c +(3 rows) + +update cursor set t='cc' where offset 0 in c1; +update cursor set t='bb' where offset -1 in c1; +update cursor set t='aa' where offset -2 in c1; +select * from cursor order by id; + id | t +----+---- + 1 | aa + 2 | bb + 3 | cc +(3 rows) + +rollback; +-- Invalid offsets +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch 3 from c1; + id | t +----+--- + 1 | a + 2 | b + 3 | c +(3 rows) + +update cursor set t='cc' where offset 100 in c1; +update cursor set t='bb' where offset -100 in c1; +select * from cursor order by id; + id | t +----+--- + 1 | a + 2 | b + 3 | c +(3 rows) + +rollback; +-- Query without FOR UPDATE +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id; +fetch 3 from c1; + id | t +----+--- + 1 | a + 2 | b + 3 | c +(3 rows) + +update cursor set t='cc' where offset 0 in c1; +ERROR: cursor "c1" must use FOR UPDATE to use WHERE OFFSET IN +rollback; diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql index eadf6ed942..1947826f73 100644 --- a/src/test/regress/sql/portals.sql +++ b/src/test/regress/sql/portals.sql @@ -581,3 +581,50 @@ declare c2 scroll cursor for select generate_series(1,3) as g; fetch all in c2; fetch backward all in c2; rollback; + +-- Check WHERE OFFSET IN + +-- The cursor ends with no current row +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch all from c1; +update cursor set t='cc' where offset -1 in c1; +update cursor set t='bb' where offset -2 in c1; +update cursor set t='aa' where offset -3 in c1; +select * from cursor order by id; +update cursor set t='xx' where offset 0 in c1; +rollback; + +-- The cursor ends with current row +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch 3 from c1; +update cursor set t='cc' where offset 0 in c1; +update cursor set t='bb' where offset -1 in c1; +update cursor set t='aa' where offset -2 in c1; +select * from cursor order by id; +rollback; + +-- Invalid offsets +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id for update; +fetch 3 from c1; +update cursor set t='cc' where offset 100 in c1; +update cursor set t='bb' where offset -100 in c1; +select * from cursor order by id; +rollback; + +-- Query without FOR UPDATE +begin; +create table cursor (id serial, t text); +insert into cursor (t) values ('a'),('b'),('c'); +declare c1 cursor for select * from cursor order by id; +fetch 3 from c1; +update cursor set t='cc' where offset 0 in c1; +rollback; -- 2.34.1 ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2022-02-07 05:59 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-19 12:34 [PATCH 3/5] pg_rewind: Replace the hybrid list+array data structure with simplehash. Heikki Linnakangas <[email protected]> 2022-02-07 05:59 [PATCH] Add UPDATE WHERE OFFSET IN clause Böszörményi Zoltán <[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