public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v60 01/17] dshash: Add sequential scan support.
17+ messages / 5 participants
[nested] [flat]
* [PATCH v60 01/17] dshash: Add sequential scan support.
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw)
Dshash did not allow scan the all entries sequentially. This adds the
functionality. The interface is similar but a bit different both from
that of dynahash and simple dshash search functions. One of the most
significant differences is the sequential scan interface of dshash
always needs a call to dshash_seq_term when scan ends. Another is
locking. Dshash holds partition lock when returning an entry,
dshash_seq_next() also holds lock when returning an entry but callers
shouldn't release it, since the lock is essential to continue a
scan. The seqscan interface allows entry deletion while a scan. The
in-scan deletion should be performed by dshash_delete_current().
Author: Kyotaro Horiguchi <[email protected]>
---
src/include/lib/dshash.h | 22 +++++
src/backend/lib/dshash.c | 162 ++++++++++++++++++++++++++++++-
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 184 insertions(+), 1 deletion(-)
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index c069ec9de7c..a6ea3771731 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -59,6 +59,21 @@ typedef struct dshash_parameters
struct dshash_table_item;
typedef struct dshash_table_item dshash_table_item;
+/*
+ * Sequential scan state. The detail is exposed to let users know the storage
+ * size but it should be considered as an opaque type by callers.
+ */
+typedef struct dshash_seq_status
+{
+ dshash_table *hash_table; /* dshash table working on */
+ int curbucket; /* bucket number we are at */
+ int nbuckets; /* total number of buckets in the dshash */
+ dshash_table_item *curitem; /* item we are currently at */
+ dsa_pointer pnextitem; /* dsa-pointer to the next item */
+ int curpartition; /* partition number we are at */
+ bool exclusive; /* locking mode */
+} dshash_seq_status;
+
/* Creating, sharing and destroying from hash tables. */
extern dshash_table *dshash_create(dsa_area *area,
const dshash_parameters *params,
@@ -80,6 +95,13 @@ extern bool dshash_delete_key(dshash_table *hash_table, const void *key);
extern void dshash_delete_entry(dshash_table *hash_table, void *entry);
extern void dshash_release_lock(dshash_table *hash_table, void *entry);
+/* seq scan support */
+extern void dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive);
+extern void *dshash_seq_next(dshash_seq_status *status);
+extern void dshash_seq_term(dshash_seq_status *status);
+extern void dshash_delete_current(dshash_seq_status *status);
+extern void *dshash_get_current(dshash_seq_status *status);
/* Convenience hash and compare functions wrapping memcmp and tag_hash. */
extern int dshash_memcmp(const void *a, const void *b, size_t size, void *arg);
extern dshash_hash dshash_memhash(const void *v, size_t size, void *arg);
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index e0c763be326..4b33862545a 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -127,6 +127,10 @@ struct dshash_table
#define NUM_SPLITS(size_log2) \
(size_log2 - DSHASH_NUM_PARTITIONS_LOG2)
+/* How many buckets are there in a given size? */
+#define NUM_BUCKETS(size_log2) \
+ (((size_t) 1) << (size_log2))
+
/* How many buckets are there in each partition at a given size? */
#define BUCKETS_PER_PARTITION(size_log2) \
(((size_t) 1) << NUM_SPLITS(size_log2))
@@ -153,6 +157,10 @@ struct dshash_table
#define BUCKET_INDEX_FOR_PARTITION(partition, size_log2) \
((partition) << NUM_SPLITS(size_log2))
+/* Choose partition based on bucket index. */
+#define PARTITION_FOR_BUCKET_INDEX(bucket_idx, size_log2) \
+ ((bucket_idx) >> NUM_SPLITS(size_log2))
+
/* The head of the active bucket for a given hash value (lvalue). */
#define BUCKET_FOR_HASH(hash_table, hash) \
(hash_table->buckets[ \
@@ -324,7 +332,7 @@ dshash_destroy(dshash_table *hash_table)
ensure_valid_bucket_pointers(hash_table);
/* Free all the entries. */
- size = ((size_t) 1) << hash_table->size_log2;
+ size = NUM_BUCKETS(hash_table->size_log2);
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
@@ -592,6 +600,158 @@ dshash_memhash(const void *v, size_t size, void *arg)
return tag_hash(v, size);
}
+/*
+ * dshash_seq_init/_next/_term
+ * Sequentially scan through dshash table and return all the
+ * elements one by one, return NULL when no more.
+ *
+ * dshash_seq_term should always be called when a scan finished.
+ * The caller may delete returned elements midst of a scan by using
+ * dshash_delete_current(). exclusive must be true to delete elements.
+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive)
+{
+ status->hash_table = hash_table;
+ status->curbucket = 0;
+ status->nbuckets = 0;
+ status->curitem = NULL;
+ status->pnextitem = InvalidDsaPointer;
+ status->curpartition = -1;
+ status->exclusive = exclusive;
+}
+
+/*
+ * Returns the next element.
+ *
+ * Returned elements are locked and the caller must not explicitly release
+ * it. It is released at the next call to dshash_next().
+ */
+void *
+dshash_seq_next(dshash_seq_status *status)
+{
+ dsa_pointer next_item_pointer;
+
+ if (status->curitem == NULL)
+ {
+ int partition;
+
+ Assert(status->curbucket == 0);
+ Assert(!status->hash_table->find_locked);
+
+ /* first shot. grab the first item. */
+ partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+ LWLockAcquire(PARTITION_LOCK(status->hash_table, partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ status->curpartition = partition;
+
+ /* resize doesn't happen from now until seq scan ends */
+ status->nbuckets =
+ NUM_BUCKETS(status->hash_table->control->size_log2);
+ ensure_valid_bucket_pointers(status->hash_table);
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+ else
+ next_item_pointer = status->pnextitem;
+
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(status->hash_table,
+ status->curpartition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED));
+
+ /* Move to the next bucket if we finished the current bucket */
+ while (!DsaPointerIsValid(next_item_pointer))
+ {
+ int next_partition;
+
+ if (++status->curbucket >= status->nbuckets)
+ {
+ /* all buckets have been scanned. finish. */
+ return NULL;
+ }
+
+ /* Check if move to the next partition */
+ next_partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+
+ if (status->curpartition != next_partition)
+ {
+ /*
+ * Move to the next partition. Lock the next partition then release
+ * the current, not in the reverse order to avoid concurrent
+ * resizing. Avoid dead lock by taking lock in the same order
+ * with resize().
+ */
+ LWLockAcquire(PARTITION_LOCK(status->hash_table,
+ next_partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ LWLockRelease(PARTITION_LOCK(status->hash_table,
+ status->curpartition));
+ status->curpartition = next_partition;
+ }
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+
+ status->curitem =
+ dsa_get_address(status->hash_table->area, next_item_pointer);
+ status->hash_table->find_locked = true;
+ status->hash_table->find_exclusively_locked = status->exclusive;
+
+ /*
+ * The caller may delete the item. Store the next item in case of deletion.
+ */
+ status->pnextitem = status->curitem->next;
+
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
+/*
+ * Terminates the seqscan and release all locks.
+ *
+ * Should be always called when finishing or exiting a seqscan.
+ */
+void
+dshash_seq_term(dshash_seq_status *status)
+{
+ status->hash_table->find_locked = false;
+ status->hash_table->find_exclusively_locked = false;
+
+ if (status->curpartition >= 0)
+ LWLockRelease(PARTITION_LOCK(status->hash_table, status->curpartition));
+}
+
+/* Remove the current entry while a seq scan. */
+void
+dshash_delete_current(dshash_seq_status *status)
+{
+ dshash_table *hash_table = status->hash_table;
+ dshash_table_item *item = status->curitem;
+ size_t partition PG_USED_FOR_ASSERTS_ONLY;
+
+ partition = PARTITION_FOR_HASH(item->hash);
+
+ Assert(status->exclusive);
+ Assert(hash_table->control->magic == DSHASH_MAGIC);
+ Assert(hash_table->find_locked);
+ Assert(hash_table->find_exclusively_locked);
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition),
+ LW_EXCLUSIVE));
+
+ delete_item(hash_table, item);
+}
+
+/* Get the current entry while a seq scan. */
+void *
+dshash_get_current(dshash_seq_status *status)
+{
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
/*
* Print debugging information about the internal state of the hash table to
* stderr. The caller must hold no partition locks.
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 6a98064b2bd..d2d96380a3b 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2960,6 +2960,7 @@ dshash_hash
dshash_hash_function
dshash_parameters
dshash_partition
+dshash_seq_status
dshash_table
dshash_table_control
dshash_table_handle
--
2.31.0.121.g9198c13e34
--zefjd6adjk3g4ecc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v60-0002-Schedule-ShutdownXLOG-in-single-user-mode-using-.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v65 01/11] dshash: Add sequential scan support.
@ 2022-03-03 02:02 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-03 02:02 UTC (permalink / raw)
Dshash did not allow scan the all entries sequentially. This adds the
functionality. The interface is similar but a bit different both from
that of dynahash and simple dshash search functions. One of the most
significant differences is the sequential scan interface of dshash
always needs a call to dshash_seq_term when scan ends. Another is
locking. Dshash holds partition lock when returning an entry,
dshash_seq_next() also holds lock when returning an entry but callers
shouldn't release it, since the lock is essential to continue a
scan. The seqscan interface allows entry deletion while a scan. The
in-scan deletion should be performed by dshash_delete_current().
Author: Kyotaro Horiguchi <[email protected]>
---
src/include/lib/dshash.h | 22 +++++
src/backend/lib/dshash.c | 162 ++++++++++++++++++++++++++++++-
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 184 insertions(+), 1 deletion(-)
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index f3c57e76bfe..8ff4dbb59a0 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -59,6 +59,21 @@ typedef struct dshash_parameters
struct dshash_table_item;
typedef struct dshash_table_item dshash_table_item;
+/*
+ * Sequential scan state. The detail is exposed to let users know the storage
+ * size but it should be considered as an opaque type by callers.
+ */
+typedef struct dshash_seq_status
+{
+ dshash_table *hash_table; /* dshash table working on */
+ int curbucket; /* bucket number we are at */
+ int nbuckets; /* total number of buckets in the dshash */
+ dshash_table_item *curitem; /* item we are currently at */
+ dsa_pointer pnextitem; /* dsa-pointer to the next item */
+ int curpartition; /* partition number we are at */
+ bool exclusive; /* locking mode */
+} dshash_seq_status;
+
/* Creating, sharing and destroying from hash tables. */
extern dshash_table *dshash_create(dsa_area *area,
const dshash_parameters *params,
@@ -80,6 +95,13 @@ extern bool dshash_delete_key(dshash_table *hash_table, const void *key);
extern void dshash_delete_entry(dshash_table *hash_table, void *entry);
extern void dshash_release_lock(dshash_table *hash_table, void *entry);
+/* seq scan support */
+extern void dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive);
+extern void *dshash_seq_next(dshash_seq_status *status);
+extern void dshash_seq_term(dshash_seq_status *status);
+extern void dshash_delete_current(dshash_seq_status *status);
+extern void *dshash_get_current(dshash_seq_status *status);
/* Convenience hash and compare functions wrapping memcmp and tag_hash. */
extern int dshash_memcmp(const void *a, const void *b, size_t size, void *arg);
extern dshash_hash dshash_memhash(const void *v, size_t size, void *arg);
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index decedb2605b..6a7201dd5a9 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -127,6 +127,10 @@ struct dshash_table
#define NUM_SPLITS(size_log2) \
(size_log2 - DSHASH_NUM_PARTITIONS_LOG2)
+/* How many buckets are there in a given size? */
+#define NUM_BUCKETS(size_log2) \
+ (((size_t) 1) << (size_log2))
+
/* How many buckets are there in each partition at a given size? */
#define BUCKETS_PER_PARTITION(size_log2) \
(((size_t) 1) << NUM_SPLITS(size_log2))
@@ -153,6 +157,10 @@ struct dshash_table
#define BUCKET_INDEX_FOR_PARTITION(partition, size_log2) \
((partition) << NUM_SPLITS(size_log2))
+/* Choose partition based on bucket index. */
+#define PARTITION_FOR_BUCKET_INDEX(bucket_idx, size_log2) \
+ ((bucket_idx) >> NUM_SPLITS(size_log2))
+
/* The head of the active bucket for a given hash value (lvalue). */
#define BUCKET_FOR_HASH(hash_table, hash) \
(hash_table->buckets[ \
@@ -324,7 +332,7 @@ dshash_destroy(dshash_table *hash_table)
ensure_valid_bucket_pointers(hash_table);
/* Free all the entries. */
- size = ((size_t) 1) << hash_table->size_log2;
+ size = NUM_BUCKETS(hash_table->size_log2);
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
@@ -592,6 +600,158 @@ dshash_memhash(const void *v, size_t size, void *arg)
return tag_hash(v, size);
}
+/*
+ * dshash_seq_init/_next/_term
+ * Sequentially scan through dshash table and return all the
+ * elements one by one, return NULL when no more.
+ *
+ * dshash_seq_term should always be called when a scan finished.
+ * The caller may delete returned elements midst of a scan by using
+ * dshash_delete_current(). exclusive must be true to delete elements.
+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive)
+{
+ status->hash_table = hash_table;
+ status->curbucket = 0;
+ status->nbuckets = 0;
+ status->curitem = NULL;
+ status->pnextitem = InvalidDsaPointer;
+ status->curpartition = -1;
+ status->exclusive = exclusive;
+}
+
+/*
+ * Returns the next element.
+ *
+ * Returned elements are locked and the caller must not explicitly release
+ * it. It is released at the next call to dshash_next().
+ */
+void *
+dshash_seq_next(dshash_seq_status *status)
+{
+ dsa_pointer next_item_pointer;
+
+ if (status->curitem == NULL)
+ {
+ int partition;
+
+ Assert(status->curbucket == 0);
+ Assert(!status->hash_table->find_locked);
+
+ /* first shot. grab the first item. */
+ partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+ LWLockAcquire(PARTITION_LOCK(status->hash_table, partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ status->curpartition = partition;
+
+ /* resize doesn't happen from now until seq scan ends */
+ status->nbuckets =
+ NUM_BUCKETS(status->hash_table->control->size_log2);
+ ensure_valid_bucket_pointers(status->hash_table);
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+ else
+ next_item_pointer = status->pnextitem;
+
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(status->hash_table,
+ status->curpartition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED));
+
+ /* Move to the next bucket if we finished the current bucket */
+ while (!DsaPointerIsValid(next_item_pointer))
+ {
+ int next_partition;
+
+ if (++status->curbucket >= status->nbuckets)
+ {
+ /* all buckets have been scanned. finish. */
+ return NULL;
+ }
+
+ /* Check if move to the next partition */
+ next_partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+
+ if (status->curpartition != next_partition)
+ {
+ /*
+ * Move to the next partition. Lock the next partition then release
+ * the current, not in the reverse order to avoid concurrent
+ * resizing. Avoid dead lock by taking lock in the same order
+ * with resize().
+ */
+ LWLockAcquire(PARTITION_LOCK(status->hash_table,
+ next_partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ LWLockRelease(PARTITION_LOCK(status->hash_table,
+ status->curpartition));
+ status->curpartition = next_partition;
+ }
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+
+ status->curitem =
+ dsa_get_address(status->hash_table->area, next_item_pointer);
+ status->hash_table->find_locked = true;
+ status->hash_table->find_exclusively_locked = status->exclusive;
+
+ /*
+ * The caller may delete the item. Store the next item in case of deletion.
+ */
+ status->pnextitem = status->curitem->next;
+
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
+/*
+ * Terminates the seqscan and release all locks.
+ *
+ * Should be always called when finishing or exiting a seqscan.
+ */
+void
+dshash_seq_term(dshash_seq_status *status)
+{
+ status->hash_table->find_locked = false;
+ status->hash_table->find_exclusively_locked = false;
+
+ if (status->curpartition >= 0)
+ LWLockRelease(PARTITION_LOCK(status->hash_table, status->curpartition));
+}
+
+/* Remove the current entry while a seq scan. */
+void
+dshash_delete_current(dshash_seq_status *status)
+{
+ dshash_table *hash_table = status->hash_table;
+ dshash_table_item *item = status->curitem;
+ size_t partition PG_USED_FOR_ASSERTS_ONLY;
+
+ partition = PARTITION_FOR_HASH(item->hash);
+
+ Assert(status->exclusive);
+ Assert(hash_table->control->magic == DSHASH_MAGIC);
+ Assert(hash_table->find_locked);
+ Assert(hash_table->find_exclusively_locked);
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition),
+ LW_EXCLUSIVE));
+
+ delete_item(hash_table, item);
+}
+
+/* Get the current entry while a seq scan. */
+void *
+dshash_get_current(dshash_seq_status *status)
+{
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
/*
* Print debugging information about the internal state of the hash table to
* stderr. The caller must hold no partition locks.
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d9b83f744fb..eaf3e7a8d44 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3103,6 +3103,7 @@ dshash_hash
dshash_hash_function
dshash_parameters
dshash_partition
+dshash_seq_status
dshash_table
dshash_table_control
dshash_table_handle
--
2.35.1.354.g715d08a9e5
--6rwz5pdckqwec52m
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v65-0002-pgstat-Introduce-pgstat_relation_should_count.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-05-11 05:28 Thomas Munro <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: Thomas Munro @ 2022-05-11 05:28 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 28, 2022 at 5:50 AM Mark Dilger
<[email protected]> wrote:
> psql:<stdin>:1: ERROR: prepared transaction with identifier "xact_012_1" does not exist
> [10:26:16.314](1.215s) not ok 11 - Rollback of PGPROC_MAX_CACHED_SUBXIDS+ prepared transaction on promoted standby
> [10:26:16.314](0.000s)
> [10:26:16.314](0.000s) # Failed test 'Rollback of PGPROC_MAX_CACHED_SUBXIDS+ prepared transaction on promoted standby'
> [10:26:16.314](0.000s) # at t/012_subtransactions.pl line 208.
> [10:26:16.314](0.000s) # got: '3'
> # expected: '0'
FWIW I see that on my FBSD/clang system when I build with
-fsanitize=undefined -fno-sanitize-recover=all. It's something to do
with our stack depth detection and tricks being used by -fsanitize,
because there's a stack depth exceeded message in the log.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-07-26 17:57 Tom Lane <[email protected]>
1 sibling, 1 reply; 17+ messages in thread
From: Tom Lane @ 2022-07-26 17:57 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; [email protected]
I wrote:
>> It's been kind of hidden by other buildfarm noise, but
>> 031_recovery_conflict.pl is not as stable as it should be [1][2][3][4].
> After digging around in the code, I think this is almost certainly
> some manifestation of the previously-complained-of problem [1] that
> RecoveryConflictInterrupt is not safe to call in a signal handler,
> leading the conflicting backend to sometimes decide that it's not
> the problem.
I happened to notice that while skink continues to fail off-and-on
in 031_recovery_conflict.pl, the symptoms have changed! What
we're getting now typically looks like [1]:
[10:45:11.475](0.023s) ok 14 - startup deadlock: lock acquisition is waiting
Waiting for replication conn standby's replay_lsn to pass 0/33FB8B0 on primary
done
timed out waiting for match: (?^:User transaction caused buffer deadlock with recovery.) at t/031_recovery_conflict.pl line 367.
where absolutely nothing happens in the standby log, until we time out:
2022-07-24 10:45:11.452 UTC [1468367][client backend][2/4:0] LOG: statement: SELECT * FROM test_recovery_conflict_table2;
2022-07-24 10:45:11.472 UTC [1468547][client backend][3/2:0] LOG: statement: SELECT 'waiting' FROM pg_locks WHERE locktype = 'relation' AND NOT granted;
2022-07-24 10:48:15.860 UTC [1468362][walreceiver][:0] FATAL: could not receive data from WAL stream: server closed the connection unexpectedly
So this is not a case of RecoveryConflictInterrupt doing the wrong thing:
the startup process hasn't detected the buffer conflict in the first
place. Don't know what to make of that, but I vaguely suspect a test
timing problem. gull has shown this once as well, although at a different
step in the script [2].
regards, tom lane
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2022-07-24%2007%3A00%3A29
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gull&dt=2022-07-23%2009%3A34%3A54
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-07-26 18:16 Andres Freund <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2022-07-26 18:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; [email protected]
Hi,
On 2022-07-26 13:57:53 -0400, Tom Lane wrote:
> I happened to notice that while skink continues to fail off-and-on
> in 031_recovery_conflict.pl, the symptoms have changed! What
> we're getting now typically looks like [1]:
>
> [10:45:11.475](0.023s) ok 14 - startup deadlock: lock acquisition is waiting
> Waiting for replication conn standby's replay_lsn to pass 0/33FB8B0 on primary
> done
> timed out waiting for match: (?^:User transaction caused buffer deadlock with recovery.) at t/031_recovery_conflict.pl line 367.
>
> where absolutely nothing happens in the standby log, until we time out:
>
> 2022-07-24 10:45:11.452 UTC [1468367][client backend][2/4:0] LOG: statement: SELECT * FROM test_recovery_conflict_table2;
> 2022-07-24 10:45:11.472 UTC [1468547][client backend][3/2:0] LOG: statement: SELECT 'waiting' FROM pg_locks WHERE locktype = 'relation' AND NOT granted;
> 2022-07-24 10:48:15.860 UTC [1468362][walreceiver][:0] FATAL: could not receive data from WAL stream: server closed the connection unexpectedly
>
> So this is not a case of RecoveryConflictInterrupt doing the wrong thing:
> the startup process hasn't detected the buffer conflict in the first
> place.
I wonder if this, at least partially, could be be due to the elog thing
I was complaining about nearby. I.e. we decide to FATAL as part of a
recovery conflict interrupt, and then during that ERROR out as part of
another recovery conflict interrupt (because nothing holds interrupts as
part of FATAL).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-07-26 18:30 Tom Lane <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2022-07-26 18:30 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; [email protected]
Andres Freund <[email protected]> writes:
> On 2022-07-26 13:57:53 -0400, Tom Lane wrote:
>> So this is not a case of RecoveryConflictInterrupt doing the wrong thing:
>> the startup process hasn't detected the buffer conflict in the first
>> place.
> I wonder if this, at least partially, could be be due to the elog thing
> I was complaining about nearby. I.e. we decide to FATAL as part of a
> recovery conflict interrupt, and then during that ERROR out as part of
> another recovery conflict interrupt (because nothing holds interrupts as
> part of FATAL).
There are all sorts of things one could imagine going wrong in the
backend receiving the recovery conflict interrupt, but AFAICS in these
failures, the startup process hasn't sent a recovery conflict interrupt.
It certainly hasn't logged anything suggesting it noticed a conflict.
regards, tom lane
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-07-26 20:03 Andres Freund <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2022-07-26 20:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; [email protected]
Hi,
On 2022-07-26 14:30:30 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > On 2022-07-26 13:57:53 -0400, Tom Lane wrote:
> >> So this is not a case of RecoveryConflictInterrupt doing the wrong thing:
> >> the startup process hasn't detected the buffer conflict in the first
> >> place.
>
> > I wonder if this, at least partially, could be be due to the elog thing
> > I was complaining about nearby. I.e. we decide to FATAL as part of a
> > recovery conflict interrupt, and then during that ERROR out as part of
> > another recovery conflict interrupt (because nothing holds interrupts as
> > part of FATAL).
>
> There are all sorts of things one could imagine going wrong in the
> backend receiving the recovery conflict interrupt, but AFAICS in these
> failures, the startup process hasn't sent a recovery conflict interrupt.
> It certainly hasn't logged anything suggesting it noticed a conflict.
I don't think we reliably emit a log message before the recovery
conflict is resolved.
I've wondered a couple times now about making tap test timeouts somehow
trigger a core dump of all processes. Certainly would make it easier to
debug some of these kinds of issues.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-08-03 17:57 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2022-08-03 17:57 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; [email protected]
Hi,
On 2022-07-26 13:03:54 -0700, Andres Freund wrote:
> On 2022-07-26 14:30:30 -0400, Tom Lane wrote:
> > Andres Freund <[email protected]> writes:
> > > On 2022-07-26 13:57:53 -0400, Tom Lane wrote:
> > >> So this is not a case of RecoveryConflictInterrupt doing the wrong thing:
> > >> the startup process hasn't detected the buffer conflict in the first
> > >> place.
> >
> > > I wonder if this, at least partially, could be be due to the elog thing
> > > I was complaining about nearby. I.e. we decide to FATAL as part of a
> > > recovery conflict interrupt, and then during that ERROR out as part of
> > > another recovery conflict interrupt (because nothing holds interrupts as
> > > part of FATAL).
> >
> > There are all sorts of things one could imagine going wrong in the
> > backend receiving the recovery conflict interrupt, but AFAICS in these
> > failures, the startup process hasn't sent a recovery conflict interrupt.
> > It certainly hasn't logged anything suggesting it noticed a conflict.
>
> I don't think we reliably emit a log message before the recovery
> conflict is resolved.
I played around trying to reproduce this kind of issue.
One way to quickly run into trouble on a slow system is that
ResolveRecoveryConflictWithVirtualXIDs() can end up sending signals more
frequently than the target can process them. The next signal can arrive by the
time SIGUSR1 processing finished, which, at least on linux, causes the queued
signal to immediately be processed, without "normal" postgres code gaining
control.
The reason nothing might get logged in some cases is that
e.g. ResolveRecoveryConflictWithLock() tells
ResolveRecoveryConflictWithVirtualXIDs() to *not* report the waiting:
/*
* Prevent ResolveRecoveryConflictWithVirtualXIDs() from reporting
* "waiting" in PS display by disabling its argument report_waiting
* because the caller, WaitOnLock(), has already reported that.
*/
so ResolveRecoveryConflictWithLock() can end up looping indefinitely without
logging anything.
Another question I have about ResolveRecoveryConflictWithLock() is whether
it's ok that we don't check deadlocks around the
ResolveRecoveryConflictWithVirtualXIDs() call? It might be ok, because we'd
only block if there's a recovery conflict, in which killing the process ought
to succeed?
I think there's also might be a problem with the wait loop in ProcSleep() wrt
recovery conflicts: We rely on interrupts to be processed to throw recovery
conflict errors, but ProcSleep() is called in a bunch of places with
interrupts held. An Assert(INTERRUPTS_CAN_BE_PROCESSED()) after releasing the
partition lock triggers a bunch. It's possible that these aren't problematic
cases for recovery conflicts, because they're all around extension locks:
#2 0x0000562032f1968d in ExceptionalCondition (conditionName=0x56203310623a "INTERRUPTS_CAN_BE_PROCESSED()", errorType=0x562033105f6c "FailedAssertion",
fileName=0x562033105f30 "/home/andres/src/postgresql/src/backend/storage/lmgr/proc.c", lineNumber=1208)
at /home/andres/src/postgresql/src/backend/utils/error/assert.c:69
#3 0x0000562032d50f41 in ProcSleep (locallock=0x562034cafaf0, lockMethodTable=0x562033281740 <default_lockmethod>)
at /home/andres/src/postgresql/src/backend/storage/lmgr/proc.c:1208
#4 0x0000562032d3e2ce in WaitOnLock (locallock=0x562034cafaf0, owner=0x562034d12c58) at /home/andres/src/postgresql/src/backend/storage/lmgr/lock.c:1859
#5 0x0000562032d3cd0a in LockAcquireExtended (locktag=0x7ffc7b4d0810, lockmode=7, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x0)
at /home/andres/src/postgresql/src/backend/storage/lmgr/lock.c:1101
#6 0x0000562032d3c1c4 in LockAcquire (locktag=0x7ffc7b4d0810, lockmode=7, sessionLock=false, dontWait=false)
at /home/andres/src/postgresql/src/backend/storage/lmgr/lock.c:752
#7 0x0000562032d3a696 in LockRelationForExtension (relation=0x7f54646b1dd8, lockmode=7) at /home/andres/src/postgresql/src/backend/storage/lmgr/lmgr.c:439
#8 0x0000562032894276 in _bt_getbuf (rel=0x7f54646b1dd8, blkno=4294967295, access=2) at /home/andres/src/postgresql/src/backend/access/nbtree/nbtpage.c:975
#9 0x000056203288f1cb in _bt_split (rel=0x7f54646b1dd8, itup_key=0x562034ea7428, buf=770, cbuf=0, newitemoff=408, newitemsz=16, newitem=0x562034ea3fc8,
orignewitem=0x0, nposting=0x0, postingoff=0) at /home/andres/src/postgresql/src/backend/access/nbtree/nbtinsert.c:1715
#10 0x000056203288e4bb in _bt_insertonpg (rel=0x7f54646b1dd8, itup_key=0x562034ea7428, buf=770, cbuf=0, stack=0x562034ea1fb8, itup=0x562034ea3fc8, itemsz=16,
newitemoff=408, postingoff=0, split_only_page=false) at /home/andres/src/postgresql/src/backend/access/nbtree/nbtinsert.c:1212
#11 0x000056203288caf9 in _bt_doinsert (rel=0x7f54646b1dd8, itup=0x562034ea3fc8, checkUnique=UNIQUE_CHECK_YES, indexUnchanged=false, heapRel=0x7f546823dde0)
at /home/andres/src/postgresql/src/backend/access/nbtree/nbtinsert.c:258
#12 0x000056203289851f in btinsert (rel=0x7f54646b1dd8, values=0x7ffc7b4d0c50, isnull=0x7ffc7b4d0c30, ht_ctid=0x562034dd083c, heapRel=0x7f546823dde0,
checkUnique=UNIQUE_CHECK_YES, indexUnchanged=false, indexInfo=0x562034ea71c0) at /home/andres/src/postgresql/src/backend/access/nbtree/nbtree.c:200
#13 0x000056203288710b in index_insert (indexRelation=0x7f54646b1dd8, values=0x7ffc7b4d0c50, isnull=0x7ffc7b4d0c30, heap_t_ctid=0x562034dd083c,
heapRelation=0x7f546823dde0, checkUnique=UNIQUE_CHECK_YES, indexUnchanged=false, indexInfo=0x562034ea71c0)
at /home/andres/src/postgresql/src/backend/access/index/indexam.c:193
#14 0x000056203292e9da in CatalogIndexInsert (indstate=0x562034dd02b0, heapTuple=0x562034dd0838)
(gdb) p num_held_lwlocks
$14 = 1
(gdb) p held_lwlocks[0]
$15 = {lock = 0x7f1a0d18d2e4, mode = LW_EXCLUSIVE}
(gdb) p held_lwlocks[0].lock->tranche
$16 = 56
(gdb) p BuiltinTrancheNames[held_lwlocks[0].lock->tranche - NUM_INDIVIDUAL_LWLOCKS]
$17 = 0x558ce5710ede "BufferContent"
Independent of recovery conflicts, isn't it dangerous that we acquire the
relation extension lock with a buffer content lock held? I *guess* it might be
ok because BufferAlloc(P_NEW) only acquires buffer content locks in a
conditional way.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-08-03 20:33 Robert Haas <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2022-08-03 20:33 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Aug 3, 2022 at 1:57 PM Andres Freund <[email protected]> wrote:
> The reason nothing might get logged in some cases is that
> e.g. ResolveRecoveryConflictWithLock() tells
> ResolveRecoveryConflictWithVirtualXIDs() to *not* report the waiting:
> /*
> * Prevent ResolveRecoveryConflictWithVirtualXIDs() from reporting
> * "waiting" in PS display by disabling its argument report_waiting
> * because the caller, WaitOnLock(), has already reported that.
> */
>
> so ResolveRecoveryConflictWithLock() can end up looping indefinitely without
> logging anything.
I understand why we need to avoid adding "waiting" to the PS status
when we've already done that, but it doesn't seem like that should
imply skipping ereport() of log messages.
I think we could redesign the way the ps display works to make things
a whole lot simpler. Let's have a function set_ps_display() and
another function set_ps_display_suffix(). What gets reported to the OS
is the concatenation of the two. Calling set_ps_display() implicitly
resets the suffix to empty.
AFAICS, that'd let us get rid of this tricky logic, and some other
tricky logic as well. Here, we'd just say set_ps_display_suffix("
waiting") and not worry about whether the caller might have already
done something similar.
> Another question I have about ResolveRecoveryConflictWithLock() is whether
> it's ok that we don't check deadlocks around the
> ResolveRecoveryConflictWithVirtualXIDs() call? It might be ok, because we'd
> only block if there's a recovery conflict, in which killing the process ought
> to succeed?
The startup process is supposed to always "win" in any deadlock
situation, so I'm not sure what you think is a problem here. We get
the conflicting lockers. We kill them. If they don't die, that's a
bug, but killing ourselves doesn't really help anything; if we die,
the whole system goes down, which seems undesirable.
> I think there's also might be a problem with the wait loop in ProcSleep() wrt
> recovery conflicts: We rely on interrupts to be processed to throw recovery
> conflict errors, but ProcSleep() is called in a bunch of places with
> interrupts held. An Assert(INTERRUPTS_CAN_BE_PROCESSED()) after releasing the
> partition lock triggers a bunch. It's possible that these aren't problematic
> cases for recovery conflicts, because they're all around extension locks:
> [...]
> Independent of recovery conflicts, isn't it dangerous that we acquire the
> relation extension lock with a buffer content lock held? I *guess* it might be
> ok because BufferAlloc(P_NEW) only acquires buffer content locks in a
> conditional way.
These things both seem a bit sketchy but it's not 100% clear to me
that anything is actually broken. Now it's also not clear to me that
nothing is broken ...
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Unstable tests for recovery conflict handling
@ 2022-08-03 22:07 Andres Freund <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Andres Freund @ 2022-08-03 22:07 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-08-03 16:33:46 -0400, Robert Haas wrote:
> On Wed, Aug 3, 2022 at 1:57 PM Andres Freund <[email protected]> wrote:
> > The reason nothing might get logged in some cases is that
> > e.g. ResolveRecoveryConflictWithLock() tells
> > ResolveRecoveryConflictWithVirtualXIDs() to *not* report the waiting:
> > /*
> > * Prevent ResolveRecoveryConflictWithVirtualXIDs() from reporting
> > * "waiting" in PS display by disabling its argument report_waiting
> > * because the caller, WaitOnLock(), has already reported that.
> > */
> >
> > so ResolveRecoveryConflictWithLock() can end up looping indefinitely without
> > logging anything.
>
> I understand why we need to avoid adding "waiting" to the PS status
> when we've already done that, but it doesn't seem like that should
> imply skipping ereport() of log messages.
>
> I think we could redesign the way the ps display works to make things
> a whole lot simpler. Let's have a function set_ps_display() and
> another function set_ps_display_suffix(). What gets reported to the OS
> is the concatenation of the two. Calling set_ps_display() implicitly
> resets the suffix to empty.
>
> AFAICS, that'd let us get rid of this tricky logic, and some other
> tricky logic as well. Here, we'd just say set_ps_display_suffix("
> waiting") and not worry about whether the caller might have already
> done something similar.
That sounds like it'd be an improvement. Of course we still need to fix that
we can signal at a rate not allowing the other side to handle the conflict,
but at least that'd be easier to identify...
> > Another question I have about ResolveRecoveryConflictWithLock() is whether
> > it's ok that we don't check deadlocks around the
> > ResolveRecoveryConflictWithVirtualXIDs() call? It might be ok, because we'd
> > only block if there's a recovery conflict, in which killing the process ought
> > to succeed?
>
> The startup process is supposed to always "win" in any deadlock
> situation, so I'm not sure what you think is a problem here. We get
> the conflicting lockers. We kill them. If they don't die, that's a
> bug, but killing ourselves doesn't really help anything; if we die,
> the whole system goes down, which seems undesirable.
The way deadlock timeout for the startup process works is that we wait for it
to pass and then send PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK to the
backends. So it's not that the startup process would die.
The question is basically whether there are cases were
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK would resolve a conflict but
PROCSIG_RECOVERY_CONFLICT_LOCK wouldn't. It seems plausible that there isn't,
but it's also not obvious enough that I'd fully trust it.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-15 18:27 Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2024-01-15 18:27 UTC (permalink / raw)
To: [email protected]
d=# create type varbitrange as range (subtype = varbit);
CREATE TYPE
d=# \dT+
List of data types
Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
--------+------------------+------------------+------+----------+----------+-------------------+-------------
public | varbitmultirange | varbitmultirange | var | | postgres | |
public | varbitrange | varbitrange | var | | postgres | |
(2 rows)
d=# create user joe;
CREATE ROLE
d=# alter type varbitrange owner to joe;
ALTER TYPE
d=# \dT+
List of data types
Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
--------+------------------+------------------+------+----------+----------+-------------------+-------------
public | varbitmultirange | varbitmultirange | var | | postgres | |
public | varbitrange | varbitrange | var | | joe | |
(2 rows)
That's pretty broken, isn't it? joe would own the multirange if he'd
created the range to start with. Even if you think the ownerships
ideally should be separable, this behavior causes existing pg_dump
files to restore incorrectly, because pg_dump assumes it need not emit
any commands about the multirange.
A related issue is that you can manually alter the multirange's
ownership:
d=# alter type varbitmultirange owner to joe;
ALTER TYPE
which while it has some value in allowing recovery from this bug,
is inconsistent with our handling of other dependent types such
as arrays:
d=# alter type _varbitrange owner to joe;
ERROR: cannot alter array type varbitrange[]
HINT: You can alter type varbitrange, which will alter the array type as well.
Possibly the thing to do about that is to forbid it in HEAD
for consistency, while still allowing it in back branches
so that people can clean up inconsistent ownership if needed.
regards, tom lane
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-15 19:17 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2024-01-15 19:17 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
On Mon, Jan 15, 2024 at 1:27 PM Tom Lane <[email protected]> wrote:
> That's pretty broken, isn't it? joe would own the multirange if he'd
> created the range to start with. Even if you think the ownerships
> ideally should be separable, this behavior causes existing pg_dump
> files to restore incorrectly, because pg_dump assumes it need not emit
> any commands about the multirange.
I agree that pg_dump doing the wrong thing is bad, but the SQL example
doesn't look broken if you ignore pg_dump. I have a feeling that the
source of the awkwardness here is that one SQL command is creating two
objects, and unlike the case of a table and a TOAST table, one is not
an implementation detail of the other or clearly subordinate to the
other. But how does that prevent us from making pg_dump restore the
ownership and permissions on each separately? If ownership is a
problem, aren't permissions also?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-15 19:28 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2024-01-15 19:28 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: [email protected]
Robert Haas <[email protected]> writes:
> On Mon, Jan 15, 2024 at 1:27 PM Tom Lane <[email protected]> wrote:
>> That's pretty broken, isn't it? joe would own the multirange if he'd
>> created the range to start with. Even if you think the ownerships
>> ideally should be separable, this behavior causes existing pg_dump
>> files to restore incorrectly, because pg_dump assumes it need not emit
>> any commands about the multirange.
> I agree that pg_dump doing the wrong thing is bad, but the SQL example
> doesn't look broken if you ignore pg_dump.
I'm reasoning by analogy to array types, which are automatically
created and automatically updated to keep the same ownership
etc. properties as their base type. To the extent that multirange
types don't act exactly like that, I say it's a bug/oversight in the
multirange patch. So I think this is a backend bug, not a pg_dump
bug.
> I have a feeling that the
> source of the awkwardness here is that one SQL command is creating two
> objects, and unlike the case of a table and a TOAST table, one is not
> an implementation detail of the other or clearly subordinate to the
> other.
How is a multirange not subordinate to the underlying range type?
It can't exist without it, and we automatically create it without
any further information when you make the range type. That smells
a lot like the way we handle array types. The array behavior is of
very long standing and surprises nobody.
> But how does that prevent us from making pg_dump restore the
> ownership and permissions on each separately? If ownership is a
> problem, aren't permissions also?
Probably, and I wouldn't be surprised if we've also failed to make
multiranges follow arrays in the permissions department. An
array type can't have an ACL of its own, IIRC.
regards, tom lane
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-15 20:01 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2024-01-15 20:01 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
On Mon, Jan 15, 2024 at 2:28 PM Tom Lane <[email protected]> wrote:
> I'm reasoning by analogy to array types, which are automatically
> created and automatically updated to keep the same ownership
> etc. properties as their base type. To the extent that multirange
> types don't act exactly like that, I say it's a bug/oversight in the
> multirange patch. So I think this is a backend bug, not a pg_dump
> bug.
Oh...
Well, I guess maybe I'm just clueless. I thought that the range and
multirange were two essentially independent objects being created by
the same command. But I haven't studied the implementation so maybe
I'm completely wrong.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-16 16:46 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2024-01-16 16:46 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: [email protected]
Robert Haas <[email protected]> writes:
> On Mon, Jan 15, 2024 at 2:28 PM Tom Lane <[email protected]> wrote:
>> I'm reasoning by analogy to array types, which are automatically
>> created and automatically updated to keep the same ownership
>> etc. properties as their base type. To the extent that multirange
>> types don't act exactly like that, I say it's a bug/oversight in the
>> multirange patch. So I think this is a backend bug, not a pg_dump
>> bug.
> Well, I guess maybe I'm just clueless. I thought that the range and
> multirange were two essentially independent objects being created by
> the same command. But I haven't studied the implementation so maybe
> I'm completely wrong.
They're by no means independent. What would it mean to have a
multirange without the underlying range type? Also, we already
treat the multirange as dependent for some things:
d=# create type varbitrange as range (subtype = varbit);
CREATE TYPE
d=# \dT
List of data types
Schema | Name | Description
--------+------------------+-------------
public | varbitmultirange |
public | varbitrange |
(2 rows)
d=# drop type varbitmultirange;
ERROR: cannot drop type varbitmultirange because type varbitrange requires it
HINT: You can drop type varbitrange instead.
d=# drop type varbitrange restrict;
DROP TYPE
d=# \dT
List of data types
Schema | Name | Description
--------+------+-------------
(0 rows)
So I think we're looking at a half-baked dependency design,
not two independent objects.
regards, tom lane
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-01-16 17:06 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2024-01-16 17:06 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
On Tue, Jan 16, 2024 at 11:46 AM Tom Lane <[email protected]> wrote:
> They're by no means independent. What would it mean to have a
> multirange without the underlying range type?
It would mean just that - no more, and no less. If it's possible to
imagine a data type that stores pairs of values from the underlying
data type with the constraint that the first is less than the second,
plus the ability to specify inclusive or exclusive bounds and the
ability to have infinite bounds, then it's equally possible to imagine
a data type that represents a set of such ranges such that no two
ranges in the set overlap. And you need not imagine that the former
data type must exist in order for the latter to exist. Theoretically,
they're just two different data types that somebody could decide to
create.
> Also, we already
> treat the multirange as dependent for some things:
But this seems like an entirely valid point.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: ALTER TYPE OWNER fails to recurse to multirange
@ 2024-02-12 22:55 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Tom Lane @ 2024-02-12 22:55 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: [email protected]
Robert Haas <[email protected]> writes:
> On Tue, Jan 16, 2024 at 11:46 AM Tom Lane <[email protected]> wrote:
>> Also, we already
>> treat the multirange as dependent for some things:
> But this seems like an entirely valid point.
Yeah, it's a bit of a muddle. But there is no syntax for making
a standalone multirange type, so it seems to me that we've mostly
determined that multiranges are dependent types. There are just
a few places that didn't get the word.
Attached is a proposed patch to enforce that ownership and permissions
of a multirange are those of the underlying range type, in ways
parallel to how we treat array types. This is all that I found by
looking for calls to IsTrueArrayType(). It's possible that there's
some dependent-type behavior somewhere that isn't conditioned on that,
but I can't think of a good way to search.
If we don't do this, then we need some hacking in pg_dump to get it
to save and restore these properties of multiranges, so it's not like
the status quo is acceptable.
I'd initially thought that perhaps we could back-patch parts of this,
but now I'm not sure; it seems like these behaviors are a bit
intertwined. Given the lack of field complaints I'm inclined to leave
things alone in the back branches.
regards, tom lane
Attachments:
[text/x-diff] v1-fix-multirange-ownership-permissions.patch (14.0K, ../../[email protected]/2-v1-fix-multirange-ownership-permissions.patch)
download | inline diff:
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 590affb79a..591e767cce 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2447,11 +2447,17 @@ ExecGrant_Type_check(InternalGrant *istmt, HeapTuple tuple)
pg_type_tuple = (Form_pg_type) GETSTRUCT(tuple);
+ /* Disallow GRANT on dependent types */
if (IsTrueArrayType(pg_type_tuple))
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("cannot set privileges of array types"),
errhint("Set the privileges of the element type instead.")));
+ if (pg_type_tuple->typtype == TYPTYPE_MULTIRANGE)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot set privileges of multirange types"),
+ errhint("Set the privileges of the range type instead.")));
/* Used GRANT DOMAIN on a non-domain? */
if (istmt->objtype == OBJECT_DOMAIN &&
@@ -3806,6 +3812,35 @@ pg_type_aclmask_ext(Oid type_oid, Oid roleid, AclMode mask, AclMaskHow how,
typeForm = (Form_pg_type) GETSTRUCT(tuple);
}
+ /*
+ * Likewise, multirange types don't manage their own permissions; consult
+ * the associated range type. (Note we must do this after the array step
+ * to get the right answer for arrays of multiranges.)
+ */
+ if (typeForm->typtype == TYPTYPE_MULTIRANGE)
+ {
+ Oid rangetype = get_multirange_range(type_oid);
+
+ ReleaseSysCache(tuple);
+
+ tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(rangetype));
+ if (!HeapTupleIsValid(tuple))
+ {
+ if (is_missing != NULL)
+ {
+ /* return "no privileges" instead of throwing an error */
+ *is_missing = true;
+ return 0;
+ }
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("type with OID %u does not exist",
+ rangetype)));
+ }
+ typeForm = (Form_pg_type) GETSTRUCT(tuple);
+ }
+
/*
* Now get the type's owner and ACL from the tuple
*/
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index d7167108fb..fe47be38d0 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -326,14 +326,15 @@ TypeCreate(Oid newTypeOid,
errmsg("fixed-size types must have storage PLAIN")));
/*
- * This is a dependent type if it's an implicitly-created array type, or
- * if it's a relation rowtype that's not a composite type. For such types
- * we'll leave the ACL empty, and we'll skip creating some dependency
- * records because there will be a dependency already through the
- * depended-on type or relation. (Caution: this is closely intertwined
- * with some behavior in GenerateTypeDependencies.)
+ * This is a dependent type if it's an implicitly-created array type or
+ * multirange type, or if it's a relation rowtype that's not a composite
+ * type. For such types we'll leave the ACL empty, and we'll skip
+ * creating some dependency records because there will be a dependency
+ * already through the depended-on type or relation. (Caution: this is
+ * closely intertwined with some behavior in GenerateTypeDependencies.)
*/
isDependentType = isImplicitArray ||
+ typeType == TYPTYPE_MULTIRANGE ||
(OidIsValid(relationOid) && relationKind != RELKIND_COMPOSITE_TYPE);
/*
@@ -534,8 +535,9 @@ TypeCreate(Oid newTypeOid,
* relationKind and isImplicitArray are likewise somewhat expensive to deduce
* from the tuple, so we make callers pass those (they're not optional).
*
- * isDependentType is true if this is an implicit array or relation rowtype;
- * that means it doesn't need its own dependencies on owner etc.
+ * isDependentType is true if this is an implicit array, multirange, or
+ * relation rowtype; that means it doesn't need its own dependencies on owner
+ * etc.
*
* We make an extension-membership dependency if we're in an extension
* script and makeExtensionDep is true (and isDependentType isn't true).
@@ -601,18 +603,23 @@ GenerateTypeDependencies(HeapTuple typeTuple,
* Make dependencies on namespace, owner, ACL, extension.
*
* Skip these for a dependent type, since it will have such dependencies
- * indirectly through its depended-on type or relation.
+ * indirectly through its depended-on type or relation. An exception is
+ * that multiranges need their own namespace dependency, since we don't
+ * force them to be in the same schema as their range type.
*/
- /* placeholder for all normal dependencies */
+ /* collects normal dependencies for bulk recording */
addrs_normal = new_object_addresses();
- if (!isDependentType)
+ if (!isDependentType || typeForm->typtype == TYPTYPE_MULTIRANGE)
{
ObjectAddressSet(referenced, NamespaceRelationId,
typeForm->typnamespace);
- recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
+ add_exact_object_address(&referenced, addrs_normal);
+ }
+ if (!isDependentType)
+ {
recordDependencyOnOwner(TypeRelationId, typeObjectId,
typeForm->typowner);
@@ -727,6 +734,16 @@ GenerateTypeDependencies(HeapTuple typeTuple,
recordDependencyOn(&myself, &referenced,
isImplicitArray ? DEPENDENCY_INTERNAL : DEPENDENCY_NORMAL);
}
+
+ /*
+ * Note: you might expect that we should record an internal dependency of
+ * a multirange on its range type here, by analogy with the cases above.
+ * But instead, that is done by RangeCreate(), which also handles
+ * recording of other range-type-specific dependencies. That's pretty
+ * bogus. It's okay for now, because there are no cases where we need to
+ * regenerate the dependencies of a range or multirange type. But someday
+ * we might need to move that logic here to allow such regeneration.
+ */
}
/*
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index a400fb39f6..e0275e5fe9 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3647,6 +3647,8 @@ RenameType(RenameStmt *stmt)
errhint("You can alter type %s, which will alter the array type as well.",
format_type_be(typTup->typelem))));
+ /* we do allow separate renaming of multirange types, though */
+
/*
* If type is composite we need to rename associated pg_class entry too.
* RenameRelationInternal will call RenameTypeInternal automatically.
@@ -3730,6 +3732,21 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
errhint("You can alter type %s, which will alter the array type as well.",
format_type_be(typTup->typelem))));
+ /* don't allow direct alteration of multirange types, either */
+ if (typTup->typtype == TYPTYPE_MULTIRANGE)
+ {
+ Oid rangetype = get_multirange_range(typeOid);
+
+ /* We don't expect get_multirange_range to fail, but cope if so */
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot alter multirange type %s",
+ format_type_be(typeOid)),
+ OidIsValid(rangetype) ?
+ errhint("You can alter type %s, which will alter the multirange type as well.",
+ format_type_be(rangetype)) : 0));
+ }
+
/*
* If the new owner is the same as the existing owner, consider the
* command to have succeeded. This is for dump restoration purposes.
@@ -3769,13 +3786,13 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
/*
* AlterTypeOwner_oid - change type owner unconditionally
*
- * This function recurses to handle a pg_class entry, if necessary. It
- * invokes any necessary access object hooks. If hasDependEntry is true, this
- * function modifies the pg_shdepend entry appropriately (this should be
- * passed as false only for table rowtypes and array types).
+ * This function recurses to handle dependent types (arrays and multiranges).
+ * It invokes any necessary access object hooks. If hasDependEntry is true,
+ * this function modifies the pg_shdepend entry appropriately (this should be
+ * passed as false only for table rowtypes and dependent types).
*
* This is used by ALTER TABLE/TYPE OWNER commands, as well as by REASSIGN
- * OWNED BY. It assumes the caller has done all needed check.
+ * OWNED BY. It assumes the caller has done all needed checks.
*/
void
AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
@@ -3815,7 +3832,7 @@ AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
* AlterTypeOwnerInternal - bare-bones type owner change.
*
* This routine simply modifies the owner of a pg_type entry, and recurses
- * to handle a possible array type.
+ * to handle any dependent types.
*/
void
AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
@@ -3865,6 +3882,19 @@ AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
if (OidIsValid(typTup->typarray))
AlterTypeOwnerInternal(typTup->typarray, newOwnerId);
+ /* If it is a range type, update the associated multirange too */
+ if (typTup->typtype == TYPTYPE_RANGE)
+ {
+ Oid multirange_typeid = get_range_multirange(typeOid);
+
+ if (!OidIsValid(multirange_typeid))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("could not find multirange type for data type %s",
+ format_type_be(typeOid))));
+ AlterTypeOwnerInternal(multirange_typeid, newOwnerId);
+ }
+
/* Clean up */
table_close(rel, RowExclusiveLock);
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 348748bae5..f40bc759c5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1868,7 +1868,7 @@ selectDumpableType(TypeInfo *tyinfo, Archive *fout)
return;
}
- /* skip auto-generated array types */
+ /* skip auto-generated array and multirange types */
if (tyinfo->isArray || tyinfo->isMultirange)
{
tyinfo->dobj.objType = DO_DUMMY_TYPE;
@@ -1876,8 +1876,8 @@ selectDumpableType(TypeInfo *tyinfo, Archive *fout)
/*
* Fall through to set the dump flag; we assume that the subsequent
* rules will do the same thing as they would for the array's base
- * type. (We cannot reliably look up the base type here, since
- * getTypes may not have processed it yet.)
+ * type or multirange's range type. (We cannot reliably look up the
+ * base type here, since getTypes may not have processed it yet.)
*/
}
@@ -5770,7 +5770,7 @@ getTypes(Archive *fout, int *numTypes)
else
tyinfo[i].isArray = false;
- if (tyinfo[i].typtype == 'm')
+ if (tyinfo[i].typtype == TYPTYPE_MULTIRANGE)
tyinfo[i].isMultirange = true;
else
tyinfo[i].isMultirange = false;
diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out
index 6d9498cdd1..5a9ee5d2cd 100644
--- a/src/test/regress/expected/dependency.out
+++ b/src/test/regress/expected/dependency.out
@@ -144,7 +144,6 @@ owner of sequence deptest_a_seq
owner of table deptest
owner of function deptest_func()
owner of type deptest_enum
-owner of type deptest_multirange
owner of type deptest_range
owner of table deptest2
owner of sequence ss1
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index 9808587532..002775b9eb 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -3115,6 +3115,36 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
drop type textrange1;
drop type textrange2;
--
+-- Multiranges don't have their own ownership or permissions.
+--
+create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
+create role regress_multirange_owner;
+alter type multitextrange1 owner to regress_multirange_owner; -- fail
+ERROR: cannot alter multirange type multitextrange1
+HINT: You can alter type textrange1, which will alter the multirange type as well.
+alter type textrange1 owner to regress_multirange_owner;
+set role regress_multirange_owner;
+revoke usage on type multitextrange1 from public; -- fail
+ERROR: cannot set privileges of multirange types
+HINT: Set the privileges of the range type instead.
+revoke usage on type textrange1 from public;
+\dT+ *textrange1*
+ List of data types
+ Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
+--------+-----------------+-----------------+------+----------+--------------------------+-----------------------------------------------------+-------------
+ public | multitextrange1 | multitextrange1 | var | | regress_multirange_owner | |
+ public | textrange1 | textrange1 | var | | regress_multirange_owner | regress_multirange_owner=U/regress_multirange_owner |
+(2 rows)
+
+create temp table test1(f1 multitextrange1);
+revoke usage on type textrange1 from regress_multirange_owner;
+create temp table test2(f1 multitextrange1); -- fail
+ERROR: permission denied for type multitextrange1
+drop table test1;
+drop type textrange1;
+reset role;
+drop role regress_multirange_owner;
+--
-- Test polymorphic type system
--
create function anyarray_anymultirange_func(a anyarray, r anymultirange)
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index cadf312031..197e3af29c 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -700,6 +700,27 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
drop type textrange1;
drop type textrange2;
+--
+-- Multiranges don't have their own ownership or permissions.
+--
+create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
+create role regress_multirange_owner;
+
+alter type multitextrange1 owner to regress_multirange_owner; -- fail
+alter type textrange1 owner to regress_multirange_owner;
+set role regress_multirange_owner;
+revoke usage on type multitextrange1 from public; -- fail
+revoke usage on type textrange1 from public;
+\dT+ *textrange1*
+create temp table test1(f1 multitextrange1);
+revoke usage on type textrange1 from regress_multirange_owner;
+create temp table test2(f1 multitextrange1); -- fail
+
+drop table test1;
+drop type textrange1;
+reset role;
+drop role regress_multirange_owner;
+
--
-- Test polymorphic type system
--
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2024-02-12 22:55 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v60 01/17] dshash: Add sequential scan support. Kyotaro Horiguchi <[email protected]>
2022-03-03 02:02 [PATCH v65 01/11] dshash: Add sequential scan support. Kyotaro Horiguchi <[email protected]>
2022-05-11 05:28 ` Re: Unstable tests for recovery conflict handling Thomas Munro <[email protected]>
2022-07-26 17:57 ` Re: Unstable tests for recovery conflict handling Tom Lane <[email protected]>
2022-07-26 18:16 ` Re: Unstable tests for recovery conflict handling Andres Freund <[email protected]>
2022-07-26 18:30 ` Re: Unstable tests for recovery conflict handling Tom Lane <[email protected]>
2022-07-26 20:03 ` Re: Unstable tests for recovery conflict handling Andres Freund <[email protected]>
2022-08-03 17:57 ` Re: Unstable tests for recovery conflict handling Andres Freund <[email protected]>
2022-08-03 20:33 ` Re: Unstable tests for recovery conflict handling Robert Haas <[email protected]>
2022-08-03 22:07 ` Re: Unstable tests for recovery conflict handling Andres Freund <[email protected]>
2024-01-15 18:27 ALTER TYPE OWNER fails to recurse to multirange Tom Lane <[email protected]>
2024-01-15 19:17 ` Re: ALTER TYPE OWNER fails to recurse to multirange Robert Haas <[email protected]>
2024-01-15 19:28 ` Re: ALTER TYPE OWNER fails to recurse to multirange Tom Lane <[email protected]>
2024-01-15 20:01 ` Re: ALTER TYPE OWNER fails to recurse to multirange Robert Haas <[email protected]>
2024-01-16 16:46 ` Re: ALTER TYPE OWNER fails to recurse to multirange Tom Lane <[email protected]>
2024-01-16 17:06 ` Re: ALTER TYPE OWNER fails to recurse to multirange Robert Haas <[email protected]>
2024-02-12 22:55 ` Re: ALTER TYPE OWNER fails to recurse to multirange Tom Lane <[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