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: Make COPY format extendable: Extract COPY TO format implementations @ 2025-04-24 06:44 Masahiko Sawada <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Masahiko Sawada @ 2025-04-24 06:44 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Fri, Apr 4, 2025 at 1:38 AM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <CAD21AoDOcYah-nREv09BB3ZoB-k+Yf1XUfJcDMoq=LLvV1v75w@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 31 Mar 2025 12:35:23 -0700, > Masahiko Sawada <[email protected]> wrote: > > > Most of the queries under test_copy_format/sql verifies the input > > patterns of the FORMAT option. I find that the regression tests > > included in that directory probably should focus on testing new > > callback APIs and some regression tests for FORMAT option handling can > > be moved into the normal regression test suite (e.g., in copy.sql or a > > new file like copy_format.sql). IIUC testing for invalid input > > patterns can be done even without creating artificial wrong handler > > functions. > > Can we clarify what should we do for the next patch set > before we create the next patch set? Are the followings > correct? > > 1. Move invalid input patterns in > src/test/modules/test_copy_format/sql/invalid.sql to > src/test/regress/sql/copy.sql as much as possible. > * We can do only 4 patterns in 16 patterns. > * Other tests in > src/test/modules/test_copy_format/sql/*.sql depend on > custom COPY handler for test. So we can't move to > src/test/regress/sql/copy.sql. > 2. Create > src/test/modules/test_copy_format/sql/test_copy_format.sql > and move all contents in existing *.sql to the file Agreed. > > >> We can do it but I suggest that we do it as a refactoring > >> (or cleanup) in a separated patch for easy to review. > > > > I think that csv_mode and binary are used mostly in > > ProcessCopyOptions() so probably we can use local variables for that. > > I find there are two other places where to use csv_mode: > > NextCopyFromRawFields() and CopyToTextLikeStart(). I think we can > > simply check the handler function's OID there, or we can define macros > > like COPY_FORMAT_IS_TEXT/CSV/BINARY checking the OID and use them > > there. > > We need this change for "ready for merge", right? I think so. > Can we clarify items should be resolved for "ready for > merge"? > > Are the followings correct? > > 1. Move invalid input patterns in > src/test/modules/test_copy_format/sql/invalid.sql to > src/test/regress/sql/copy.sql as much as possible. > 2. Create > src/test/modules/test_copy_format/sql/test_copy_format.sql > and move all contents in existing *.sql to the file. > 3. Add comments what the tests expect to > src/test/modules/test_copy_format/sql/test_copy_format.sql. > 4. Remove CopyFormatOptions::{binary,csv_mode}. Agreed with the above items. > 5. Squash the "Support custom format" patch and the "Define > handler functions for built-in formats" patch. > * Could you do it when you push it? Or is it required for > "ready for merge"? Let's keep them for now. > 6. Use handler OID for detecting the default built-in format > instead of comparing the given format as string. > 7. Update documentation. Agreed. > > There are 3 unconfirmed suggested changes for tests in: > https://www.postgresql.org/message-id/20250330.113126.433742864258096312.kou%40clear-code.com > > Here are my opinions for them: > > > 1.: There is no difference between single-quoting and > > double-quoting here. Because the information what quote > > was used for the given FORMAT value isn't remained > > here. Should we update gram.y? > > > > 2.: I don't have a strong opinion for it. If nobody objects > > it, I'll remove them. > > > > 3.: I don't have a strong opinion for it. If nobody objects > > it, I'll remove them. > > Is the 1. required for "ready for merge"? If so, is there > any suggestion? I don't have a strong opinion for it. > > If there are no more opinions for 2. and 3., I'll remove > them. Agreed. I think we would still need some rounds of reviews but the patch is getting in good shape. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2025-04-24 06:44 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]> 2025-04-24 06:44 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[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