public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix misuses of RelationNeedsWAL
2+ messages / 2 participants
[nested] [flat]
* [PATCH] Fix misuses of RelationNeedsWAL
@ 2021-01-13 06:52 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-13 06:52 UTC (permalink / raw)
The definition of the macro RelationNeedsWAL has been changed by
c6b92041d3 to include conditions related to the WAL-skip optimization
but some uses of the macro are not relevant to the optimization. That
misuses are harmless for now as they are only executed while wal_level
>= replica or WAL-skipping is inactive. However, this should be
corrected to prevent future hazard.
---
src/backend/catalog/pg_publication.c | 2 +-
src/backend/optimizer/util/plancat.c | 2 +-
src/include/utils/rel.h | 15 +++++++++++----
src/include/utils/snapmgr.h | 2 +-
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c64e1..f3060a4cf1 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -67,7 +67,7 @@ check_publication_add_relation(Relation targetrel)
errdetail("System tables cannot be added to publications.")));
/* UNLOGGED and TEMP relations cannot be part of publication. */
- if (!RelationNeedsWAL(targetrel))
+ if (!RelationIsWalLogged(targetrel))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("table \"%s\" cannot be replicated",
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index da322b453e..0500efcdb9 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -126,7 +126,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
relation = table_open(relationObjectId, NoLock);
/* Temporary and unlogged relations are inaccessible during recovery. */
- if (!RelationNeedsWAL(relation) && RecoveryInProgress())
+ if (!RelationIsWalLogged(relation) && RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary or unlogged relations during recovery")));
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 10b63982c0..810806a542 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -552,16 +552,23 @@ typedef struct ViewOptions
(relation)->rd_smgr->smgr_targblock = (targblock); \
} while (0)
+/*
+ * RelationIsPermanent
+ * True if relation is WAL-logged.
+ */
+#define RelationIsWalLogged(relation) \
+ ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+
/*
* RelationNeedsWAL
- * True if relation needs WAL.
+ * True if relation needs WAL at the time.
*
* Returns false if wal_level = minimal and this relation is created or
* truncated in the current transaction. See "Skipping WAL for New
* RelFileNode" in src/backend/access/transam/README.
*/
#define RelationNeedsWAL(relation) \
- ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT && \
+ (RelationIsWalLogged(relation) && \
(XLogIsNeeded() || \
(relation->rd_createSubid == InvalidSubTransactionId && \
relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))
@@ -619,7 +626,7 @@ typedef struct ViewOptions
*/
#define RelationIsAccessibleInLogicalDecoding(relation) \
(XLogLogicalInfoActive() && \
- RelationNeedsWAL(relation) && \
+ RelationIsWalLogged(relation) && \
(IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation)))
/*
@@ -635,7 +642,7 @@ typedef struct ViewOptions
*/
#define RelationIsLogicallyLogged(relation) \
(XLogLogicalInfoActive() && \
- RelationNeedsWAL(relation) && \
+ RelationIsWalLogged(relation) && \
!IsCatalogRelation(relation))
/* routines in utils/cache/relcache.c */
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 579be352c5..7be922a9f1 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -37,7 +37,7 @@
*/
#define RelationAllowsEarlyPruning(rel) \
( \
- RelationNeedsWAL(rel) \
+ RelationIsWalLogged(rel) \
&& !IsCatalogRelation(rel) \
&& !RelationIsAccessibleInLogicalDecoding(rel) \
)
--
2.27.0
----Next_Part(Wed_Jan_13_16_07_05_2021_621)----
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION
@ 2024-07-29 08:24 Андрей Рачицкий <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Андрей Рачицкий @ 2024-07-29 08:24 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; Robert Haas <[email protected]>
Hi, Tom! Thank you for work on the subject. After applying patch, problem is no longer reproducible.
---
Best regards,
Andrey Rachitskiy
Postgres Professional: http://postgrespro.com
>Суббота, 20 июля 2024, 0:04 +05:00 от Tom Lane <[email protected]>:
>
>PG Bug reporting form < [email protected] > writes:
>> postgres=# BEGIN;
>> BEGIN
>> postgres=*# CREATE USER regress_priv_user8;
>> CREATE ROLE
>> postgres=*# SET SESSION AUTHORIZATION regress_priv_user8;
>> SET
>> postgres=*> SET LOCAL debug_parallel_query = 1;
>> SET
>> postgres=*> \dt+;
>> ERROR: 22023: role "regress_priv_user8" does not exist
>> CONTEXT: while setting parameter "session_authorization" to "regress_priv_user8"
>> parallel worker
>So this has exactly nothing to do with \dt+; any parallel query
>will hit it. The problem is that parallel workers do
>RestoreGUCState() before they've restored the leader's snapshot.
>Thus, in this example where session_authorization refers to an
>uncommitted pg_authid entry, the workers don't see that entry.
>It seems likely that similar failures are possible with other
>GUCs that perform catalog lookups.
>
>I experimented with two different ways to fix this:
>
>1. Run RestoreGUCState() outside a transaction, thus preventing
>catalog lookups. Assume that individual GUC check hooks that
>would wish to do a catalog lookup will cope. Unfortunately,
>some of them don't and would need fixed; check_role and
>check_session_authorization for two.
>
>2. Delay RestoreGUCState() into the parallel worker's main
>transaction, after we've restored the leader's snapshot.
>This turns out to break a different set of check hooks, notably
>check_transaction_deferrable.
>
>I think that the blast radius of option 2 is probably smaller than
>option 1's, because it should only matter to check hooks that think
>they should run before the transaction has set a snapshot, and there
>are few of those. check_transaction_read_only already had a guard,
>but I added similar ones to check_transaction_isolation and
>check_transaction_deferrable.
>
>The attached draft patch also contains changes to prevent
>check_session_authorization from doing anything during parallel
>worker startup. That's left over from experimenting with option 1,
>and is not strictly necessary with option 2. I left it in anyway
>because it's saving some unnecessary work. (For some reason,
>check_role seems not to fail if you modify the test case to use
>SET ROLE. I did not figure out why not. I kind of want to modify
>check_role to be a no-op too when InitializingParallelWorker,
>but did not touch that here pending more investigation.)
>
>Another thing I'm wondering about is whether to postpone
>RestoreLibraryState similarly. Its current placement is said
>to be "before restoring GUC values", so it looks a little out
>of place now. Moving it into the main transaction would save
>one StartTransactionCommand/CommitTransactionCommand pair
>during parallel worker start, which is worth something.
>But I think the real argument for it is that if any loaded
>libraries try to do catalog lookups during load, we'd rather
>that they see the same catalog state the leader does.
>As against that, it feels like there's a nonzero risk of
>breaking some third-party code if we move that call.
>
>Thoughts?
>
>regards, tom lane
>
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2024-07-29 08:24 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-13 06:52 [PATCH] Fix misuses of RelationNeedsWAL Kyotaro Horiguchi <[email protected]>
2024-07-29 08:24 Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION Андрей Рачицкий <[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