public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] Do not use RelationNeedsWAL to identify relation persistence
7+ messages / 4 participants
[nested] [flat]
* [PATCH v2] Do not use RelationNeedsWAL to identify relation persistence
@ 2021-01-18 05:47 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-18 05:47 UTC (permalink / raw)
RelationNeedsWAL() may return false for a permanent relation when
wal_level=minimal and the relation is created or truncated in the
current transaction. Directly examine relpersistence instead of using
the function to know relation persistence.
---
src/backend/catalog/pg_publication.c | 2 +-
src/backend/optimizer/util/plancat.c | 3 ++-
src/include/utils/rel.h | 9 ++++++++-
src/include/utils/snapmgr.h | 2 +-
src/test/subscription/t/001_rep_changes.pl | 20 +++++++++++++++++++-
5 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c64e1..84d2efcfd2 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 (targetrel->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
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..177e6e336a 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -126,7 +126,8 @@ 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 (relation->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT &&
+ 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..1e2c11fdd3 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -552,9 +552,16 @@ 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
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 579be352c5..35acccb29c 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -37,7 +37,7 @@
*/
#define RelationAllowsEarlyPruning(rel) \
( \
- RelationNeedsWAL(rel) \
+ rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT \
&& !IsCatalogRelation(rel) \
&& !RelationIsAccessibleInLogicalDecoding(rel) \
)
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0680f44a1a..ed9b48e8bc 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 24;
# Initialize publisher node
my $node_publisher = get_new_node('publisher');
@@ -358,3 +358,21 @@ is($result, qq(0), 'check replication origin was dropped on subscriber');
$node_subscriber->stop('fast');
$node_publisher->stop('fast');
+
+#
+# CREATE PUBLICATION while wal_level=mimal should succeed, with a WARNING
+$node_publisher->append_conf(
+ 'postgresql.conf', qq(
+wal_level=minimal
+max_wal_senders=0
+));
+$node_publisher->start;
+($result, my $retout, my $reterr) = $node_publisher->psql(
+ 'postgres', qq{
+BEGIN;
+CREATE TABLE skip_wal();
+CREATE PUBLICATION tap_pub2 FOR TABLE skip_wal;
+ROLLBACK;
+});
+ok($reterr =~ m/WARNING: wal_level is insufficient to publish logical changes/,
+ 'test CREATE PUBLICATION can be done while wal_leve=minimal');
--
2.27.0
----Next_Part(Mon_Jan_18_15_08_38_2021_069)----
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
@ 2023-11-07 21:00 Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2023-11-07 21:00 UTC (permalink / raw)
To: Euler Taveira <[email protected]>; [email protected]; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On 23.10.23 05:53, Euler Taveira wrote:
> Let's continue with the bike shedding... I agree with Peter E that this name
> does not express what this tool is. At the moment, it only have one action:
> create. If I have to suggest other actions I would say that it could support
> switchover option too (that removes the infrastructure created by this
> tool).
> If we decide to keep this name, it should be a good idea to add an option to
> indicate what action it is executing (similar to pg_recvlogical) as
> suggested
> by Peter.
Speaking of which, would it make sense to put this tool (whatever the
name) into the pg_basebackup directory? It's sort of related, and it
also shares some code.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
@ 2023-11-07 23:12 ` Michael Paquier <[email protected]>
2023-11-08 12:50 ` Re: speed up a logical replica setup Euler Taveira <[email protected]>
2023-11-09 14:41 ` Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Michael Paquier @ 2023-11-07 23:12 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected]; Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On Tue, Nov 07, 2023 at 10:00:39PM +0100, Peter Eisentraut wrote:
> Speaking of which, would it make sense to put this tool (whatever the name)
> into the pg_basebackup directory? It's sort of related, and it also shares
> some code.
I've read the patch, and the additions to streamutil.h and
streamutil.c make it kind of natural to have it sit in pg_basebackup/.
There's pg_recvlogical already there. I am wondering about two
things, though:
- Should the subdirectory pg_basebackup be renamed into something more
generic at this point? All these things are frontend tools that deal
in some way with the replication protocol to do their work. Say
a replication_tools?
- And if it would be better to refactor some of the code generic to
all these streaming tools to fe_utils. What makes streamutil.h a bit
less pluggable are all its extern variables to control the connection,
but perhaps that can be an advantage, as well, in some cases.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
@ 2023-11-08 12:50 ` Euler Taveira <[email protected]>
2023-11-08 23:13 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Euler Taveira @ 2023-11-08 12:50 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: [email protected]; Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On Tue, Nov 7, 2023, at 8:12 PM, Michael Paquier wrote:
> On Tue, Nov 07, 2023 at 10:00:39PM +0100, Peter Eisentraut wrote:
> > Speaking of which, would it make sense to put this tool (whatever the name)
> > into the pg_basebackup directory? It's sort of related, and it also shares
> > some code.
I used the CreateReplicationSlot() from streamutil.h but decided to use the
CREATE_REPLICATION_SLOT command directly because it needs the LSN as output. As
you noticed at that time I wouldn't like a dependency in the pg_basebackup
header files; if we move this binary to base backup directory, it seems natural
to refactor the referred function and use it.
> I've read the patch, and the additions to streamutil.h and
> streamutil.c make it kind of natural to have it sit in pg_basebackup/.
> There's pg_recvlogical already there. I am wondering about two
> things, though:
> - Should the subdirectory pg_basebackup be renamed into something more
> generic at this point? All these things are frontend tools that deal
> in some way with the replication protocol to do their work. Say
> a replication_tools?
It is a good fit for this tool since it is another replication tool. I also
agree with the directory renaming; it seems confusing that the directory has
the same name as one binary but also contains other related binaries in it.
> - And if it would be better to refactor some of the code generic to
> all these streaming tools to fe_utils. What makes streamutil.h a bit
> less pluggable are all its extern variables to control the connection,
> but perhaps that can be an advantage, as well, in some cases.
I like it. There are common functions such as GetConnection(),
CreateReplicationSlot(), DropReplicationSlot() and RunIdentifySystem() that is
used by all of these replication tools. We can move the extern variables into
parameters to have a pluggable streamutil.h.
--
Euler Taveira
EDB https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
2023-11-08 12:50 ` Re: speed up a logical replica setup Euler Taveira <[email protected]>
@ 2023-11-08 23:13 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2023-11-08 23:13 UTC (permalink / raw)
To: Euler Taveira <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; [email protected]; Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On Wed, Nov 08, 2023 at 09:50:47AM -0300, Euler Taveira wrote:
> On Tue, Nov 7, 2023, at 8:12 PM, Michael Paquier wrote:
> I used the CreateReplicationSlot() from streamutil.h but decided to use the
> CREATE_REPLICATION_SLOT command directly because it needs the LSN as output. As
> you noticed at that time I wouldn't like a dependency in the pg_basebackup
> header files; if we move this binary to base backup directory, it seems natural
> to refactor the referred function and use it.
Right. That should be OK to store that in an optional XLogRecPtr
pointer, aka by letting the option to pass NULL as argument of the
function if the caller needs nothing.
>> I've read the patch, and the additions to streamutil.h and
>> streamutil.c make it kind of natural to have it sit in pg_basebackup/.
>> There's pg_recvlogical already there. I am wondering about two
>> things, though:
>> - Should the subdirectory pg_basebackup be renamed into something more
>> generic at this point? All these things are frontend tools that deal
>> in some way with the replication protocol to do their work. Say
>> a replication_tools?
>
> It is a good fit for this tool since it is another replication tool. I also
> agree with the directory renaming; it seems confusing that the directory has
> the same name as one binary but also contains other related binaries in it.
Or cluster_tools? Or stream_tools? replication_tools may be OK, but
I have a bad sense in naming new things around here. So if anybody
has a better idea, feel free..
>> - And if it would be better to refactor some of the code generic to
>> all these streaming tools to fe_utils. What makes streamutil.h a bit
>> less pluggable are all its extern variables to control the connection,
>> but perhaps that can be an advantage, as well, in some cases.
>
> I like it. There are common functions such as GetConnection(),
> CreateReplicationSlot(), DropReplicationSlot() and RunIdentifySystem() that is
> used by all of these replication tools. We can move the extern variables into
> parameters to have a pluggable streamutil.h.
And perhaps RetrieveWalSegSize() as well as GetSlotInformation().
These kick replication commands.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
@ 2023-11-09 14:41 ` Peter Eisentraut <[email protected]>
2023-11-09 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2023-11-09 14:41 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected]; Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On 08.11.23 00:12, Michael Paquier wrote:
> - Should the subdirectory pg_basebackup be renamed into something more
> generic at this point? All these things are frontend tools that deal
> in some way with the replication protocol to do their work. Say
> a replication_tools?
Seems like unnecessary churn. Nobody has complained about any of the
other tools in there.
> - And if it would be better to refactor some of the code generic to
> all these streaming tools to fe_utils. What makes streamutil.h a bit
> less pluggable are all its extern variables to control the connection,
> but perhaps that can be an advantage, as well, in some cases.
Does anyone outside of pg_basebackup + existing friends + new friend
need that? Seems like extra complications.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: speed up a logical replica setup
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
2023-11-09 14:41 ` Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
@ 2023-11-09 23:12 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2023-11-09 23:12 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected]; Amit Kapila <[email protected]>; Andres Freund <[email protected]>
On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> On 08.11.23 00:12, Michael Paquier wrote:
>> - Should the subdirectory pg_basebackup be renamed into something more
>> generic at this point? All these things are frontend tools that deal
>> in some way with the replication protocol to do their work. Say
>> a replication_tools?
>
> Seems like unnecessary churn. Nobody has complained about any of the other
> tools in there.
Not sure. We rename things across releases in the tree from time to
time, and here that's straight-forward.
>> - And if it would be better to refactor some of the code generic to
>> all these streaming tools to fe_utils. What makes streamutil.h a bit
>> less pluggable are all its extern variables to control the connection,
>> but perhaps that can be an advantage, as well, in some cases.
>
> Does anyone outside of pg_basebackup + existing friends + new friend need
> that? Seems like extra complications.
Actually, yes, I've used these utility routines in some past work, and
having the wrapper routines able to run the replication commands in
fe_utils would have been nicer than having to link to a source tree.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-11-09 23:12 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-18 05:47 [PATCH v2] Do not use RelationNeedsWAL to identify relation persistence Kyotaro Horiguchi <[email protected]>
2023-11-07 21:00 Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-07 23:12 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
2023-11-08 12:50 ` Re: speed up a logical replica setup Euler Taveira <[email protected]>
2023-11-08 23:13 ` Re: speed up a logical replica setup Michael Paquier <[email protected]>
2023-11-09 14:41 ` Re: speed up a logical replica setup Peter Eisentraut <[email protected]>
2023-11-09 23:12 ` Re: speed up a logical replica setup Michael Paquier <[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